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

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

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@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...

Cc: stable@vger.kernel.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mark Janes <mark.a.janes@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reported-by: Mark Janes <mark.a.janes@intel.com>
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ä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  5 +++--
 drivers/gpu/drm/i915/intel_pm.c      | 40 ++++++++++++++++--------------------
 2 files changed, 21 insertions(+), 24 deletions(-)

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 */
-- 
2.13.6

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

[Chris Wilson]

Quoting Ville Syrjala (2017-11-03 13:03:37)
> From: Ville Syrjälä <ville.syrjala@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()?
 
> 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

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

[Ville Syrjälä]

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@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?

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

-- 
Ville Syrj�l�
Intel OTC

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

[Chris Wilson]

Quoting Ville Syrjala (2017-11-03 13:03:37)
> From: Ville Syrjälä <ville.syrjala@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...
> 
> Cc: stable@vger.kernel.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mark Janes <mark.a.janes@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reported-by: Mark Janes <mark.a.janes@intel.com>
> 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ä <ville.syrjala@linux.intel.com>

Wrt to the reported CTS regressions, they are fixed.
Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

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

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@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...

v2: Keep WaRsPkgCStateDisplayPMReq:hsw early as it really must
    be done before all planes might get disabled.

Cc: stable@vger.kernel.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mark Janes <mark.a.janes@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reported-by: Mark Janes <mark.a.janes@intel.com>
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ä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++--
 drivers/gpu/drm/i915/intel_pm.c      | 44 +++++++++++++++---------------------
 2 files changed, 30 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 737de251d0f8..8174392acc18 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);
 }
 
 /*
@@ -15079,6 +15078,15 @@ intel_modeset_setup_hw_state(struct drm_device *dev,
 	struct intel_encoder *encoder;
 	int i;
 
+	if (IS_HASWELL(dev_priv)) {
+		/*
+		 * WaRsPkgCStateDisplayPMReq:hsw
+		 * System hang if this isn't done before disabling all planes!
+		 */
+		I915_WRITE(CHICKEN_PAR1_1,
+			   I915_READ(CHICKEN_PAR1_1) | FORCE_ARB_IDLE_PLANES);
+	}
+
 	intel_modeset_readout_hw_state(dev);
 
 	/* HW state is read out, now we need to sanitize this mess. */
@@ -15176,6 +15184,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..b712ee30a06c 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,
@@ -8697,10 +8695,6 @@ static void hsw_init_clock_gating(struct drm_i915_private *dev_priv)
 	/* WaSwitchSolVfFArbitrationPriority:hsw */
 	I915_WRITE(GAM_ECOCHK, I915_READ(GAM_ECOCHK) | HSW_ECOCHK_ARB_PRIO_SOL);
 
-	/* WaRsPkgCStateDisplayPMReq:hsw */
-	I915_WRITE(CHICKEN_PAR1_1,
-		   I915_READ(CHICKEN_PAR1_1) | FORCE_ARB_IDLE_PLANES);
-
 	lpt_init_clock_gating(dev_priv);
 }
 
@@ -8708,8 +8702,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 */
-- 
2.13.6

Re: [PATCH v2] drm/i915: Move init_clock_gating() back to where it was

[Chris Wilson]

Quoting Ville Syrjala (2017-11-08 13:35:55)
> From: Ville Syrjälä <ville.syrjala@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...
> 
> v2: Keep WaRsPkgCStateDisplayPMReq:hsw early as it really must
>     be done before all planes might get disabled.
> 
> Cc: stable@vger.kernel.org
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mark Janes <mark.a.janes@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> Reported-by: Mark Janes <mark.a.janes@intel.com>
> 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ä <ville.syrjala@linux.intel.com>

CTS remains fixed,
Tested-by: Chris Wilson <chris@chris-wilson.co.uk>

I know of no reason why this should work in this order,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
(but I didn't know about the v2 requirement either!)
-Chris

Re: [PATCH v2] drm/i915: Move init_clock_gating() back to where it was

[Chris Wilson]

Quoting Chris Wilson (2017-11-08 16:18:27)
> Quoting Ville Syrjala (2017-11-08 13:35:55)
> > From: Ville Syrjälä <ville.syrjala@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...
> > 
> > v2: Keep WaRsPkgCStateDisplayPMReq:hsw early as it really must
> >     be done before all planes might get disabled.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mark Janes <mark.a.janes@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Reported-by: Mark Janes <mark.a.janes@intel.com>
> > 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ä <ville.syrjala@linux.intel.com>
> 
> CTS remains fixed,
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I know of no reason why this should work in this order,
s/should work/should not/work ;)

> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> (but I didn't know about the v2 requirement either!)
> -Chris

Re: [PATCH v2] drm/i915: Move init_clock_gating() back to where it was

[Ville Syrjälä]

On Wed, Nov 08, 2017 at 04:18:27PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjala (2017-11-08 13:35:55)
> > From: Ville Syrj�l� <ville.syrjala@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...
> > 
> > v2: Keep WaRsPkgCStateDisplayPMReq:hsw early as it really must
> >     be done before all planes might get disabled.
> > 
> > Cc: stable@vger.kernel.org
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mark Janes <mark.a.janes@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Oscar Mateo <oscar.mateo@intel.com>
> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> > Reported-by: Mark Janes <mark.a.janes@intel.com>
> > 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� <ville.syrjala@linux.intel.com>
> 
> CTS remains fixed,
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I know of no reason why this should work in this order,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> (but I didn't know about the v2 requirement either!)

It's somewhat baffling how this stuff managed to work before I
moved .init_clock_gating() to happen earlier. AFAICS that plane
disabling vs. the chicken bit should have bitten us back then as
well.

Patch pushed to dinq. Thanks for doing all the heavy lifting on
this one.

-- 
Ville Syrj�l�
Intel OTC