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

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
> From: Michel Dänzer <mdaenzer@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.
> 
> atomic_remove_fb disables 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):
> 
> * The legacy cursor ioctl returned EINVAL for a non-0 cursor FB ID
>   (which enables the cursor plane).
> * If the cursor plane was enabled, changing the legacy DPMS property
>   value from off to on returned EINVAL.
> 
> v2:
> * Minor changes to code comment and commit log, per review feedback.
> 
> 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@ffwll.ch>
> Signed-off-by: Michel Dänzer <mdaenzer@redhat.com>

Commit message agrees with my understand of this maze now, so:

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++-------------
>  1 file changed, 10 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 45f5f4b7024d..c5f9452490d6 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5269,19 +5269,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;
> @@ -5345,19 +5332,20 @@ 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
> +	 * drm_mode_cursor_universal may end up trying to enable the cursor plane while all other
> +	 * planes are 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;
>  
> -- 
> 2.28.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx