[Intel-gfx] [PATCH] drm/i915: Delay the relase of the forcewake by a jiffie

[Ben Widawsky]

On Mon, Feb 24, 2014 at 08:30:22AM +0000, Chris Wilson wrote:
> On Sun, Feb 23, 2014 at 12:12:25PM -0800, Ben Widawsky wrote:
> > On Mon, Aug 26, 2013 at 12:06:43PM +0100, Chris Wilson wrote:
> > > Obtaining the forcwake requires expensive and time consuming
> > > serialisation. And we often try to obtain the forcewake multiple times
> > > in very quick succession. We can reduce the overhead of these sequences
> > > by delaying the forcewake release, and so not hammer the hw quite so
> > > hard.
> > > 
> > > I was hoping this would help with the spurious
> > > [drm:__gen6_gt_force_wake_mt_get] *ERROR* Timed out waiting for forcewake old ack to clear.
> > > found on Haswell. Alas not.
> > > 
> > > v2: Fix teardown ordering - unmap the regs after turning off forcewake,
> > > and make sure we do turn off forcewake - both found by Ville.
> > > 
> > > Note: I have no claims for improved performance, stablity or power
> > > comsumption for this patch. We should not be hitting the registers often
> > > enough for this to improve benchmarks, but given the nature of our hw it
> > > is likely to improve long term stability.
> > 
> > I don't understand how or why but from casual powertop observation, this
> > workqueue uses between 4x and 50x or the nearest other i915 workqueue
> > (i915_gem_retire_work_handler). On my x240...
> 
> What does that mean? We expect this to be frequently hit since we use it
> so often, but retire_work_handler is only run every couple of seconds to
> trim our lists (in case of userspace failure).
> 
> The idea is to have the forcewake reset run during the next scheduler event,
> so using a workqueue was probably the wrong choice and perhaps we should have
> used a timer instead?
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

I just posted it for the sake of potentially starting a conversation. I
assume we always expected power consumption to go up since [it would
seem at least] we're usually just deferring the release of the last
put(). But then we have the race to idle argument in there as well.

In any event, I wasn't asking for any change, was just surprised by the
data.


-- 
Ben Widawsky, Intel Open Source Technology Center