| Message ID | 20171003070614.18396-12-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? 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);
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 >
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; + + 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);
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(-)