On Tue, Jan 3, 2012 at 22:13, Keith Packard <keithp at keithp.com> wrote: > 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? Nope, current hangcheck blows up, and we have an i-g-t testcase for it (which the commit msg clearly states). There are also numerous bug reports where a dying gpu results in tons of WARN_ON(!mutex_locked(dev->struct_mutex)) noise in dmesg (which drowns out the gpu hang warning). The locking change fixes this. > 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. The patch adds the required locking to i915_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. ... but is currently unused and inherently racy. Which is why the patch drops it. -- Daniel Vetter daniel.vetter at ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch
