Message ID | 20171003070614.18396-11-rodrigo.vivi@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote: > When port clock is zero or undefined we base our > calculation on cdclk. So, same function can be used > for port clock == 0 now that we have the same default "2". > > v2: s/get/new: When documenting "get" sounded ambiguous > because we could be getting the current level at pcode. > > 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 | 28 +++++++++++++---------- > ----- > drivers/gpu/drm/i915/intel_dpll_mgr.c | 12 +----------- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 3 files changed, 15 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > b/drivers/gpu/drm/i915/intel_cdclk.c > index 7e9c4444c844..c62d6e752fb7 100644 > --- a/drivers/gpu/drm/i915/intel_cdclk.c > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > @@ -1535,12 +1535,22 @@ void cnl_dvfs_post_change(struct > drm_i915_private *dev_priv, int level) > mutex_unlock(&dev_priv->rps.hw_lock); > } > > +int cnl_dvfs_new_level(int cdclk, int portclk) > +{ > + if (cdclk == 168000 && portclk <= 594000) > + return 0; > + else if (cdclk == 336000 && portclk <= 594000) > + return 1; > + else > + return 2; > +} > + Maybe we could leave the function name as cnl_get_dvfs_level()? The original one is stripped down later in this patch. > static void cnl_set_cdclk(struct drm_i915_private *dev_priv, > const struct intel_cdclk_state > *cdclk_state) > { > int cdclk = cdclk_state->cdclk; > int vco = cdclk_state->vco; > - u32 val, divider, pcu_ack; > + u32 val, divider, level; > > if (cnl_dvfs_pre_change(dev_priv)) > return; > @@ -1561,19 +1571,6 @@ static void cnl_set_cdclk(struct > drm_i915_private *dev_priv, > break; > } > > - switch (cdclk) { > - case 168000: > - pcu_ack = 0; > - break; > - case 336000: > - pcu_ack = 1; > - break; > - case 528000: > - default: > - pcu_ack = 2; > - break; > - } > - > if (dev_priv->cdclk.hw.vco != 0 && > dev_priv->cdclk.hw.vco != vco) > cnl_cdclk_pll_disable(dev_priv); > @@ -1590,7 +1587,8 @@ static void cnl_set_cdclk(struct > drm_i915_private *dev_priv, > I915_WRITE(CDCLK_CTL, val); > > /* inform PCU of the change */ > - cnl_dvfs_post_change(dev_priv, pcu_ack); > + level = cnl_dvfs_new_level(cdclk, 0); > + cnl_dvfs_post_change(dev_priv, level); > > intel_update_cdclk(dev_priv); > } > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > index a71a6c396bbd..0dddbd3a7a97 100644 > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > @@ -1966,16 +1966,6 @@ static const struct intel_dpll_mgr bxt_pll_mgr > = { > .dump_hw_state = bxt_dump_hw_state, > }; > > -static int cnl_get_dvfs_level(int cdclk, int portclk) > -{ > - if (cdclk == 168000 && portclk <= 594000) > - return 0; > - else if (cdclk == 336000 && portclk <= 594000) > - return 1; > - else > - return 2; > -} > - > static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv, > struct intel_shared_dpll *pll) > { > @@ -2044,7 +2034,7 @@ static void cnl_ddi_pll_enable(struct > drm_i915_private *dev_priv, > if (ret == 0) { > cdclk = dev_priv->cdclk.hw.cdclk; > portclk = intel_ddi_port_clock(dev_priv, pll->id); > - level = cnl_get_dvfs_level(cdclk, portclk); > + level = cnl_dvfs_new_level(cdclk, portclk); > cnl_dvfs_post_change(dev_priv, level); > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 934ccf17f8ab..dec11d7b15ab 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1325,6 +1325,7 @@ void cnl_init_cdclk(struct drm_i915_private > *dev_priv); > 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); > 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 Wed, Oct 04, 2017 at 01:20:57PM +0000, Mika Kahola wrote: > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote: > > When port clock is zero or undefined we base our > > calculation on cdclk. So, same function can be used > > for port clock == 0 now that we have the same default "2". > > > > v2: s/get/new: When documenting "get" sounded ambiguous > > because we could be getting the current level at pcode. > > > > 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 | 28 +++++++++++++---------- > > ----- > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 12 +----------- > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > 3 files changed, 15 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > b/drivers/gpu/drm/i915/intel_cdclk.c > > index 7e9c4444c844..c62d6e752fb7 100644 > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > @@ -1535,12 +1535,22 @@ void cnl_dvfs_post_change(struct > > drm_i915_private *dev_priv, int level) > > mutex_unlock(&dev_priv->rps.hw_lock); > > } > > > > +int cnl_dvfs_new_level(int cdclk, int portclk) > > +{ > > + if (cdclk == 168000 && portclk <= 594000) > > + return 0; > > + else if (cdclk == 336000 && portclk <= 594000) > > + return 1; > > + else > > + return 2; > > +} > > + > Maybe we could leave the function name as cnl_get_dvfs_level()? > The original one is stripped down later in this patch. Since I was exporting I decided to use the same standard as others and move dvfs back a bit so cnl_dvfs_get_level... But also when I was documenting on the last patch I noticed that "get level" is kind of ambiguous... which level? the current or the new? the more natural is current, but this function also get the new desired/needed. But I'm open to other suggestions that fix the ambiguity. > > > static void cnl_set_cdclk(struct drm_i915_private *dev_priv, > > const struct intel_cdclk_state > > *cdclk_state) > > { > > int cdclk = cdclk_state->cdclk; > > int vco = cdclk_state->vco; > > - u32 val, divider, pcu_ack; > > + u32 val, divider, level; > > > > if (cnl_dvfs_pre_change(dev_priv)) > > return; > > @@ -1561,19 +1571,6 @@ static void cnl_set_cdclk(struct > > drm_i915_private *dev_priv, > > break; > > } > > > > - switch (cdclk) { > > - case 168000: > > - pcu_ack = 0; > > - break; > > - case 336000: > > - pcu_ack = 1; > > - break; > > - case 528000: > > - default: > > - pcu_ack = 2; > > - break; > > - } > > - > > if (dev_priv->cdclk.hw.vco != 0 && > > dev_priv->cdclk.hw.vco != vco) > > cnl_cdclk_pll_disable(dev_priv); > > @@ -1590,7 +1587,8 @@ static void cnl_set_cdclk(struct > > drm_i915_private *dev_priv, > > I915_WRITE(CDCLK_CTL, val); > > > > /* inform PCU of the change */ > > - cnl_dvfs_post_change(dev_priv, pcu_ack); > > + level = cnl_dvfs_new_level(cdclk, 0); > > + cnl_dvfs_post_change(dev_priv, level); > > > > intel_update_cdclk(dev_priv); > > } > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > index a71a6c396bbd..0dddbd3a7a97 100644 > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > @@ -1966,16 +1966,6 @@ static const struct intel_dpll_mgr bxt_pll_mgr > > = { > > .dump_hw_state = bxt_dump_hw_state, > > }; > > > > -static int cnl_get_dvfs_level(int cdclk, int portclk) > > -{ > > - if (cdclk == 168000 && portclk <= 594000) > > - return 0; > > - else if (cdclk == 336000 && portclk <= 594000) > > - return 1; > > - else > > - return 2; > > -} > > - > > static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv, > > struct intel_shared_dpll *pll) > > { > > @@ -2044,7 +2034,7 @@ static void cnl_ddi_pll_enable(struct > > drm_i915_private *dev_priv, > > if (ret == 0) { > > cdclk = dev_priv->cdclk.hw.cdclk; > > portclk = intel_ddi_port_clock(dev_priv, pll->id); > > - level = cnl_get_dvfs_level(cdclk, portclk); > > + level = cnl_dvfs_new_level(cdclk, portclk); > > cnl_dvfs_post_change(dev_priv, level); > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 934ccf17f8ab..dec11d7b15ab 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -1325,6 +1325,7 @@ void cnl_init_cdclk(struct drm_i915_private > > *dev_priv); > > 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); > > 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 >
Em Qui, 2017-10-05 às 07:59 -0700, Rodrigo Vivi escreveu: > On Wed, Oct 04, 2017 at 01:20:57PM +0000, Mika Kahola wrote: > > On Tue, 2017-10-03 at 00:06 -0700, Rodrigo Vivi wrote: > > > When port clock is zero or undefined we base our > > > calculation on cdclk. So, same function can be used > > > for port clock == 0 now that we have the same default "2". > > > > > > v2: s/get/new: When documenting "get" sounded ambiguous > > > because we could be getting the current level at pcode. > > > > > > 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 | 28 +++++++++++++------ > > > ---- > > > ----- > > > drivers/gpu/drm/i915/intel_dpll_mgr.c | 12 +----------- > > > drivers/gpu/drm/i915/intel_drv.h | 1 + > > > 3 files changed, 15 insertions(+), 26 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c > > > b/drivers/gpu/drm/i915/intel_cdclk.c > > > index 7e9c4444c844..c62d6e752fb7 100644 > > > --- a/drivers/gpu/drm/i915/intel_cdclk.c > > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c > > > @@ -1535,12 +1535,22 @@ void cnl_dvfs_post_change(struct > > > drm_i915_private *dev_priv, int level) > > > mutex_unlock(&dev_priv->rps.hw_lock); > > > } > > > > > > +int cnl_dvfs_new_level(int cdclk, int portclk) > > > +{ > > > + if (cdclk == 168000 && portclk <= 594000) > > > + return 0; > > > + else if (cdclk == 336000 && portclk <= 594000) > > > + return 1; > > > + else > > > + return 2; > > > +} > > > + > > > > Maybe we could leave the function name as cnl_get_dvfs_level()? > > The original one is stripped down later in this patch. > > Since I was exporting I decided to use the same standard as others > and move dvfs back a bit > so cnl_dvfs_get_level... > But also when I was documenting on the last patch I noticed > that "get level" is kind of ambiguous... which level? the current > or the new? the more natural is current, but this function also > get the new desired/needed. > > But I'm open to other suggestions that fix the ambiguity. Get current, compute/calculate new. > > > > > > static void cnl_set_cdclk(struct drm_i915_private *dev_priv, > > > const struct intel_cdclk_state > > > *cdclk_state) > > > { > > > int cdclk = cdclk_state->cdclk; > > > int vco = cdclk_state->vco; > > > - u32 val, divider, pcu_ack; > > > + u32 val, divider, level; > > > > > > if (cnl_dvfs_pre_change(dev_priv)) > > > return; > > > @@ -1561,19 +1571,6 @@ static void cnl_set_cdclk(struct > > > drm_i915_private *dev_priv, > > > break; > > > } > > > > > > - switch (cdclk) { > > > - case 168000: > > > - pcu_ack = 0; > > > - break; > > > - case 336000: > > > - pcu_ack = 1; > > > - break; > > > - case 528000: > > > - default: > > > - pcu_ack = 2; > > > - break; > > > - } > > > - > > > if (dev_priv->cdclk.hw.vco != 0 && > > > dev_priv->cdclk.hw.vco != vco) > > > cnl_cdclk_pll_disable(dev_priv); > > > @@ -1590,7 +1587,8 @@ static void cnl_set_cdclk(struct > > > drm_i915_private *dev_priv, > > > I915_WRITE(CDCLK_CTL, val); > > > > > > /* inform PCU of the change */ > > > - cnl_dvfs_post_change(dev_priv, pcu_ack); > > > + level = cnl_dvfs_new_level(cdclk, 0); > > > + cnl_dvfs_post_change(dev_priv, level); > > > > > > intel_update_cdclk(dev_priv); > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > index a71a6c396bbd..0dddbd3a7a97 100644 > > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c > > > @@ -1966,16 +1966,6 @@ static const struct intel_dpll_mgr > > > bxt_pll_mgr > > > = { > > > .dump_hw_state = bxt_dump_hw_state, > > > }; > > > > > > -static int cnl_get_dvfs_level(int cdclk, int portclk) > > > -{ > > > - if (cdclk == 168000 && portclk <= 594000) > > > - return 0; > > > - else if (cdclk == 336000 && portclk <= 594000) > > > - return 1; > > > - else > > > - return 2; > > > -} > > > - > > > static void cnl_ddi_pll_enable(struct drm_i915_private > > > *dev_priv, > > > struct intel_shared_dpll *pll) > > > { > > > @@ -2044,7 +2034,7 @@ static void cnl_ddi_pll_enable(struct > > > drm_i915_private *dev_priv, > > > if (ret == 0) { > > > cdclk = dev_priv->cdclk.hw.cdclk; > > > portclk = intel_ddi_port_clock(dev_priv, pll- > > > >id); > > > - level = cnl_get_dvfs_level(cdclk, portclk); > > > + level = cnl_dvfs_new_level(cdclk, portclk); > > > cnl_dvfs_post_change(dev_priv, level); > > > } > > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > > b/drivers/gpu/drm/i915/intel_drv.h > > > index 934ccf17f8ab..dec11d7b15ab 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -1325,6 +1325,7 @@ void cnl_init_cdclk(struct drm_i915_private > > > *dev_priv); > > > 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); > > > 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 7e9c4444c844..c62d6e752fb7 100644 --- a/drivers/gpu/drm/i915/intel_cdclk.c +++ b/drivers/gpu/drm/i915/intel_cdclk.c @@ -1535,12 +1535,22 @@ void cnl_dvfs_post_change(struct drm_i915_private *dev_priv, int level) mutex_unlock(&dev_priv->rps.hw_lock); } +int cnl_dvfs_new_level(int cdclk, int portclk) +{ + if (cdclk == 168000 && portclk <= 594000) + return 0; + else if (cdclk == 336000 && portclk <= 594000) + return 1; + else + return 2; +} + static void cnl_set_cdclk(struct drm_i915_private *dev_priv, const struct intel_cdclk_state *cdclk_state) { int cdclk = cdclk_state->cdclk; int vco = cdclk_state->vco; - u32 val, divider, pcu_ack; + u32 val, divider, level; if (cnl_dvfs_pre_change(dev_priv)) return; @@ -1561,19 +1571,6 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv, break; } - switch (cdclk) { - case 168000: - pcu_ack = 0; - break; - case 336000: - pcu_ack = 1; - break; - case 528000: - default: - pcu_ack = 2; - break; - } - if (dev_priv->cdclk.hw.vco != 0 && dev_priv->cdclk.hw.vco != vco) cnl_cdclk_pll_disable(dev_priv); @@ -1590,7 +1587,8 @@ static void cnl_set_cdclk(struct drm_i915_private *dev_priv, I915_WRITE(CDCLK_CTL, val); /* inform PCU of the change */ - cnl_dvfs_post_change(dev_priv, pcu_ack); + level = cnl_dvfs_new_level(cdclk, 0); + cnl_dvfs_post_change(dev_priv, level); intel_update_cdclk(dev_priv); } diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c b/drivers/gpu/drm/i915/intel_dpll_mgr.c index a71a6c396bbd..0dddbd3a7a97 100644 --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c @@ -1966,16 +1966,6 @@ static const struct intel_dpll_mgr bxt_pll_mgr = { .dump_hw_state = bxt_dump_hw_state, }; -static int cnl_get_dvfs_level(int cdclk, int portclk) -{ - if (cdclk == 168000 && portclk <= 594000) - return 0; - else if (cdclk == 336000 && portclk <= 594000) - return 1; - else - return 2; -} - static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv, struct intel_shared_dpll *pll) { @@ -2044,7 +2034,7 @@ static void cnl_ddi_pll_enable(struct drm_i915_private *dev_priv, if (ret == 0) { cdclk = dev_priv->cdclk.hw.cdclk; portclk = intel_ddi_port_clock(dev_priv, pll->id); - level = cnl_get_dvfs_level(cdclk, portclk); + level = cnl_dvfs_new_level(cdclk, portclk); cnl_dvfs_post_change(dev_priv, level); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 934ccf17f8ab..dec11d7b15ab 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1325,6 +1325,7 @@ void cnl_init_cdclk(struct drm_i915_private *dev_priv); 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); 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);
When port clock is zero or undefined we base our calculation on cdclk. So, same function can be used for port clock == 0 now that we have the same default "2". v2: s/get/new: When documenting "get" sounded ambiguous because we could be getting the current level at pcode. 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 | 28 +++++++++++++--------------- drivers/gpu/drm/i915/intel_dpll_mgr.c | 12 +----------- drivers/gpu/drm/i915/intel_drv.h | 1 + 3 files changed, 15 insertions(+), 26 deletions(-)