Re: [PATCH v2 3/8] drm/i915: Use cdclk_state->voltage on VLV/CHV

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

On Thu, Oct 19, 2017 at 04:42:52PM -0700, Rodrigo Vivi wrote:
> On Thu, Oct 19, 2017 at 05:43:29PM +0000, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Store the punit DSPFREQUAR value into cdclk_state->voltage on
> > VLV/CHV. Since we can actually read that out from the hardware
> > this can give us a bit more cross checking between the hardware
> > and software state.
> > 
> > v2: Don't break waiting for cdclk change on VLV/CHV
> > 
> > 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 | 66 +++++++++++++++++++++++++++++---------
> >  1 file changed, 50 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 522222f8bb50..9cc4374797eb 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -437,13 +437,45 @@ static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
> >  		return 200000;
> >  }
> >  
> > +static u8 vlv_calc_voltage(struct drm_i915_private *dev_priv, int cdclk)
> > +{
> > +	if (IS_VALLEYVIEW(dev_priv)) {
> > +		if (cdclk >= 320000) /* jump to highest voltage for 400MHz too */
> > +			return 2;
> > +		else if (cdclk >= 266667)
> > +			return 1;
> > +		else
> > +			return 0;
> > +	} else {
> > +		/*
> > +		 * Specs are full of misinformation, but testing on actual
> > +		 * hardware has shown that we just need to write the desired
> > +		 * CCK divider into the Punit register.
> > +		 */
> > +		return DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> > +	}
> > +}
> > +
> >  static void vlv_get_cdclk(struct drm_i915_private *dev_priv,
> >  			  struct intel_cdclk_state *cdclk_state)
> >  {
> > +	u32 val;
> > +
> >  	cdclk_state->vco = vlv_get_hpll_vco(dev_priv);
> >  	cdclk_state->cdclk = vlv_get_cck_clock(dev_priv, "cdclk",
> >  					       CCK_DISPLAY_CLOCK_CONTROL,
> >  					       cdclk_state->vco);
> > +
> > +	mutex_lock(&dev_priv->pcu_lock);
> > +	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> > +	mutex_unlock(&dev_priv->pcu_lock);
> > +
> > +	if (IS_VALLEYVIEW(dev_priv))
> > +		cdclk_state->voltage = (val & DSPFREQGUAR_MASK) >>
> > +			DSPFREQGUAR_SHIFT;
> > +	else
> > +		cdclk_state->voltage = (val & DSPFREQGUAR_MASK_CHV) >>
> > +			DSPFREQGUAR_SHIFT_CHV;
> 
> Oh interresting that vlv is the only one where we can actually read it. :(
> 
> I was going to ask pointers to spec, but just by {vlv,chv}_set_cdclk it was
> possible to verify this is right.

Yeah, the iosf sideband mailbox is much nicer because you have actual
read vs. write opcodes in addition to the port+register offset
information which just specify what you want to access.

> 
> >  }
> >  
> >  static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> > @@ -486,7 +518,19 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> >  			  const struct intel_cdclk_state *cdclk_state)
> >  {
> >  	int cdclk = cdclk_state->cdclk;
> > -	u32 val, cmd;
> > +	u32 val, cmd = cdclk_state->voltage;
> > +
> > +	switch (cdclk) {
> > +	case 400000:
> > +	case 333333:
> > +	case 320000:
> > +	case 266667:
> > +	case 200000:
> > +		break;
> > +	default:
> > +		MISSING_CASE(cdclk);
> > +		return;
> > +	}
> 
> I believe this should be in a separated patch, no?!
> Seems something we were already missing anyways.

I was going back and forth between removing this from the
chv_set_cdclk(), and in the end I decided to keep it. And looks like I
then accidentally added it into vlv_set_cdclk() as well thinking both
had it originally. I guess I'm still leaning towards keeping it, so
I'll go ahead and split this out to a separate patch.

> 
> But anyways, if you decide to split or not
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> >  
> >  	/* There are cases where we can end up here with power domains
> >  	 * off and a CDCLK frequency other than the minimum, like when
> > @@ -496,13 +540,6 @@ static void vlv_set_cdclk(struct drm_i915_private *dev_priv,
> >  	 */
> >  	intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_A);
> >  
> > -	if (cdclk >= 320000) /* jump to highest voltage for 400MHz too */
> > -		cmd = 2;
> > -	else if (cdclk == 266667)
> > -		cmd = 1;
> > -	else
> > -		cmd = 0;
> > -
> >  	mutex_lock(&dev_priv->pcu_lock);
> >  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >  	val &= ~DSPFREQGUAR_MASK;
> > @@ -562,7 +599,7 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
> >  			  const struct intel_cdclk_state *cdclk_state)
> >  {
> >  	int cdclk = cdclk_state->cdclk;
> > -	u32 val, cmd;
> > +	u32 val, cmd = cdclk_state->voltage;
> >  
> >  	switch (cdclk) {
> >  	case 333333:
> > @@ -583,13 +620,6 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
> >  	 */
> >  	intel_display_power_get(dev_priv, POWER_DOMAIN_PIPE_A);
> >  
> > -	/*
> > -	 * Specs are full of misinformation, but testing on actual
> > -	 * hardware has shown that we just need to write the desired
> > -	 * CCK divider into the Punit register.
> > -	 */
> > -	cmd = DIV_ROUND_CLOSEST(dev_priv->hpll_freq << 1, cdclk) - 1;
> > -
> >  	mutex_lock(&dev_priv->pcu_lock);
> >  	val = vlv_punit_read(dev_priv, PUNIT_REG_DSPFREQ);
> >  	val &= ~DSPFREQGUAR_MASK_CHV;
> > @@ -1859,11 +1889,15 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  	cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> >  
> >  	intel_state->cdclk.logical.cdclk = cdclk;
> > +	intel_state->cdclk.logical.voltage =
> > +		vlv_calc_voltage(dev_priv, cdclk);
> >  
> >  	if (!intel_state->active_crtcs) {
> >  		cdclk = vlv_calc_cdclk(dev_priv, 0);
> >  
> >  		intel_state->cdclk.actual.cdclk = cdclk;
> > +		intel_state->cdclk.actual.voltage =
> > +			vlv_calc_voltage(dev_priv, 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