On Tue, 01 Sep 2015, Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
>> 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@xxxxxxxxx>
>> 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@xxxxxxxxxxxxxxxxxx>
>> Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
>> Cc: Bob Beckett <robert.beckett@xxxxxxxxx>
>> Cc: Imre Deak <imre.deak@xxxxxxxxx>
>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> ---
>> 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@xxxxxxxxx>
>
>> }
>>
>> + /* 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
