On Tue, May 28, 2013 at 07:22:34PM -0700, Ben Widawsky wrote:
> From: "Xiang, Haihao" <haihao.xiang at intel.com>
>
> This will let userland only try to use the new ring
> when the appropriate kernel is present
>
> Signed-off-by: Xiang, Haihao <haihao.xiang at intel.com>
> Reviewed-by: Damien Lespiau <damien.lespiau at intel.com>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
So originally I wanted to take a closer look at the interrupt handling
changes before merging them. But somehow my brain was notoriously not up
to the task of reviewing tricky interrupt stuff. Anyway here's my list of
things I've spotted while applying patches:
- Unrelated, but spotted while checking interrupt code:
ironlake_enable_display_irq is not called with the irq_lock held
everywhere, and some are outside of the irq setup/teradown (so real
races). Specifically
commit 8664281b64c457705db72fc60143d03827e75ca9
Author: Paulo Zanoni <paulo.r.zanoni at intel.com>
Date: Fri Apr 12 17:57:57 2013 -0300
drm/i915: report Gen5+ CPU and PCH FIFO underruns
broke stuff. But I guess a full review of the interrupt handling code
should be in order ... At least we should sprinkle assert_spin_locked
harder.
Now the real stuff:
- rmw register access isn't paranoid, but imo fragile. Either we don't
need it, and then it just obfuscates the code (especially with scary
comments around). Or there's indeed a race somewhere, and then rmw is
_really_ good at papering over it. Until it blows up randomly and no one
has a clue why.
So I'm not a fan, and I've pretty much exlcusive just killed them. These
patches add _lots_ of them instead.
- rps.lock could just be killed and existing users switched over to
irq_lock. Would simplify a lot in the interrupt handling. E.g. the
special ring->irqrefcount could just be dropped.
- rps setup is async now (yeah, that's newer than these patches, still). I
think I've seen a few races around that ...
- There's no inconsistencies in the PM rps interrupt setup on snb vs
ivb/hsw. Such inconsistency tend to be fragile (but I haven't spotted
something which would blow up on snb).
- CS_MASTER interrupt handling: None of the other rings seem to enable
this on gen5+. Not sure whether it actually works correctly, I suspect
at least i915_report_and_clear_eir needs some more code ...
- Calling queue_work from the spin_lock protection smells a bit like
cargo-culting (I've reinstated that one since later patches would have
been broken).
- hsw_pm_irq_handler has no need to unconditionally take the spinlock.
Patches are merged for now, but I'm not happy by far.
Cheers, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
