Re: [Intel-gfx] [PATCH] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()
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ä > > > > 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ä > > 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(>mode, mode, sizeof(*mode)) == 0) > > - return 0; > > + if (state->enable) { > > + if (mode && memcmp(>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 -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()
On Tue, Nov 20, 2018 at 07:55:42PM +0200, Ville Syrjala wrote: > From: Ville Syrjälä > > 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ä 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(>mode, mode, sizeof(*mode)) == 0) > - return 0; > + if (state->enable) { > + if (mode && memcmp(>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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()
From: Ville Syrjälä 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ä --- 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(>mode, mode, sizeof(*mode)) == 0) - return 0; + if (state->enable) { + if (mode && memcmp(>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