[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 at emersion.fr>
> Reported-by: Pierre-Loup A. Griffais <pgriffais at valvesoftware.com>
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Harry Wentland <hwentlan at amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>

Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas at 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;
>   	}
>   
>