[PATCH] drm/amd/display: add basic atomic check for cursor plane

[Kazlauskas, Nicholas]

On 2020-03-30 5:23 a.m., Simon Ser wrote:
> This patch adds a basic cursor check when an atomic test-only commit is
> performed. The position and size of the cursor plane is checked.
> 
> This should fix user-space relying on atomic checks to assign buffers to
> planes.
> 
> Signed-off-by: Simon Ser <contact at emersion.fr>
> Reported-by: Roman Gilg <subdiff at gmail.com>
> References: https://github.com/emersion/libliftoff/issues/46
> Cc: Alex Deucher <alexander.deucher at amd.com>
> Cc: Harry Wentland <hwentlan at amd.com>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 26 +++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 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 f6b0b9a121fd..e1b084318ad6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7843,6 +7843,7 @@ static int dm_update_plane_state(struct dc *dc,
>   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>   	struct dm_crtc_state *dm_new_crtc_state, *dm_old_crtc_state;
>   	struct dm_plane_state *dm_new_plane_state, *dm_old_plane_state;
> +	struct amdgpu_crtc *new_acrtc;
>   	bool needs_reset;
>   	int ret = 0;
> 
> @@ -7852,9 +7853,30 @@ static int dm_update_plane_state(struct dc *dc,
>   	dm_new_plane_state = to_dm_plane_state(new_plane_state);
>   	dm_old_plane_state = to_dm_plane_state(old_plane_state);
> 
> -	/*TODO Implement atomic check for cursor plane */
> -	if (plane->type == DRM_PLANE_TYPE_CURSOR)
> +	/*TODO Implement better atomic check for cursor plane */
> +	if (plane->type == DRM_PLANE_TYPE_CURSOR) {
> +		if (!enable || !new_plane_crtc ||
> +			drm_atomic_plane_disabling(plane->state, new_plane_state))
> +			return 0;
> +
> +		new_acrtc = to_amdgpu_crtc(new_plane_crtc);
> +
> +		if ((new_plane_state->crtc_w > new_acrtc->max_cursor_width) ||
> +			(new_plane_state->crtc_h > new_acrtc->max_cursor_height)) {
> +			DRM_DEBUG_ATOMIC("Bad cursor size %d x %d\n",
> +							 new_plane_state->crtc_w, new_plane_state->crtc_h);
> +			return -EINVAL;
> +		}
> +
> +		if (new_plane_state->crtc_x <= -new_acrtc->max_cursor_width ||
> +			new_plane_state->crtc_y <= -new_acrtc->max_cursor_height) {
> +			DRM_DEBUG_ATOMIC("Bad cursor position %d, %d\n",
> +							 new_plane_state->crtc_x, new_plane_state->crtc_y);
> +			return -EINVAL;

We've been doing these checks for position before but I don't think we 
really need them. DC should be disabling the cursor when we ask for a 
position completely off the screen.

I think that's better than rejecting the commit entirely at least.

Nicholas Kazlauskas

> +		}
> +
>   		return 0;
> +	}
> 
>   	needs_reset = should_reset_plane(state, plane, old_plane_state,
>   					 new_plane_state);
> --
> 2.26.0
> 
> 
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>