On Tue, Jan 3, 2012 at 19:51, Keith Packard <keithp at keithp.com> wrote: > On Wed, 14 Dec 2011 13:57:03 +0100, Daniel Vetter <daniel.vetter at ffwll.ch> wrote: > >> The problem this patch solves is that the forcewake accounting >> necessary for register reads is protected by dev->struct_mutex. But the >> hangcheck and error_capture code need to access registers without >> grabbing this mutex because we hold it while waiting for the gpu. >> So a new lock is required. Because currently the error_state capture >> is called from the error irq handler and the hangcheck code runs from >> a timer, it needs to be an irqsafe spinlock (note that the registers >> used by the irq handler (neglecting the error handling part) only uses >> registers that don't need the forcewake dance). > > I think this description is wrong -- the only difference between using > atomic objects and using a spinlock is that with the spinlock the call > to ->force_wake_get is correctly serialized so that no register access > can occur without the chip being awoken. Without a spinlock, a second > thread can pass right through gen6_gt_force_wake_get and then go touch > registers while the first thread is busy waking the chip up. 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. Afaik the atomic ops stuff is just ducttape for paranoia reasons. -Daniel -- Daniel Vetter daniel.vetter at ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch
