[PATCH 1/2] drm/i915: Print ring min freq scaling

[PATCH 1/2] drm/i915: Print ring min freq scaling

[Ben Widawsky]

This allows us to keep track of the values being set if we want to tweak
this code.

Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index d27eda6..31cf188 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3671,6 +3671,9 @@ void gen6_update_ring_freq(struct drm_device *dev)
 			ring_freq = (gpu_freq * 5 + 3) / 4;
 			ring_freq = max(min_ring_freq, ring_freq);
 			/* leave ia_freq as the default, chosen by cpufreq */
+
+			DRM_DEBUG_DRIVER("Setup min ring frequency %dMHz for GT freq %dMHz\n",
+					 ring_freq * 100, gpu_freq * 50);
 		} else {
 			/* On older processors, there is no separate ring
 			 * clock domain, so in order to boost the bandwidth
@@ -3684,6 +3687,9 @@ void gen6_update_ring_freq(struct drm_device *dev)
 			else
 				ia_freq = max_ia_freq - ((diff * scaling_factor) / 2);
 			ia_freq = DIV_ROUND_CLOSEST(ia_freq, 100);
+
+			DRM_DEBUG_DRIVER("Setup min ring frequency %dMHz for GT freq %dMHz\n",
+					 ia_freq * 100, gpu_freq * 50);
 		}
 
 		sandybridge_pcode_write(dev_priv,
-- 
1.8.4

[PATCH 2/2] drm/i915: Use the real cpu max frequency for ring scaling

[Ben Widawsky]

The policy's max frequency is not equal to the CPU's max frequency. The
ring frequency is derived from the CPU frequency, and not the policy
frequency.

One example of how this may differ through sysfs. If the sysfs max
frequency is modified, that will be used for the max ring frequency
calculation.
(/sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq). As far as I
know, no current governor uses anything but max as the default, but in
theory, they could. Similarly distributions might set policy as part of
their init process.

It's ideal to use the real frequency because when we're currently scaled
up on the GPU. In this case we likely want to race to idle, and using a
less than max ring frequency is non-optimal for this situation.

AFAIK, this patch should have no impact on a majority of people.

This behavior hasn't been changed since it was first introduced:
commit 23b2f8bb92feb83127679c53633def32d3108e70
Author: Jesse Barnes <jbarnes@virtuousgeek.org>
Date:   Tue Jun 28 13:04:16 2011 -0700

    drm/i915: load a ring frequency scaling table v3

CC: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/intel_pm.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 31cf188..3212d3b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3639,16 +3639,21 @@ void gen6_update_ring_freq(struct drm_device *dev)
 	unsigned int gpu_freq;
 	unsigned int max_ia_freq, min_ring_freq;
 	int scaling_factor = 180;
+	struct cpufreq_policy *policy;
 
 	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
 
-	max_ia_freq = cpufreq_quick_get_max(0);
-	/*
-	 * Default to measured freq if none found, PCU will ensure we don't go
-	 * over
-	 */
-	if (!max_ia_freq)
+	policy = cpufreq_cpu_get(0);
+	if (policy) {
+		max_ia_freq = policy->cpuinfo.max_freq;
+		cpufreq_cpu_put(policy);
+	} else {
+		/*
+		 * Default to measured freq if none found, PCU will ensure we
+		 * don't go over
+		 */
 		max_ia_freq = tsc_khz;
+	}
 
 	/* Convert from kHz to MHz */
 	max_ia_freq /= 1000;
-- 
1.8.4

Re: [PATCH 2/2] drm/i915: Use the real cpu max frequency for ring scaling

[Chris Wilson]

On Wed, Sep 25, 2013 at 04:35:33PM -0700, Ben Widawsky wrote:
> The policy's max frequency is not equal to the CPU's max frequency. The
> ring frequency is derived from the CPU frequency, and not the policy
> frequency.
> 
> One example of how this may differ through sysfs. If the sysfs max
> frequency is modified, that will be used for the max ring frequency
> calculation.
> (/sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq). As far as I
> know, no current governor uses anything but max as the default, but in
> theory, they could. Similarly distributions might set policy as part of
> their init process.

Actually this seems to differ based on policy as well. The pstate may
disable turbo mode, in which case policy->max <= policy->cpuinfo.max.
But excluding user intervention, it seems that max is what we want, and
it is possibly arguable that we do not want to push the core to turbo
for a GPU bound workload - though I'm not sure if the ring frequency
continues to scale with turbo cpu frequencies.
 
> It's ideal to use the real frequency because when we're currently scaled
> up on the GPU. In this case we likely want to race to idle, and using a
> less than max ring frequency is non-optimal for this situation.

Also note that scaling the cpu frequency cuts into our thermal headroom,
so for ULT devices it may not be clearly beneficial.
 
> AFAIK, this patch should have no impact on a majority of people.

And also obsolete since HSW.
 
> This behavior hasn't been changed since it was first introduced:
> commit 23b2f8bb92feb83127679c53633def32d3108e70
> Author: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date:   Tue Jun 28 13:04:16 2011 -0700
> 
>     drm/i915: load a ring frequency scaling table v3
> 
> CC: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>

FWIW,

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I think using the real range of the CPU frequency rather than the
adjusted policy maximum is sensible. Just the hw implementation is just
a bit silly and it is not clear exactly what the best policy should be.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Print ring min freq scaling

[Chris Wilson]

On Wed, Sep 25, 2013 at 04:35:32PM -0700, Ben Widawsky wrote:
> This allows us to keep track of the values being set if we want to tweak
> this code.
> 
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d27eda6..31cf188 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3671,6 +3671,9 @@ void gen6_update_ring_freq(struct drm_device *dev)
>  			ring_freq = (gpu_freq * 5 + 3) / 4;
>  			ring_freq = max(min_ring_freq, ring_freq);
>  			/* leave ia_freq as the default, chosen by cpufreq */
> +
> +			DRM_DEBUG_DRIVER("Setup min ring frequency %dMHz for GT freq %dMHz\n",
> +					 ring_freq * 100, gpu_freq * 50);
>  		} else {
>  			/* On older processors, there is no separate ring
>  			 * clock domain, so in order to boost the bandwidth
> @@ -3684,6 +3687,9 @@ void gen6_update_ring_freq(struct drm_device *dev)
>  			else
>  				ia_freq = max_ia_freq - ((diff * scaling_factor) / 2);
>  			ia_freq = DIV_ROUND_CLOSEST(ia_freq, 100);
> +
> +			DRM_DEBUG_DRIVER("Setup min ring frequency %dMHz for GT freq %dMHz\n",
> +					 ia_freq * 100, gpu_freq * 50);

This is not a ring freq, but a cpu core freq.
Also would be wise to point out that this information is in
/sys/kernel/debug/dri/0/i915_ring_freq_table
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Print ring min freq scaling

[Ben Widawsky]

On Thu, Sep 26, 2013 at 12:49:37AM +0100, Chris Wilson wrote:
> On Wed, Sep 25, 2013 at 04:35:32PM -0700, Ben Widawsky wrote:
> > This allows us to keep track of the values being set if we want to tweak
> > this code.
> > 
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index d27eda6..31cf188 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3671,6 +3671,9 @@ void gen6_update_ring_freq(struct drm_device *dev)
> >  			ring_freq = (gpu_freq * 5 + 3) / 4;
> >  			ring_freq = max(min_ring_freq, ring_freq);
> >  			/* leave ia_freq as the default, chosen by cpufreq */
> > +
> > +			DRM_DEBUG_DRIVER("Setup min ring frequency %dMHz for GT freq %dMHz\n",
> > +					 ring_freq * 100, gpu_freq * 50);
> >  		} else {
> >  			/* On older processors, there is no separate ring
> >  			 * clock domain, so in order to boost the bandwidth
> > @@ -3684,6 +3687,9 @@ void gen6_update_ring_freq(struct drm_device *dev)
> >  			else
> >  				ia_freq = max_ia_freq - ((diff * scaling_factor) / 2);
> >  			ia_freq = DIV_ROUND_CLOSEST(ia_freq, 100);
> > +
> > +			DRM_DEBUG_DRIVER("Setup min ring frequency %dMHz for GT freq %dMHz\n",
> > +					 ia_freq * 100, gpu_freq * 50);
> 
> This is not a ring freq, but a cpu core freq.
> Also would be wise to point out that this information is in
> /sys/kernel/debug/dri/0/i915_ring_freq_table
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

Ooops. I forgot about that interface. Ignore this patch.

You are incorrect in saying it's _just_ the CPU core frequency. This is
the ring and IA frequency on SNB + IVB. The frequency is tied to the CPU
core on SNB + IVB. It is not tied to the cpu core on Haswell.

To make the print more generic, and correct across the board it makes
sense to say we're setting the ring frequency (to me). Even the
programming guides typically call this, "IA/ring" frequency.

Above is only for posterity. Again, forget the patch.

-- 
Ben Widawsky, Intel Open Source Technology Center