drm/amd/display: Fix cursor pos and hotspot calcs

Submitted by Kazlauskas, Nicholas on Dec. 12, 2018, 2:04 p.m.

Details

Message ID 20181212140434.3622-1-nicholas.kazlauskas@amd.com
State New
Headers show
Series "drm/amd/display: Fix cursor pos and hotspot calcs" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Kazlauskas, Nicholas Dec. 12, 2018, 2:04 p.m.
[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@amd.com>
Cc: Harry Wentland <harry.wentland@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 23 +++++++------------
 1 file changed, 8 insertions(+), 15 deletions(-)

Patch hide | download patch | download mbox

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;
 
 	return 0;
 }

Comments

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@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@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.
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@amd.com>

>> Cc: Harry Wentland <harry.wentland@amd.com>

>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@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.

Nicholas Kazlauskas
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@amd.com>
>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@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.
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@amd.com>

>>>> Cc: Harry Wentland <harry.wentland@amd.com>

>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@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
On Wed, Dec 12, 2018 at 9:04 AM Nicholas Kazlauskas
<nicholas.kazlauskas@amd.com> 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.

Does this patch fix this bug?
https://bugzilla.kernel.org/show_bug.cgi?id=201815

Alex

>
> Cc: Leo Li <sunpeng.li@amd.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@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;
>
>         return 0;
>  }
> --
> 2.17.1
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 12/12/18 10:18 AM, Alex Deucher wrote:
> On Wed, Dec 12, 2018 at 9:04 AM Nicholas Kazlauskas

> <nicholas.kazlauskas@amd.com> 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.

> 

> Does this patch fix this bug?

> https://bugzilla.kernel.org/show_bug.cgi?id=201815

> 

> Alex


I have another patch that should directly address this bug - it doesn't 
depend on this one, however.

Nicholas Kazlauskas

> 

>>

>> Cc: Leo Li <sunpeng.li@amd.com>

>> Cc: Harry Wentland <harry.wentland@amd.com>

>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@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;

>>

>>          return 0;

>>   }

>> --

>> 2.17.1

>>

>> _______________________________________________

>> amd-gfx mailing list

>> amd-gfx@lists.freedesktop.org

>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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@amd.com>
>>>>> Cc: Harry Wentland <harry.wentland@amd.com>
>>>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@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.