Subject: Re: [PATCH v2] drm: support hotspot for universal plane cursors
To: John Keeping <john@metanate.com>,
        =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= <ville.syrjala@linux.intel.com>,
        Alex Deucher <alexdeucher@gmail.com>
References: <1447762218-11017-1-git-send-email-john@metanate.com>
 <1447772734-6480-1-git-send-email-john@metanate.com>
 <20151117153932.GU4437@intel.com> <20151117155943.1e83b053.john@metanate.com>
 <20151117162935.GK16848@phenom.ffwll.local>
From: =?UTF-8?Q?Michel_D=c3=a4nzer?= <michel@daenzer.net>
Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org
Message-ID: <564C394B.8070408@daenzer.net>
Date: Wed, 18 Nov 2015 17:39:39 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Icedove/38.3.0
MIME-Version: 1.0
In-Reply-To: <20151117162935.GK16848@phenom.ffwll.local>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3978
Lines: 100

On 18.11.2015 01:29, Daniel Vetter wrote:
> On Tue, Nov 17, 2015 at 03:59:43PM +0000, John Keeping wrote:
>> On Tue, 17 Nov 2015 17:39:32 +0200, Ville Syrjälä wrote:
>>
>>> On Tue, Nov 17, 2015 at 03:05:34PM +0000, John Keeping wrote:
>>>> The request's hot_x and hot_y are set correctly for both
>>>> DRM_IOCTL_MODE_CURSOR and DRM_IOCTL_MODE_CURSOR2 so we just need to
>>>> save the values and then apply the offset to the cursor plane when
>>>> the cursor moves.
>>>>
>>>> Signed-off-by: John Keeping <john@metanate.com>
>>>> ---
>>>> v2:
>>>>   - add kerneldoc for hot_x and hot_y in struct drm_crtc
>>>>
>>>>  drivers/gpu/drm/drm_crtc.c | 11 +++++++----
>>>>  include/drm/drm_crtc.h     |  6 ++++++
>>>>  2 files changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
>>>> index 720a153..40f5b84 100644
>>>> --- a/drivers/gpu/drm/drm_crtc.c
>>>> +++ b/drivers/gpu/drm/drm_crtc.c
>>>> @@ -2831,6 +2831,9 @@ static int drm_mode_cursor_universal(struct
>>>> drm_crtc *crtc, DRM_DEBUG_KMS("failed to wrap cursor buffer in drm
>>>> framebuffer\n"); return PTR_ERR(fb);
>>>>  			}
>>>> +
>>>> +			crtc->hot_x = req->hot_x;
>>>> +			crtc->hot_y = req->hot_y;
>>>>  		} else {
>>>>  			fb = NULL;
>>>>  		}
>>>> @@ -2841,11 +2844,11 @@ static int drm_mode_cursor_universal(struct
>>>> drm_crtc *crtc, }
>>>>  
>>>>  	if (req->flags & DRM_MODE_CURSOR_MOVE) {
>>>> -		crtc_x = req->x;
>>>> -		crtc_y = req->y;
>>>> +		crtc_x = req->x - crtc->hot_x;
>>>> +		crtc_y = req->y - crtc->hot_y;
>>>>  	} else {
>>>> -		crtc_x = crtc->cursor_x;
>>>> -		crtc_y = crtc->cursor_y;
>>>> +		crtc_x = crtc->cursor_x - crtc->hot_x;
>>>> +		crtc_y = crtc->cursor_y - crtc->hot_y;  
>>>
>>> Why does the location of the hotspot affect the plane position?
>>
>> hot_{x,y} specify the location of the active pixel within the cursor
>> plane and cursor_{x,y} specify the location of the active pixel on the
>> display so we need to offset the plane position in order for the active
>> pixel to be in the correct place.
> 
> Nope, hot_x/y is just for virtual machines to indicate where the logical
> cursor position is within the cursor plane. It should have 2 effect on how
> something is displayed.

Agreed: Since the DRM_IOCTL_MODE_CURSOR ioctl doesn't contain any
information about the hotspot, the x/y coordinates passed in the
DRM_IOCTL_MODE_CURSOR(2) ioctls can only refer to the position of the
top left corner of the cursor.


> And no, I have absolutely no idea why radeon is pulling some tricks here,
> which have been added in
> 
> commit 78b1a6010b46a69bcd47b723a80f92693f26d17b
> Author: Michel Dänzer <michel.daenzer@amd.com>
> Date:   Tue Nov 18 18:00:08 2014 +0900
> 
>     drm/radeon: Use cursor_set2 hook for enabling / disabling the HW cursor
> 
> Michel/Alex, can you please shed some light onto this?

As described in the rest of the commit log, the intention was to avoid
the cursor intermittently appearing in the wrong location with existing
userspace which sets the cursor BO in one ioctl call and the new
position in another ioctl call.


> radeon is the only driver doing this, making this interface inconsistent.

It's only inconsistent in the case that userspace updates the cursor
position to account for the new hotspot position in one ioctl call
first, and only then sets the new BO in another ioctl call. In all other
cases, the cursor position passed in by userspace is preserved.

Anyway, in the meantime it has become apparent that this change didn't
fully fix the problem, so feel free to revert it.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/