[Linux-stable-mirror] [PATCH v2] drm/i915: Move init_clock_gating() back to where it was
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 */
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
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
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.
linux-stable-mirror@lists.linaro.org
-
Chris Wilson -
Ville Syrjala -
Ville Syrjälä