[Intel-gfx] [PATCH] drm/i915: Revert RPS UP_EI value for SandyBridge and IvyBridge

[Ben Widawsky]

On Thu, 20 Dec 2012 17:57:04 -0800
Ben Widawsky <ben at bwidawsk.net> wrote:

> On Thu, Dec 20, 2012 at 03:34:11PM -0800, Jesse Barnes wrote:
> > On Wed, 24 Oct 2012 12:05:35 -0700
> > Ben Widawsky <ben at bwidawsk.net> wrote:
> > 
> > > On Sun, 21 Oct 2012 15:44:02 +0100
> > > Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > > 
> > > > Even though we do not use the EI mode for determining when to
> > > > change GPU frequencies for RPS, changing this value causes no
> > > > up interrupts to be generated whilst an OpenGL client runs.
> > > > 
> > > > Fixes regression from commit
> > > > 1ee9ae3244c4789f3184c5123f3b2d7e405b3f4c Author: Daniel Vetter
> > > > <daniel.vetter at ffwll.ch> Date:   Wed Aug 15 10:41:45 2012 +0200
> > > > 
> > > >     drm/i915: use hsw rps tuning values everywhere on gen6+
> > > > 
> > > > Reported-by: Eric Anholt <eric at anholt.net>
> > > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > > Cc: Eric Anholt <eric at anholt.net>
> > > > Cc: Ben Widawsky <ben at bwidawsk.net>
> > > > Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
> > > > Cc: stable at vger.kernel.org
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_pm.c |    2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c index 81e88c2..15b585e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -2493,7 +2493,7 @@ static void gen6_enable_rps(struct
> > > > drm_device *dev) 
> > > >  	I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
> > > >  	I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 245000);
> > > > -	I915_WRITE(GEN6_RP_UP_EI, 66000);
> > > > +	I915_WRITE(GEN6_RP_UP_EI, IS_HASWELL(dev) ? 66000 :
> > > > 100000); I915_WRITE(GEN6_RP_DOWN_EI, 350000);
> > > >  
> > > >  	I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 10);
> > > 
> > > 
> > > 
> > > 
> > > I've not verified this with interrupts, but simply reading back
> > > the current frequency using the sysfs interface.
> > > 
> > > What I've seen running xonotic on Ivybridge is that we do bump the
> > > frequency initially, and then stops. Nearing the end of the demo,
> > > we again raise the frequency. Note that on my system, both
> > > before, and after this patch, I am able to get to the max GPU
> > > frequency with the xonotic demo.
> > > 
> > > Specifically, on my IVB which has a range of 350->1100 with an
> > > RP1 of 650. I see the following (the demo is roughly 2 minutes)
> > > 
> > > without patch:
> > > Within a few seconds we cycle up to 750
> > > Nothing for about 30 seconds
> > > very slowly cycle up to 1100 (*just* before the demo ends)
> > > demo ends; throttle down to 350 quickly
> > > 
> > > with patch:
> > > Within a few seconds we cycle up to 1000
> > > Nothing for about 30 seconds
> > > cycle up to 1100
> > > demo ends; throttle down to 350 slowly
> > > 
> > > I think if this fixes someones critical issue, it's great, but
> > > unfortunately I do not see the problem the patch claims to fix.
> > > Furthermore, none of us can really make sense of why this has the
> > > effect that it does, but I believe a lot of that is because the
> > > workloads we run (in this case xonotic) are very blackbox.
> > > 
> > > Personally, on this IVB, I think the behavior before the patch is
> > > more desirable because it stays near RP1 for a longer period of
> > > time, and drops to RP0 quickly (but it's definitely a matter of
> > > opinion).
> > 
> > It's a balance between power and performance.  Running at the higher
> > freqs is definitely less power efficient, but without this patch we
> > definitely have a performance regression (running at 750 instead of
> > 1000 MHz for most of the demo, plus the cases Chris saw).
> > 
> > But we also don't want to prevent RC6 entry on vulnerable systems,
> > so maybe we need two sets of values or a broader set of changes
> > that work better for a swath of workloads.
> 
> To me this sounds like an idea which is better on paper than in
> practice. Unless we can generalize the various sets of values with
> knobs for users to pick from, it will really be overwhelming (I
> feel), and will probably provide more bad data than good. Fine
> grained knobs are already given via registers, and really pestering
> users can toy with those, if they want. Perhaps it's on us to add
> better comments to i915_reg.h to explain how we think it works a bit
> better?
> 
> > 
> > Also, the initial patch to use the HSW values implies that the
> > values are applicable across generations.  They're not.  They're
> > very specific to a given generation and potentially even different
> > versions of a given generation, so sharing the values is probably a
> > bad idea in general.
> > 
> > -- 
> > Jesse Barnes, Intel Open Source Technology Center
> 
> To set the record straight, I am pretty sure we can agree on a few
> things.
> 1. We don't really know wtf.
> 2. There will be no globally good set of values.
> 2a. There may even be no globally decent set of values.
> 3. 1 workload is insufficient to determine anything
> 
> If a workload is obviously too GPU render heavy, I think a decent
> solution is to just use the sysfs parameters to force a specific min.
> We don't really need to be skittish about setting this as really
> *most* things are too GPU render heavy for us at the moment. For the
> general case I'll assert the performance 'profile' is the right thing
> unless someone proves otherwise, and here we just want the set of
> values which throttle down faster than they throttle up.
> 

In the email in my head, that said "the power 'profile'"

> I think there is still some value in the proof of concept stuff you
> were hoping to do around a system wide GPU power governor thing.
> Maybe we can just set the RPS autothrottle values to be as dumb as
> possible, and then use that governor to set the min/max frequencies
> as it sees fit. Another similar option is to try to use SW RC6. If we
> had an intern, I'd even suggest to pick some decent ranges for all
> the thresholds and have that intern run benchmarks with all the
> different values.
> 
> Okay, done wasting your time now..