Quoting Ville Syrjala (2017-11-03 13:03:37)
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> Apparently setting up a bunch of GT registers before we've properly
> initialized the rest of the GT hardware leads to these setting being
> lost. So looks like I broke HSW with commit b7048ea12fbb ("drm/i915:
> Do .init_clock_gating() earlier to avoid it clobbering watermarks")
> by doing init_clock_gating() too early. This should actually affect
> other platforms as well, but apparently not to such a great degree.
>
> What I was ultimately after in that commit was to move the
> ilk_init_lp_watermarks() call earlier. So let's undo the damage and
> move init_clock_gating() back to where it was, and call
> ilk_init_lp_watermarks() just before the watermark state readout.
>
> This highlights how fragile and messed up our init order really is.
> I wonder why we even initialize the display before gem. The opposite
> order would make much more sense to me...
Indeed this will cause some fun for me momentarily as I will have to
move init_clock_gating() to i915_gem_init(). We can only wish for that
magic wand to be waved sooner.
Does that make sense, or will I have to start carving up
init_clock_gating()?
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 07118c0b69d3..352a6739ed70 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5754,12 +5754,30 @@ void vlv_wm_sanitize(struct drm_i915_private *dev_priv)
> mutex_unlock(&dev_priv->wm.wm_mutex);
> }
>
> +/*
> + * FIXME should probably kill this and improve
> + * the real watermark readout/sanitation instead
> + */
> +static void ilk_init_lp_watermarks(struct drm_i915_private *dev_priv)
> +{
> + I915_WRITE(WM3_LP_ILK, I915_READ(WM3_LP_ILK) & ~WM1_LP_SR_EN);
> + I915_WRITE(WM2_LP_ILK, I915_READ(WM2_LP_ILK) & ~WM1_LP_SR_EN);
> + I915_WRITE(WM1_LP_ILK, I915_READ(WM1_LP_ILK) & ~WM1_LP_SR_EN);
> +
> + /*
> + * Don't touch WM1S_LP_EN here.
> + * Doing so could cause underruns.
> + */
> +}
> +
> void ilk_wm_get_hw_state(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = to_i915(dev);
> struct ilk_wm_values *hw = &dev_priv->wm.hw;
> struct drm_crtc *crtc;
>
> + ilk_init_lp_watermarks(dev_priv);
Wasn't expecting this, but the rest lgtm. Could you explain you decision
to put it here in a bit more detail?
-Chris