On Fri, Sep 04, 2020 at 12:43:04PM +0200, Michel Dänzer wrote: > From: Michel Dänzer <mdaenzer@xxxxxxxxxx> > > 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@xxxxxxxx> > Signed-off-by: Michel Dänzer <mdaenzer@xxxxxxxxxx> Commit message agrees with my understand of this maze now, so: Acked-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > --- > .../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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
