[PATCH v2] drm/amd/display: Fix two cursor duplication when using overlay

[Harry Wentland]

On 2021-04-13 8:06 p.m., Rodrigo Siqueira wrote:
> Our driver supports overlay planes, and as expected, some userspace
> compositor takes advantage of these features. If the userspace is not
> enabling the cursor, they can use multiple planes as they please.
> Nevertheless, we start to have constraints when userspace tries to
> enable hardware cursor with various planes. Basically, we cannot draw
> the cursor at the same size and position on two separated pipes since it
> uses extra bandwidth and DML only run with one cursor.
> 
> For those reasons, when we enable hardware cursor and multiple planes,
> our driver should accept variations like the ones described below:
> 
>    +-------------+   +--------------+
>    | +---------+ |   |              |
>    | |Primary  | |   | Primary      |
>    | |         | |   | Overlay      |
>    | +---------+ |   |              |
>    |Overlay      |   |              |
>    +-------------+   +--------------+
> 
> In this scenario, we can have the desktop UI in the overlay and some
> other framebuffer attached to the primary plane (e.g., video). However,
> userspace needs to obey some rules and avoid scenarios like the ones
> described below (when enabling hw cursor):
> 
>                                        +--------+
>                                        |Overlay |
>   +-------------+    +-----+-------+ +-|        |--+
>   | +--------+  | +--------+       | | +--------+  |
>   | |Overlay |  | |Overlay |       | |             |
>   | |        |  | |        |       | |             |
>   | +--------+  | +--------+       | |             |
>   | Primary     |    | Primary     | | Primary     |
>   +-------------+    +-------------+ +-------------+
> 
>   +-------------+   +-------------+
>   |     +--------+  |  Primary    |
>   |     |Overlay |  |             |
>   |     |        |  |             |
>   |     +--------+  | +--------+  |
>   | Primary     |   | |Overlay |  |
>   +-------------+   +-|        |--+
>                       +--------+
> 
> If the userspace violates some of the above scenarios, our driver needs
> to reject the commit; otherwise, we can have unexpected behavior. Since
> we don't have a proper driver validation for the above case, we can see
> some problems like a duplicate cursor in applications that use multiple
> planes. This commit fixes the cursor issue and others by adding adequate
> verification for multiple planes.
> 

It might be good to have a brief mention that regardless of whether we 
have a cursor plane we want to reject the problematic plane configs 
since some userspace doesn't handle atomic check/commit rejections well 
when only the cursor enablement changes.

> Change since V1 (Harry and Sean):
> - Remove cursor verification from the equation.
> 
> Cc: Louis Li <Ching-shih.Li at amd.com>
> Cc: Nicholas Kazlauskas <Nicholas.Kazlauskas at amd.com>
> Cc: Harry Wentland <Harry.Wentland at amd.com>
> Cc: Hersen Wu <hersenxs.wu at amd.com>
> Cc: Sean Paul <seanpaul at chromium.org>
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>

EIther way, this patch is
Reviewed-by: Harry Wentland <harry.wentland at amd.com>

Harry

> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 51 +++++++++++++++++++
>   1 file changed, 51 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 e29cb2e956db..ac1408d52eff 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -9946,6 +9946,53 @@ static int add_affected_mst_dsc_crtcs(struct drm_atomic_state *state, struct drm
>   }
>   #endif
>   
> +static int validate_overlay(struct drm_atomic_state *state)
> +{
> +	int i;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
> +	struct drm_plane_state *primary_state, *overlay_state = NULL;
> +
> +	/* Check if primary plane is contained inside overlay */
> +	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> +		if (plane->type == DRM_PLANE_TYPE_OVERLAY) {
> +			if (drm_atomic_plane_disabling(plane->state, new_plane_state))
> +				return 0;
> +
> +			overlay_state = new_plane_state;
> +			continue;
> +		}
> +	}
> +
> +	/* check if we're making changes to the overlay plane */
> +	if (!overlay_state)
> +		return 0;
> +
> +	/* check if overlay plane is enabled */
> +	if (!overlay_state->crtc)
> +		return 0;
> +
> +	/* find the primary plane for the CRTC that the overlay is enabled on */
> +	primary_state = drm_atomic_get_plane_state(state, overlay_state->crtc->primary);
> +	if (IS_ERR(primary_state))
> +		return PTR_ERR(primary_state);
> +
> +	/* check if primary plane is enabled */
> +	if (!primary_state->crtc)
> +		return 0;
> +
> +	/* Perform the bounds check to ensure the overlay plane covers the primary */
> +	if (primary_state->crtc_x < overlay_state->crtc_x ||
> +	    primary_state->crtc_y < overlay_state->crtc_y ||
> +	    primary_state->crtc_x + primary_state->crtc_w > overlay_state->crtc_x + overlay_state->crtc_w ||
> +	    primary_state->crtc_y + primary_state->crtc_h > overlay_state->crtc_y + overlay_state->crtc_h) {
> +		DRM_DEBUG_ATOMIC("Overlay plane is enabled with hardware cursor but does not fully cover primary plane\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>   /**
>    * amdgpu_dm_atomic_check() - Atomic check implementation for AMDgpu DM.
>    * @dev: The DRM device
> @@ -10120,6 +10167,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   			goto fail;
>   	}
>   
> +	ret = validate_overlay(state);
> +	if (ret)
> +		goto fail;
> +
>   	/* Add new/modified planes */
>   	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
>   		ret = dm_update_plane_state(dc, state, plane,
>