On Tue, May 24, 2016 at 02:59:55PM +0300, Imre Deak wrote:
> On ti, 2016-05-24 at 13:22 +0300, Ville Syrjälä wrote:
> > On Tue, May 24, 2016 at 12:27:50PM +0300, Imre Deak wrote:
> > > If the CDCLK PLL isn't locked we can just assume that it's off resulting
> > > in fully re-initializing both CDCLK PLL and CDCLK dividers. This way the
> > > CDCLK PLL sanitization added in the following patch can be done on BXT
> > > the same way as it's done on SKL.
> > >
> > > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++------------
> > > 1 file changed, 11 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index c1e666b..b8e5995 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5461,14 +5461,14 @@ skl_dpll0_update(struct drm_i915_private *dev_priv)
> > > u32 val;
> > >
> > > dev_priv->cdclk_pll.ref = 24000;
> > > + dev_priv->cdclk_pll.vco = 0;
> > >
> > > val = I915_READ(LCPLL1_CTL);
> > > - if ((val & LCPLL_PLL_ENABLE) == 0) {
> > > - dev_priv->cdclk_pll.vco = 0;
> > > + if ((val & LCPLL_PLL_ENABLE) == 0)
> > > return;
> > > - }
> > >
> > > - WARN_ON((val & LCPLL_PLL_LOCK) == 0);
> > > + if (WARN_ON((val & LCPLL_PLL_LOCK) == 0))
> > > + return;
> > >
> > > val = I915_READ(DPLL_CTRL1);
> > >
> > > @@ -5690,9 +5690,10 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> > > if ((I915_READ(SWF_ILK(0x18)) & 0x00FFFFFF) == 0)
> > > goto sanitize;
> > >
> > > + intel_update_cdclk(dev_priv->dev);
> > > /* Is PLL enabled and locked ? */
> > > - if ((I915_READ(LCPLL1_CTL) & (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK)) !=
> > > - (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK))
> > > + if (!dev_priv->cdclk_pll.vco ||
> >
> > == 0 please. I find that more pleasing to the eye when we end up mixing
> > with == anyway on the next line.
>
> Ok.
>
> > Actually is there any extra benefit from the cdclk_freq check? As
> > in would vco==0 be sufficient on its own?
>
> The other check is for the case of an invalid CDCLK divider setting.
> Don't we care about that?
Oh right. I guess there's no harm in checking for it.
>
> > > + dev_priv->cdclk_freq == dev_priv->cdclk_pll.ref)
> > > goto sanitize;
> > >
> > > if ((I915_READ(DPLL_CTRL1) & (DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) |
> >
> > Maybe toss out this DPLL_CTRL1 check that I added as well then, and have
> > skl_dpll0_update() set the vco to 0 when it's crap. If we ever actually
> > hit this in the real world, we'll get the warn, and then we perhaps get
> > to rethink this stuff, but for now simpler seems better.
>
> Ok, makes sense.
>
> > > @@ -5701,8 +5702,6 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> > > DPLL_CTRL1_OVERRIDE(SKL_DPLL0))
> > > goto sanitize;
> > >
> > > - intel_update_cdclk(dev_priv->dev);
> > > -
> > > /* DPLL okay; verify the cdclock
> > > *
> > > * Noticed in some instances that the freq selection is correct but
> > > @@ -6608,14 +6607,14 @@ static void bxt_de_pll_update(struct drm_i915_private *dev_priv)
> > > u32 val;
> > >
> > > dev_priv->cdclk_pll.ref = 19200;
> > > + dev_priv->cdclk_pll.vco = 0;
> > >
> > > val = I915_READ(BXT_DE_PLL_ENABLE);
> > > - if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) {
> > > - dev_priv->cdclk_pll.vco = 0;
> > > + if ((val & BXT_DE_PLL_PLL_ENABLE) == 0)
> > > return;
> > > - }
> > >
> > > - WARN_ON((val & BXT_DE_PLL_LOCK) == 0);
> > > + if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0))
> > > + return;
> > >
> > > val = I915_READ(BXT_DE_PLL_CTL);
> > > dev_priv->cdclk_pll.vco = (val & BXT_DE_PLL_RATIO_MASK) *
> > > --
> > > 2.5.0
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
