[PATCH] drm/amd/display: Make underlay rules less strict

[Kazlauskas, Nicholas]

On 2021-05-07 10:37 a.m., Rodrigo Siqueira wrote:
> Currently, we reject all conditions where the underlay plane goes
> outside the overlay plane limits, which is not entirely correct since we
> reject some valid cases like the ones illustrated below:
> 
>    +--------------------+  +--------------------+
>    |   Overlay plane    |  |   Overlay plane    |
>    |                    |  |        +-----------|--+
>    | +--------------+   |  |        |           |  |
>    | |              |   |  |        |           |  |
>    +--------------------+  +--------------------+  |
>      | Primary plane|               +--------------+
>      |  (underlay)  |
>      +--------------+
>    +-+--------------+---+  +--------------------+
>    |    Overlay plane   |  |    Overlay plane   |
> +-|------------+       |  |       +--------------+
> | |            |       |  |       |            | |
> | |            |       |  |       |            | |
> | |            |       |  |       |            | |
> +-|------------+       |  |       +--------------+
>    +--------------------+  +--------------------+
> 
> This patch fixes this issue by only rejecting commit requests where the
> underlay is entirely outside the overlay limits. After applying this
> patch, a set of subtests related to kms_plane, kms_plane_alpha_blend,
> and kms_plane_scaling will pass.
> 
> Signed-off-by: Rodrigo Siqueira <Rodrigo.Siqueira at amd.com>

What's the size of the overlay plane in your examples? If the overlay 
plane does not cover the entire screen then this patch is incorrect.

We don't want to be enabling the cursor on multiple pipes and the checks 
in DC to allow disabling cursor on bottom pipes only work if the 
underlay is entirely contained within the overlay.

In the case where the primary (underlay) plane extends beyond the screen 
boundaries it should be preclipped by userspace or earlier in the DM 
code before this check.

Feel free to follow up with clarification, but for now this patch is a 
NAK from me.

Regards,
Nicholas Kazlauskas

> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 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 cc048c348a92..15006aafc630 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10098,10 +10098,10 @@ static int validate_overlay(struct drm_atomic_state *state)
>   		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) {
> +	if (primary_state->crtc_x + primary_state->crtc_w < overlay_state->crtc_x ||
> +	    primary_state->crtc_x > overlay_state->crtc_x + overlay_state->crtc_w ||
> +	    primary_state->crtc_y > overlay_state->crtc_y + overlay_state->crtc_h ||
> +	    primary_state->crtc_y + primary_state->crtc_h < overlay_state->crtc_y) {
>   		DRM_DEBUG_ATOMIC("Overlay plane is enabled with hardware cursor but does not fully cover primary plane\n");
>   		return -EINVAL;
>   	}
>