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