Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo

[Daniel Vetter <daniel@ffwll.ch>]

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@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@ffwll.ch
Mobile: +41 (0)79 365 57 48