| Message ID | 1410774079-20374-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Sep 15, 2014 at 11:41:19AM +0200, Daniel Vetter wrote: > Yet another place that wasn't properly transformed when implementing > SOix. While at it convert the checks to WARN_ON on gen5+ (since we > don't have UMS potentially doing stupid things on those platforms). > And also add the corresponding checks to the put functions (again with > a WARN_ON) for gen5+. > > Cc: Imre Deak <imre.deak@intel.com> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org> > Cc: "Volkin, Bradley D" <bradley.d.volkin@intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/i915/intel_lrc.c | 4 +++- > drivers/gpu/drm/i915/intel_ringbuffer.c | 23 ++++++++++++++--------- > 2 files changed, 17 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c > index bafd38b5703e..5fb61d76fd60 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -1063,7 +1063,7 @@ static bool gen8_logical_ring_get_irq(struct intel_engine_cs *ring) > struct drm_i915_private *dev_priv = dev->dev_private; > unsigned long flags; > > - if (!dev->irq_enabled) > + if (WARN_ON(!intel_irqs_enabled(dev_priv))) > return false; > > spin_lock_irqsave(&dev_priv->irq_lock, flags); > @@ -1082,6 +1082,8 @@ static void gen8_logical_ring_put_irq(struct 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(). -Chris
On Mon, Sep 15, 2014 at 11:48 AM, Chris Wilson <chris@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. Similar checks I've added just recently to pipestate_enable/disable caught a bug in the resume code. Using a BUG instead of WARN would have meant some serious debugging, but as-is a look at the backtrace was all that was needed to analyze the bug. I know a lot of developers disagree, but debugging random crap in the field is _so_ much easier with WARN than BUG that it's not even up for discussion imo. Thanks, Daniel
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@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. -Chris
On Mon, Sep 15, 2014 at 11:56 AM, Chris Wilson <chris@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@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. Of course it might mean that we get 2 backtraces in some case if e.g. a wait happens somehow before anything is really set up (in driver load). But I think the additional coverage makes this worth it. If it's too annoying we can always back it out again. -Daniel
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@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@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
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index bafd38b5703e..5fb61d76fd60 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -1063,7 +1063,7 @@ static bool gen8_logical_ring_get_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; - if (!dev->irq_enabled) + if (WARN_ON(!intel_irqs_enabled(dev_priv))) return false; spin_lock_irqsave(&dev_priv->irq_lock, flags); @@ -1082,6 +1082,8 @@ static void gen8_logical_ring_put_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; + WARN_ON(!intel_irqs_enabled(dev_priv)); + spin_lock_irqsave(&dev_priv->irq_lock, flags); if (--ring->irq_refcount == 0) { I915_WRITE_IMR(ring, ~ring->irq_keep_mask); diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 264d2827b2fb..b568c1f2c57b 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1197,7 +1197,7 @@ gen5_ring_get_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; - if (!dev->irq_enabled) + if (WARN_ON(!intel_irqs_enabled(dev_priv))) return false; spin_lock_irqsave(&dev_priv->irq_lock, flags); @@ -1215,6 +1215,8 @@ gen5_ring_put_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; + WARN_ON(!intel_irqs_enabled(dev_priv)); + spin_lock_irqsave(&dev_priv->irq_lock, flags); if (--ring->irq_refcount == 0) gen5_disable_gt_irq(dev_priv, ring->irq_enable_mask); @@ -1228,7 +1230,7 @@ i9xx_ring_get_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; - if (!dev->irq_enabled) + if (!intel_irqs_enabled(dev_priv)) return false; spin_lock_irqsave(&dev_priv->irq_lock, flags); @@ -1265,7 +1267,7 @@ i8xx_ring_get_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; - if (!dev->irq_enabled) + if (!intel_irqs_enabled(dev_priv)) return false; spin_lock_irqsave(&dev_priv->irq_lock, flags); @@ -1399,8 +1401,8 @@ gen6_ring_get_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; - if (!dev->irq_enabled) - return false; + if (WARN_ON(!intel_irqs_enabled(dev_priv))) + return false; spin_lock_irqsave(&dev_priv->irq_lock, flags); if (ring->irq_refcount++ == 0) { @@ -1424,6 +1426,8 @@ gen6_ring_put_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; + WARN_ON(!intel_irqs_enabled(dev_priv)); + spin_lock_irqsave(&dev_priv->irq_lock, flags); if (--ring->irq_refcount == 0) { if (HAS_L3_DPF(dev) && ring->id == RCS) @@ -1442,7 +1446,7 @@ hsw_vebox_get_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; - if (!dev->irq_enabled) + if (WARN_ON(!intel_irqs_enabled(dev_priv))) return false; spin_lock_irqsave(&dev_priv->irq_lock, flags); @@ -1462,8 +1466,7 @@ hsw_vebox_put_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; - if (!dev->irq_enabled) - return; + WARN_ON(!intel_irqs_enabled(dev_priv)); spin_lock_irqsave(&dev_priv->irq_lock, flags); if (--ring->irq_refcount == 0) { @@ -1480,7 +1483,7 @@ gen8_ring_get_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; - if (!dev->irq_enabled) + if (WARN_ON(!intel_irqs_enabled(dev_priv))) return false; spin_lock_irqsave(&dev_priv->irq_lock, flags); @@ -1506,6 +1509,8 @@ gen8_ring_put_irq(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = dev->dev_private; unsigned long flags; + WARN_ON(!intel_irqs_enabled(dev_priv)); + spin_lock_irqsave(&dev_priv->irq_lock, flags); if (--ring->irq_refcount == 0) { if (HAS_L3_DPF(dev) && ring->id == RCS) {
Yet another place that wasn't properly transformed when implementing SOix. While at it convert the checks to WARN_ON on gen5+ (since we don't have UMS potentially doing stupid things on those platforms). And also add the corresponding checks to the put functions (again with a WARN_ON) for gen5+. Cc: Imre Deak <imre.deak@intel.com> Cc: Jesse Barnes <jbarnes@virtuousgeek.org> Cc: "Volkin, Bradley D" <bradley.d.volkin@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/intel_lrc.c | 4 +++- drivers/gpu/drm/i915/intel_ringbuffer.c | 23 ++++++++++++++--------- 2 files changed, 17 insertions(+), 10 deletions(-)