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

[Keith Packard]

On Tue, 3 Jan 2012 20:12:35 +0100, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:

> I'm a bit confused by this. With the current code forcewake is
> protected by dev->struct_mutex. Which doesn't work out because we need
> to access registers that require the forcewake dance from non-process
> context.

Right, I like adding a spinlock around this to allow it to be called
without needing to be able to lock the struct_mutex. (I remember
suggesting that a spinlock would be necessary when the force wake code
first showed up...)

However, the commit message talks about the error capture and
hang check code, but doesn't appear to change them at all.

I think all this patch does is replace the locking for forcewake_count
From struct_mutex to a new irq-safe spinlock, the commit message makes
it sound like it's actually fixing stuff, which it isn't; it just
enables fixing stuff in future patches, right?

Reading through this a bit more, I think your patch opens up a hole in
i915_reset. i915_reset takes struct_mutex, then resets the chip and
restores the forcewake status. If we aren't relying on struct_mutex to
protect the forcewake bits, then there's nothing preventing a thread
From accessing the registers with the chip sleeping between the reset
and the force wake reset.

> Afaik the atomic ops stuff is just ducttape for paranoia reasons.

The atomic ops stuff would allow reading of the value without holding
struct_mutex, if that were actually useful.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20120103/74980618/attachment.sig>