Re: [PATCH 07/21] drm/i915: Allow enable/disable of DPLL0 around cdclk changes on SKL

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

On Thu, May 19, 2016 at 04:04:40PM +0300, Imre Deak wrote:
> On pe, 2016-05-13 at 23:41 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > In case we originally guessed wrong which lcpll vco frequency to use,
> > we will need to shut down the pll and restart it when reprogamming the
> > cdclk.
> > 
> > This also allows us to track the actual vco frequency in dev_priv
> > instead of just a guess.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 54 +++++++++++++++++++-----------------
> >  1 file changed, 29 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 95997eed9dd6..cd2809179042 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5626,6 +5626,8 @@ skl_dpll0_enable(struct drm_i915_private *dev_priv, int vco)
> >  
> >  	if (wait_for(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK, 5))
> >  		DRM_ERROR("DPLL0 not locked\n");
> > +
> > +	dev_priv->skl_vco_freq = vco;
> >  }
> >  
> >  static void
> > @@ -5634,6 +5636,8 @@ skl_dpll0_disable(struct drm_i915_private *dev_priv)
> >  	I915_WRITE(LCPLL1_CTL, I915_READ(LCPLL1_CTL) & ~LCPLL_PLL_ENABLE);
> >  	if (wait_for(!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_LOCK), 1))
> >  		DRM_ERROR("Couldn't disable DPLL0\n");
> > +
> > +	dev_priv->skl_vco_freq = 0;
> >  }
> >  
> >  static bool skl_cdclk_pcu_ready(struct drm_i915_private *dev_priv)
> > @@ -5663,12 +5667,14 @@ static bool skl_cdclk_wait_for_pcu_ready(struct drm_i915_private *dev_priv)
> >  	return false;
> >  }
> >  
> > -static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk)
> > +static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk, int vco)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  	u32 freq_select, pcu_ack;
> >  
> > -	DRM_DEBUG_DRIVER("Changing CDCLK to %dKHz\n", cdclk);
> > +	WARN_ON((cdclk == 24000) != (vco == 0));
> > +
> > +	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz (VCO %d MHz)\n", cdclk, vco);
> >  
> >  	if (!skl_cdclk_wait_for_pcu_ready(dev_priv)) {
> >  		DRM_ERROR("failed to inform PCU about cdclk change\n");
> > @@ -5699,6 +5705,13 @@ static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk)
> >  		break;
> >  	}
> >  
> > +	if (dev_priv->skl_vco_freq != 0 &&
> > +	    dev_priv->skl_vco_freq != vco)
> > +		skl_dpll0_disable(dev_priv);
> > +
> > +	if (dev_priv->skl_vco_freq != vco)
> > +		skl_dpll0_enable(dev_priv, vco);
> > +
> >  	I915_WRITE(CDCLK_CTL, freq_select | skl_cdclk_decimal(cdclk));
> >  	POSTING_READ(CDCLK_CTL);
> >  
> > @@ -5721,26 +5734,21 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
> >  	if (I915_READ(DBUF_CTL) & DBUF_POWER_STATE)
> >  		DRM_ERROR("DBuf power disable timeout\n");
> >  
> > -	skl_dpll0_disable(dev_priv);
> > +	skl_set_cdclk(dev_priv, 24000, 0);
> 
> The spec doesn't say how to program CDCLK for the bypass case and we'll
> program the 'frequency decimal' field to something isn't listed as a
> valid value (and not matching the CD frequency select value). But I
> think CDCLK is don't-care in that case, so it's fine.
> 
> >  }
> >  
> >  void skl_init_cdclk(struct drm_i915_private *dev_priv)
> >  {
> > -	unsigned int cdclk;
> > -
> >  	/* DPLL0 not enabled (happens on early BIOS versions) */
> > -	if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
> > -		/* enable DPLL0 */
> > -		if (dev_priv->skl_vco_freq != 8640)
> > -			dev_priv->skl_vco_freq = 8100;
> > -		skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq);
> > -		cdclk = skl_calc_cdclk(0, dev_priv->skl_vco_freq);
> > -	} else {
> > -		cdclk = dev_priv->cdclk_freq;
> > -	}
> > +	if (dev_priv->skl_vco_freq == 0) {
> > +		int cdclk, vco;
> >  
> > -	/* set CDCLK to the lowest frequency, Modeset follows */
> > -	skl_set_cdclk(dev_priv, cdclk);
> > +		/* set CDCLK to the lowest frequency, Modeset follows */
> > +		vco = 8100;
> > +		cdclk = skl_calc_cdclk(0, vco);
> > +
> > +		skl_set_cdclk(dev_priv, cdclk, vco);
> > +	}
> >  
> >  	/* enable DBUF power */
> >  	I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> > @@ -9777,16 +9785,12 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  
> >  static void skl_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> 
> Why is the above called old_state?

Because atomic is stupid. There's just one top level state which starts
out as the new state, but later we swap the connector/crtc/plane state
pointers around so that the top level state points to the old state
for those things. That's why it's called old_state here. Everything
else stored in that top level state is not swapped though so it's
actually an extremely confusing mix of old and new state.

I think what we should do is pull all the state stored in the top level
atomic state structure into a separate structure and store a pointer to
that in the top level atomic state just like for everything else. We
can then swap that pointer as well. That way all the old vs. new stuff
would be consistent. It migth also clean up the horrible mess with the
atomic_cdclk stuff and whatnot.

> 
> The patch looks ok:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
> 
> >  {
> > -	struct drm_device *dev = old_state->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	unsigned int req_cdclk = to_intel_atomic_state(old_state)->dev_cdclk;
> > -
> > -	/*
> > -	 * FIXME disable/enable PLL should wrap set_cdclk()
> > -	 */
> > -	skl_set_cdclk(dev_priv, req_cdclk);
> > +	struct drm_i915_private *dev_priv = to_i915(old_state->dev);
> > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(old_state);
> > +	unsigned int req_cdclk = intel_state->dev_cdclk;
> > +	unsigned int req_vco = intel_state->cdclk_pll_vco;
> >  
> > -	dev_priv->skl_vco_freq = to_intel_atomic_state(old_state)->cdclk_pll_vco;
> > +	skl_set_cdclk(dev_priv, req_cdclk, req_vco);
> >  }
> >  
> >  static int haswell_crtc_compute_clock(struct intel_crtc *crtc,

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