[PATCH] drm/amd/display: add cursor pitch check

[PATCH] drm/amd/display: add cursor pitch check

[Simon Ser]

This patch expands the cursor checks added in "drm/amd/display: add basic
atomic check for cursor plane" to also include a pitch check. Without
this patch, setting a FB smaller than max_cursor_size with an invalid
pitch would result in amdgpu error messages and a fallback to a 64-byte
pitch:

    [drm:hubp1_cursor_set_attributes [amdgpu]] *ERROR* Invalid cursor pitch of 100. Only 64/128/256 is supported on DCN.

Signed-off-by: Simon Ser <contact@emersion.fr>
Reported-by: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Harry Wentland <hwentlan@amd.com>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---

Couple questions:

- This implements a single check for all GPU generations. Is my
  assumption correct here? It seems like this check is OK for at least
  DCN 1.0 and DCN 2.0.
- We should really implement better checks. What features are supported
  on the cursor plane? Is scaling supported? Is cropping supported? Is
  rotation always supported?

 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

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 2855bb918535..42b0ade7de39 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8902,6 +8902,20 @@ static int dm_update_plane_state(struct dc *dc,
 			return -EINVAL;
 		}
 
+		if (new_plane_state->fb) {
+			switch (new_plane_state->fb->pitches[0]) {
+			case 64:
+			case 128:
+			case 256:
+				/* Pitch is supported by cursor plane */
+				break;
+			default:
+				DRM_DEBUG_ATOMIC("Bad cursor pitch %d\n",
+						 new_plane_state->fb->pitches[0]);
+				return -EINVAL;
+			}
+		}
+
 		return 0;
 	}
 
-- 
2.29.2


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: add cursor pitch check

[Kazlauskas, Nicholas]

On 2020-11-12 12:37 p.m., Simon Ser wrote:
> This patch expands the cursor checks added in "drm/amd/display: add basic
> atomic check for cursor plane" to also include a pitch check. Without
> this patch, setting a FB smaller than max_cursor_size with an invalid
> pitch would result in amdgpu error messages and a fallback to a 64-byte
> pitch:
> 
>      [drm:hubp1_cursor_set_attributes [amdgpu]] *ERROR* Invalid cursor pitch of 100. Only 64/128/256 is supported on DCN.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Reported-by: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

But with some comments below:

> ---
> 
> Couple questions:
> 
> - This implements a single check for all GPU generations. Is my
>    assumption correct here? It seems like this check is OK for at least
>    DCN 1.0 and DCN 2.0.
> - We should really implement better checks. What features are supported
>    on the cursor plane? Is scaling supported? Is cropping supported? Is
>    rotation always supported?

On DCE and DCN there is no dedicated hardware cursor plane. You get a 
cursor per pipe but it's going to inherit the scaling and positioning 
from the underlying pipe.

There's software logic to ensure we position the cursor in the correct 
location in CRTC space independent on the underlying DRM plane's scaling 
and positioning but there's no way for us to correct the scaling. Cursor 
will always be 64, 128, or 256 in the pipe's destination space.

Cursor can be independently rotated in hardware but this isn't something 
we expose support for to userspace.

The pitch check of 64/128/256 is OK but we don't support 256 on DCE.

Regards,
Nicholas Kazlauskas

> 
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> 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 2855bb918535..42b0ade7de39 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8902,6 +8902,20 @@ static int dm_update_plane_state(struct dc *dc,
>   			return -EINVAL;
>   		}
>   
> +		if (new_plane_state->fb) {
> +			switch (new_plane_state->fb->pitches[0]) {
> +			case 64:
> +			case 128:
> +			case 256:
> +				/* Pitch is supported by cursor plane */
> +				break;
> +			default:
> +				DRM_DEBUG_ATOMIC("Bad cursor pitch %d\n",
> +						 new_plane_state->fb->pitches[0]);
> +				return -EINVAL;
> +			}
> +		}
> +
>   		return 0;
>   	}
>   
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: add cursor pitch check

[Simon Ser]

CC Daniel Vetter and Bas, see below…

On Thursday, November 12, 2020 8:56 PM, Kazlauskas, Nicholas <nicholas.kazlauskas@amd.com> wrote:

> Reviewed-by: Nicholas Kazlauskasnicholas.kazlauskas@amd.com

Thanks for the review!

> > Couple questions:
> >
> > - This implements a single check for all GPU generations. Is my
> >   assumption correct here? It seems like this check is OK for at least
> >   DCN 1.0 and DCN 2.0.
> >
> > - We should really implement better checks. What features are supported
> >   on the cursor plane? Is scaling supported? Is cropping supported? Is
> >   rotation always supported?
> >
>
> On DCE and DCN there is no dedicated hardware cursor plane. You get a
> cursor per pipe but it's going to inherit the scaling and positioning
> from the underlying pipe.
>
> There's software logic to ensure we position the cursor in the correct
> location in CRTC space independent on the underlying DRM plane's scaling
> and positioning but there's no way for us to correct the scaling. Cursor
> will always be 64, 128, or 256 in the pipe's destination space.

Interesting.

Daniel Vetter: what would be the best way to expose this to user-space?
Maybe we should just make atomic commits with a cursor plane fail when
scaling is used on the primary plane?

Disabling the cursor plane sounds better than displaying the wrong
image.

> Cursor can be independently rotated in hardware but this isn't something
> we expose support for to userspace.

Hmm, I see that cursor planes have the "rotation" property exposed:

    "rotation": bitmask {rotate-0, rotate-90, rotate-180, rotate-270}

In fact all planes have it. It's done in amdgpu_dm_plane_init (behind a
`dm->adev->asic_type >= CHIP_BONAIRE` condition).

Is this an oversight?

> The pitch check of 64/128/256 is OK but we don't support 256 on DCE.

Yeah, I've noticed that. The size check right above should catch it
in most cases I think, because max_cursor_size is 128 on DCE. Side
note, max_cursor_size is 64 on DCE 6.0.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: add cursor pitch check

[Alex Deucher]

On Thu, Nov 12, 2020 at 3:07 PM Simon Ser <contact@emersion.fr> wrote:
>
> CC Daniel Vetter and Bas, see below…
>
> On Thursday, November 12, 2020 8:56 PM, Kazlauskas, Nicholas <nicholas.kazlauskas@amd.com> wrote:
>
> > Reviewed-by: Nicholas Kazlauskasnicholas.kazlauskas@amd.com
>
> Thanks for the review!
>
> > > Couple questions:
> > >
> > > - This implements a single check for all GPU generations. Is my
> > >   assumption correct here? It seems like this check is OK for at least
> > >   DCN 1.0 and DCN 2.0.
> > >
> > > - We should really implement better checks. What features are supported
> > >   on the cursor plane? Is scaling supported? Is cropping supported? Is
> > >   rotation always supported?
> > >
> >
> > On DCE and DCN there is no dedicated hardware cursor plane. You get a
> > cursor per pipe but it's going to inherit the scaling and positioning
> > from the underlying pipe.
> >
> > There's software logic to ensure we position the cursor in the correct
> > location in CRTC space independent on the underlying DRM plane's scaling
> > and positioning but there's no way for us to correct the scaling. Cursor
> > will always be 64, 128, or 256 in the pipe's destination space.
>
> Interesting.
>
> Daniel Vetter: what would be the best way to expose this to user-space?
> Maybe we should just make atomic commits with a cursor plane fail when
> scaling is used on the primary plane?
>
> Disabling the cursor plane sounds better than displaying the wrong
> image.
>
> > Cursor can be independently rotated in hardware but this isn't something
> > we expose support for to userspace.
>
> Hmm, I see that cursor planes have the "rotation" property exposed:
>
>     "rotation": bitmask {rotate-0, rotate-90, rotate-180, rotate-270}
>
> In fact all planes have it. It's done in amdgpu_dm_plane_init (behind a
> `dm->adev->asic_type >= CHIP_BONAIRE` condition).
>
> Is this an oversight?
>
> > The pitch check of 64/128/256 is OK but we don't support 256 on DCE.
>
> Yeah, I've noticed that. The size check right above should catch it
> in most cases I think, because max_cursor_size is 128 on DCE. Side
> note, max_cursor_size is 64 on DCE 6.0.

Maybe something like:
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 77c06f999040..a1ea195a7041 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -8999,6 +8999,43 @@ static int dm_update_plane_state(struct dc *dc,
                        return -EINVAL;
                }

+               switch(adev->family) {
+               case AMDGPU_FAMILY_SI:
+                       /* DCE6 only supports 64x64 cursors */
+                       if (new_plane_state->fb->pitches[0] == 64) {
+                               break;
+                       } else {
+                               DRM_DEBUG_ATOMIC("Bad cursor pitch %d\n",
+
new_plane_state->fb->pitches[0]);
+                               return -EINVAL;
+                       }
+               case AMDGPU_FAMILY_KV:
+               case AMDGPU_FAMILY_CI:
+               case AMDGPU_FAMILY_CZ:
+               case AMDGPU_FAMILY_VI:
+               case AMDGPU_FAMILY_AI:
+                       /* DCE8-12 only supports 64x64 and 128x128 cursors */
+                       if ((new_plane_state->fb->pitches[0] == 64) ||
+                           (new_plane_state->fb->pitches[0] == 128)) {
+                               break;
+                       } else {
+                               DRM_DEBUG_ATOMIC("Bad cursor pitch %d\n",
+
new_plane_state->fb->pitches[0]);
+                               return -EINVAL;
+                       }
+               default:
+                       /* DCN supports 64x64, 128x128, and 256x256 cursors */
+                       if ((new_plane_state->fb->pitches[0] == 64) ||
+                           (new_plane_state->fb->pitches[0] == 128) ||
+                           (new_plane_state->fb->pitches[0] == 256)) {
+                               break;
+                       } else {
+                               DRM_DEBUG_ATOMIC("Bad cursor pitch %d\n",
+
new_plane_state->fb->pitches[0]);
+                               return -EINVAL;
+                       }
+               }
+
                return 0;
        }



> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: add cursor pitch check

[Kazlauskas, Nicholas]

On 2020-11-12 3:56 p.m., Alex Deucher wrote:
> On Thu, Nov 12, 2020 at 3:07 PM Simon Ser <contact@emersion.fr> wrote:
>>
>> CC Daniel Vetter and Bas, see below…
>>
>> On Thursday, November 12, 2020 8:56 PM, Kazlauskas, Nicholas <nicholas.kazlauskas@amd.com> wrote:
>>
>>> Reviewed-by: Nicholas Kazlauskasnicholas.kazlauskas@amd.com
>>
>> Thanks for the review!
>>
>>>> Couple questions:
>>>>
>>>> - This implements a single check for all GPU generations. Is my
>>>>    assumption correct here? It seems like this check is OK for at least
>>>>    DCN 1.0 and DCN 2.0.
>>>>
>>>> - We should really implement better checks. What features are supported
>>>>    on the cursor plane? Is scaling supported? Is cropping supported? Is
>>>>    rotation always supported?
>>>>
>>>
>>> On DCE and DCN there is no dedicated hardware cursor plane. You get a
>>> cursor per pipe but it's going to inherit the scaling and positioning
>>> from the underlying pipe.
>>>
>>> There's software logic to ensure we position the cursor in the correct
>>> location in CRTC space independent on the underlying DRM plane's scaling
>>> and positioning but there's no way for us to correct the scaling. Cursor
>>> will always be 64, 128, or 256 in the pipe's destination space.
>>
>> Interesting.
>>
>> Daniel Vetter: what would be the best way to expose this to user-space?
>> Maybe we should just make atomic commits with a cursor plane fail when
>> scaling is used on the primary plane?
>>
>> Disabling the cursor plane sounds better than displaying the wrong
>> image.
>>
>>> Cursor can be independently rotated in hardware but this isn't something
>>> we expose support for to userspace.
>>
>> Hmm, I see that cursor planes have the "rotation" property exposed:
>>
>>      "rotation": bitmask {rotate-0, rotate-90, rotate-180, rotate-270}
>>
>> In fact all planes have it. It's done in amdgpu_dm_plane_init (behind a
>> `dm->adev->asic_type >= CHIP_BONAIRE` condition).
>>
>> Is this an oversight?
>>
>>> The pitch check of 64/128/256 is OK but we don't support 256 on DCE.
>>
>> Yeah, I've noticed that. The size check right above should catch it
>> in most cases I think, because max_cursor_size is 128 on DCE. Side
>> note, max_cursor_size is 64 on DCE 6.0.
> 
> Maybe something like:
> 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 77c06f999040..a1ea195a7041 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -8999,6 +8999,43 @@ static int dm_update_plane_state(struct dc *dc,
>                          return -EINVAL;
>                  }
> 
> +               switch(adev->family) {
> +               case AMDGPU_FAMILY_SI:
> +                       /* DCE6 only supports 64x64 cursors */
> +                       if (new_plane_state->fb->pitches[0] == 64) {
> +                               break;
> +                       } else {
> +                               DRM_DEBUG_ATOMIC("Bad cursor pitch %d\n",
> +
> new_plane_state->fb->pitches[0]);
> +                               return -EINVAL;
> +                       }
> +               case AMDGPU_FAMILY_KV:
> +               case AMDGPU_FAMILY_CI:
> +               case AMDGPU_FAMILY_CZ:
> +               case AMDGPU_FAMILY_VI:
> +               case AMDGPU_FAMILY_AI:
> +                       /* DCE8-12 only supports 64x64 and 128x128 cursors */
> +                       if ((new_plane_state->fb->pitches[0] == 64) ||
> +                           (new_plane_state->fb->pitches[0] == 128)) {
> +                               break;
> +                       } else {
> +                               DRM_DEBUG_ATOMIC("Bad cursor pitch %d\n",
> +
> new_plane_state->fb->pitches[0]);
> +                               return -EINVAL;
> +                       }
> +               default:
> +                       /* DCN supports 64x64, 128x128, and 256x256 cursors */
> +                       if ((new_plane_state->fb->pitches[0] == 64) ||
> +                           (new_plane_state->fb->pitches[0] == 128) ||
> +                           (new_plane_state->fb->pitches[0] == 256)) {
> +                               break;
> +                       } else {
> +                               DRM_DEBUG_ATOMIC("Bad cursor pitch %d\n",
> +
> new_plane_state->fb->pitches[0]);
> +                               return -EINVAL;
> +                       }
> +               }
> +

If we're going to extend it to something like this I think that this 
should be extracted out to its own function to reduce some of this 
indenting.

I think the simpler approach here is to just block 256 if the 
max_cursor_size in amdgpu is 128.

Regards,
Nicholas Kazlauskas

>                  return 0;
>          }
> 
> 
> 
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C70872c6a2aa944b67ac408d8874d6871%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408113864433577%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=nIkL9sjlwRKQl3akctzIlTUBD0fBTsx9TfQ9GBtZhqE%3D&amp;reserved=0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: add cursor pitch check

[Simon Ser]

Hmm. I've written new patches to check the remaining plane state [1],
but I'm confused. On my RX 580 machine, it turns out the pitch used by
my cursor is not 128 or 256, it's 512. Here's a drm_info excerpt:

    ├───DRM_CAP_CURSOR_WIDTH = 128
    ├───DRM_CAP_CURSOR_HEIGHT = 128
    […]
    ├───Plane 6
    │   ├───Object ID: 52
    │   ├───CRTCs: {0}
    │   ├───Formats:
    │   │   └───ARGB8888 (0x34325241)
    │   └───Properties
    │       ├───"type" (immutable): enum {Overlay, Primary, Cursor} = Cursor
    │       ├───"FB_ID" (atomic): object framebuffer = 106
    │       │   ├───Object ID: 106
    │       │   ├───Size: 128x128
    │       │   ├───Format: ARGB8888 (0x34325241)
    │       │   └───Planes:
    │       │       └───Plane 0: offset = 0, pitch = 512
    │       ├───"IN_FENCE_FD" (atomic): srange [-1, INT32_MAX] = -1
    │       ├───"CRTC_ID" (atomic): object CRTC = 54
    │       ├───"CRTC_X" (atomic): srange [INT32_MIN, INT32_MAX] = 792
    │       ├───"CRTC_Y" (atomic): srange [INT32_MIN, INT32_MAX] = 1302
    │       ├───"CRTC_W" (atomic): range [0, INT32_MAX] = 128
    │       ├───"CRTC_H" (atomic): range [0, INT32_MAX] = 128
    │       ├───"SRC_X" (atomic): range [0, UINT32_MAX] = 0
    │       ├───"SRC_Y" (atomic): range [0, UINT32_MAX] = 0
    │       ├───"SRC_W" (atomic): range [0, UINT32_MAX] = 128
    │       ├───"SRC_H" (atomic): range [0, UINT32_MAX] = 128

However the cursor is displayed just fine. It seems like amdgpu sets
dc_cursor_attributes.pitch to the FB width in handle_cursor_update:

    attributes.pitch = attributes.width;

Is this expected? Did I get the cursor pitch constraint wrong? Should
we check for alignment instead?

Thanks,

Simon

[1]: https://github.com/emersion/linux/commits/amdgpu-cursor-pitch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: add cursor pitch check

[Daniel Vetter]

On Thu, Nov 12, 2020 at 9:07 PM Simon Ser <contact@emersion.fr> wrote:
>
> CC Daniel Vetter and Bas, see below…
>
> On Thursday, November 12, 2020 8:56 PM, Kazlauskas, Nicholas <nicholas.kazlauskas@amd.com> wrote:
>
> > Reviewed-by: Nicholas Kazlauskasnicholas.kazlauskas@amd.com
>
> Thanks for the review!
>
> > > Couple questions:
> > >
> > > - This implements a single check for all GPU generations. Is my
> > >   assumption correct here? It seems like this check is OK for at least
> > >   DCN 1.0 and DCN 2.0.
> > >
> > > - We should really implement better checks. What features are supported
> > >   on the cursor plane? Is scaling supported? Is cropping supported? Is
> > >   rotation always supported?
> > >
> >
> > On DCE and DCN there is no dedicated hardware cursor plane. You get a
> > cursor per pipe but it's going to inherit the scaling and positioning
> > from the underlying pipe.
> >
> > There's software logic to ensure we position the cursor in the correct
> > location in CRTC space independent on the underlying DRM plane's scaling
> > and positioning but there's no way for us to correct the scaling. Cursor
> > will always be 64, 128, or 256 in the pipe's destination space.
>
> Interesting.
>
> Daniel Vetter: what would be the best way to expose this to user-space?
> Maybe we should just make atomic commits with a cursor plane fail when
> scaling is used on the primary plane?

I think there's been discussion for a pipe scaling property on the
crtc. As long as we don't have that, and you're using the pipe scaling
to scale the primary plane, then I guess you have to reject the cursor
if it's enabled. Except maybe if the scaling is the same one, dunno
whether that ever happens.
-Daniel


> Disabling the cursor plane sounds better than displaying the wrong
> image.
>
> > Cursor can be independently rotated in hardware but this isn't something
> > we expose support for to userspace.
>
> Hmm, I see that cursor planes have the "rotation" property exposed:
>
>     "rotation": bitmask {rotate-0, rotate-90, rotate-180, rotate-270}
>
> In fact all planes have it. It's done in amdgpu_dm_plane_init (behind a
> `dm->adev->asic_type >= CHIP_BONAIRE` condition).
>
> Is this an oversight?
>
> > The pitch check of 64/128/256 is OK but we don't support 256 on DCE.
>
> Yeah, I've noticed that. The size check right above should catch it
> in most cases I think, because max_cursor_size is 128 on DCE. Side
> note, max_cursor_size is 64 on DCE 6.0.



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx