| Message ID | 1439573072-8927-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 14, 2015 at 06:24:32PM +0100, Chris Wilson wrote: > The PIPE.STAT register contains some interrupt status bits per pipe, and > if assert cause the corresponding bit in the IIR to be asserted (thus > raising an interrupt). When handling an interrupt, we should clear the > PIPE.STAT generator first before clearing the IIR so that we do not miss > events or cause spurious work. > > This ordering was broken by > > commit 27b6c122512ca30399bb1b39cc42eda83901f304 > Author: Oscar Mateo <oscar.mateo@intel.com> > Date: Mon Jun 16 16:11:00 2014 +0100 > > drm/i915/chv: Ack interrupts before handling them (CHV) > > commit 3ff60f89bc4836583f5bd195062f16c563bd97aa > Author: Oscar Mateo <oscar.mateo@intel.com> > Date: Mon Jun 16 16:10:58 2014 +0100 > > drm/i915/vlv: Ack interrupts before handling them (VLV) > > in attempting to reduce the amount of work done between receiving an > interrupt and acknowledging it. In light of this motivation, split the > pipestat interrupt handler into two, one to acknowledge and clear the > interrupt and a later pass to process it. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Bob Beckett <robert.beckett@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Oscar/Imre, where's your review on this? Thanks, Daniel > --- > drivers/gpu/drm/i915/i915_irq.c | 55 +++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 66064511cffb..92bdfe6f91d8 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1457,24 +1457,21 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir) > } > } > > -static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe) > +static inline bool > +intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe) > { > - if (!drm_handle_vblank(dev, pipe)) > - return false; > - > - return true; > + return drm_handle_vblank(dev, pipe); > } > > -static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > +static void valleyview_pipestat_irq_get(struct drm_i915_private *dev_priv, > + u32 iir, u32 *pipe_stats) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - u32 pipe_stats[I915_MAX_PIPES] = { }; > int pipe; > > spin_lock(&dev_priv->irq_lock); > for_each_pipe(dev_priv, pipe) { > + u32 mask, val, iir_bit = 0; > int reg; > - u32 mask, iir_bit = 0; > > /* > * PIPESTAT bits get signalled even when the interrupt is > @@ -1486,6 +1483,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > > /* fifo underruns are filterered in the underrun handler. */ > mask = PIPE_FIFO_UNDERRUN_STATUS; > + mask |= PIPESTAT_INT_ENABLE_MASK; > > switch (pipe) { > case PIPE_A: > @@ -1501,21 +1499,25 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > if (iir & iir_bit) > mask |= dev_priv->pipestat_irq_mask[pipe]; > > - if (!mask) > - continue; > - > reg = PIPESTAT(pipe); > - mask |= PIPESTAT_INT_ENABLE_MASK; > - pipe_stats[pipe] = I915_READ(reg) & mask; > + val = I915_READ(reg); > + pipe_stats[pipe] |= val & mask; > > /* > * Clear the PIPE*STAT regs before the IIR > */ > - if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS | > - PIPESTAT_INT_STATUS_MASK)) > - I915_WRITE(reg, pipe_stats[pipe]); > + if (val & (PIPE_FIFO_UNDERRUN_STATUS | > + PIPESTAT_INT_STATUS_MASK)) > + I915_WRITE(reg, val); > } > spin_unlock(&dev_priv->irq_lock); > +} > + > +static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv, > + u32 *pipe_stats) > +{ > + struct drm_device *dev = dev_priv->dev; > + int pipe; > > for_each_pipe(dev_priv, pipe) { > if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS && > @@ -1578,6 +1580,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) > { > struct drm_device *dev = arg; > struct drm_i915_private *dev_priv = dev->dev_private; > + u32 pipe_stats[I915_MAX_PIPES] = {}; > u32 iir, gt_iir, pm_iir; > irqreturn_t ret = IRQ_NONE; > > @@ -1600,6 +1603,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) > /* Consume port before clearing IIR or we'll miss events */ > if (iir & I915_DISPLAY_PORT_INTERRUPT) > i9xx_hpd_irq_handler(dev); > + valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats); > I915_WRITE(VLV_IIR, iir); > } > > @@ -1612,12 +1616,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) > snb_gt_irq_handler(dev, dev_priv, gt_iir); > if (pm_iir) > gen6_rps_irq_handler(dev_priv, pm_iir); > - /* Call regardless, as some status bits might not be > - * signalled in iir */ > - valleyview_pipestat_irq_handler(dev, iir); > } > > out: > + > + /* Call regardless, as some status bits might not be > + * signalled in iir */ > + valleyview_pipestat_irq_handler(dev_priv, pipe_stats); > return ret; > } > > @@ -1625,6 +1630,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) > { > struct drm_device *dev = arg; > struct drm_i915_private *dev_priv = dev->dev_private; > + u32 pipe_stats[I915_MAX_PIPES] = {}; > u32 master_ctl, iir; > irqreturn_t ret = IRQ_NONE; > > @@ -1648,18 +1654,19 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) > /* Consume port before clearing IIR or we'll miss events */ > if (iir & I915_DISPLAY_PORT_INTERRUPT) > i9xx_hpd_irq_handler(dev); > + valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats); > I915_WRITE(VLV_IIR, iir); > } > > gen8_gt_irq_handler(dev_priv, master_ctl); > > - /* Call regardless, as some status bits might not be > - * signalled in iir */ > - valleyview_pipestat_irq_handler(dev, iir); > - > I915_WRITE_FW(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL); > } > > + /* Call regardless, as some status bits might not be > + * signalled in iir */ > + valleyview_pipestat_irq_handler(dev_priv, pipe_stats); > + > return ret; > } > > -- > 2.5.0 >
On pe, 2015-08-14 at 18:24 +0100, Chris Wilson wrote: > The PIPE.STAT register contains some interrupt status bits per pipe, and > if assert cause the corresponding bit in the IIR to be asserted (thus > raising an interrupt). When handling an interrupt, we should clear the > PIPE.STAT generator first before clearing the IIR so that we do not miss > events or cause spurious work. > > This ordering was broken by > > commit 27b6c122512ca30399bb1b39cc42eda83901f304 > Author: Oscar Mateo <oscar.mateo@intel.com> > Date: Mon Jun 16 16:11:00 2014 +0100 > > drm/i915/chv: Ack interrupts before handling them (CHV) > > commit 3ff60f89bc4836583f5bd195062f16c563bd97aa > Author: Oscar Mateo <oscar.mateo@intel.com> > Date: Mon Jun 16 16:10:58 2014 +0100 > > drm/i915/vlv: Ack interrupts before handling them (VLV) > > in attempting to reduce the amount of work done between receiving an > interrupt and acknowledging it. In light of this motivation, split the > pipestat interrupt handler into two, one to acknowledge and clear the > interrupt and a later pass to process it. Yes, after thinking about hierarchical interrupt clearing/handling this makes complete sense. It was even hinted at by Ville in the discussion of the above patches, but I missed his point back then. > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Oscar Mateo <oscar.mateo@intel.com> > Cc: Bob Beckett <robert.beckett@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_irq.c | 55 +++++++++++++++++++++++------------------ > 1 file changed, 31 insertions(+), 24 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 66064511cffb..92bdfe6f91d8 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -1457,24 +1457,21 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir) > } > } > > -static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe) > +static inline bool > +intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe) > { > - if (!drm_handle_vblank(dev, pipe)) > - return false; > - > - return true; > + return drm_handle_vblank(dev, pipe); > } > > -static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > +static void valleyview_pipestat_irq_get(struct drm_i915_private *dev_priv, > + u32 iir, u32 *pipe_stats) > { > - struct drm_i915_private *dev_priv = dev->dev_private; > - u32 pipe_stats[I915_MAX_PIPES] = { }; > int pipe; > > spin_lock(&dev_priv->irq_lock); > for_each_pipe(dev_priv, pipe) { > + u32 mask, val, iir_bit = 0; > int reg; > - u32 mask, iir_bit = 0; > > /* > * PIPESTAT bits get signalled even when the interrupt is > @@ -1486,6 +1483,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > > /* fifo underruns are filterered in the underrun handler. */ > mask = PIPE_FIFO_UNDERRUN_STATUS; > + mask |= PIPESTAT_INT_ENABLE_MASK; > > switch (pipe) { > case PIPE_A: > @@ -1501,21 +1499,25 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) > if (iir & iir_bit) > mask |= dev_priv->pipestat_irq_mask[pipe]; > > - if (!mask) > - continue; > - > reg = PIPESTAT(pipe); > - mask |= PIPESTAT_INT_ENABLE_MASK; > - pipe_stats[pipe] = I915_READ(reg) & mask; > + val = I915_READ(reg); > + pipe_stats[pipe] |= val & mask; > > /* > * Clear the PIPE*STAT regs before the IIR > */ > - if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS | > - PIPESTAT_INT_STATUS_MASK)) > - I915_WRITE(reg, pipe_stats[pipe]); > + if (val & (PIPE_FIFO_UNDERRUN_STATUS | > + PIPESTAT_INT_STATUS_MASK)) > + I915_WRITE(reg, val); > } > spin_unlock(&dev_priv->irq_lock); > +} > + > +static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv, > + u32 *pipe_stats) > +{ > + struct drm_device *dev = dev_priv->dev; > + int pipe; > > for_each_pipe(dev_priv, pipe) { > if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS && > @@ -1578,6 +1580,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) > { > struct drm_device *dev = arg; > struct drm_i915_private *dev_priv = dev->dev_private; > + u32 pipe_stats[I915_MAX_PIPES] = {}; > u32 iir, gt_iir, pm_iir; > irqreturn_t ret = IRQ_NONE; > > @@ -1600,6 +1603,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) > /* Consume port before clearing IIR or we'll miss events */ > if (iir & I915_DISPLAY_PORT_INTERRUPT) > i9xx_hpd_irq_handler(dev); > + valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats); This should be called even with iir==0 to get a possible underflow flag. Although I realize now this wasn't handled properly even before this patch, you needed at least one of the gt_iir/pm_iir/iir bits to be set. > I915_WRITE(VLV_IIR, iir); > } > > @@ -1612,12 +1616,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) > snb_gt_irq_handler(dev, dev_priv, gt_iir); > if (pm_iir) > gen6_rps_irq_handler(dev_priv, pm_iir); > - /* Call regardless, as some status bits might not be > - * signalled in iir */ > - valleyview_pipestat_irq_handler(dev, iir); > } > > out: > + > + /* Call regardless, as some status bits might not be > + * signalled in iir */ > + valleyview_pipestat_irq_handler(dev_priv, pipe_stats); > return ret; > } > > @@ -1625,6 +1630,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) > { > struct drm_device *dev = arg; > struct drm_i915_private *dev_priv = dev->dev_private; > + u32 pipe_stats[I915_MAX_PIPES] = {}; > u32 master_ctl, iir; > irqreturn_t ret = IRQ_NONE; > > @@ -1648,18 +1654,19 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) > /* Consume port before clearing IIR or we'll miss events */ > if (iir & I915_DISPLAY_PORT_INTERRUPT) > i9xx_hpd_irq_handler(dev); > + valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats); > I915_WRITE(VLV_IIR, iir); > } > > gen8_gt_irq_handler(dev_priv, master_ctl); I guess you'll want to clear/handle these IIRs in the same way, but that would be a follow-up change. > > - /* Call regardless, as some status bits might not be > - * signalled in iir */ > - valleyview_pipestat_irq_handler(dev, iir); > - > I915_WRITE_FW(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL); The above is still a regular I915_WRITE/POSTING_READ in -nightly, so needs a rebase. With or without fixing the underrun flag readout: Reviewed-by: Imre Deak <imre.deak@intel.com> > } > > + /* Call regardless, as some status bits might not be > + * signalled in iir */ > + valleyview_pipestat_irq_handler(dev_priv, pipe_stats); > + > return ret; > } >
On Tue, 01 Sep 2015, Imre Deak <imre.deak@intel.com> wrote: > On pe, 2015-08-14 at 18:24 +0100, Chris Wilson wrote: >> The PIPE.STAT register contains some interrupt status bits per pipe, and >> if assert cause the corresponding bit in the IIR to be asserted (thus >> raising an interrupt). When handling an interrupt, we should clear the >> PIPE.STAT generator first before clearing the IIR so that we do not miss >> events or cause spurious work. >> >> This ordering was broken by >> >> commit 27b6c122512ca30399bb1b39cc42eda83901f304 >> Author: Oscar Mateo <oscar.mateo@intel.com> >> Date: Mon Jun 16 16:11:00 2014 +0100 >> >> drm/i915/chv: Ack interrupts before handling them (CHV) >> >> commit 3ff60f89bc4836583f5bd195062f16c563bd97aa >> Author: Oscar Mateo <oscar.mateo@intel.com> >> Date: Mon Jun 16 16:10:58 2014 +0100 >> >> drm/i915/vlv: Ack interrupts before handling them (VLV) >> >> in attempting to reduce the amount of work done between receiving an >> interrupt and acknowledging it. In light of this motivation, split the >> pipestat interrupt handler into two, one to acknowledge and clear the >> interrupt and a later pass to process it. > > Yes, after thinking about hierarchical interrupt clearing/handling this > makes complete sense. It was even hinted at by Ville in the discussion > of the above patches, but I missed his point back then. > >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Oscar Mateo <oscar.mateo@intel.com> >> Cc: Bob Beckett <robert.beckett@intel.com> >> Cc: Imre Deak <imre.deak@intel.com> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> >> --- >> drivers/gpu/drm/i915/i915_irq.c | 55 +++++++++++++++++++++++------------------ >> 1 file changed, 31 insertions(+), 24 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c >> index 66064511cffb..92bdfe6f91d8 100644 >> --- a/drivers/gpu/drm/i915/i915_irq.c >> +++ b/drivers/gpu/drm/i915/i915_irq.c >> @@ -1457,24 +1457,21 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir) >> } >> } >> >> -static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe) >> +static inline bool >> +intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe) >> { >> - if (!drm_handle_vblank(dev, pipe)) >> - return false; >> - >> - return true; >> + return drm_handle_vblank(dev, pipe); >> } >> >> -static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) >> +static void valleyview_pipestat_irq_get(struct drm_i915_private *dev_priv, >> + u32 iir, u32 *pipe_stats) >> { >> - struct drm_i915_private *dev_priv = dev->dev_private; >> - u32 pipe_stats[I915_MAX_PIPES] = { }; >> int pipe; >> >> spin_lock(&dev_priv->irq_lock); >> for_each_pipe(dev_priv, pipe) { >> + u32 mask, val, iir_bit = 0; >> int reg; >> - u32 mask, iir_bit = 0; >> >> /* >> * PIPESTAT bits get signalled even when the interrupt is >> @@ -1486,6 +1483,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) >> >> /* fifo underruns are filterered in the underrun handler. */ >> mask = PIPE_FIFO_UNDERRUN_STATUS; >> + mask |= PIPESTAT_INT_ENABLE_MASK; >> >> switch (pipe) { >> case PIPE_A: >> @@ -1501,21 +1499,25 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) >> if (iir & iir_bit) >> mask |= dev_priv->pipestat_irq_mask[pipe]; >> >> - if (!mask) >> - continue; >> - >> reg = PIPESTAT(pipe); >> - mask |= PIPESTAT_INT_ENABLE_MASK; >> - pipe_stats[pipe] = I915_READ(reg) & mask; >> + val = I915_READ(reg); >> + pipe_stats[pipe] |= val & mask; >> >> /* >> * Clear the PIPE*STAT regs before the IIR >> */ >> - if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS | >> - PIPESTAT_INT_STATUS_MASK)) >> - I915_WRITE(reg, pipe_stats[pipe]); >> + if (val & (PIPE_FIFO_UNDERRUN_STATUS | >> + PIPESTAT_INT_STATUS_MASK)) >> + I915_WRITE(reg, val); >> } >> spin_unlock(&dev_priv->irq_lock); >> +} >> + >> +static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv, >> + u32 *pipe_stats) >> +{ >> + struct drm_device *dev = dev_priv->dev; >> + int pipe; >> >> for_each_pipe(dev_priv, pipe) { >> if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS && >> @@ -1578,6 +1580,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) >> { >> struct drm_device *dev = arg; >> struct drm_i915_private *dev_priv = dev->dev_private; >> + u32 pipe_stats[I915_MAX_PIPES] = {}; >> u32 iir, gt_iir, pm_iir; >> irqreturn_t ret = IRQ_NONE; >> >> @@ -1600,6 +1603,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) >> /* Consume port before clearing IIR or we'll miss events */ >> if (iir & I915_DISPLAY_PORT_INTERRUPT) >> i9xx_hpd_irq_handler(dev); >> + valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats); > > This should be called even with iir==0 to get a possible underflow flag. > Although I realize now this wasn't handled properly even before this > patch, you needed at least one of the gt_iir/pm_iir/iir bits to be set. > >> I915_WRITE(VLV_IIR, iir); >> } >> >> @@ -1612,12 +1616,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) >> snb_gt_irq_handler(dev, dev_priv, gt_iir); >> if (pm_iir) >> gen6_rps_irq_handler(dev_priv, pm_iir); >> - /* Call regardless, as some status bits might not be >> - * signalled in iir */ >> - valleyview_pipestat_irq_handler(dev, iir); >> } >> >> out: >> + >> + /* Call regardless, as some status bits might not be >> + * signalled in iir */ >> + valleyview_pipestat_irq_handler(dev_priv, pipe_stats); >> return ret; >> } >> >> @@ -1625,6 +1630,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) >> { >> struct drm_device *dev = arg; >> struct drm_i915_private *dev_priv = dev->dev_private; >> + u32 pipe_stats[I915_MAX_PIPES] = {}; >> u32 master_ctl, iir; >> irqreturn_t ret = IRQ_NONE; >> >> @@ -1648,18 +1654,19 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) >> /* Consume port before clearing IIR or we'll miss events */ >> if (iir & I915_DISPLAY_PORT_INTERRUPT) >> i9xx_hpd_irq_handler(dev); >> + valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats); >> I915_WRITE(VLV_IIR, iir); >> } >> >> gen8_gt_irq_handler(dev_priv, master_ctl); > > I guess you'll want to clear/handle these IIRs in the same way, but that > would be a follow-up change. > >> >> - /* Call regardless, as some status bits might not be >> - * signalled in iir */ >> - valleyview_pipestat_irq_handler(dev, iir); >> - >> I915_WRITE_FW(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL); > > The above is still a regular I915_WRITE/POSTING_READ in -nightly, so > needs a rebase. Chris, if the patch is still valid, please rebase and repost. Thanks, Jani. > > With or without fixing the underrun flag readout: > Reviewed-by: Imre Deak <imre.deak@intel.com> > >> } >> >> + /* Call regardless, as some status bits might not be >> + * signalled in iir */ >> + valleyview_pipestat_irq_handler(dev_priv, pipe_stats); >> + >> return ret; >> } >> > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 66064511cffb..92bdfe6f91d8 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1457,24 +1457,21 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32 pm_iir) } } -static bool intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe) +static inline bool +intel_pipe_handle_vblank(struct drm_device *dev, enum pipe pipe) { - if (!drm_handle_vblank(dev, pipe)) - return false; - - return true; + return drm_handle_vblank(dev, pipe); } -static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) +static void valleyview_pipestat_irq_get(struct drm_i915_private *dev_priv, + u32 iir, u32 *pipe_stats) { - struct drm_i915_private *dev_priv = dev->dev_private; - u32 pipe_stats[I915_MAX_PIPES] = { }; int pipe; spin_lock(&dev_priv->irq_lock); for_each_pipe(dev_priv, pipe) { + u32 mask, val, iir_bit = 0; int reg; - u32 mask, iir_bit = 0; /* * PIPESTAT bits get signalled even when the interrupt is @@ -1486,6 +1483,7 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) /* fifo underruns are filterered in the underrun handler. */ mask = PIPE_FIFO_UNDERRUN_STATUS; + mask |= PIPESTAT_INT_ENABLE_MASK; switch (pipe) { case PIPE_A: @@ -1501,21 +1499,25 @@ static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir) if (iir & iir_bit) mask |= dev_priv->pipestat_irq_mask[pipe]; - if (!mask) - continue; - reg = PIPESTAT(pipe); - mask |= PIPESTAT_INT_ENABLE_MASK; - pipe_stats[pipe] = I915_READ(reg) & mask; + val = I915_READ(reg); + pipe_stats[pipe] |= val & mask; /* * Clear the PIPE*STAT regs before the IIR */ - if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS | - PIPESTAT_INT_STATUS_MASK)) - I915_WRITE(reg, pipe_stats[pipe]); + if (val & (PIPE_FIFO_UNDERRUN_STATUS | + PIPESTAT_INT_STATUS_MASK)) + I915_WRITE(reg, val); } spin_unlock(&dev_priv->irq_lock); +} + +static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv, + u32 *pipe_stats) +{ + struct drm_device *dev = dev_priv->dev; + int pipe; for_each_pipe(dev_priv, pipe) { if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS && @@ -1578,6 +1580,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) { struct drm_device *dev = arg; struct drm_i915_private *dev_priv = dev->dev_private; + u32 pipe_stats[I915_MAX_PIPES] = {}; u32 iir, gt_iir, pm_iir; irqreturn_t ret = IRQ_NONE; @@ -1600,6 +1603,7 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) /* Consume port before clearing IIR or we'll miss events */ if (iir & I915_DISPLAY_PORT_INTERRUPT) i9xx_hpd_irq_handler(dev); + valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats); I915_WRITE(VLV_IIR, iir); } @@ -1612,12 +1616,13 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg) snb_gt_irq_handler(dev, dev_priv, gt_iir); if (pm_iir) gen6_rps_irq_handler(dev_priv, pm_iir); - /* Call regardless, as some status bits might not be - * signalled in iir */ - valleyview_pipestat_irq_handler(dev, iir); } out: + + /* Call regardless, as some status bits might not be + * signalled in iir */ + valleyview_pipestat_irq_handler(dev_priv, pipe_stats); return ret; } @@ -1625,6 +1630,7 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) { struct drm_device *dev = arg; struct drm_i915_private *dev_priv = dev->dev_private; + u32 pipe_stats[I915_MAX_PIPES] = {}; u32 master_ctl, iir; irqreturn_t ret = IRQ_NONE; @@ -1648,18 +1654,19 @@ static irqreturn_t cherryview_irq_handler(int irq, void *arg) /* Consume port before clearing IIR or we'll miss events */ if (iir & I915_DISPLAY_PORT_INTERRUPT) i9xx_hpd_irq_handler(dev); + valleyview_pipestat_irq_get(dev_priv, iir, pipe_stats); I915_WRITE(VLV_IIR, iir); } gen8_gt_irq_handler(dev_priv, master_ctl); - /* Call regardless, as some status bits might not be - * signalled in iir */ - valleyview_pipestat_irq_handler(dev, iir); - I915_WRITE_FW(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL); } + /* Call regardless, as some status bits might not be + * signalled in iir */ + valleyview_pipestat_irq_handler(dev_priv, pipe_stats); + return ret; }
The PIPE.STAT register contains some interrupt status bits per pipe, and if assert cause the corresponding bit in the IIR to be asserted (thus raising an interrupt). When handling an interrupt, we should clear the PIPE.STAT generator first before clearing the IIR so that we do not miss events or cause spurious work. This ordering was broken by commit 27b6c122512ca30399bb1b39cc42eda83901f304 Author: Oscar Mateo <oscar.mateo@intel.com> Date: Mon Jun 16 16:11:00 2014 +0100 drm/i915/chv: Ack interrupts before handling them (CHV) commit 3ff60f89bc4836583f5bd195062f16c563bd97aa Author: Oscar Mateo <oscar.mateo@intel.com> Date: Mon Jun 16 16:10:58 2014 +0100 drm/i915/vlv: Ack interrupts before handling them (VLV) in attempting to reduce the amount of work done between receiving an interrupt and acknowledging it. In light of this motivation, split the pipestat interrupt handler into two, one to acknowledge and clear the interrupt and a later pass to process it. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Oscar Mateo <oscar.mateo@intel.com> Cc: Bob Beckett <robert.beckett@intel.com> Cc: Imre Deak <imre.deak@intel.com> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 55 +++++++++++++++++++++++------------------ 1 file changed, 31 insertions(+), 24 deletions(-)