[Intel-gfx] [PATCH] drm/i915: Clear PIPE.STAT before IIR on VLV/CHV
Imre Deak
imre.deak at intel.com
Tue Sep 1 04:19:35 PDT 2015
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 at 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 at 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 at chris-wilson.co.uk>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Bob Beckett <robert.beckett at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Daniel Vetter <daniel.vetter at 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 at intel.com>
> }
>
> + /* Call regardless, as some status bits might not be
> + * signalled in iir */
> + valleyview_pipestat_irq_handler(dev_priv, pipe_stats);
> +
> return ret;
> }
>
More information about the Intel-gfx
mailing list