| Message ID | 20191115194204.22244-7-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Random pile of core stuff | expand |
On Fri, Nov 15, 2019 at 09:42:03PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > The early return in drm_atomic_set_mode_for_crtc() isn't quite > right. It would mistakenly return and fail to update > crtc_state->enable if someone actually tried to set a zeroed > mode on a currently disabled crtc. That should never actually > happen in response to any userspace request as the zeroed mode > would get rejected earlier. However there is some chance of this > happening internally (eg. during hw state readout) so it seems > best to not let the state become totally inconsistent. > > Additionally the early return will not be taken if we're trying to > disable an already disabled crtc. While that is not actually > harmful it is inconsistent, so let's handle that case as well. > > Testcase: igt/kms_selftest/check_atomic_set_mode_for_crtc > Testcase: igt/kms_selftest/check_atomic_set_zeroed_mode_fort_crtc > Reviewed-by: Daniel Vetter <daniel@ffwll.ch> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/drm_atomic_uapi.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c > index 0d466d3b0809..a3a6a8137af4 100644 > --- a/drivers/gpu/drm/drm_atomic_uapi.c > +++ b/drivers/gpu/drm/drm_atomic_uapi.c > @@ -68,8 +68,13 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, > struct drm_mode_modeinfo umode; > > /* Early return for no change. */ > - if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0) > - return 0; > + if (state->enable) { Hm I think this would be clearer if you go with if (state->enable == !!mode && (!mode || memcmp(&state->mode, mode, sizeof(*mode)) == 0)) return 0; But also somewhat a bikeshed. I'm also wondering whether we shouldn't just declare this a driver bug (it means enable and mode are already out of sync), but I guess hw state readout is special sometimes. Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > + if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0) > + return 0; > + } else { > + if (!mode) > + return 0; > + } > > drm_property_blob_put(state->mode_blob); > state->mode_blob = NULL; > -- > 2.23.0 >
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 0d466d3b0809..a3a6a8137af4 100644 --- a/drivers/gpu/drm/drm_atomic_uapi.c +++ b/drivers/gpu/drm/drm_atomic_uapi.c @@ -68,8 +68,13 @@ int drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state, struct drm_mode_modeinfo umode; /* Early return for no change. */ - if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0) - return 0; + if (state->enable) { + if (mode && memcmp(&state->mode, mode, sizeof(*mode)) == 0) + return 0; + } else { + if (!mode) + return 0; + } drm_property_blob_put(state->mode_blob); state->mode_blob = NULL;