[Intel-gfx] [PATCH] drm/i915: Move init_clock_gating() back to where it was

[Chris Wilson]

Quoting Ville Syrjälä (2017-11-03 14:20:33)
> On Fri, Nov 03, 2017 at 01:27:55PM +0000, Chris Wilson wrote:
> > Quoting Ville Syrjala (2017-11-03 13:03:37)
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > 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()?
> 
> i915_gem_init() should be fine as far as the display is concerned
> at least, albeit a bit unexpected. Do we need to do that already in
> this patch, or a followup?

After this.

> > > 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?
> 
> The original problem was that this guy turned off the LP1+ watermarks
> after we'd already done the state readout in ilk_wm_get_hw_state(). So
> the state we had read out no longer matched the hardware state.

Ah. Dim light bulb flickers.
 
> To keep the software and hardware states in sync we just need to make
> sure ilk_init_lp_watermarks() is called before the readout. And the
> obvious thing to do then is to call it immediately before to the
> readout.

Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
-Chris