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

[Kazlauskas, Nicholas]

On 12/12/18 9:40 AM, Michel Dänzer wrote:
> 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.
> 
> 

Ah, I see what you mean.

The X cursor makes it really obvious as to what's actually going on 
regrading pos/hotspot calculations too. The reason why this looked 
correct before is that the destination position seems to come pre-offset 
by the cursor hotspot. So DC hotspot parameters of (0, 0) still look 
correct even if that doesn't reflect what's actually happening (though 
certainly looks wrong from first glance, especially with the min(-x, 
...) part).

So I suppose this patch doesn't correct any sort of incorrect behavior 
but should be more of a refactor than anything (ie. stop lying to DC 
what the hotspot is).

Nicholas Kazlauskas