| Message ID | 20181120175542.26083-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc() | expand |
On Tue, Nov 20, 2018 at 07:55:42PM +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. I suppose that should never > happen but better safe than sorry. > > Additionally the early return will not be taken if we're trying to > disable an already disable crtc. While that is not actually harmful > it is inconsistent, so let's handle that case as well. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Do we have an igt for this? I.e. trying to set a all-0 mode for a disabled CRTC and seeing what happens ... Patch itself looks fine, has my r-b if the igt materializes. -Daniel > --- > 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 86ac33922b09..ed0ea82e8a1d 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; > -- > 2.18.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 21, 2018 at 10:59:56AM +0100, Daniel Vetter wrote: > On Tue, Nov 20, 2018 at 07:55:42PM +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. I suppose that should never > > happen but better safe than sorry. > > > > Additionally the early return will not be taken if we're trying to > > disable an already disable crtc. While that is not actually harmful > > it is inconsistent, so let's handle that case as well. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Do we have an igt for this? I.e. trying to set a all-0 mode for a disabled > CRTC and seeing what happens ... It should get rejected by drm_mode_convert_umode()->drm_mode_validate_basic() so presumably you shouldn't be able to do this from userspace. In kernel users could bypass that check and sneak something like this in however. But I guess having an igt to verify that we do indeed reject bad modes would a decent idea. Looks like currently we only have kms_invalid_dotclock. > > Patch itself looks fine, has my r-b if the igt materializes. > -Daniel > > > --- > > 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 86ac33922b09..ed0ea82e8a1d 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; > > -- > > 2.18.1 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c index 86ac33922b09..ed0ea82e8a1d 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;