drm/i915: Move init_clock_gating() back to where it was

Commit Message

Ville Syrjälä Nov. 3, 2017, 1:03 p.m. UTC

From: Ville Syrjälä <[e-mail]>

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...

Cc: [e-mail]
Cc: Chris Wilson <[e-mail]>
Cc: Mark Janes <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Daniel Vetter <[e-mail]>
Cc: Joonas Lahtinen <[e-mail]>
Cc: Oscar Mateo <[e-mail]>
Cc: Mika Kuoppala <[e-mail]>
Reported-by: Mark Janes <[e-mail]>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103549
Fixes: b7048ea12fbb ("drm/i915: Do .init_clock_gating() earlier to avoid it clobbering watermarks")
References: https://lists.freedesktop.org/archives/intel-gfx/2017-November/145432.html
Signed-off-by: Ville Syrjälä <[e-mail]>
---
 drivers/gpu/drm/i915/intel_display.c |  5 +++--
 drivers/gpu/drm/i915/intel_pm.c      | 40 ++++++++++++++++--------------------
 2 files changed, 21 insertions(+), 24 deletions(-)

Comments

Chris Wilson Nov. 3, 2017, 1:27 p.m. UTC | #1

Quoting Ville Syrjala (2017-11-03 13:03:37)
> From: Ville Syrjälä <[e-mail]>
> 
> 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

Ville Syrjälä Nov. 3, 2017, 2:20 p.m. UTC | #2

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ä <[e-mail]>
> > 
> > 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?

>  
> > 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.

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.

Chris Wilson Nov. 3, 2017, 2:41 p.m. UTC | #3

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ä <[e-mail]>
> > > 
> > > 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 <[e-mail]>
-Chris

Chris Wilson Nov. 3, 2017, 2:58 p.m. UTC | #4

Quoting Ville Syrjala (2017-11-03 13:03:37)
> From: Ville Syrjälä <[e-mail]>
> 
> 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...
> 
> Cc: [e-mail]
> Cc: Chris Wilson <[e-mail]>
> Cc: Mark Janes <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Daniel Vetter <[e-mail]>
> Cc: Joonas Lahtinen <[e-mail]>
> Cc: Oscar Mateo <[e-mail]>
> Cc: Mika Kuoppala <[e-mail]>
> Reported-by: Mark Janes <[e-mail]>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103549
> Fixes: b7048ea12fbb ("drm/i915: Do .init_clock_gating() earlier to avoid it clobbering watermarks")
> References: https://lists.freedesktop.org/archives/intel-gfx/2017-November/145432.html
> Signed-off-by: Ville Syrjälä <[e-mail]>

Wrt to the reported CTS regressions, they are fixed.
Tested-by: Chris Wilson <[e-mail]>
-Chris

Chris Wilson Nov. 3, 2017, 5:08 p.m. UTC | #5

Quoting Patchwork (2017-11-03 17:05:20)
> == Series Details ==
> 
> Series: drm/i915: Move init_clock_gating() back to where it was
> URL   : https://patchwork.freedesktop.org/series/33124/
> State : failure
> 
> == Summary ==
> 
> Series 33124v1 drm/i915: Move init_clock_gating() back to where it was
> https://patchwork.freedesktop.org/api/1.0/series/33124/revisions/1/mbox/
> 
> Test chamelium:
>         Subgroup dp-crc-fast:
>                 pass       -> FAIL       (fi-kbl-7500u) fdo#102514
> Test gem_exec_reloc:
>         Subgroup basic-write-gtt-active:
>                 fail       -> PASS       (fi-gdg-551) fdo#102582
> Test kms_busy:
>         Subgroup basic-flip-b:
>                 fail       -> PASS       (fi-bwr-2160)
>                 pass       -> INCOMPLETE (fi-hsw-4770r)
>         Subgroup basic-flip-c:
>                 pass       -> INCOMPLETE (fi-hsw-4770)

Darn persistent. Third time lucky?
-Chris

Ville Syrjälä Nov. 3, 2017, 5:37 p.m. UTC | #6

On Fri, Nov 03, 2017 at 05:08:07PM +0000, Chris Wilson wrote:
> Quoting Patchwork (2017-11-03 17:05:20)
> > == Series Details ==
> > 
> > Series: drm/i915: Move init_clock_gating() back to where it was
> > URL   : https://patchwork.freedesktop.org/series/33124/
> > State : failure
> > 
> > == Summary ==
> > 
> > Series 33124v1 drm/i915: Move init_clock_gating() back to where it was
> > https://patchwork.freedesktop.org/api/1.0/series/33124/revisions/1/mbox/
> > 
> > Test chamelium:
> >         Subgroup dp-crc-fast:
> >                 pass       -> FAIL       (fi-kbl-7500u) fdo#102514
> > Test gem_exec_reloc:
> >         Subgroup basic-write-gtt-active:
> >                 fail       -> PASS       (fi-gdg-551) fdo#102582
> > Test kms_busy:
> >         Subgroup basic-flip-b:
> >                 fail       -> PASS       (fi-bwr-2160)
> >                 pass       -> INCOMPLETE (fi-hsw-4770r)
> >         Subgroup basic-flip-c:
> >                 pass       -> INCOMPLETE (fi-hsw-4770)
> 
> Darn persistent. Third time lucky?

Let's find out.

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 737de251d0f8..d4576c2ae31d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3676,6 +3676,7 @@  void intel_finish_reset(struct drm_i915_private *dev_priv)
 
 		intel_pps_unlock_regs_wa(dev_priv);
 		intel_modeset_init_hw(dev);
+		intel_init_clock_gating(dev_priv);
 
 		spin_lock_irq(&dev_priv->irq_lock);
 		if (dev_priv->display.hpd_irq_setup)
@@ -14365,8 +14366,6 @@  void intel_modeset_init_hw(struct drm_device *dev)
 	intel_update_cdclk(dev_priv);
 	intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDCLK");
 	dev_priv->cdclk.logical = dev_priv->cdclk.actual = dev_priv->cdclk.hw;
-
-	intel_init_clock_gating(dev_priv);
 }
 
 /*
@@ -15176,6 +15175,8 @@  void intel_modeset_gem_init(struct drm_device *dev)
 
 	intel_init_gt_powersave(dev_priv);
 
+	intel_init_clock_gating(dev_priv);
+
 	intel_setup_overlay(dev_priv);
 }
 
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);
+
 	for_each_crtc(dev, crtc)
 		ilk_pipe_wm_get_hw_state(crtc);
 
@@ -8213,18 +8231,6 @@  static void g4x_disable_trickle_feed(struct drm_i915_private *dev_priv)
 	}
 }
 
-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.
-	 */
-}
-
 static void ilk_init_clock_gating(struct drm_i915_private *dev_priv)
 {
 	uint32_t dspclk_gate = ILK_VRHUNIT_CLOCK_GATE_DISABLE;
@@ -8258,8 +8264,6 @@  static void ilk_init_clock_gating(struct drm_i915_private *dev_priv)
 		   (I915_READ(DISP_ARB_CTL) |
 		    DISP_FBC_WM_DIS));
 
-	ilk_init_lp_watermarks(dev_priv);
-
 	/*
 	 * Based on the document from hardware guys the following bits
 	 * should be set unconditionally in order to enable FBC.
@@ -8372,8 +8376,6 @@  static void gen6_init_clock_gating(struct drm_i915_private *dev_priv)
 	I915_WRITE(GEN6_GT_MODE,
 		   _MASKED_FIELD(GEN6_WIZ_HASHING_MASK, GEN6_WIZ_HASHING_16x4));
 
-	ilk_init_lp_watermarks(dev_priv);
-
 	I915_WRITE(CACHE_MODE_0,
 		   _MASKED_BIT_DISABLE(CM0_STC_EVICT_DISABLE_LRA_SNB));
 
@@ -8600,8 +8602,6 @@  static void bdw_init_clock_gating(struct drm_i915_private *dev_priv)
 						 I915_GTT_PAGE_SIZE_2M);
 	enum pipe pipe;
 
-	ilk_init_lp_watermarks(dev_priv);
-
 	/* WaSwitchSolVfFArbitrationPriority:bdw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
 
@@ -8652,8 +8652,6 @@  static void bdw_init_clock_gating(struct drm_i915_private *dev_priv)
 
 static void hsw_init_clock_gating(struct drm_i915_private *dev_priv)
 {
-	ilk_init_lp_watermarks(dev_priv);
-
 	/* L3 caching of data atomics doesn't work -- disable it. */
 	I915_WRITE(HSW_SCRATCH1, HSW_SCRATCH1_L3_DATA_ATOMICS_DISABLE);
 	I915_WRITE(HSW_ROW_CHICKEN3,
@@ -8708,8 +8706,6 @@  static void ivb_init_clock_gating(struct drm_i915_private *dev_priv)
 {
 	uint32_t snpcr;
 
-	ilk_init_lp_watermarks(dev_priv);
-
 	I915_WRITE(ILK_DSPCLK_GATE_D, ILK_VRHUNIT_CLOCK_GATE_DISABLE);
 
 	/* WaDisableEarlyCull:ivb */