Re: [PATCH] drm/i915: Clear PIPE.STAT before IIR on VLV/CHV

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx