Re: [PATCH 1/2] drm/i915: Use efficient frequency for HSW/BDW

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 11, 2014 at 04:12:22PM +0000, Chris Wilson wrote:
> On Tue, Nov 11, 2014 at 04:13:09PM +0100, Daniel Vetter wrote:
> > On Sat, Nov 08, 2014 at 08:25:04AM +0000, Chris Wilson wrote:
> > > On Fri, Nov 07, 2014 at 02:25:38PM -0800, O'Rourke, Tom wrote:
> > > > On Fri, Nov 07, 2014 at 10:41:26AM +0000, Chris Wilson wrote:
> > > > > On Wed, Nov 05, 2014 at 05:31:34PM -0800, Tom.O'Rourke@intel.com wrote:
> > > > > > From: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > > > > 
> > > > > > Updated gen6|8_enable_rps() for Haswell and Broadwell
> > > > > > to use the efficient frequency read from pcode.
> > > > > > 
> > > > > > Added hsw_use_efficient_freq() to read efficient
> > > > > > frequency (aka RPe) from pcode.  The efficiency is
> > > > > > based on the frequency/power ratio (MHz/W); this is
> > > > > > considering GT power and not package power.  The
> > > > > > efficent frequency is the highest frequency for which
> > > > > > the frequency/power ratio is within some threshold of
> > > > > > the highest frequency/power ratio.
> > > > > > 
> > > > > > Also set the min_freq_softlimit to the efficient
> > > > > > frequency.  A fixed decrease in frequency results in
> > > > > > smaller decrease in power at frequencies less than RPe
> > > > > > than at frequencies above RPe.
> > > > > > 
> > > > > > Signed-off-by: Tom O'Rourke <Tom.O'Rourke@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/i915_reg.h |    1 +
> > > > > >  drivers/gpu/drm/i915/intel_pm.c |   22 ++++++++++++++++++++++
> > > > > >  2 files changed, 23 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > index d43fa0e..6fbfdec 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > > @@ -6010,6 +6010,7 @@ enum punit_power_well {
> > > > > >  #define   GEN6_ENCODE_RC6_VID(mv)		(((mv) - 245) / 5)
> > > > > >  #define   GEN6_DECODE_RC6_VID(vids)		(((vids) * 5) + 245)
> > > > > >  #define   DISPLAY_IPS_CONTROL			0x19
> > > > > > +#define	  HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL	0x1A
> > > > > >  #define GEN6_PCODE_DATA				0x138128
> > > > > >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> > > > > >  #define   GEN6_PCODE_FREQ_RING_RATIO_SHIFT	16
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > index 300d7e5..e4347d9 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > > @@ -4706,6 +4706,18 @@ static void parse_rp_state_cap(struct drm_i915_private *dev_priv, u32 rp_state_c
> > > > > >  		dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq;
> > > > > >  }
> > > > > >  
> > > > > > +static void hsw_use_efficient_freq(struct drm_i915_private *dev_priv)
> > > > > 
> > > > > use_efficient_freq() ?
> > > > > 
> > > > > Shouldn't this be:
> > > > > 
> > > > > parse_rp_state_cap()
> > > > >   {
> > > > >   ...
> > > > >   dev_priv->rps.efficient_freq = rps_rpe_freq(dev_priv);
> > > > >   ...
> > > > >   }
> > > > > 
> > > > The parse_rp_state_cap() function is for parsing the 
> > > > RP_STATE_CAP register.  The pcode read to get the 
> > > > efficient frequency seems out of scope.
> > > 
> > > Rename it then.  The function that sets rpe seems to be the ideal point
> > > to set rpe...
> > 
> > Well reading the rpe value seems to be platforms specific. I think
> > updating the comment as I've suggested is good enough, even when not
> > really pretty.
> 
> So why not add it to piece of platform specific code we already had for
> the rpe value?
> 
> Besides which the point that use_efficient_freq() is a poor function
> name seems to have fallen by the wayside.

Ok I've actually looked at the code and vlv/chv is completely separate. So
yeah I think best to drop the default value from parse_rp_state_cap, drop
the use_efficient_freq func and just open-code stuff like in vlv with an
if/else. Less code, less comments (no more XXX, yay!) and should be
clearer.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx