On Fri, Jan 13, 2012 at 04:11:43PM -0800, Keith Packard wrote: > On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter <daniel at ffwll.ch> wrote: > > acc101d drm/i915: Hold gt_lock across forcewake register reads > > > > Imo this is a simple cleanup (reading forcewake-protected registers isn't > > really a fast-path for us), so material for -next. > > The 'optimization' is just a side benefit. The fix is to prevent reads > From happening without forcewake being set. I still fail to see how you can sneak a read in there without forcewake being asserted. And assuming I haven't understood you the last time around we've discussed this, you've agreed. > > > 0f0e134 drm/i915: Hold gt_lock during reset > > > > I still don't see what race you're trying to protect here, after all the > > gpu just died, things are confusing anyway (and anyone accessing the gpu > > in such a state should take that into account). Currently that's no one > > afaics. So imo at most -next material. > > These two patches work together to ensure that no-one reads from the GPU > without forcewake being set correctly, even across reset. Mostly, I > changed the code to make it obvious that the whole read operation was > now atomic; before, I had to read a lot of code to convince myself that > the read couldn't happen without forcewake being set, except under the > reset condition. Forcing me to read code closely to prove it correct > doesn't make me happy. Having a spinlock held over the entire > section makes the whole thing obviously correct to even casual inspection. > > So, I'd like these to go into -next so that when I read this code next > year, I won't have to figure all of this out again. Agreed, it's not the most obvious code around ;-) > > 176b987 drm/i915: Move reset forcewake processing to gen6_do_reset > > > > Again this is imo just a cleanup. Furthermore the commit msg is lying a > > bit because it fails to mention the fix to use the forcewake function > > pointer. So the cleanup is imo for -next and the bugfix is really old, > > see: > > Yes, I didn't even notice that I'd fixed that bug when I moved the code... > > The bugfix is, however required, and needs to be in -fixes. Yeah, I've silently implied that. > So, I think for -fixes we get: > > 1) The forcewake spinlock patch. > > 2) The bugfix to the reset path. > > 3) forcewake while waiting for interrupts for IVB only, not for SNB, > with updated commit message. > > This minimizes the potential for regressions in SNB (by not affecting it > at all) while fixing the IVB issues. > > For -next, we should have > > 1) forcewake for interrupts on IVB and SNB > > 2) Removal of the HWSTAM hacks > > 3) The spinlock cleanups that make me happy > > I'd love to hear back from some SNB owners that the forcewake IRQ issue > resolves problems and doesn't cause any regressions. If so, we can > reconsider it for 3.3. If it doesn't fix anything, then I don't think it > should go into 3.3. Sounds like a good merge plan. I'll wrestle the patch to add the IS_IVB check and the documentation reference. -Daniel -- Daniel Vetter Mail: daniel at ffwll.ch Mobile: +41 (0)79 365 57 48
