Message ID | 1379620838-1491-3-git-send-email-przanoni@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, Sep 19, 2013 at 05:00:36PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > We currently disable the ERR_INT interrupts while running the IRQ > handler because we fear that if we do an unclaimed register access > from inside the IRQ handler we'll keep triggering the IRQ handler > forever. > > The problem is that since we always disable the ERR_INT interrupts at > the IRQ handler, when we get a FIFO underrun we'll always print both > messages: > - "uncleared fifo underrun on pipe A" > - "Pipe A FIFO underrun" > > Because the "was_enabled" variable from > ivybridge_set_fifo_underrun_reporting will always be false (since we > disable ERR int at the IRQ handler!). > > Instead of actually fixing ivybridge_set_fifo_underrun_reporting, > let's just remove the "disable ERR_INT during the IRQ handler" code. > As far as we know we shouldn't really be triggering ERR_INT interrupts > from the IRQ handler, so if we ever get stuck in the endless loop of > interrupts we can git-bisect and revert (and we can even bisect and > revert this patch in case I'm just wrong). As a bonus, our IRQ handler > is now simpler and a few nanoseconds faster. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> One could argue that by unmasking the err interrupt we prevent bugs creeping into the interrupt handler. Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Thu, Sep 19, 2013 at 09:18:24PM +0100, Chris Wilson wrote: > On Thu, Sep 19, 2013 at 05:00:36PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > We currently disable the ERR_INT interrupts while running the IRQ > > handler because we fear that if we do an unclaimed register access > > from inside the IRQ handler we'll keep triggering the IRQ handler > > forever. > > > > The problem is that since we always disable the ERR_INT interrupts at > > the IRQ handler, when we get a FIFO underrun we'll always print both > > messages: > > - "uncleared fifo underrun on pipe A" > > - "Pipe A FIFO underrun" > > > > Because the "was_enabled" variable from > > ivybridge_set_fifo_underrun_reporting will always be false (since we > > disable ERR int at the IRQ handler!). > > > > Instead of actually fixing ivybridge_set_fifo_underrun_reporting, > > let's just remove the "disable ERR_INT during the IRQ handler" code. > > As far as we know we shouldn't really be triggering ERR_INT interrupts > > from the IRQ handler, so if we ever get stuck in the endless loop of > > interrupts we can git-bisect and revert (and we can even bisect and > > revert this patch in case I'm just wrong). As a bonus, our IRQ handler > > is now simpler and a few nanoseconds faster. > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > One could argue that by unmasking the err interrupt we prevent bugs > creeping into the interrupt handler. > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Queued for -next, thanks for the patch. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 9ecc9d9..e9f6bc3 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1394,7 +1394,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg) drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private; u32 de_iir, gt_iir, de_ier, sde_ier = 0; irqreturn_t ret = IRQ_NONE; - bool err_int_reenable = false; atomic_inc(&dev_priv->irq_received); @@ -1418,17 +1417,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg) POSTING_READ(SDEIER); } - /* On Haswell, also mask ERR_INT because we don't want to risk - * generating "unclaimed register" interrupts from inside the interrupt - * handler. */ - if (IS_HASWELL(dev)) { - spin_lock(&dev_priv->irq_lock); - err_int_reenable = ~dev_priv->irq_mask & DE_ERR_INT_IVB; - if (err_int_reenable) - ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB); - spin_unlock(&dev_priv->irq_lock); - } - gt_iir = I915_READ(GTIIR); if (gt_iir) { if (INTEL_INFO(dev)->gen >= 6) @@ -1458,13 +1446,6 @@ static irqreturn_t ironlake_irq_handler(int irq, void *arg) } } - if (err_int_reenable) { - spin_lock(&dev_priv->irq_lock); - if (ivb_can_enable_err_int(dev)) - ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB); - spin_unlock(&dev_priv->irq_lock); - } - I915_WRITE(DEIER, de_ier); POSTING_READ(DEIER); if (!HAS_PCH_NOP(dev)) {