[6/7] drm/atomic: Fix the early return in drm_atomic_set_mode_for_crtc()
diff mbox series

Message ID 20191115194204.22244-7-ville.syrjala@linux.intel.com
State New
Headers show
Series
  • drm: Random pile of core stuff
Related show

Commit Message

Ville Syrjälä Nov. 15, 2019, 7:42 p.m. UTC
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(-)

Comments

Daniel Vetter Nov. 19, 2019, 10:20 a.m. UTC | #1
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
>

Patch
diff mbox series

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;