[PATCH] drm/amd/display: Fix cursor pos and hotspot calcs

[Michel Dänzer]

On 2018-12-12 3:17 p.m., Kazlauskas, Nicholas wrote:
> On 12/12/18 9:10 AM, Michel Dänzer wrote:
>> On 2018-12-12 3:04 p.m., Nicholas Kazlauskas wrote:
>>> [Why]
>>> The cursor calculations in amdgpu_dm incorrectly assume that the
>>> cursor hotspot is always (0, 0) and don't respect the hot_x and hot_y
>>> attributes that can be passed in via the drm_mode_cursor2_ioctl.
>>>
>>> The DC hotspot parameters are also incorrectly used to offset the
>>> cursor when it goes beyond the bounds of the screen instead of
>>> being clamped.
>>>
>>> [How]
>>> Use the hot_x and hot_y attributes from the fb to directly fill in the
>>> DC hotspot attributes.
>>>
>>> Clamp the cursor position on the edges of the screen instead of using
>>> the hotspot to offset it back in.
>>>
>>> Cc: Leo Li <sunpeng.li at amd.com>
>>> Cc: Harry Wentland <harry.wentland at amd.com>
>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>>> ---
>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++------------
>>>   1 file changed, 8 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 01badda14079..61f2eae0b67f 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -4266,7 +4266,6 @@ static int get_cursor_position(struct drm_plane *plane, struct drm_crtc *crtc,
>>>   {
>>>   	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>>>   	int x, y;
>>> -	int xorigin = 0, yorigin = 0;
>>>   
>>>   	if (!crtc || !plane->state->fb) {
>>>   		position->enable = false;
>>> @@ -4284,24 +4283,18 @@ static int get_cursor_position(struct drm_plane *plane, struct drm_crtc *crtc,
>>>   		return -EINVAL;
>>>   	}
>>>   
>>> -	x = plane->state->crtc_x;
>>> -	y = plane->state->crtc_y;
>>> +	x = plane->state->crtc_x + plane->state->fb->hot_x;
>>> +	y = plane->state->crtc_y + plane->state->fb->hot_y;
>>> +
>>>   	/* avivo cursor are offset into the total surface */
>>>   	x += crtc->primary->state->src_x >> 16;
>>>   	y += crtc->primary->state->src_y >> 16;
>>> -	if (x < 0) {
>>> -		xorigin = min(-x, amdgpu_crtc->max_cursor_width - 1);
>>> -		x = 0;
>>> -	}
>>> -	if (y < 0) {
>>> -		yorigin = min(-y, amdgpu_crtc->max_cursor_height - 1);
>>> -		y = 0;
>>> -	}
>>> +
>>>   	position->enable = true;
>>> -	position->x = x;
>>> -	position->y = y;
>>> -	position->x_hotspot = xorigin;
>>> -	position->y_hotspot = yorigin;
>>> +	position->x = x >= 0 ? x : 0;
>>> +	position->y = y >= 0 ? y : 0;
>>> +	position->x_hotspot = plane->state->fb->hot_x;
>>> +	position->y_hotspot = plane->state->fb->hot_y;
>>
>> I don't see how this could work correctly. Try moving a cursor which
>> doesn't have the hot spot at the top/left edge (e.g. the X-shaped cursor
>> used by an X server without any clients) beyond the top/left edge of the
>> monitor.
>>
>>
> Do you mean the clamping? I was just keeping the driver's previous 
> behavior regarding this.
> 
> It previously forced the position to 0 but offset the cursor using the 
> hotspot attribute the more it sunk into the top or left edge. You can 
> see this with the default GNOME3 cursor (which doesn't have a hotspot of 
> 0, 0) and it sinks into the edge on the screen by a few pixels.
> 
> With this patch the cursor position is now always aligned with the first 
> black pixel instead of the white border - and not just whenever it 
> starts sinking into the top or left edge.

I think you need to at least remove the clamping to 0:

	position->x = x >= 0 ? x : 0;
	position->y = y >= 0 ? y : 0;

Otherwise the cursor cannot move up/left beyond its hotspot, but that
can be necessary e.g. when moving the cursor between monitors.


That is assuming other code handles position position->x/y and
position->x/y_hotspot correctly in all cases other than that.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer