[Intel-gfx] [PATCH 2/2] drm/i915: Fix irq checks in ring->irq_get/put functions

[Chris Wilson]

On Mon, Sep 15, 2014 at 12:08:39PM +0200, Daniel Vetter wrote:
> On Mon, Sep 15, 2014 at 11:56 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> > On Mon, Sep 15, 2014 at 11:51:42AM +0200, Daniel Vetter wrote:
> >> On Mon, Sep 15, 2014 at 11:48 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> >> > intel_engine_cs *ring)
> >> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >> >>       unsigned long flags;
> >> >>
> >> >> +     WARN_ON(!intel_irqs_enabled(dev_priv));
> >> >
> >> > Please no. This would be a BUG().
> >>
> >> No BUG if not doing it means the driver will survive for a bit longer.
> >> And doing a few bogus register writes usually means it'll surive.
> >
> > I mean that this offers no additional benefit over the first WARN. Which
> > is already redundant as we WARN in the caller. There are more meaningful
> > places where that WARN would be appropriate, but after the event of
> > something else screwing up is usually close to useless.
> 
> If we drop the runtime pm reference before the put_irq then we'll WARN
> here. Which is somewhat likely if some waiter doesn't have it's own
> runtime pm reference but relies upon someone else holding that for
> him. Then the get_irq will likely happen while the gpu is still busy,
> but the put_irq has a good chance to only happen once we've cleaned
> up.

Put the warn before you sleep awaiting the irq. Every other condition is
besides the point. You can't detect some other thread breaking in whilst
you are asleep and when you are awake the request is either complete or
it is not. Reading any registers with the rpm is itself going to
generate the WARN so adding yet another WARN afterwards is simply
confusing.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre