On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote:
> From: Michel Dänzer <mdaen...@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.vet...@ffwll.ch>
> Signed-off-by: Michel Dänzer <mdaen...@redhat.com>
Commit message agrees with my understand of this maze now, so:
Acked-by: Daniel Vetter <daniel.vet...@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
