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

[Keith Packard]

On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter <daniel at ffwll.ch> wrote:

> - I think we need to amend the commit msg of the voodoo patch with the
>   piece of doc I've discovered. If you want I can send out a v3 with that.

Sounds good.

> - I think the HWSTAM revert is material for -next

I'd be OK with pushing all of the SNB changes to -next and leaving the
patches only affecting IVB, where we know it is necessary.

Note that this branch wasn't written with an eye to merging to -fixes,
but rather as what we want the code to look like eventually. As such, we
need to identify separately

 1) What needs to go into -fixes for 3.3.

 2) What needs to go into -next for 3.4.

> - Can you post your 3 patches on intel-gfx, I think I can shoot at them a
>   bit ;-)

Sure; it's all a work in progress.

> 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.

> 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.

> 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.

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.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20120113/839ce0ff/attachment.sig>