[PATCH] drm/amdgpu/dc: Require primary plane to be enabled whenever the CRTC is

[Kazlauskas, Nicholas]

On 2020-08-21 12:57 p.m., Michel Dänzer wrote:
> From: Michel Dänzer <mdaenzer at redhat.com>
> 
> Don't check drm_crtc_state::active for this either, per its
> documentation in include/drm/drm_crtc.h:
> 
>   * Hence drivers must not consult @active in their various
>   * &drm_mode_config_funcs.atomic_check callback to reject an atomic
>   * commit.
> 
> The atomic helpers disable the CRTC as needed for disabling the primary
> plane.
> 
> This prevents at least the following problems if the primary plane gets
> disabled (e.g. due to destroying the FB assigned to the primary plane,
> as happens e.g. with mutter in Wayland mode):
> 
> * Toggling CRTC active to 1 failed if the cursor plane was enabled
>    (e.g. via legacy DPMS property & cursor ioctl).
> * Enabling the cursor plane failed, e.g. via the legacy cursor ioctl.

We previously had the requirement that the primary plane must be enabled 
but some userspace expects that they can enable just the overlay plane 
without anything else.

I think the chromuiumos atomictest validates that this works as well:

So is DRM going forward then with the expectation that this is wrong 
behavior from userspace?

We require at least one plane to be enabled to display a cursor, but it 
doesn't necessarily need to be the primary.

Regards,
Nicholas Kazlauskas

> 
> GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1108
> GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1165
> GitLab: https://gitlab.gnome.org/GNOME/mutter/-/issues/1344
> Suggested-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> Signed-off-by: Michel Dänzer <mdaenzer at redhat.com>
> ---
> 
> Note that this will cause some IGT tests to fail without
> https://patchwork.freedesktop.org/series/80904/ .
> 
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 +++++++------------
>   1 file changed, 11 insertions(+), 22 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 897d60ade1e4..33c5739e221b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5262,19 +5262,6 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc)
>   {
>   }
>   
> -static bool does_crtc_have_active_cursor(struct drm_crtc_state *new_crtc_state)
> -{
> -	struct drm_device *dev = new_crtc_state->crtc->dev;
> -	struct drm_plane *plane;
> -
> -	drm_for_each_plane_mask(plane, dev, new_crtc_state->plane_mask) {
> -		if (plane->type == DRM_PLANE_TYPE_CURSOR)
> -			return true;
> -	}
> -
> -	return false;
> -}
> -
>   static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state)
>   {
>   	struct drm_atomic_state *state = new_crtc_state->state;
> @@ -5338,19 +5325,21 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
>   		return ret;
>   	}
>   
> -	/* In some use cases, like reset, no stream is attached */
> -	if (!dm_crtc_state->stream)
> -		return 0;
> -
>   	/*
> -	 * We want at least one hardware plane enabled to use
> -	 * the stream with a cursor enabled.
> +	 * We require the primary plane to be enabled whenever the CRTC is,
> +	 * otherwise the legacy cursor ioctl helper may end up trying to enable
> +	 * the cursor plane while the primary plane is disabled, which is not
> +	 * supported by the hardware. And there is legacy userspace which stops
> +	 * using the HW cursor altogether in response to the resulting EINVAL.
>   	 */
> -	if (state->enable && state->active &&
> -	    does_crtc_have_active_cursor(state) &&
> -	    dm_crtc_state->active_planes == 0)
> +	if (state->enable &&
> +	    !(state->plane_mask & drm_plane_mask(crtc->primary)))
>   		return -EINVAL;
>   
> +	/* In some use cases, like reset, no stream is attached */
> +	if (!dm_crtc_state->stream)
> +		return 0;
> +
>   	if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK)
>   		return 0;
>   
>