Re: [PATCH 11/13] drm/i915/cnl: Only request voltage frequency switching when needed.

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Oct 05, 2017 at 12:07:08PM +0000, Mika Kahola wrote:
> On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote:
> > At cdclk initialization at spec they tell us to always use dvfs
> > or avoid change cdclk if it fails.
> > 
> > Here on pll sequence, spec tells us to only use dvfs "if the
> > frequency will result in a change to the voltage requirement."
> > 
> > So in order to respect that and avoid necessary interactions with
> > PCODE we compare our current needed level with the current set level
> > and only perform dvfs sequences when we need to change that level.
> > 
> > v2: - Add missing blank line after declaration block
> >     - s/need/needs: When adding doc needs sounded better.
> > 
> > Cc: Mika Kahola <mika.kahola@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_cdclk.c    | 10 ++++++++++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c | 16 +++++++++-------
> >  drivers/gpu/drm/i915/intel_drv.h      |  1 +
> >  3 files changed, 20 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c
> > index c62d6e752fb7..8111a13079e1 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1545,6 +1545,16 @@ int cnl_dvfs_new_level(int cdclk, int portclk)
> >  		return 2;
> >  }
> >  
> > +bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int
> > level)
> > +{
> > +	int old_level;
> I was thinking of naming of this variable. We are fetching the current
> level so why not rename this variable as cur_level?

good idea. cur_level it will be on the next version! ;)
thanks

> 
> Either way,
> 
> Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> 
> > +
> > +	mutex_lock(&dev_priv->rps.hw_lock);
> > +	sandybridge_pcode_read(dev_priv, SKL_PCODE_CDCLK_CONTROL,
> > &old_level);
> > +	mutex_unlock(&dev_priv->rps.hw_lock);
> > +	return level != old_level;
> > +}
> > +
> >  static void cnl_set_cdclk(struct drm_i915_private *dev_priv,
> >  			  const struct intel_cdclk_state
> > *cdclk_state)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index 0dddbd3a7a97..85c000891439 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -1970,9 +1970,10 @@ static void cnl_ddi_pll_enable(struct
> > drm_i915_private *dev_priv,
> >  			       struct intel_shared_dpll *pll)
> >  {
> >  	uint32_t val;
> > -	int ret;
> > +	int ret = 0;
> >  	int level;
> >  	int cdclk, portclk;
> > +	bool change_level;
> >  
> >  	/* 1. Enable DPLL power in DPLL_ENABLE. */
> >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > @@ -2011,7 +2012,12 @@ static void cnl_ddi_pll_enable(struct
> > drm_i915_private *dev_priv,
> >  	 * requirement, follow the Display Voltage Frequency
> > Switching
> >  	 * (DVFS) Sequence Before Frequency Change
> >  	 */
> > -	ret = cnl_dvfs_pre_change(dev_priv);
> > +	cdclk = dev_priv->cdclk.hw.cdclk;
> > +	portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > +	level = cnl_dvfs_new_level(cdclk, portclk);
> > +	change_level = cnl_dvfs_needs_change(dev_priv, level);
> > +	if (change_level)
> > +		ret = cnl_dvfs_pre_change(dev_priv);
> >  
> >  	/* 6. Enable DPLL in DPLL_ENABLE. */
> >  	val = I915_READ(CNL_DPLL_ENABLE(pll->id));
> > @@ -2031,12 +2037,8 @@ static void cnl_ddi_pll_enable(struct
> > drm_i915_private *dev_priv,
> >  	 * requirement, follow the Display Voltage Frequency
> > Switching
> >  	 * (DVFS) Sequence After Frequency Change
> >  	 */
> > -	if (ret == 0) {
> > -		cdclk = dev_priv->cdclk.hw.cdclk;
> > -		portclk = intel_ddi_port_clock(dev_priv, pll->id);
> > -		level = cnl_dvfs_new_level(cdclk, portclk);
> > +	if (change_level && ret == 0)
> >  		cnl_dvfs_post_change(dev_priv, level);
> > -	}
> >  
> >  	/*
> >  	 * 9. turn on the clock for the DDI and map the DPLL to the
> > DDI
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index dec11d7b15ab..8621110401e3 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1326,6 +1326,7 @@ void cnl_uninit_cdclk(struct drm_i915_private
> > *dev_priv);
> >  int cnl_dvfs_pre_change(struct drm_i915_private *dev_priv);
> >  void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int
> > level);
> >  int cnl_dvfs_new_level(int cdclk, int portclk);
> > +bool cnl_dvfs_needs_change(struct drm_i915_private *dev_priv, int
> > level);
> >  void bxt_init_cdclk(struct drm_i915_private *dev_priv);
> >  void bxt_uninit_cdclk(struct drm_i915_private *dev_priv);
> >  void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv);
> -- 
> Mika Kahola - Intel OTC
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx