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

[Keith Packard <keithp@keithp.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2004 bytes --]

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.

> - 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.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx