[Intel-gfx] [PATCH] drm/i915: paper over missed irq issues with force wake vodoo
daniel at ffwll.ch
Sat Jan 14 01:31:40 CET 2012
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.
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48
More information about the Intel-gfx