Re: [PATCH 2/8] drm/i915: Start tracking voltage level in the cdclk state

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

On Fri, Oct 20, 2017 at 01:43:51PM -0700, Rodrigo Vivi wrote:
> On Fri, Oct 20, 2017 at 02:01:37PM +0000, Ville Syrjälä wrote:
> > On Wed, Oct 18, 2017 at 11:48:19PM +0300, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > For CNL we'll need to start considering the port clocks when we select
> > > the voltage level for the system agent. To that end start tracking the
> > > voltage in the cdclk state (since that already has to adjust it).
> > > 
> > > 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/i915_drv.h         |  1 +
> > >  drivers/gpu/drm/i915/intel_cdclk.c      | 31 ++++++++++++++++++++++++-------
> > >  drivers/gpu/drm/i915/intel_display.c    |  8 ++++----
> > >  drivers/gpu/drm/i915/intel_drv.h        |  4 +++-
> > >  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 ++-
> > >  5 files changed, 34 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > > index f01c80076c59..d3ac58dc275f 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -2227,6 +2227,7 @@ struct i915_oa_ops {
> > >  
> > >  struct intel_cdclk_state {
> > >  	unsigned int cdclk, vco, ref;
> > > +	u8 voltage;
> > 
> > BTW now that I decided to abuse this for all platforms the name
> > "voltage" might not be entirely accurate anymore. Especially on VLV/CHV
> > the DSPFREQUAR value isn't directly a voltage value, but instead a
> > request to punit to change the cdclk frequency to a specific value.
> > Although naturally punit will also make sure the voltage will be
> > set sufficiently high as well.
> > 
> > So I'm not entirely sure we want to call it "voltage" anymore. Something
> > abstract like "level" is also what came to mind when I was thingking
> > about this. If anyone is irked by "voltage" and would like to see
> > something different there, let me know. Otherwise I think I might just
> > leave it as is for now.
> 
> I honestly prefer "dvfs_level", voltage_level or anything_level.

I think voltage_level makes a ton of sense, and looks like that's
actually what the CNL spec calls it as well (at least in some places).
While still not 100% accurate for VLV/CHV I suppose, I think it's more
clear than just "level" for more recent platforms.

Any objections to "voltage_level"?

> 
> Even if on VLV/CHV if it was also voltage related and not
> frequency related. It is strange for my brain to read
> voltage 1,2,3... my poor braing automatically reads 1V, 2V, 3V!
> So level would be better imho.
> 
> But it is up to you.
> 
> > 
> > >  };
> > >  
> > >  struct drm_i915_private {
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > > index 4bffd31a8924..522222f8bb50 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -1690,17 +1690,34 @@ void cnl_uninit_cdclk(struct drm_i915_private *dev_priv)
> > >  }
> > >  
> > >  /**
> > > - * intel_cdclk_state_compare - Determine if two CDCLK states differ
> > > + * intel_cdclk_needs_modeset - Determine if two CDCLK states require a modeset on all pipes
> > >   * @a: first CDCLK state
> > >   * @b: second CDCLK state
> > >   *
> > >   * Returns:
> > > - * True if the CDCLK states are identical, false if they differ.
> > > + * True if the CDCLK states require pipes to be off during reprogramming, false if not.
> > >   */
> > > -bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
> > > +bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> > >  			       const struct intel_cdclk_state *b)
> > >  {
> > > -	return memcmp(a, b, sizeof(*a)) == 0;
> > > +	return a->cdclk != b->cdclk ||
> > > +		a->vco != b->vco ||
> > > +		a->ref != b->ref;
> > > +}
> > > +
> > > +/**
> > > + * intel_cdclk_changed - Determine if two CDCLK states are different
> > > + * @a: first CDCLK state
> > > + * @b: second CDCLK state
> > > + *
> > > + * Returns:
> > > + * True if the CDCLK states don't match, false if they do.
> > > + */
> > > +bool intel_cdclk_changed(const struct intel_cdclk_state *a,
> > > +			 const struct intel_cdclk_state *b)
> > > +{
> > > +	return intel_cdclk_needs_modeset(a, b) ||
> > > +		a->voltage != b->voltage;
> > >  }
> > >  
> > >  /**
> > > @@ -1714,15 +1731,15 @@ bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
> > >  void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > >  		     const struct intel_cdclk_state *cdclk_state)
> > >  {
> > > -	if (intel_cdclk_state_compare(&dev_priv->cdclk.hw, cdclk_state))
> > > +	if (!intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state))
> > >  		return;
> > >  
> > >  	if (WARN_ON_ONCE(!dev_priv->display.set_cdclk))
> > >  		return;
> > >  
> > > -	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz, VCO %d kHz, ref %d kHz\n",
> > > +	DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz, VCO %d kHz, ref %d kHz, voltage %d\n",
> > >  			 cdclk_state->cdclk, cdclk_state->vco,
> > > -			 cdclk_state->ref);
> > > +			 cdclk_state->ref, cdclk_state->voltage);
> > >  
> > >  	dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index bd62c0a65bcd..32e7cca52da2 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11946,16 +11946,16 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> > >  		 * holding all the crtc locks, even if we don't end up
> > >  		 * touching the hardware
> > >  		 */
> > > -		if (!intel_cdclk_state_compare(&dev_priv->cdclk.logical,
> > > -					       &intel_state->cdclk.logical)) {
> > > +		if (intel_cdclk_changed(&dev_priv->cdclk.logical,
> > > +					&intel_state->cdclk.logical)) {
> > >  			ret = intel_lock_all_pipes(state);
> > >  			if (ret < 0)
> > >  				return ret;
> > >  		}
> > >  
> > >  		/* All pipes must be switched off while we change the cdclk. */
> > > -		if (!intel_cdclk_state_compare(&dev_priv->cdclk.actual,
> > > -					       &intel_state->cdclk.actual)) {
> > > +		if (intel_cdclk_needs_modeset(&dev_priv->cdclk.actual,
> > > +					      &intel_state->cdclk.actual)) {
> > >  			ret = intel_modeset_all_pipes(state);
> > >  			if (ret < 0)
> > >  				return ret;
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index a05ab3a1ab27..765a737700c5 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1324,8 +1324,10 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> > >  void intel_update_max_cdclk(struct drm_i915_private *dev_priv);
> > >  void intel_update_cdclk(struct drm_i915_private *dev_priv);
> > >  void intel_update_rawclk(struct drm_i915_private *dev_priv);
> > > -bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
> > > +bool intel_cdclk_needs_modeset(const struct intel_cdclk_state *a,
> > >  			       const struct intel_cdclk_state *b);
> > > +bool intel_cdclk_changed(const struct intel_cdclk_state *a,
> > > +			 const struct intel_cdclk_state *b);
> > >  void intel_set_cdclk(struct drm_i915_private *dev_priv,
> > >  		     const struct intel_cdclk_state *cdclk_state);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index 8af286c63d3b..5ac553fab7c7 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -705,7 +705,8 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
> > >  	gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
> > >  
> > >  	dev_priv->display.get_cdclk(dev_priv, &cdclk_state);
> > > -	WARN_ON(!intel_cdclk_state_compare(&dev_priv->cdclk.hw, &cdclk_state));
> > > +	/* Can't read out the voltage so can't use intel_cdclk_changed() */
> > > +	WARN_ON(intel_cdclk_needs_modeset(&dev_priv->cdclk.hw, &cdclk_state));
> > >  
> > >  	gen9_assert_dbuf_enabled(dev_priv);
> > >  
> > > -- 
> > > 2.13.6
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

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