Re: [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock

[Daniel Vetter <daniel@ffwll.ch>]

Looks like we managed to clear up our mutual confusion here ;-)

On Thu, Jan 05, 2012 at 07:49:12AM -0800, Keith Packard wrote:
> On Thu, 5 Jan 2012 12:29:08 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > - The reset code (running from a workqueue) does hold sturct mutex. It's
> >   the hangcheck and error state capture code running from softirq/timer
> >   context causing issues.
> 
> Right, I mis-wrote; I meant the hangcheck timer (which I always think of
> as part of the reset code).
> 
> > - Forcewake works like a reference counted resources. As long as all
> >   _get/_put calls are properly balanced, I don't see how somebody could
> >   sneak in in between (when we don't hold the spinlock) and cause havoc.
> >   For paranaoia we might want to drop a WARN_ON in the _put call to check
> >   whether it ever drops below 0. But all current users are clearly
> >   balanced, so I didn't bother with it.
> 
> Right, I was just confused somehow. Still seems weird to me to drop a
> spinlock, execute a single instruction, and then immediately re-acquire
> it, along with bumping forcewake_count twice.

Absolutely agreed, it's really ugly. But especially for locking changes
I'd like a patch to do one thing, and one thing only. And I didn't see the
upside of a separate patch to fix things up, also because the current
I915_WRTE|READ macro maze is a bit hellish.

> > - My missed IRQ w/a actually relies on this by grabbing a forcewake ref in
> >   get_irq and dropping it again in put_irq. In between there's usually a
> >   schedule().
> 
> This is essentially the same as the user-level forcewake and would be
> handled in the same way -- keep forcewake_count, but use it only for
> long-term values.
> 
> > - I've pondered with Chris whether we should do your proposed optimization
> >   but we then noticed that the gem code doesn't actually read from any
> >   forcewake protected registers in normal execution (I don't consider
> >   running the hangcheck timer normal ;-). With my missed irq w/a that now
> >   changes, so we might need to reconsider this. But imo that's material
> >   for a separate patch.
> 
> Yeah, all sounds reasonable. That separate patch can actually use
> per-chip functions to read/write from the chip so we can also avoid
> checking the forcewake stuff on register reads for older generation
> hardware.
> 
> Make it work, then make it work faster.

Absolutely agreed, maybe with the adadendum to only try to make things
faster if it's actually a problem and shows up in a fast-path we care
about.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48