[Intel-gfx] [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock
Daniel Vetter
daniel at ffwll.ch
Thu Jan 5 17:59:47 CET 2012
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 at 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 at ffwll.ch
Mobile: +41 (0)79 365 57 48
More information about the Intel-gfx
mailing list