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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Oct 17, 2017 at 07:25:46PM +0000, Ville Syrjälä wrote:
> On Tue, Oct 17, 2017 at 10:38:19AM -0700, Rodrigo Vivi 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."
> > 
> > However our code is already not following the
> > "Skylake Seqyences to Initialize Display" line by line
> > so it was really hard to map this workaround there.
> > 
> > The biggest difference is that Spec sequence expect that
> > by the time we are enabling DPLL0 we already know the
> > eDP link rate. What it is not true for us. We handle eDP
> > link rate as any other DP link rate at the modeset. With
> > only one small difference that we check a VCO when that
> > is available.
> > 
> > WARN: It seems that DPLL0 link rate was not designed to
> > change on the fly and we will probably need to find a more
> > robuts solution caching the eDP link rate somehow.
> > 
> > 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.
> 
> I think the eDP link rate just refers to the VCO freq. So we should
> apparently do the w/a only if 8640 MHz is used. But I'd rather do it
> always if we can to keep the code simpler.

hmmm... this directly map to the vco was one of the things that confused me.

> 
> Our current sequence is pretty much this:
> if necessary to disable DPLL0
> 	disable DPLL0
> if necessary to enable DPLL0
> 	write CDLCK_CTL
> 		freq_select = 2
> 		decimal = whatever
> 	enable DPLL0
> write CDCLK_CTL
> 	freq_select = final
> 	decimal = final

also this step with 2 different cdclk_ctl writes wasn't like I map the spec.

> 
> IIRC the first CDCLK_CTL write was put there just to make sure we start
> at the minimum CDCLK when DPLL0 is first enabled. For this w/a we
> should apparently just do that write even if DPLL0 is already enabled, 
> and change it to select 450/432 MHz instead.
> 
> As far as the divmux override goes, I wonder if we can just do this
> with two CDCLK_CTL writes, or if we really need four.

I hope Art can answer if this block below would be enough:

> 
> So I was thinking we'd just do this:
> if necessary to disable DPLL0
> 	disable DPLL0
> write CDLCK_CTL
> 	divmux_override = 1
> 	freq_select = 0
> 	decimal = whatever
> if necessary to enable DPLL0
> 	enable DPLL0
> write CDCLK_CTL
> 	freq_select = final
> 	decimal = final
> 
> Or should we even do the first CDCLK_CTL write before potentially
> disabling DPLL0? Not that we would normally do that, but in theory
> it can happen if the machine boots with the wrong DPLL0 settings.
> 
> And if we really need the four writes then I guess we'd maybe end
> up doing something like:
> if necessary to disable DPLL0
> 	disable DPLL0
> read-modify-write CDLCK_CTL
> 	divmux_override = 1
> write CDLCK_CTL
> 	divmux_override = 1
> 	freq_select = 0
> 	decimal = whatever
> if necessary to enable DPLL0
> 	enable DPLL0
> write CDCLK_CTL
> 	divmux_override = 1
> 	freq_select = final
> 	decimal = final
> read-modify-write CDLCK_CTL
> 	divmux_override = 0
> 
> With the DPLL0 disable potentially moved to just before DPLL0 enable.

Thanks a lot for all comments here.

> 
> > 
> > Cc: Arthur J Runyan <arthur.j.runyan@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h         |  2 ++
> >  drivers/gpu/drm/i915/intel_cdclk.c      | 33 +++++++++++++++++++++++++++------
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 10 ++++++++++
> >  3 files changed, 39 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 5f99d4d6291b..446f4b6fade1 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6980,6 +6980,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)
> > @@ -8525,6 +8526,7 @@ enum skl_power_gate {
> >  #define  BXT_CDCLK_CD2X_DIV_SEL_4	(3<<22)
> >  #define  BXT_CDCLK_CD2X_PIPE(pipe)	((pipe)<<20)
> >  #define  BXT_CDCLK_CD2X_PIPE_NONE	BXT_CDCLK_CD2X_PIPE(3)
> > +#define  DIVMUX_CD_OVERRIDE		(1<<19)
> >  #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 b2a6d62b71c0..e193912b21ec 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -858,7 +858,8 @@ 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)
> >  {
> >  	int min_cdclk = skl_calc_cdclk(0, vco);
> >  	u32 val;
> > @@ -894,6 +895,11 @@ 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 */
> > +	val = I915_READ(CDCLK_CTL);
> > +	val |= DIVMUX_CD_OVERRIDE;
> > +	I915_WRITE(CDCLK_CTL, val);
> > +
> >  	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) | LCPLL_PLL_ENABLE);
> >  
> >  	if (intel_wait_for_register(dev_priv,
> > @@ -901,6 +907,18 @@ 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 */
> > +	val = I915_READ(CDCLK_CTL);
> > +	val &= ~CDCLK_FREQ_SEL_MASK;;
> > +	I915_WRITE(CDCLK_CTL, val);
> > +
> > +	I915_WRITE(CDCLK_CTL, freq_select);
> > +	POSTING_READ(CDCLK_CTL);
> > +
> > +	val = I915_READ(CDCLK_CTL);
> > +	val &= ~DIVMUX_CD_OVERRIDE;
> > +	I915_WRITE(CDCLK_CTL, val);
> > +
> >  	dev_priv->cdclk.hw.vco = vco;
> >  
> >  	/* We'll want to keep using the current vco from now on. */
> > @@ -964,15 +982,18 @@ static void skl_set_cdclk(struct drm_i915_private *dev_priv,
> >  		break;
> >  	}
> >  
> > +	freq_select |= skl_cdclk_decimal(cdclk);
> > +
> >  	if (dev_priv->cdclk.hw.vco != 0 &&
> >  	    dev_priv->cdclk.hw.vco != vco)
> >  		skl_dpll0_disable(dev_priv);
> >  
> > -	if (dev_priv->cdclk.hw.vco != vco)
> > -		skl_dpll0_enable(dev_priv, vco);
> > -
> > -	I915_WRITE(CDCLK_CTL, freq_select | skl_cdclk_decimal(cdclk));
> > -	POSTING_READ(CDCLK_CTL);
> > +	if (dev_priv->cdclk.hw.vco != vco) {
> > +		skl_dpll0_enable(dev_priv, vco, freq_select);
> > +	} else {
> > +		I915_WRITE(CDCLK_CTL, freq_select);
> > +		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 8af286c63d3b..e0bc2debdad0 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.13.5
> 
> -- 
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx