Re: [PATCH 2/3] drm/i915: reference counted forcewake

[Ben Widawsky <ben@bwidawsk.net>]

On Sat, Apr 16, 2011 at 07:52:44AM +0100, Chris Wilson wrote:
> On Thu, 14 Apr 2011 11:13:46 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> > Provide a reference count to track the forcewake state of the GPU. The
> > savings is up to 1 UC read if the unit is already awake, but the main
> > goal is to give userspace some mechanism to wake the GPU.
> > 
> > v2:
> > Added a spin_lock for synchronizing access to forcewake. The lock
> > should not be heavily contested because any caller will either have
> > struct_mutex or config.mutex, and those locks should be much more
> > serializing than this. In terms of locking order:
> > 
> > 1.[config.mutex]
> > 2.	[struct_mutex]
> > 3.		forcewake_lock
> > 
> > By default i915_read[bwlq] functions now acquire forcewake_lock. Added a new
> > _locked version of the read function for callers wishing to prevent the
> > GT from possibly going to sleep in between reads. (We could also use
> > these functions to try to get system state if the lock somehow becomes
> > stuck).
> 
> This patch needs to be split again. I want the update to the existing
> interface and users to be separate from the introduction of a new
> interface. (This means that should we ever regret that interface we can
> rip it out without undoing the fixes.)
> 
> Replacing the get()...long sequence of read/writes...put() did make me cry
> a little, but it is indeed safer to move the check into the individual
> reads (though there is nothing to prevent the rc6-window from closing
> during the middle of that sequence... we need to check that is ok) and too
> ugly to special case those rare times when we do modify 20 regs at once
> (or maybe we have to?).

In case you didn't notice, there is a new interface
i915_read##x##_awake. This should serve that need. In other words:
gen6_gt_force_wake_get()
while(foo)
	i915_read32_awake()
gen6_gt_force_wake_put()

I'll split the patches. I can also use the awake() variant for the
existing users, if you're okay with the awake() function (I was actually
expecting a comment from you on that). For the relevant functions, it
should be as simple as:
s/I915_READ32/i915_read32_awake
s/__gen6_gt_force_wake_/gen6_gt_force_wake_/

Jesse has started the email to verify IIR is in fact a problem for us.
So I'll postpone resubmitting the patches until that's confirmed.

> 
> Again, let's change the macro without removing the existing get()...put()
> users and then remove those as a patch+review on a per-case basis.
> 
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -2025,6 +2025,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >  
> >  	spin_lock_init(&dev_priv->irq_lock);
> >  	spin_lock_init(&dev_priv->error_lock);
> > +	if (IS_GEN6(dev))
> > +		spin_lock_init(&dev_priv->forcewake_lock);
> 
> Just do it.

Once again, I went back and forth and picked the one I thought you would
like. I'm learning...

> -Chris