[Intel-gfx] [PATCH] drm/i915: Only write watermark registers if a pipe is active

[Ville Syrjälä]

On Thu, Mar 03, 2016 at 02:27:21PM -0800, Matt Roper wrote:
> If all pipes are off, then we may have entered runtime suspend and
> writing these registers will have no effect anyway.  When a pipe is
> re-enabled, it's crtc_enable will take care of programming appropriate
> watermark values.
> 
> Cc: arun.siluvery at linux.intel.com
> Cc: ville.syrjala at linux.intel.com
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94349
> Fixes: ed4a6a7ca853 ("drm/i915: Add two-stage ILK-style watermark programming (v11)")
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> I've still been unable to reproduce this defect on my SNB system; none of the
> suggested tests people have noted in Bugzilla have triggered it for me.  But
> based on the stack trace I'm guessing that the problem happens because we've
> disabled the last pipe and gone into runtime suspend before we get around to
> optimizing the watermarks.
> 
> I'm not super familiar with the details of runtime PM, so if there's a better
> way this should be handled, I'm open to suggestions.

This definitely feels like papering over the problem. Why is the modeset
code still calling watermark updates at the wrong time?

With my original two stage wm code most wm updates got kicked off from
the plane commits stage, and the only manual calls were in plaform
specific code just before a new pipe is enabled, or just after one is
disabled. Very little room for these sorts of errors in that scheme.
What we have currently isn't done like that and instead it seems to
me we've somehow sprinkled stuff all over some common codepaths which
apparently leads to these problems.

> 
>  drivers/gpu/drm/i915/intel_pm.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index f65e841..8c7fddf 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2802,8 +2802,6 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv,
>  		I915_WRITE(WM2_LP_ILK, results->wm_lp[1]);
>  	if (dirty & WM_DIRTY_LP(3) && previous->wm_lp[2] != results->wm_lp[2])
>  		I915_WRITE(WM3_LP_ILK, results->wm_lp[2]);
> -
> -	dev_priv->wm.hw = *results;
>  }
>  
>  bool ilk_disable_lp_wm(struct drm_device *dev)
> @@ -3731,7 +3729,10 @@ static void ilk_program_watermarks(struct drm_i915_private *dev_priv)
>  
>  	ilk_compute_wm_results(dev, best_lp_wm, partitioning, &results);
>  
> -	ilk_write_wm_values(dev_priv, &results);
> +	if (config.num_pipes_active > 0)
> +		ilk_write_wm_values(dev_priv, &results);
> +
> +	dev_priv->wm.hw = results;
>  }
>  
>  static void ilk_initial_watermarks(struct intel_crtc_state *cstate)
> -- 
> 2.1.4

-- 
Ville Syrjälä
ntel OTC