[Intel-gfx] [PATCH] drm/i915: Keep crtc_state->active in sync with enable.

[Daniel Vetter]

On Mon, May 11, 2015 at 10:45:15AM +0200, Maarten Lankhorst wrote:
> With the recent modeset internal rework, we wind up setting crtc_state->enable
> to false, but leave crtc_state->active as true following a
> drmModeSetCrtc(fb=0), which is incorrect.  This mismatch gets caught by
> drm_atomic_crtc_check() and causes subsequent atomic operations (such as plane
> updates while the CRTC is disabled) to fail.
> 
> Bisect points to
> 
>         commit dad9a7d6d96630182fb52aae7c3856e9e7285e13
>         Author: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
>         Date:   Tue Apr 21 17:13:19 2015 +0300
> 
>             drm/i915: Use atomic helpers for computing changed flags
> 
> as the commit that actually triggers the regression.
> 
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira at intel.com>
> Reported-and-Tested-by: Kenneth Graunke <kenneth at whitecape.org>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>

Yeah this makes more sense, in the legacy codepaths we always implicitly
smash the dpms state (i.e. state->active) to the logical pipe state (i.e.
state->enable). Queued for -next, thanks for the patch.
> ---
> The original patch for this is wrong, we should keep enable and active in sync when changed.
> Please revert "drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)".

I've squashed the revert right into this one, imo didn't make sense
separetely.
-Daniel

> 
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 22e6644f755e..e223209e2d81 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9868,7 +9868,7 @@ retry:
>  		goto fail;
>  	}
>  
> -	crtc_state->base.enable = true;
> +	crtc_state->base.active = crtc_state->base.enable = true;
>  
>  	if (!mode)
>  		mode = &load_detect_mode;
> @@ -9965,7 +9965,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
>  		connector_state->best_encoder = NULL;
>  		connector_state->crtc = NULL;
>  
> -		crtc_state->base.enable = false;
> +		crtc_state->base.enable = crtc_state->base.active = false;
>  
>  		ret = intel_modeset_setup_plane_state(state, crtc, NULL, NULL,
>  						      0, 0);
> @@ -12492,7 +12492,8 @@ void intel_crtc_restore_mode(struct drm_crtc *crtc)
>  			continue;
>  		}
>  
> -		crtc_state->base.enable = intel_crtc->new_enabled;
> +		crtc_state->base.active = crtc_state->base.enable =
> +			intel_crtc->new_enabled;
>  
>  		if (&intel_crtc->base == crtc)
>  			drm_mode_copy(&crtc_state->base.mode, &crtc->mode);
> @@ -12617,11 +12618,16 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	}
>  
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		bool has_connectors;
> +
>  		ret = drm_atomic_add_affected_connectors(state, crtc);
>  		if (ret)
>  			return ret;
>  
> -		crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc);
> +		has_connectors = !!drm_atomic_connectors_for_crtc(state, crtc);
> +		if (has_connectors != crtc_state->enable)
> +			crtc_state->enable =
> +			crtc_state->active = has_connectors;
>  	}
>  
>  	ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode,
> @@ -14598,6 +14604,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  
>  		WARN_ON(crtc->active);
>  		crtc->base.state->enable = false;
> +		crtc->base.state->active = false;
>  		crtc->base.enabled = false;
>  	}
>  
> @@ -14626,6 +14633,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
>  			      crtc->active ? "enabled" : "disabled");
>  
>  		crtc->base.state->enable = crtc->active;
> +		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  
>  		/* Because we only establish the connector -> encoder ->
> @@ -14764,6 +14772,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  								 crtc->config);
>  
>  		crtc->base.state->enable = crtc->active;
> +		crtc->base.state->active = crtc->active;
>  		crtc->base.enabled = crtc->active;
>  
>  		plane_state = to_intel_plane_state(primary->state);
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch