Re: [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked

[Imre Deak <imre.deak@intel.com>]

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@intel.com>
> > ---
> >  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?

> > +	    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@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx