| Message ID | 20171109105804.19774-1-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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.
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
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); }
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(-)