[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 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;
> +                       }
> +               }
> +

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 at lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7C70872c6a2aa944b67ac408d8874d6871%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408113864433577%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=nIkL9sjlwRKQl3akctzIlTUBD0fBTsx9TfQ9GBtZhqE%3D&reserved=0