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

[Alex Deucher]

On Thu, Nov 12, 2020 at 3:07 PM Simon Ser <contact at emersion.fr> wrote:
>
> CC Daniel Vetter and Bas, see below…
>
> On Thursday, November 12, 2020 8:56 PM, Kazlauskas, Nicholas <nicholas.kazlauskas at amd.com> wrote:
>
> > Reviewed-by: Nicholas Kazlauskasnicholas.kazlauskas at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx