[PATCH] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[PATCH] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[Ville Syrjala]

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>
---
 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

✓ Fi.CI.BAT: success for drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()
URL   : https://patchwork.freedesktop.org/series/52777/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5174 -> Patchwork_10867 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/52777/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_10867 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_ctx_create@basic-files:
      fi-icl-u2:          PASS -> DMESG-WARN (fdo#107724)
      fi-bsw-kefka:       PASS -> FAIL (fdo#108656)

    igt@kms_flip@basic-flip-vs-wf_vblank:
      fi-bsw-n3050:       PASS -> FAIL (fdo#100368)

    igt@kms_pipe_crc_basic@nonblocking-crc-pipe-a-frame-sequence:
      fi-byt-clapper:     PASS -> FAIL (fdo#107362, fdo#103191)

    
    ==== Possible fixes ====

    igt@gem_exec_suspend@basic-s3:
      fi-icl-u2:          DMESG-WARN (fdo#107724) -> PASS

    igt@i915_selftest@live_execlists:
      fi-apl-guc:         DMESG-WARN (fdo#108622) -> PASS

    igt@kms_flip@basic-flip-vs-dpms:
      fi-skl-6700hq:      DMESG-WARN (fdo#105998) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#105998 https://bugs.freedesktop.org/show_bug.cgi?id=105998
  fdo#107362 https://bugs.freedesktop.org/show_bug.cgi?id=107362
  fdo#107724 https://bugs.freedesktop.org/show_bug.cgi?id=107724
  fdo#108622 https://bugs.freedesktop.org/show_bug.cgi?id=108622
  fdo#108656 https://bugs.freedesktop.org/show_bug.cgi?id=108656


== Participating hosts (52 -> 45) ==

  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-icl-u3 fi-icl-y 


== Build changes ==

    * Linux: CI_DRM_5174 -> Patchwork_10867

  CI_DRM_5174: 0bfa7192170c039a271ebc27222b4b91516e73f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4722: fdcdfa1e220c5070072d5dac9523cd105e7406c2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10867: 3ee4095248db4aa5a00447be02bba09be878caf8 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

3ee4095248db drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10867/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[Patchwork]

== Series Details ==

Series: drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()
URL   : https://patchwork.freedesktop.org/series/52777/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_5174_full -> Patchwork_10867_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_10867_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_10867_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_10867_full:

  === IGT changes ===

    ==== Warnings ====

    igt@pm_rc6_residency@rc6-accuracy:
      shard-snb:          SKIP -> PASS

    
== Known issues ==

  Here are the changes found in Patchwork_10867_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_softpin@noreloc-s3:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108, fdo#107773)

    igt@kms_cursor_crc@cursor-128x42-onscreen:
      shard-glk:          PASS -> FAIL (fdo#103232)

    igt@kms_cursor_crc@cursor-256x256-suspend:
      shard-skl:          PASS -> FAIL (fdo#103232, fdo#103191)

    igt@kms_cursor_crc@cursor-size-change:
      shard-apl:          PASS -> FAIL (fdo#103232)

    igt@kms_fbcon_fbt@psr:
      shard-skl:          NOTRUN -> FAIL (fdo#107882)

    igt@kms_flip@2x-flip-vs-expired-vblank-interruptible:
      shard-glk:          PASS -> FAIL (fdo#105363)

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-blt:
      shard-glk:          PASS -> FAIL (fdo#103167) +5

    igt@kms_frontbuffer_tracking@fbcpsr-suspend:
      shard-skl:          PASS -> INCOMPLETE (fdo#104108, fdo#106978)

    igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
      shard-skl:          PASS -> FAIL (fdo#107815)

    igt@kms_plane_alpha_blend@pipe-c-alpha-7efc:
      shard-skl:          NOTRUN -> FAIL (fdo#108145, fdo#107815) +1

    igt@kms_universal_plane@universal-plane-pipe-a-functional:
      shard-apl:          PASS -> FAIL (fdo#103166) +1

    igt@kms_universal_plane@universal-plane-pipe-c-functional:
      shard-glk:          PASS -> FAIL (fdo#103166) +2

    igt@perf@blocking:
      shard-hsw:          PASS -> FAIL (fdo#102252)

    igt@pm_rpm@gem-mmap-cpu:
      shard-skl:          PASS -> INCOMPLETE (fdo#107807)

    
    ==== Possible fixes ====

    igt@drm_import_export@import-close-race-flink:
      shard-skl:          TIMEOUT (fdo#108667) -> PASS

    igt@gem_pwrite@big-gtt-backwards:
      shard-apl:          INCOMPLETE (fdo#103927) -> PASS

    igt@kms_cursor_crc@cursor-256x85-sliding:
      shard-apl:          FAIL (fdo#103232) -> PASS +1

    igt@kms_cursor_legacy@cursora-vs-flipa-toggle:
      shard-glk:          DMESG-WARN (fdo#106538, fdo#105763) -> PASS

    igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-gtt:
      shard-glk:          FAIL (fdo#103167) -> PASS

    igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
      shard-skl:          FAIL (fdo#107815) -> PASS

    igt@kms_plane_multiple@atomic-pipe-b-tiling-y:
      shard-glk:          FAIL (fdo#103166) -> PASS +1

    igt@kms_plane_multiple@atomic-pipe-c-tiling-yf:
      shard-apl:          FAIL (fdo#103166) -> PASS

    
  fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
  fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
  fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
  fdo#103191 https://bugs.freedesktop.org/show_bug.cgi?id=103191
  fdo#103232 https://bugs.freedesktop.org/show_bug.cgi?id=103232
  fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927
  fdo#104108 https://bugs.freedesktop.org/show_bug.cgi?id=104108
  fdo#105363 https://bugs.freedesktop.org/show_bug.cgi?id=105363
  fdo#105763 https://bugs.freedesktop.org/show_bug.cgi?id=105763
  fdo#106538 https://bugs.freedesktop.org/show_bug.cgi?id=106538
  fdo#106978 https://bugs.freedesktop.org/show_bug.cgi?id=106978
  fdo#107773 https://bugs.freedesktop.org/show_bug.cgi?id=107773
  fdo#107807 https://bugs.freedesktop.org/show_bug.cgi?id=107807
  fdo#107815 https://bugs.freedesktop.org/show_bug.cgi?id=107815
  fdo#107882 https://bugs.freedesktop.org/show_bug.cgi?id=107882
  fdo#108145 https://bugs.freedesktop.org/show_bug.cgi?id=108145
  fdo#108667 https://bugs.freedesktop.org/show_bug.cgi?id=108667


== Participating hosts (7 -> 6) ==

  Missing    (1): shard-iclb 


== Build changes ==

    * Linux: CI_DRM_5174 -> Patchwork_10867

  CI_DRM_5174: 0bfa7192170c039a271ebc27222b4b91516e73f6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4722: fdcdfa1e220c5070072d5dac9523cd105e7406c2 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_10867: 3ee4095248db4aa5a00447be02bba09be878caf8 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_10867/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[Daniel Vetter]

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

-- 
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

Re: [PATCH] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx