On Fri, May 31, 2013 at 12:52:03PM -0700, Ben Widawsky wrote:
> On Fri, May 31, 2013 at 08:52:29PM +0200, Daniel Vetter wrote:
> > 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
*now*
> > 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
>
> Of these, which have you changed/fixed? That way if someone wants to
> start addressing the issues, they know where to start.
I've added a FIXME comment for the dropped WARN (which isn't in this list,
oops). Otherwise I've dropped all my changes since it grew unwiedly, fast.
But anyway that list above is only a rough guideline of things that had a
funny smell while reading. Like I've said my brain wasn't on top of things
to do a full irq handling review.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
