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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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?

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx