[Intel-gfx] [PATCH] drm/i915: Reset the interrupt mask on disabling interrupts

[Chris Wilson]

Quoting Ville Syrjälä (2020-10-22 12:32:52)
> On Thu, Oct 22, 2020 at 08:16:37AM +0100, Chris Wilson wrote:
> > As we disable the interrupt during suspend, also reset the irq_mask to
> > short-circuit subsystems that later try to turn off their interrupt
> > source.
> > 
> > <4>[  101.816730] i915 0000:00:02.0: drm_WARN_ON(!intel_irqs_enabled(dev_priv))
> > <4>[  101.816853] WARNING: CPU: 3 PID: 4241 at drivers/gpu/drm/i915/i915_irq.c:343 ilk_update_display_irq+0xb3/0x130 [i915]
> 
> Doh. Lack of irq symmetry in suspend vs. resume strikes again :(
> 
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 25 ++++++++++---------------
> >  1 file changed, 10 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 09221ca1ffb2..cbb71fc73313 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -337,17 +337,14 @@ void ilk_update_display_irq(struct drm_i915_private *dev_priv,
> >       u32 new_val;
> >  
> >       lockdep_assert_held(&dev_priv->irq_lock);
> > -
> >       drm_WARN_ON(&dev_priv->drm, enabled_irq_mask & ~interrupt_mask);
> >  
> > -     if (drm_WARN_ON(&dev_priv->drm, !intel_irqs_enabled(dev_priv)))
> > -             return;
> > -
> >       new_val = dev_priv->irq_mask;
> >       new_val &= ~interrupt_mask;
> >       new_val |= (~enabled_irq_mask & interrupt_mask);
> >  
> > -     if (new_val != dev_priv->irq_mask) {
> > +     if (new_val != dev_priv->irq_mask &&
> > +         !drm_WARN_ON(&dev_priv->drm, !intel_irqs_enabled(dev_priv))) {
> >               dev_priv->irq_mask = new_val;
> >               I915_WRITE(DEIMR, dev_priv->irq_mask);
> >               POSTING_READ(DEIMR);
> > @@ -368,19 +365,16 @@ static void bdw_update_port_irq(struct drm_i915_private *dev_priv,
> >       u32 old_val;
> >  
> >       lockdep_assert_held(&dev_priv->irq_lock);
> > -
> >       drm_WARN_ON(&dev_priv->drm, enabled_irq_mask & ~interrupt_mask);
> >  
> > -     if (drm_WARN_ON(&dev_priv->drm, !intel_irqs_enabled(dev_priv)))
> > -             return;
> > -
> >       old_val = I915_READ(GEN8_DE_PORT_IMR);
> >  
> >       new_val = old_val;
> >       new_val &= ~interrupt_mask;
> >       new_val |= (~enabled_irq_mask & interrupt_mask);
> >  
> > -     if (new_val != old_val) {
> > +     if (new_val != old_val &&
> > +         !drm_WARN_ON(&dev_priv->drm, !intel_irqs_enabled(dev_priv))) {
> >               I915_WRITE(GEN8_DE_PORT_IMR, new_val);
> >               POSTING_READ(GEN8_DE_PORT_IMR);
> >       }
> > @@ -401,17 +395,14 @@ void bdw_update_pipe_irq(struct drm_i915_private *dev_priv,
> >       u32 new_val;
> >  
> >       lockdep_assert_held(&dev_priv->irq_lock);
> > -
> >       drm_WARN_ON(&dev_priv->drm, enabled_irq_mask & ~interrupt_mask);
> >  
> > -     if (drm_WARN_ON(&dev_priv->drm, !intel_irqs_enabled(dev_priv)))
> > -             return;
> > -
> >       new_val = dev_priv->de_irq_mask[pipe];
> >       new_val &= ~interrupt_mask;
> >       new_val |= (~enabled_irq_mask & interrupt_mask);
> >  
> > -     if (new_val != dev_priv->de_irq_mask[pipe]) {
> > +     if (new_val != dev_priv->de_irq_mask[pipe] &&
> > +         !drm_WARN_ON(&dev_priv->drm, !intel_irqs_enabled(dev_priv))) {
> >               dev_priv->de_irq_mask[pipe] = new_val;
> >               I915_WRITE(GEN8_DE_PIPE_IMR(pipe), dev_priv->de_irq_mask[pipe]);
> >               POSTING_READ(GEN8_DE_PIPE_IMR(pipe));
> 
> Not resetting de_irq_mask[] anywhere? Hmm. we seem to be lacking a
> gen8_de_irq_reset()...

Maybe I was being a bit optimistic, and erred on the side of sticking to
the simple resets.

> > @@ -2951,6 +2942,8 @@ static void ilk_irq_reset(struct drm_i915_private *dev_priv)
> >       struct intel_uncore *uncore = &dev_priv->uncore;
> >  
> >       GEN3_IRQ_RESET(uncore, DE);
> > +     dev_priv->irq_mask = ~0u;
> > +
> >       if (IS_GEN(dev_priv, 7))
> >               intel_uncore_write(uncore, GEN7_ERR_INT, 0xffffffff);
> >  
> > @@ -3864,6 +3857,7 @@ static void i915_irq_reset(struct drm_i915_private *dev_priv)
> >       i9xx_pipestat_irq_reset(dev_priv);
> >  
> >       GEN3_IRQ_RESET(uncore, GEN2_);
> > +     dev_priv->irq_mask = ~0u;
> >  }
> >  
> >  static void i915_irq_postinstall(struct drm_i915_private *dev_priv)
> > @@ -3970,6 +3964,7 @@ static void i965_irq_reset(struct drm_i915_private *dev_priv)
> >       i9xx_pipestat_irq_reset(dev_priv);
> >  
> >       GEN3_IRQ_RESET(uncore, GEN2_);
> > +     dev_priv->irq_mask = ~0u;
> >  }
> 
> Missing gen2?

Hmm, I saw GEN2 and assumed it was covered. i8xx_irq_reset() looks
straightforward.

I guess split this patch to only think about dev_priv->irq_mask...
-Chris