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.run...@intel.com>
> > Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.v...@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
