Re: [PATCH 4/8] drm/i915: Use cdclk_state->voltage on BDW

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

On Thu, Oct 19, 2017 at 04:44:34PM -0700, Rodrigo Vivi wrote:
> On Wed, Oct 18, 2017 at 08:48:21PM +0000, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Track the system agent voltage we request from pcode in the cdclk state
> > on BDW. Annoyingly we can't actually read out the current value since
> > there's no pcode command to do that, so we'll have to just assume that
> > it worked.
> > 
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c | 36 ++++++++++++++++++++++++++----------
> >  1 file changed, 26 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index df71667c9fd6..7442e9443ffa 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -648,6 +648,21 @@ static int bdw_calc_cdclk(int min_cdclk)
> >  		return 337500;
> >  }
> >  
> > +static u8 bdw_calc_voltage(int cdclk)
> > +{
> > +	switch (cdclk) {
> > +	default:
> > +	case 337500:
> > +		return 2;
> > +	case 450000:
> > +		return 0;
> > +	case 540000:
> > +		return 1;
> > +	case 675000:
> > +		return 3;
> > +	}
> > +}
> > +
> >  static void bdw_get_cdclk(struct drm_i915_private *dev_priv,
> >  			  struct intel_cdclk_state *cdclk_state)
> >  {
> > @@ -666,13 +681,19 @@ static void bdw_get_cdclk(struct drm_i915_private *dev_priv,
> >  		cdclk_state->cdclk = 337500;
> >  	else
> >  		cdclk_state->cdclk = 675000;
> > +
> > +	/*
> > +	 * Can't read this out :( Let's assume it's
> > +	 * at least what the CDCLK frequency requires.
> > +	 */
> > +	cdclk_state->voltage = bdw_calc_voltage(cdclk_state->cdclk);
> >  }
> >  
> >  static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
> >  			  const struct intel_cdclk_state *cdclk_state)
> >  {
> >  	int cdclk = cdclk_state->cdclk;
> > -	uint32_t val, data;
> > +	uint32_t val;
> >  	int ret;
> >  
> >  	if (WARN((I915_READ(LCPLL_CTL) &
> > @@ -713,19 +734,15 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
> >  		/* fall through */
> >  	case 337500:
> >  		val |= LCPLL_CLK_FREQ_337_5_BDW;
> > -		data = 2;
> >  		break;
> >  	case 450000:
> >  		val |= LCPLL_CLK_FREQ_450;
> > -		data = 0;
> >  		break;
> >  	case 540000:
> >  		val |= LCPLL_CLK_FREQ_54O_BDW;
> > -		data = 1;
> >  		break;
> >  	case 675000:
> >  		val |= LCPLL_CLK_FREQ_675_BDW;
> > -		data = 3;
> >  		break;
> >  	}
> >  
> > @@ -740,16 +757,13 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
> >  		DRM_ERROR("Switching back to LCPLL failed\n");
> >  
> >  	mutex_lock(&dev_priv->pcu_lock);
> > -	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ, data);
> > +	sandybridge_pcode_write(dev_priv, HSW_PCODE_DE_WRITE_FREQ_REQ,
> > +				cdclk_state->voltage);
> >  	mutex_unlock(&dev_priv->pcu_lock);
> >  
> >  	I915_WRITE(CDCLK_FREQ, DIV_ROUND_CLOSEST(cdclk, 1000) - 1);
> >  
> >  	intel_update_cdclk(dev_priv);
> > -
> > -	WARN(cdclk != dev_priv->cdclk.hw.cdclk,
> > -	     "cdclk requested %d kHz but got %d kHz\n",
> > -	     cdclk, dev_priv->cdclk.hw.cdclk);
> 
> Why?

We don't have such a thing anywhere else either. It would be better
to stick something like that into a more central location. Probably
we should just go for a full cdclk state verification somewhere.

Apparently I forgot to split this out to a separate patch.

> 
> >  }
> >  
> >  static int skl_calc_cdclk(int min_cdclk, int vco)
> > @@ -1919,11 +1933,13 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	cdclk = bdw_calc_cdclk(min_cdclk);
> >  
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> > +	intel_state->cdclk.logical.voltage = bdw_calc_voltage(cdclk);
> >  
> >  	if (!intel_state->active_crtcs) {
> >  		cdclk = bdw_calc_cdclk(0);
> >  
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> > +		intel_state->cdclk.actual.voltage = bdw_calc_voltage(cdclk);
> >  	} else {
> >  		intel_state->cdclk.actual =
> >  			intel_state->cdclk.logical;
> > -- 
> > 2.13.6
> > 

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