Re: [PATCH] drm/i915: Set crtc_state->active to false when CRTC is disabled (v2)

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Thu, 2015-05-07 at 14:31 -0700, Matt Roper wrote:
> With the recent modeset internal rework, we wind up setting
> crtc_state->enable to false, but leave crtc_state->active as true, 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@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.
> 
> v2: Update to alter in-flight state rather than already-committed state
>     (first version was accidentally based on a midpoint of Ander's
>     modeset rework series, before his final patches that add proper
>     state swapping to the legacy modeset path).
> 
> Cc: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Testcase: igt/kms_universal_plane
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c297cdc..981478a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12340,6 +12340,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc,
>  			continue;
>  
>  		if (!crtc_state->enable) {
> +			crtc_state->active = false;

I believe a more appropriate fix would be to set crtc_state->active when
we set crtc_state->enable to false in intel_modeset_stage_output_state()
and the HW state read out. But I think Maarten does that in his later
patch series, so as a stopgap solution this is fine.

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>


>  			intel_crtc_disable(crtc);
>  		} else if (crtc->state->enable) {
>  			intel_crtc_disable_planes(crtc);


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx