| Message ID | 20171017173819.8054-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. 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 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. 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. > > 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
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
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. > > 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 > > 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. > > 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 ok, so the code I posted do: if necessary to disable DPLL0 disable DPLL0 if necessary to enable DPLL0 write CDLCK_CTL freq_select = 337_308 (min) decimal = decimal(min_cdclk) rm-write CDLCK_CTL divmux_override = 1 rm-write CDLCK_CTL freq_select = 0 enable DPLL0 write CDCLK_CTL freq_select = final decimal = final rm-write CDLCK_CTL divmux_override = 0 5 writes actually to cdclk_ctl. and ouch! I just saw we are doing a non rmw on the middle... a v2 is needed anyways. Art confirmed that we need to interleave the writes... Maybe I thought that we could avoid the first one that is the minimun CDCLK before enabling DPLL, it seems there is no impact on changing that before enabling it. Also I saw that what I can avoid are the many mmio reads since we are constructing the right "val" (just carefully to rename it to avoid mistaking with val usage on ctrl1. Thoughts? > > 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. > > > > > 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
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); }
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. 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(-)