[PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Lucas De Marchi]

Wa Display #1183 was recently added to workaround
"Failures when enabling DPLL0 with eDP link rate 2.16
or 4.32 GHz and CD clock frequency 308.57 or 617.14 MHz
(CDCLK_CTL CD Frequency Select 10b or 11b) used in this
 enabling or in previous enabling."

This Workaround was designed to minimize the impact only
to save the bad case with that link rates. But HW engineers
indicated that it should be safe to apply broadly. Although
they were expecting the DPLL0 link rate to be unchanged on
runtime.

We need to cover 2 cases: when we are in fact enabling DPLL0
and when we are just changing the frequency. The workaround
for those cases are similar but different enough to have them
done in different places.

This is based on previous patch by Rodrigo Vivi with suggestions
from Ville Syrjälä.

Cc: Arthur J Runyan <arthur.j.runyan@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
---

I tried to test this but both on SKL and KBL that I have the bug that requires
the WA isn't triggered. 

 drivers/gpu/drm/i915/i915_reg.h         |  2 ++
 drivers/gpu/drm/i915/intel_cdclk.c      | 51 ++++++++++++++++++++++++++-------
 drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++++
 3 files changed, 53 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 6ef33422f762..a32d8200bb47 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6981,6 +6981,7 @@ enum {
 #define  RESET_PCH_HANDSHAKE_ENABLE	(1<<4)
 
 #define GEN8_CHICKEN_DCPR_1		_MMIO(0x46430)
+#define   SKL_SELECT_ALTERNATE_DC_EXIT	(1<<30)
 #define   MASK_WAKEMEM			(1<<13)
 
 #define SKL_DFSM			_MMIO(0x51000)
@@ -8535,6 +8536,7 @@ enum skl_power_gate {
 #define  BXT_CDCLK_CD2X_DIV_SEL_2	(2<<22)
 #define  BXT_CDCLK_CD2X_DIV_SEL_4	(3<<22)
 #define  BXT_CDCLK_CD2X_PIPE(pipe)	((pipe)<<20)
+#define  CDCLK_DIVMUX_CD_OVERRIDE	(1<<19)
 #define  BXT_CDCLK_CD2X_PIPE_NONE	BXT_CDCLK_CD2X_PIPE(3)
 #define  BXT_CDCLK_SSA_PRECHARGE_ENABLE	(1<<16)
 #define  CDCLK_FREQ_DECIMAL_MASK	(0x7ff)
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index e8884c2ade98..5e6fc2602711 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -929,17 +929,18 @@ static void skl_set_preferred_cdclk_vco(struct drm_i915_private *dev_priv,
 		intel_update_max_cdclk(dev_priv);
 }
 
-static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
+static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco,
+			     u32 freq_select, u32 cdclk)
 {
-	int min_cdclk = skl_calc_cdclk(0, vco);
-	u32 val;
+	u32 val, cdctl;
 
 	WARN_ON(vco != 8100000 && vco != 8640000);
 
-	/* select the minimum CDCLK before enabling DPLL 0 */
-	val = CDCLK_FREQ_337_308 | skl_cdclk_decimal(min_cdclk);
-	I915_WRITE(CDCLK_CTL, val);
-	POSTING_READ(CDCLK_CTL);
+	/* Wa Display #1183: skl,kbl,cfl */
+	cdctl = I915_READ(CDCLK_CTL);
+	cdctl &= ~(CDCLK_FREQ_SEL_MASK | CDCLK_FREQ_DECIMAL_MASK);
+	cdctl |= freq_select | skl_cdclk_decimal(cdclk);
+	I915_WRITE(CDCLK_CTL, cdctl);
 
 	/*
 	 * We always enable DPLL0 with the lowest link rate possible, but still
@@ -965,6 +966,10 @@ static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
 	I915_WRITE(DPLL_CTRL1, val);
 	POSTING_READ(DPLL_CTRL1);
 
+	/* Wa Display #1183: skl,kbl,cfl */
+	cdctl |= CDCLK_DIVMUX_CD_OVERRIDE;
+	I915_WRITE(CDCLK_CTL, cdctl);
+
 	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) | LCPLL_PLL_ENABLE);
 
 	if (intel_wait_for_register(dev_priv,
@@ -972,6 +977,17 @@ static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
 				    5))
 		DRM_ERROR("DPLL0 not locked\n");
 
+	/* Wa Display #1183: skl,kbl,cfl */
+	cdctl &= ~CDCLK_FREQ_SEL_MASK;
+	I915_WRITE(CDCLK_CTL, cdctl);
+
+	cdctl |= freq_select;
+	I915_WRITE(CDCLK_CTL, cdctl);
+
+	cdctl &= ~CDCLK_DIVMUX_CD_OVERRIDE;
+	I915_WRITE(CDCLK_CTL, cdctl);
+	POSTING_READ(CDCLK_CTL);
+
 	dev_priv->cdclk.hw.vco = vco;
 
 	/* We'll want to keep using the current vco from now on. */
@@ -1037,10 +1053,25 @@ static void skl_set_cdclk(struct drm_i915_private *dev_priv,
 		skl_dpll0_disable(dev_priv);
 
 	if (dev_priv->cdclk.hw.vco != vco)
-		skl_dpll0_enable(dev_priv, vco);
+		skl_dpll0_enable(dev_priv, vco, freq_select, cdclk);
+	else {
+		u32 cdctl = I915_READ(CDCLK_CTL);
 
-	I915_WRITE(CDCLK_CTL, freq_select | skl_cdclk_decimal(cdclk));
-	POSTING_READ(CDCLK_CTL);
+		/* Wa Display #1183: skl,kbl,cfl */
+		cdctl |= CDCLK_DIVMUX_CD_OVERRIDE;
+		I915_WRITE(CDCLK_CTL, cdctl);
+
+		cdctl &= ~CDCLK_FREQ_SEL_MASK;
+		I915_WRITE(CDCLK_CTL, cdctl);
+
+		cdctl &= ~CDCLK_FREQ_DECIMAL_MASK;
+		cdctl |= freq_select | skl_cdclk_decimal(cdclk);
+		I915_WRITE(CDCLK_CTL, cdctl);
+
+		cdctl &= ~CDCLK_DIVMUX_CD_OVERRIDE;
+		I915_WRITE(CDCLK_CTL, cdctl);
+		POSTING_READ(CDCLK_CTL);
+	}
 
 	/* inform PCU of the change */
 	mutex_lock(&dev_priv->pcu_lock);
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 8315499452dc..35796fa8e6b4 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -598,6 +598,11 @@ void gen9_enable_dc5(struct drm_i915_private *dev_priv)
 
 	DRM_DEBUG_KMS("Enabling DC5\n");
 
+	/* Wa Display #1183: skl,kbl,cfl */
+	if (IS_GEN9_BC(dev_priv))
+		I915_WRITE(GEN8_CHICKEN_DCPR_1, I915_READ(GEN8_CHICKEN_DCPR_1) |
+			   SKL_SELECT_ALTERNATE_DC_EXIT);
+
 	gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
 }
 
@@ -625,6 +630,11 @@ void skl_disable_dc6(struct drm_i915_private *dev_priv)
 {
 	DRM_DEBUG_KMS("Disabling DC6\n");
 
+	/* Wa Display #1183: skl,kbl,cfl */
+	if (IS_GEN9_BC(dev_priv))
+		I915_WRITE(GEN8_CHICKEN_DCPR_1, I915_READ(GEN8_CHICKEN_DCPR_1) |
+			   SKL_SELECT_ALTERNATE_DC_EXIT);
+
 	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 }
 
-- 
2.14.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Patchwork]

== Series Details ==

Series: drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl
URL   : https://patchwork.freedesktop.org/series/33508/
State : success

== Summary ==

Series 33508v1 drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl
https://patchwork.freedesktop.org/api/1.0/series/33508/revisions/1/mbox/

Test gem_exec_reloc:
        Subgroup basic-gtt-active:
                fail       -> PASS       (fi-gdg-551) fdo#102582 +2

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:449s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:460s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:379s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:532s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:275s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:500s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:494s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:483s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:434s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:264s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:542s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:438s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:424s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:486s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:459s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:485s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:532s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:540s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:566s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:457s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:543s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:567s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:521s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:499s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:460s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:563s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:424s
Blacklisted hosts:
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:551s
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:490s

65dc54b704d3ee0486f9f5b11f00c28973f783a2 drm-tip: 2017y-11m-09d-08h-53m-46s UTC integration manifest
8770aa4699a2 drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7033/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Ville Syrjälä]

On Thu, Nov 09, 2017 at 02:58:04AM -0800, Lucas De Marchi wrote:
> Wa Display #1183 was recently added to workaround
> "Failures when enabling DPLL0 with eDP link rate 2.16
> or 4.32 GHz and CD clock frequency 308.57 or 617.14 MHz
> (CDCLK_CTL CD Frequency Select 10b or 11b) used in this
>  enabling or in previous enabling."
> 
> This Workaround was designed to minimize the impact only
> to save the bad case with that link rates. But HW engineers
> indicated that it should be safe to apply broadly. Although
> they were expecting the DPLL0 link rate to be unchanged on
> runtime.
> 
> We need to cover 2 cases: when we are in fact enabling DPLL0
> and when we are just changing the frequency. The workaround
> for those cases are similar but different enough to have them
> done in different places.
> 
> This is based on previous patch by Rodrigo Vivi with suggestions
> from Ville Syrjälä.

Still doesn't look like what I suggested.

> 
> Cc: Arthur J Runyan <arthur.j.runyan@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
> 
> I tried to test this but both on SKL and KBL that I have the bug that requires
> the WA isn't triggered. 
> 
>  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
>  drivers/gpu/drm/i915/intel_cdclk.c      | 51 ++++++++++++++++++++++++++-------
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++++
>  3 files changed, 53 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 6ef33422f762..a32d8200bb47 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6981,6 +6981,7 @@ enum {
>  #define  RESET_PCH_HANDSHAKE_ENABLE	(1<<4)
>  
>  #define GEN8_CHICKEN_DCPR_1		_MMIO(0x46430)
> +#define   SKL_SELECT_ALTERNATE_DC_EXIT	(1<<30)
>  #define   MASK_WAKEMEM			(1<<13)
>  
>  #define SKL_DFSM			_MMIO(0x51000)
> @@ -8535,6 +8536,7 @@ enum skl_power_gate {
>  #define  BXT_CDCLK_CD2X_DIV_SEL_2	(2<<22)
>  #define  BXT_CDCLK_CD2X_DIV_SEL_4	(3<<22)
>  #define  BXT_CDCLK_CD2X_PIPE(pipe)	((pipe)<<20)
> +#define  CDCLK_DIVMUX_CD_OVERRIDE	(1<<19)
>  #define  BXT_CDCLK_CD2X_PIPE_NONE	BXT_CDCLK_CD2X_PIPE(3)
>  #define  BXT_CDCLK_SSA_PRECHARGE_ENABLE	(1<<16)
>  #define  CDCLK_FREQ_DECIMAL_MASK	(0x7ff)
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index e8884c2ade98..5e6fc2602711 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -929,17 +929,18 @@ static void skl_set_preferred_cdclk_vco(struct drm_i915_private *dev_priv,
>  		intel_update_max_cdclk(dev_priv);
>  }
>  
> -static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
> +static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco,
> +			     u32 freq_select, u32 cdclk)
>  {
> -	int min_cdclk = skl_calc_cdclk(0, vco);
> -	u32 val;
> +	u32 val, cdctl;
>  
>  	WARN_ON(vco != 8100000 && vco != 8640000);
>  
> -	/* select the minimum CDCLK before enabling DPLL 0 */
> -	val = CDCLK_FREQ_337_308 | skl_cdclk_decimal(min_cdclk);
> -	I915_WRITE(CDCLK_CTL, val);
> -	POSTING_READ(CDCLK_CTL);
> +	/* Wa Display #1183: skl,kbl,cfl */
> +	cdctl = I915_READ(CDCLK_CTL);
> +	cdctl &= ~(CDCLK_FREQ_SEL_MASK | CDCLK_FREQ_DECIMAL_MASK);
> +	cdctl |= freq_select | skl_cdclk_decimal(cdclk);
> +	I915_WRITE(CDCLK_CTL, cdctl);
>  
>  	/*
>  	 * We always enable DPLL0 with the lowest link rate possible, but still
> @@ -965,6 +966,10 @@ static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
>  	I915_WRITE(DPLL_CTRL1, val);
>  	POSTING_READ(DPLL_CTRL1);
>  
> +	/* Wa Display #1183: skl,kbl,cfl */
> +	cdctl |= CDCLK_DIVMUX_CD_OVERRIDE;
> +	I915_WRITE(CDCLK_CTL, cdctl);
> +
>  	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) | LCPLL_PLL_ENABLE);
>  
>  	if (intel_wait_for_register(dev_priv,
> @@ -972,6 +977,17 @@ static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
>  				    5))
>  		DRM_ERROR("DPLL0 not locked\n");
>  
> +	/* Wa Display #1183: skl,kbl,cfl */
> +	cdctl &= ~CDCLK_FREQ_SEL_MASK;
> +	I915_WRITE(CDCLK_CTL, cdctl);
> +
> +	cdctl |= freq_select;
> +	I915_WRITE(CDCLK_CTL, cdctl);
> +
> +	cdctl &= ~CDCLK_DIVMUX_CD_OVERRIDE;
> +	I915_WRITE(CDCLK_CTL, cdctl);
> +	POSTING_READ(CDCLK_CTL);
> +
>  	dev_priv->cdclk.hw.vco = vco;
>  
>  	/* We'll want to keep using the current vco from now on. */
> @@ -1037,10 +1053,25 @@ static void skl_set_cdclk(struct drm_i915_private *dev_priv,
>  		skl_dpll0_disable(dev_priv);
>  
>  	if (dev_priv->cdclk.hw.vco != vco)
> -		skl_dpll0_enable(dev_priv, vco);
> +		skl_dpll0_enable(dev_priv, vco, freq_select, cdclk);
> +	else {
> +		u32 cdctl = I915_READ(CDCLK_CTL);
>  
> -	I915_WRITE(CDCLK_CTL, freq_select | skl_cdclk_decimal(cdclk));
> -	POSTING_READ(CDCLK_CTL);
> +		/* Wa Display #1183: skl,kbl,cfl */
> +		cdctl |= CDCLK_DIVMUX_CD_OVERRIDE;
> +		I915_WRITE(CDCLK_CTL, cdctl);
> +
> +		cdctl &= ~CDCLK_FREQ_SEL_MASK;
> +		I915_WRITE(CDCLK_CTL, cdctl);
> +
> +		cdctl &= ~CDCLK_FREQ_DECIMAL_MASK;
> +		cdctl |= freq_select | skl_cdclk_decimal(cdclk);
> +		I915_WRITE(CDCLK_CTL, cdctl);
> +
> +		cdctl &= ~CDCLK_DIVMUX_CD_OVERRIDE;
> +		I915_WRITE(CDCLK_CTL, cdctl);
> +		POSTING_READ(CDCLK_CTL);
> +	}
>  
>  	/* inform PCU of the change */
>  	mutex_lock(&dev_priv->pcu_lock);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 8315499452dc..35796fa8e6b4 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -598,6 +598,11 @@ void gen9_enable_dc5(struct drm_i915_private *dev_priv)
>  
>  	DRM_DEBUG_KMS("Enabling DC5\n");
>  
> +	/* Wa Display #1183: skl,kbl,cfl */
> +	if (IS_GEN9_BC(dev_priv))
> +		I915_WRITE(GEN8_CHICKEN_DCPR_1, I915_READ(GEN8_CHICKEN_DCPR_1) |
> +			   SKL_SELECT_ALTERNATE_DC_EXIT);
> +
>  	gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
>  }
>  
> @@ -625,6 +630,11 @@ void skl_disable_dc6(struct drm_i915_private *dev_priv)
>  {
>  	DRM_DEBUG_KMS("Disabling DC6\n");
>  
> +	/* Wa Display #1183: skl,kbl,cfl */
> +	if (IS_GEN9_BC(dev_priv))
> +		I915_WRITE(GEN8_CHICKEN_DCPR_1, I915_READ(GEN8_CHICKEN_DCPR_1) |
> +			   SKL_SELECT_ALTERNATE_DC_EXIT);
> +
>  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>  }
>  
> -- 
> 2.14.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Patchwork]

== Series Details ==

Series: drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl
URL   : https://patchwork.freedesktop.org/series/33508/
State : success

== Summary ==

Test perf:
        Subgroup blocking:
                fail       -> PASS       (shard-hsw) fdo#102252
Test kms_flip:
        Subgroup plain-flip-ts-check:
                pass       -> FAIL       (shard-hsw) fdo#100368

fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368

shard-hsw        total:2540 pass:1432 dwarn:0   dfail:0   fail:11  skip:1097 time:9342s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7033/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Lucas De Marchi]

On Thu, Nov 9, 2017 at 5:11 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Nov 09, 2017 at 02:58:04AM -0800, Lucas De Marchi wrote:
>> Wa Display #1183 was recently added to workaround
>> "Failures when enabling DPLL0 with eDP link rate 2.16
>> or 4.32 GHz and CD clock frequency 308.57 or 617.14 MHz
>> (CDCLK_CTL CD Frequency Select 10b or 11b) used in this
>>  enabling or in previous enabling."
>>
>> This Workaround was designed to minimize the impact only
>> to save the bad case with that link rates. But HW engineers
>> indicated that it should be safe to apply broadly. Although
>> they were expecting the DPLL0 link rate to be unchanged on
>> runtime.
>>
>> We need to cover 2 cases: when we are in fact enabling DPLL0
>> and when we are just changing the frequency. The workaround
>> for those cases are similar but different enough to have them
>> done in different places.
>>
>> This is based on previous patch by Rodrigo Vivi with suggestions
>> from Ville Syrjälä.
>
> Still doesn't look like what I suggested.

I agree with your suggestion of moving stuff to skl_set_cdclk() to
cover the case in which
vco isn't changing. However see the paragraph I added above on why I
need to do it
differently. In short: the sequence on the WA for enabling and
updating cdclck are different,
with some code duplication unfortunately. I don't see you covering
that case in your
suggestion. Have I missed anything?


Lucas De Marchi

>
>>
>> Cc: Arthur J Runyan <arthur.j.runyan@intel.com>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
>> ---
>>
>> I tried to test this but both on SKL and KBL that I have the bug that requires
>> the WA isn't triggered.
>>
>>  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
>>  drivers/gpu/drm/i915/intel_cdclk.c      | 51 ++++++++++++++++++++++++++-------
>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 10 +++++++
>>  3 files changed, 53 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 6ef33422f762..a32d8200bb47 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -6981,6 +6981,7 @@ enum {
>>  #define  RESET_PCH_HANDSHAKE_ENABLE  (1<<4)
>>
>>  #define GEN8_CHICKEN_DCPR_1          _MMIO(0x46430)
>> +#define   SKL_SELECT_ALTERNATE_DC_EXIT       (1<<30)
>>  #define   MASK_WAKEMEM                       (1<<13)
>>
>>  #define SKL_DFSM                     _MMIO(0x51000)
>> @@ -8535,6 +8536,7 @@ enum skl_power_gate {
>>  #define  BXT_CDCLK_CD2X_DIV_SEL_2    (2<<22)
>>  #define  BXT_CDCLK_CD2X_DIV_SEL_4    (3<<22)
>>  #define  BXT_CDCLK_CD2X_PIPE(pipe)   ((pipe)<<20)
>> +#define  CDCLK_DIVMUX_CD_OVERRIDE    (1<<19)
>>  #define  BXT_CDCLK_CD2X_PIPE_NONE    BXT_CDCLK_CD2X_PIPE(3)
>>  #define  BXT_CDCLK_SSA_PRECHARGE_ENABLE      (1<<16)
>>  #define  CDCLK_FREQ_DECIMAL_MASK     (0x7ff)
>> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
>> index e8884c2ade98..5e6fc2602711 100644
>> --- a/drivers/gpu/drm/i915/intel_cdclk.c
>> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
>> @@ -929,17 +929,18 @@ static void skl_set_preferred_cdclk_vco(struct drm_i915_private *dev_priv,
>>               intel_update_max_cdclk(dev_priv);
>>  }
>>
>> -static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
>> +static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco,
>> +                          u32 freq_select, u32 cdclk)
>>  {
>> -     int min_cdclk = skl_calc_cdclk(0, vco);
>> -     u32 val;
>> +     u32 val, cdctl;
>>
>>       WARN_ON(vco != 8100000 && vco != 8640000);
>>
>> -     /* select the minimum CDCLK before enabling DPLL 0 */
>> -     val = CDCLK_FREQ_337_308 | skl_cdclk_decimal(min_cdclk);
>> -     I915_WRITE(CDCLK_CTL, val);
>> -     POSTING_READ(CDCLK_CTL);
>> +     /* Wa Display #1183: skl,kbl,cfl */
>> +     cdctl = I915_READ(CDCLK_CTL);
>> +     cdctl &= ~(CDCLK_FREQ_SEL_MASK | CDCLK_FREQ_DECIMAL_MASK);
>> +     cdctl |= freq_select | skl_cdclk_decimal(cdclk);
>> +     I915_WRITE(CDCLK_CTL, cdctl);
>>
>>       /*
>>        * We always enable DPLL0 with the lowest link rate possible, but still
>> @@ -965,6 +966,10 @@ static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
>>       I915_WRITE(DPLL_CTRL1, val);
>>       POSTING_READ(DPLL_CTRL1);
>>
>> +     /* Wa Display #1183: skl,kbl,cfl */
>> +     cdctl |= CDCLK_DIVMUX_CD_OVERRIDE;
>> +     I915_WRITE(CDCLK_CTL, cdctl);
>> +
>>       I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) | LCPLL_PLL_ENABLE);
>>
>>       if (intel_wait_for_register(dev_priv,
>> @@ -972,6 +977,17 @@ static void skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
>>                                   5))
>>               DRM_ERROR("DPLL0 not locked\n");
>>
>> +     /* Wa Display #1183: skl,kbl,cfl */
>> +     cdctl &= ~CDCLK_FREQ_SEL_MASK;
>> +     I915_WRITE(CDCLK_CTL, cdctl);
>> +
>> +     cdctl |= freq_select;
>> +     I915_WRITE(CDCLK_CTL, cdctl);
>> +
>> +     cdctl &= ~CDCLK_DIVMUX_CD_OVERRIDE;
>> +     I915_WRITE(CDCLK_CTL, cdctl);
>> +     POSTING_READ(CDCLK_CTL);
>> +
>>       dev_priv->cdclk.hw.vco = vco;
>>
>>       /* We'll want to keep using the current vco from now on. */
>> @@ -1037,10 +1053,25 @@ static void skl_set_cdclk(struct drm_i915_private *dev_priv,
>>               skl_dpll0_disable(dev_priv);
>>
>>       if (dev_priv->cdclk.hw.vco != vco)
>> -             skl_dpll0_enable(dev_priv, vco);
>> +             skl_dpll0_enable(dev_priv, vco, freq_select, cdclk);
>> +     else {
>> +             u32 cdctl = I915_READ(CDCLK_CTL);
>>
>> -     I915_WRITE(CDCLK_CTL, freq_select | skl_cdclk_decimal(cdclk));
>> -     POSTING_READ(CDCLK_CTL);
>> +             /* Wa Display #1183: skl,kbl,cfl */
>> +             cdctl |= CDCLK_DIVMUX_CD_OVERRIDE;
>> +             I915_WRITE(CDCLK_CTL, cdctl);
>> +
>> +             cdctl &= ~CDCLK_FREQ_SEL_MASK;
>> +             I915_WRITE(CDCLK_CTL, cdctl);
>> +
>> +             cdctl &= ~CDCLK_FREQ_DECIMAL_MASK;
>> +             cdctl |= freq_select | skl_cdclk_decimal(cdclk);
>> +             I915_WRITE(CDCLK_CTL, cdctl);
>> +
>> +             cdctl &= ~CDCLK_DIVMUX_CD_OVERRIDE;
>> +             I915_WRITE(CDCLK_CTL, cdctl);
>> +             POSTING_READ(CDCLK_CTL);
>> +     }
>>
>>       /* inform PCU of the change */
>>       mutex_lock(&dev_priv->pcu_lock);
>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> index 8315499452dc..35796fa8e6b4 100644
>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> @@ -598,6 +598,11 @@ void gen9_enable_dc5(struct drm_i915_private *dev_priv)
>>
>>       DRM_DEBUG_KMS("Enabling DC5\n");
>>
>> +     /* Wa Display #1183: skl,kbl,cfl */
>> +     if (IS_GEN9_BC(dev_priv))
>> +             I915_WRITE(GEN8_CHICKEN_DCPR_1, I915_READ(GEN8_CHICKEN_DCPR_1) |
>> +                        SKL_SELECT_ALTERNATE_DC_EXIT);
>> +
>>       gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
>>  }
>>
>> @@ -625,6 +630,11 @@ void skl_disable_dc6(struct drm_i915_private *dev_priv)
>>  {
>>       DRM_DEBUG_KMS("Disabling DC6\n");
>>
>> +     /* Wa Display #1183: skl,kbl,cfl */
>> +     if (IS_GEN9_BC(dev_priv))
>> +             I915_WRITE(GEN8_CHICKEN_DCPR_1, I915_READ(GEN8_CHICKEN_DCPR_1) |
>> +                        SKL_SELECT_ALTERNATE_DC_EXIT);
>> +
>>       gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
>>  }
>>
>> --
>> 2.14.3
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Ville Syrjälä]

On Thu, Nov 09, 2017 at 08:02:40AM -0800, Lucas De Marchi wrote:
> On Thu, Nov 9, 2017 at 5:11 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Nov 09, 2017 at 02:58:04AM -0800, Lucas De Marchi wrote:
> >> Wa Display #1183 was recently added to workaround
> >> "Failures when enabling DPLL0 with eDP link rate 2.16
> >> or 4.32 GHz and CD clock frequency 308.57 or 617.14 MHz
> >> (CDCLK_CTL CD Frequency Select 10b or 11b) used in this
> >>  enabling or in previous enabling."
> >>
> >> This Workaround was designed to minimize the impact only
> >> to save the bad case with that link rates. But HW engineers
> >> indicated that it should be safe to apply broadly. Although
> >> they were expecting the DPLL0 link rate to be unchanged on
> >> runtime.
> >>
> >> We need to cover 2 cases: when we are in fact enabling DPLL0
> >> and when we are just changing the frequency. The workaround
> >> for those cases are similar but different enough to have them
> >> done in different places.
> >>
> >> This is based on previous patch by Rodrigo Vivi with suggestions
> >> from Ville Syrjälä.
> >
> > Still doesn't look like what I suggested.
> 
> I agree with your suggestion of moving stuff to skl_set_cdclk() to
> cover the case in which
> vco isn't changing. However see the paragraph I added above on why I
> need to do it
> differently. In short: the sequence on the WA for enabling and
> updating cdclck are different,
> with some code duplication unfortunately. I don't see you covering
> that case in your
> suggestion. Have I missed anything?

Even if we follow the spec literally I think we can do it all
in skl_set_cdclk(). I think the following should dtrt:

pcu start

if (...)
	disable_dpll0()

cdclk_sel = real

if (need_wa)
	divmux=1 

if (...)
	enable_dpll0()

if (need_wa) {
	cdclk_sel = 0
	cdclk_sel = real 
	divmux=0 
}

pcu done

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Lucas De Marchi]

Hi Ville,

On Thu, Nov 9, 2017 at 8:58 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Nov 09, 2017 at 08:02:40AM -0800, Lucas De Marchi wrote:
>> On Thu, Nov 9, 2017 at 5:11 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Thu, Nov 09, 2017 at 02:58:04AM -0800, Lucas De Marchi wrote:
>> >> Wa Display #1183 was recently added to workaround
>> >> "Failures when enabling DPLL0 with eDP link rate 2.16
>> >> or 4.32 GHz and CD clock frequency 308.57 or 617.14 MHz
>> >> (CDCLK_CTL CD Frequency Select 10b or 11b) used in this
>> >>  enabling or in previous enabling."
>> >>
>> >> This Workaround was designed to minimize the impact only
>> >> to save the bad case with that link rates. But HW engineers
>> >> indicated that it should be safe to apply broadly. Although
>> >> they were expecting the DPLL0 link rate to be unchanged on
>> >> runtime.
>> >>
>> >> We need to cover 2 cases: when we are in fact enabling DPLL0
>> >> and when we are just changing the frequency. The workaround
>> >> for those cases are similar but different enough to have them
>> >> done in different places.
>> >>
>> >> This is based on previous patch by Rodrigo Vivi with suggestions
>> >> from Ville Syrjälä.
>> >
>> > Still doesn't look like what I suggested.
>>
>> I agree with your suggestion of moving stuff to skl_set_cdclk() to
>> cover the case in which
>> vco isn't changing. However see the paragraph I added above on why I
>> need to do it
>> differently. In short: the sequence on the WA for enabling and
>> updating cdclck are different,
>> with some code duplication unfortunately. I don't see you covering
>> that case in your
>> suggestion. Have I missed anything?
>
> Even if we follow the spec literally I think we can do it all
> in skl_set_cdclk(). I think the following should dtrt:

There are some subtle differences wrt to the initialize and update
sequences according to the WA
that I'd like to clarify.

>
> pcu start
>
> if (...)
>         disable_dpll0()
>
> cdclk_sel = real

We should only do this if we are enabling, but not when updating. In
the latter case
cdclk_sel should only be touched after setting divmux to 1.

>
> if (need_wa)
>         divmux=1

Reading the WA to the letter, in the enabling case this should happen between
DPLL_CTRL1 and LCPLL1_CTL are written.  Here you are moving it to happen before
the write to DPLL_CTRL1.

>
> if (...)
>         enable_dpll0()
>
> if (need_wa) {
>         cdclk_sel = 0
>         cdclk_sel = real

When updating we should set both freq_sel and and freq_decimal. When
enabling, only freq_sel (but I guess
it doesn't matter since we set this same register above).


>         divmux=0
> }

With this sequence you would actually not change the frequency for the
cases in which the WA is not
required. AFAIU from previous version of this patch it's ok to always
follow the WA path so we wouldn't
have a "need_wa". Is that ok?  I can come up with a patch that shares
more code, but I don't think your
approach is following the spec literally.


thanks
Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Ville Syrjälä]

On Mon, Nov 13, 2017 at 01:47:26PM -0800, Lucas De Marchi wrote:
> Hi Ville,
> 
> On Thu, Nov 9, 2017 at 8:58 AM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Nov 09, 2017 at 08:02:40AM -0800, Lucas De Marchi wrote:
> >> On Thu, Nov 9, 2017 at 5:11 AM, Ville Syrjälä
> >> <ville.syrjala@linux.intel.com> wrote:
> >> > On Thu, Nov 09, 2017 at 02:58:04AM -0800, Lucas De Marchi wrote:
> >> >> Wa Display #1183 was recently added to workaround
> >> >> "Failures when enabling DPLL0 with eDP link rate 2.16
> >> >> or 4.32 GHz and CD clock frequency 308.57 or 617.14 MHz
> >> >> (CDCLK_CTL CD Frequency Select 10b or 11b) used in this
> >> >>  enabling or in previous enabling."
> >> >>
> >> >> This Workaround was designed to minimize the impact only
> >> >> to save the bad case with that link rates. But HW engineers
> >> >> indicated that it should be safe to apply broadly. Although
> >> >> they were expecting the DPLL0 link rate to be unchanged on
> >> >> runtime.
> >> >>
> >> >> We need to cover 2 cases: when we are in fact enabling DPLL0
> >> >> and when we are just changing the frequency. The workaround
> >> >> for those cases are similar but different enough to have them
> >> >> done in different places.
> >> >>
> >> >> This is based on previous patch by Rodrigo Vivi with suggestions
> >> >> from Ville Syrjälä.
> >> >
> >> > Still doesn't look like what I suggested.
> >>
> >> I agree with your suggestion of moving stuff to skl_set_cdclk() to
> >> cover the case in which
> >> vco isn't changing. However see the paragraph I added above on why I
> >> need to do it
> >> differently. In short: the sequence on the WA for enabling and
> >> updating cdclck are different,
> >> with some code duplication unfortunately. I don't see you covering
> >> that case in your
> >> suggestion. Have I missed anything?
> >
> > Even if we follow the spec literally I think we can do it all
> > in skl_set_cdclk(). I think the following should dtrt:
> 
> There are some subtle differences wrt to the initialize and update
> sequences according to the WA
> that I'd like to clarify.
> 
> >
> > pcu start
> >
> > if (...)
> >         disable_dpll0()
> >
> > cdclk_sel = real
> 
> We should only do this if we are enabling, but not when updating. In
> the latter case
> cdclk_sel should only be touched after setting divmux to 1.

Seems like a pointless distinction to me. We'll be doing the 0->real
toggle anyway while divmux_override==1. But if we want to be pedantic,
then we could of course just skip this if the DPLL is already enabled.

> 
> >
> > if (need_wa)
> >         divmux=1
> 
> Reading the WA to the letter, in the enabling case this should happen between
> DPLL_CTRL1 and LCPLL1_CTL are written.  Here you are moving it to happen before
> the write to DPLL_CTRL1.

I assume that until the DPLL is enabled the settings in DPLL_CTRL1
don't actually matter.

> 
> >
> > if (...)
> >         enable_dpll0()
> >
> > if (need_wa) {
> >         cdclk_sel = 0
> >         cdclk_sel = real
> 
> When updating we should set both freq_sel and and freq_decimal. When
> enabling, only freq_sel (but I guess
> it doesn't matter since we set this same register above).

I think the implication is just that the "decimal" frequency doesn't
matter until something actually starts to use cdclk. The safe bet
would be to always program it to match the frequency select.

> 
> 
> >         divmux=0
> > }
> 
> With this sequence you would actually not change the frequency for the
> cases in which the WA is not
> required. AFAIU from previous version of this patch it's ok to always
> follow the WA path so we wouldn't
> have a "need_wa". Is that ok?  I can come up with a patch that shares
> more code, but I don't think your
> approach is following the spec literally.

Yeah, the exact conditions for need_wa seem a bit unlcear to me since it
says "... used in this enabling or in previous enabling". I'm not sure
if it's referring to the DPLL or CDCLK frequency or both. Maybe safer to
just always follow the w/a sequence.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Apply Wa Display #1183 on skl, kbl, and cfl

[Lucas De Marchi]

On Tue, Nov 14, 2017 at 5:10 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Mon, Nov 13, 2017 at 01:47:26PM -0800, Lucas De Marchi wrote:
>> Hi Ville,
>>
>> On Thu, Nov 9, 2017 at 8:58 AM, Ville Syrjälä
>> <ville.syrjala@linux.intel.com> wrote:
>> > On Thu, Nov 09, 2017 at 08:02:40AM -0800, Lucas De Marchi wrote:
>> >> On Thu, Nov 9, 2017 at 5:11 AM, Ville Syrjälä
>> >> <ville.syrjala@linux.intel.com> wrote:
>> >> > On Thu, Nov 09, 2017 at 02:58:04AM -0800, Lucas De Marchi wrote:
>> >> >> Wa Display #1183 was recently added to workaround
>> >> >> "Failures when enabling DPLL0 with eDP link rate 2.16
>> >> >> or 4.32 GHz and CD clock frequency 308.57 or 617.14 MHz
>> >> >> (CDCLK_CTL CD Frequency Select 10b or 11b) used in this
>> >> >>  enabling or in previous enabling."
>> >> >>
>> >> >> This Workaround was designed to minimize the impact only
>> >> >> to save the bad case with that link rates. But HW engineers
>> >> >> indicated that it should be safe to apply broadly. Although
>> >> >> they were expecting the DPLL0 link rate to be unchanged on
>> >> >> runtime.
>> >> >>
>> >> >> We need to cover 2 cases: when we are in fact enabling DPLL0
>> >> >> and when we are just changing the frequency. The workaround
>> >> >> for those cases are similar but different enough to have them
>> >> >> done in different places.
>> >> >>
>> >> >> This is based on previous patch by Rodrigo Vivi with suggestions
>> >> >> from Ville Syrjälä.
>> >> >
>> >> > Still doesn't look like what I suggested.
>> >>
>> >> I agree with your suggestion of moving stuff to skl_set_cdclk() to
>> >> cover the case in which
>> >> vco isn't changing. However see the paragraph I added above on why I
>> >> need to do it
>> >> differently. In short: the sequence on the WA for enabling and
>> >> updating cdclck are different,
>> >> with some code duplication unfortunately. I don't see you covering
>> >> that case in your
>> >> suggestion. Have I missed anything?
>> >
>> > Even if we follow the spec literally I think we can do it all
>> > in skl_set_cdclk(). I think the following should dtrt:
>>
>> There are some subtle differences wrt to the initialize and update
>> sequences according to the WA
>> that I'd like to clarify.
>>
>> >
>> > pcu start
>> >
>> > if (...)
>> >         disable_dpll0()
>> >
>> > cdclk_sel = real
>>
>> We should only do this if we are enabling, but not when updating. In
>> the latter case
>> cdclk_sel should only be touched after setting divmux to 1.
>
> Seems like a pointless distinction to me. We'll be doing the 0->real
> toggle anyway while divmux_override==1. But if we want to be pedantic,
> then we could of course just skip this if the DPLL is already enabled.
>
>>
>> >
>> > if (need_wa)
>> >         divmux=1
>>
>> Reading the WA to the letter, in the enabling case this should happen between
>> DPLL_CTRL1 and LCPLL1_CTL are written.  Here you are moving it to happen before
>> the write to DPLL_CTRL1.
>
> I assume that until the DPLL is enabled the settings in DPLL_CTRL1
> don't actually matter.
>
>>
>> >
>> > if (...)
>> >         enable_dpll0()
>> >
>> > if (need_wa) {
>> >         cdclk_sel = 0
>> >         cdclk_sel = real
>>
>> When updating we should set both freq_sel and and freq_decimal. When
>> enabling, only freq_sel (but I guess
>> it doesn't matter since we set this same register above).
>
> I think the implication is just that the "decimal" frequency doesn't
> matter until something actually starts to use cdclk. The safe bet
> would be to always program it to match the frequency select.
>
>>
>>
>> >         divmux=0
>> > }
>>
>> With this sequence you would actually not change the frequency for the
>> cases in which the WA is not
>> required. AFAIU from previous version of this patch it's ok to always
>> follow the WA path so we wouldn't
>> have a "need_wa". Is that ok?  I can come up with a patch that shares
>> more code, but I don't think your
>> approach is following the spec literally.
>
> Yeah, the exact conditions for need_wa seem a bit unlcear to me since it
> says "... used in this enabling or in previous enabling". I'm not sure
> if it's referring to the DPLL or CDCLK frequency or both. Maybe safer to
> just always follow the w/a sequence.


Ok. I thought it would be better to follow the exact steps from the WA
as I actually
couldn't reproduce the bug. I implemented what you said and tested
both approaches
to check it's not regressing.  I will submit a new version with your
comments addressed.

thanks
Lucas De Marchi
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx