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

Message ID	1415237495-89576-2-git-send-email-Tom.O'Rourke@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Tom.O'Rourke@intel.com To: intel-gfx@lists.freedesktop.org Date: Wed, 5 Nov 2014 17:31:34 -0800 Message-Id: <1415237495-89576-2-git-send-email-Tom.O'Rourke@intel.com> In-Reply-To: <1415237495-89576-1-git-send-email-Tom.O'Rourke@intel.com> References: <1415237495-89576-1-git-send-email-Tom.O'Rourke@intel.com> Subject: [Intel-gfx] [PATCH 1/2] drm/i915: Use efficient frequency for HSW/BDW Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Tom.O'Rourke@intel.com Nov. 6, 2014, 1:31 a.m. UTC

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(+)

Daniel Vetter Nov. 7, 2014, 9:50 a.m. UTC | #1

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)
> +{
> +	u32 ddcc_status = 0;
> +	int ret;
> +
> +	ret = sandybridge_pcode_read(dev_priv,
> +					HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> +					&ddcc_status);
> +	if (0 == ret)
> +		dev_priv->rps.efficient_freq = (ddcc_status >> 8) & 0xff;

Control flow in the error case looks funny here - shouldn't we put the
adjustment of the softlimit into the (ret == 0) case here to avoid putting
garbage into it in case the pcode read falls over?

That would make the function name match reality a bit more too, since atm
it just reads the efficient_freq but doesn't use it. So as-is a bit
misnamed.
-Daniel

> +}
> +
>  static void gen9_enable_rps(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -4765,6 +4777,11 @@ static void gen8_enable_rps(struct drm_device *dev)
>  	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
>  	parse_rp_state_cap(dev_priv, rp_state_cap);
>  
> +	if (IS_BROADWELL(dev)) {
> +		hsw_use_efficient_freq(dev_priv);
> +		dev_priv->rps.min_freq_softlimit = dev_priv->rps.efficient_freq;
> +	}
> +
>  	/* 2b: Program RC6 thresholds.*/
>  	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
>  	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
> @@ -4860,6 +4877,11 @@ static void gen6_enable_rps(struct drm_device *dev)
>  
>  	parse_rp_state_cap(dev_priv, rp_state_cap);
>  
> +	if (IS_HASWELL(dev)) {
> +		hsw_use_efficient_freq(dev_priv);
> +		dev_priv->rps.min_freq_softlimit = dev_priv->rps.efficient_freq;
> +	}
> +
>  	/* disable the counters and set deterministic thresholds */
>  	I915_WRITE(GEN6_RC_CONTROL, 0);
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Chris Wilson Nov. 7, 2014, 10:41 a.m. UTC | #2

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);
  ...
  }


static in rps_rpe_freq(dev_priv) 
  {
	/* By default, prefer the nominal frequency for efficiency */
	int rpe = dev_priv->rps.rp1_freq;

	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) {
		u32 ddcc_status = 0;
		int ret;

		/* Some useful and informative comment */
		if (sandybridge_pcode_read(dev_priv,
					   HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
					   &ddcc_status) == 0)
			rpe = (ddcc_status >> 8) & 0xff;
	}

	return rpe;
  }

Tom.O'Rourke@intel.com Nov. 7, 2014, 10:20 p.m. UTC | #3

On Fri, Nov 07, 2014 at 10:50:02AM +0100, Daniel Vetter 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)
> > +{
> > +	u32 ddcc_status = 0;
> > +	int ret;
> > +
> > +	ret = sandybridge_pcode_read(dev_priv,
> > +					HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> > +					&ddcc_status);
> > +	if (0 == ret)
> > +		dev_priv->rps.efficient_freq = (ddcc_status >> 8) & 0xff;
> 
> Control flow in the error case looks funny here - shouldn't we put the
> adjustment of the softlimit into the (ret == 0) case here to avoid putting
> garbage into it in case the pcode read falls over?
> 
If the pcode read fails, the efficient_freq will not 
be set here and will still have the non-garbage value 
set in parse_rp_state_cap.

> That would make the function name match reality a bit more too, since atm
> it just reads the efficient_freq but doesn't use it. So as-is a bit
> misnamed.
> -Daniel
>
I agree the function is misnamed as-is.  If there are 
no objections, I will rewrite the patch to follow 
Daniel's recommendation and move adjusting the 
softlimit to hsw_use_efficient_freq.
 
> > +}
> > +
> >  static void gen9_enable_rps(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -4765,6 +4777,11 @@ static void gen8_enable_rps(struct drm_device *dev)
> >  	rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
> >  	parse_rp_state_cap(dev_priv, rp_state_cap);
> >  
> > +	if (IS_BROADWELL(dev)) {
> > +		hsw_use_efficient_freq(dev_priv);
> > +		dev_priv->rps.min_freq_softlimit = dev_priv->rps.efficient_freq;
> > +	}
> > +
> >  	/* 2b: Program RC6 thresholds.*/
> >  	I915_WRITE(GEN6_RC6_WAKE_RATE_LIMIT, 40 << 16);
> >  	I915_WRITE(GEN6_RC_EVALUATION_INTERVAL, 125000); /* 12500 * 1280ns */
> > @@ -4860,6 +4877,11 @@ static void gen6_enable_rps(struct drm_device *dev)
> >  
> >  	parse_rp_state_cap(dev_priv, rp_state_cap);
> >  
> > +	if (IS_HASWELL(dev)) {
> > +		hsw_use_efficient_freq(dev_priv);
> > +		dev_priv->rps.min_freq_softlimit = dev_priv->rps.efficient_freq;
> > +	}
> > +
> >  	/* disable the counters and set deterministic thresholds */
> >  	I915_WRITE(GEN6_RC_CONTROL, 0);
> >  
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Tom.O'Rourke@intel.com Nov. 7, 2014, 10:25 p.m. UTC | #4

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.

> 
> static in rps_rpe_freq(dev_priv) 
>   {
> 	/* By default, prefer the nominal frequency for efficiency */
> 	int rpe = dev_priv->rps.rp1_freq;
> 
> 	if (INTEL_GEN(dev_priv) >= 8 || IS_HASWELL(dev_priv)) {
> 		u32 ddcc_status = 0;
> 		int ret;
> 
> 		/* Some useful and informative comment */
> 		if (sandybridge_pcode_read(dev_priv,
> 					   HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> 					   &ddcc_status) == 0)
> 			rpe = (ddcc_status >> 8) & 0xff;
> 	}
> 
> 	return rpe;
>   }
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Chris Wilson Nov. 8, 2014, 8:25 a.m. UTC | #5

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...
-Chris

Daniel Vetter Nov. 11, 2014, 3:11 p.m. UTC | #6

On Fri, Nov 07, 2014 at 02:20:17PM -0800, O'Rourke, Tom wrote:
> On Fri, Nov 07, 2014 at 10:50:02AM +0100, Daniel Vetter wrote:
> > > +	ret = sandybridge_pcode_read(dev_priv,
> > > +					HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL,
> > > +					&ddcc_status);
> > > +	if (0 == ret)
> > > +		dev_priv->rps.efficient_freq = (ddcc_status >> 8) & 0xff;
> > 
> > Control flow in the error case looks funny here - shouldn't we put the
> > adjustment of the softlimit into the (ret == 0) case here to avoid putting
> > garbage into it in case the pcode read falls over?
> > 
> If the pcode read fails, the efficient_freq will not 
> be set here and will still have the non-garbage value 
> set in parse_rp_state_cap.

Oh right I've missed this. Maybe update the XXX comment in that function
and replace it with

	/* Platforms which support special RP-efficient values will
	 * overwrite this in their setup fucntions, use rp1_freq as
	 * default. */

Cheers, Daniel

Daniel Vetter Nov. 11, 2014, 3:13 p.m. UTC | #7

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.
-Daniel

Chris Wilson Nov. 11, 2014, 4:12 p.m. UTC | #8

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.
-Chris

Daniel Vetter Nov. 12, 2014, 8:37 a.m. UTC | #9

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

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

Commit Message

Comments

Patch