On Mon, Feb 23, 2015 at 04:31:49PM +0200, Imre Deak wrote: > Atm, it's possible that the interrupt handler is called when the device > is in D3 or some other low-power state. It can be due to another device > that is still in D0 state and shares the interrupt line with i915, or on > some platforms there could be spurious interrupts even without sharing > the interrupt line. The latter case was reported by Klaus Ethgen using a > Lenovo x61p machine (gen 4). He noticed this issue via a system > suspend/resume hang and bisected it to the following commit: > > commit e11aa362308f5de467ce355a2a2471321b15a35c > Author: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> > Date: Wed Jun 18 09:52:55 2014 -0700 > > drm/i915: use runtime irq suspend/resume in freeze/thaw > > This is a problem, since in low-power states IIR will always read > 0xffffffff resulting in an endless IRQ servicing loop. > > Fix this by handling interrupts only when the driver explicitly enables > them and so it's guaranteed that the interrupt registers return a valid > value. > > Note that this issue existed even before the above commit, since during > runtime suspend/resume we never unregistered the handler. > > v2: > - clarify the purpose of smp_mb() vs. synchronize_irq() in the > code comment (Chris) > > v3: > - no need for an explicit smp_mb(), we can assume that synchronize_irq() > and the mmio read/writes in the install hooks provide for this (Daniel) > - remove code comment as the remaining synchronize_irq() is self > explanatory (Daniel) > > Reference: https://lkml.org/lkml/2015/2/11/205 > Reported-and-bisected-by: Klaus Ethgen <Klaus@xxxxxxxxx> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx> > --- > drivers/gpu/drm/i915/i915_irq.c | 23 +++++++++++++++++++++++ > 1 file changed, 23 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 9073119..612c9c0 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1889,6 +1889,9 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) > u32 iir, gt_iir, pm_iir; > irqreturn_t ret = IRQ_NONE; > > + if (!intel_irqs_enabled(dev_priv)) > + return IRQ_NONE; > + > while (true) { > /* Find, clear, then process each source of interrupt */ > > @@ -1933,6 +1936,9 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) > u32 master_ctl, iir; > irqreturn_t ret = IRQ_NONE; > > + if (!intel_irqs_enabled(dev_priv)) > + return IRQ_NONE; > + > for (;;) { > master_ctl = I915_READ(GEN8_MASTER_IRQ) & ~GEN8_MASTER_IRQ_CONTROL; > iir = I915_READ(VLV_IIR); > @@ -2205,6 +2211,9 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg) > u32 de_iir, gt_iir, de_ier, sde_ier = 0; > irqreturn_t ret = IRQ_NONE; > > + if (!intel_irqs_enabled(dev_priv)) > + return IRQ_NONE; > + > /* We get interrupts on unclaimed registers, so check for this before we > * do any I915_{READ,WRITE}. */ > intel_uncore_check_errors(dev); > @@ -2276,6 +2285,9 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg) > enum pipe pipe; > u32 aux_mask = GEN8_AUX_CHANNEL_A; > > + if (!intel_irqs_enabled(dev_priv)) > + return IRQ_NONE; > + > if (IS_GEN9(dev)) > aux_mask |= GEN9_AUX_CHANNEL_B | GEN9_AUX_CHANNEL_C | > GEN9_AUX_CHANNEL_D; > @@ -3768,6 +3780,9 @@ static irqreturn_t i8xx_irq_handler(int irq, void *arg) > I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | > I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; > > + if (!intel_irqs_enabled(dev_priv)) > + return IRQ_NONE; > + > iir = I915_READ16(IIR); > if (iir == 0) > return IRQ_NONE; > @@ -3948,6 +3963,9 @@ static irqreturn_t i915_irq_handler(int irq, void *arg) > I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; > int pipe, ret = IRQ_NONE; > > + if (!intel_irqs_enabled(dev_priv)) > + return IRQ_NONE; > + > iir = I915_READ(IIR); > do { > bool irq_received = (iir & ~flip_mask) != 0; > @@ -4168,6 +4186,9 @@ static irqreturn_t i965_irq_handler(int irq, void *arg) > I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | > I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT; > > + if (!intel_irqs_enabled(dev_priv)) > + return IRQ_NONE; > + > iir = I915_READ(IIR); > > for (;;) { > @@ -4504,6 +4525,7 @@ void intel_irq_uninstall(struct drm_i915_private *dev_priv) > drm_irq_uninstall(dev_priv->dev); > intel_hpd_cancel_work(dev_priv); > dev_priv->pm.irqs_enabled = false; > + synchronize_irq(dev_priv->dev->irq); drm_irq_uninstall calls free_irq which means there's no way at all for our handler to still run. The synchronize_irq here is hence redundnant. With that hunk removed this is Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > } > > /** > @@ -4517,6 +4539,7 @@ void intel_runtime_pm_disable_interrupts(struct drm_i915_private *dev_priv) > { > dev_priv->dev->driver->irq_uninstall(dev_priv->dev); > dev_priv->pm.irqs_enabled = false; > + synchronize_irq(dev_priv->dev->irq); > } > > /** > -- > 2.1.0 > -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
