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

[Michel Dänzer]

On 2018-12-12 3:58 p.m., Kazlauskas, Nicholas wrote:
> 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).

Beware that 'hotspot' has slightly different meanings at the X / KMS /
hardware level.

At the KMS API level, the hotspot is just a hint, which can be used for
automatically re-positioning the HW cursor when the hotspot changes. The
cursor position in the API always refers to the top/left corner of the
cursor regardless of the hotspot.

Assuming the position->x/y(_hotspot) values are programmed to the
CUR_POSITION/HOT_SPOT registers directly, the code is correct as is.


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