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

[Daniel Vetter]

On Thu, Nov 12, 2020 at 9: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?

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