Re: [PATCH v2] drm/i915: Don't forget to mark crtc as inactive after disable

[Damien Lespiau <damien.lespiau@intel.com>]

Hi Patrik,

Please do Cc the patch author and reviewer when finding a regression,
they are superb candidates for the review, especially when they are busy
rewriting the display code.

On Wed, Jul 08, 2015 at 03:31:52PM +0200, Patrik Jakobsson wrote:
> Watermark calculations depend on the intel_crtc->active flag to be set
> properly. Suspend/resume is broken on SKL and we also get DDB mismatches
> without this patch.
> 
> The regression was introduced in:
> 
> commit eddfcbcdc27fbecb33bff098967bbdd7ca75bfa6
> Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Date:   Mon Jun 15 12:33:53 2015 +0200
> 
>     drm/i915: Update less state during modeset.
> 
>     No need to repeatedly call update_watermarks, or update_fbc.
>     Down to a single call to update_watermarks in .crtc_enable
> 
>     Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>     Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>     Tested-by(IVB): Matt Roper <matthew.d.roper@intel.com>
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> v2: Don't touch disable_shared_dpll()
> 
> Signed-off-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>

We do need to update the watermarks in disable() as well, to repartition
the DDB and update the watermarks based on the new allocation.

Maarten, Matt, I've no clue where you want the crtc state update, where
the atomic WM work is at, could you comment?

Thanks,

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3c2425f..b4f7a4f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5046,6 +5046,9 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  
>  		ironlake_fdi_pll_disable(intel_crtc);
>  	}
> +
> +	intel_crtc->active = false;
> +	intel_update_watermarks(crtc);
>  }
>  
>  static void haswell_crtc_disable(struct drm_crtc *crtc)
> @@ -5091,6 +5094,9 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		if (encoder->post_disable)
>  			encoder->post_disable(encoder);
> +
> +	intel_crtc->active = false;
> +	intel_update_watermarks(crtc);
>  }
>  
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> @@ -6155,6 +6161,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  
>  	if (!IS_GEN2(dev))
>  		intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, false);
> +
> +	intel_crtc->active = false;
> +	intel_update_watermarks(crtc);
>  }
>  
>  static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx