Re: [PATCH 18/18] drm/i915: add I915_PARAM_HAS_VEBOX to i915_getparam

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 28, 2013 at 07:22:34PM -0700, Ben Widawsky wrote:
> From: "Xiang, Haihao" <haihao.xiang@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@intel.com>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Ben Widawsky <ben@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@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