Re: [PATCH v3 1/2] amd/display: check cursor plane matches underlying plane

[Harry Wentland <harry.wentland@amd.com>]

On 2021-09-29 15:06, Simon Ser wrote:
> The current logic checks whether the cursor plane blending
> properties match the primary plane's. However that's wrong,
> because the cursor is painted on all planes underneath. If
> the cursor is over the primary plane and the cursor plane,

Do you mean "and the underlay plane" here, instead of "and
the cursor plane"?

Harry

> it's painted on both pipes.
> 
> Iterate over the CRTC planes and check their scaling match
> the cursor's.
> 
> Signed-off-by: Simon Ser <contact@emersion.fr>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Harry Wentland <hwentlan@amd.com>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Cc: Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 49 +++++++++++++------
>  1 file changed, 34 insertions(+), 15 deletions(-)
> 
> 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 3c7a8f869b40..6472c0032b54 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10505,18 +10505,18 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>  				struct drm_crtc *crtc,
>  				struct drm_crtc_state *new_crtc_state)
>  {
> -	struct drm_plane_state *new_cursor_state, *new_primary_state;
> -	int cursor_scale_w, cursor_scale_h, primary_scale_w, primary_scale_h;
> +	struct drm_plane *cursor = crtc->cursor, *underlying;
> +	struct drm_plane_state *new_cursor_state, *new_underlying_state;
> +	int i;
> +	int cursor_scale_w, cursor_scale_h, underlying_scale_w, underlying_scale_h;
>  
>  	/* On DCE and DCN there is no dedicated hardware cursor plane. We get a
>  	 * cursor per pipe but it's going to inherit the scaling and
>  	 * positioning from the underlying pipe. Check the cursor plane's
> -	 * blending properties match the primary plane's. */
> +	 * blending properties match the underlying planes'. */
>  
> -	new_cursor_state = drm_atomic_get_new_plane_state(state, crtc->cursor);
> -	new_primary_state = drm_atomic_get_new_plane_state(state, crtc->primary);
> -	if (!new_cursor_state || !new_primary_state ||
> -	    !new_cursor_state->fb || !new_primary_state->fb) {
> +	new_cursor_state = drm_atomic_get_new_plane_state(state, cursor);
> +	if (!new_cursor_state || !new_cursor_state->fb) {
>  		return 0;
>  	}
>  
> @@ -10525,15 +10525,34 @@ static int dm_check_crtc_cursor(struct drm_atomic_state *state,
>  	cursor_scale_h = new_cursor_state->crtc_h * 1000 /
>  			 (new_cursor_state->src_h >> 16);
>  
> -	primary_scale_w = new_primary_state->crtc_w * 1000 /
> -			 (new_primary_state->src_w >> 16);
> -	primary_scale_h = new_primary_state->crtc_h * 1000 /
> -			 (new_primary_state->src_h >> 16);
> +	for_each_new_plane_in_state_reverse(state, underlying, new_underlying_state, i) {
> +		/* Narrow down to non-cursor planes on the same CRTC as the cursor */
> +		if (new_underlying_state->crtc != crtc || underlying == crtc->cursor)
> +			continue;
>  
> -	if (cursor_scale_w != primary_scale_w ||
> -	    cursor_scale_h != primary_scale_h) {
> -		drm_dbg_atomic(crtc->dev, "Cursor plane scaling doesn't match primary plane\n");
> -		return -EINVAL;
> +		/* Ignore disabled planes */
> +		if (!new_underlying_state->fb)
> +			continue;
> +
> +		underlying_scale_w = new_underlying_state->crtc_w * 1000 /
> +				     (new_underlying_state->src_w >> 16);
> +		underlying_scale_h = new_underlying_state->crtc_h * 1000 /
> +				     (new_underlying_state->src_h >> 16);
> +
> +		if (cursor_scale_w != underlying_scale_w ||
> +		    cursor_scale_h != underlying_scale_h) {
> +			drm_dbg_atomic(crtc->dev,
> +				       "Cursor [PLANE:%d:%s] scaling doesn't match underlying [PLANE:%d:%s]\n",
> +				       cursor->base.id, cursor->name, underlying->base.id, underlying->name);
> +			return -EINVAL;
> +		}
> +
> +		/* If this plane covers the whole CRTC, no need to check planes underneath */
> +		if (new_underlying_state->crtc_x <= 0 &&
> +		    new_underlying_state->crtc_y <= 0 &&
> +		    new_underlying_state->crtc_x + new_underlying_state->crtc_w >= new_crtc_state->mode.hdisplay &&
> +		    new_underlying_state->crtc_y + new_underlying_state->crtc_h >= new_crtc_state->mode.vdisplay)
> +			break;
>  	}
>  
>  	return 0;
>