Re: [PATCH 2/4] drm/i915/vlv: Ack interrupts before handling them (VLV)

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Jun 16, 2014 at 12:30:27PM +0100, oscar.mateo@intel.com wrote:
> From: Oscar Mateo <oscar.mateo@intel.com>
> 
> Otherwise, we might receive a new interrupt before we have time to
> ack the first one, eventually missing it.
> 
> Notice that, before clearing a port-sourced interrupt in the IIR, the
> corresponding interrupt source status in the PORT_HOTPLUG_STAT must be
> cleared.

I believe PIPESTAT (and actually all multi-level interrupts) should be
handled the same way.

The way I would write interrupt handlers is something like this:

{
 iir = read(IIR);
 if (iir & X) {
 	iir_x = read(IIR_X);
 	write(IIR_X, iir_x);
 }
 if (iir & Y) {
 	iir_y = read(IIR_Y);
 	write(IIR_Y, iir_y);
 }
 ...
 write(IIR, iir);
 
 process_x(iir_x);
 process_y(iir_y);
 ...
}

And then we hope the hardware is sane enough to keep the IIR bit high as
long as the relevant sub-IIR bits are high, and also that the CPU interrupt
would be re-raised if any IIR bits are high when exiting the interrupt
handler. But I have the impression our hardware isn't quite that sane.

> 
> Spotted by Bob Beckett <robert.beckett@intel.com>.
> 
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 61 +++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 4439e2d..9d381cc 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1813,26 +1813,28 @@ static void i9xx_hpd_irq_handler(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	u32 hotplug_status = I915_READ(PORT_HOTPLUG_STAT);
>  
> -	if (IS_G4X(dev)) {
> -		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
> +	if (hotplug_status) {
> +		I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
> +		/*
> +		 * Make sure hotplug status is cleared before we clear IIR, or else we
> +		 * may miss hotplug events.
> +		 */
> +		POSTING_READ(PORT_HOTPLUG_STAT);
>  
> -		intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_g4x);
> -	} else {
> -		u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
> +		if (IS_G4X(dev)) {
> +			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_G4X;
>  
> -		intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_i915);
> -	}
> +			intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_g4x);
> +		} else {
> +			u32 hotplug_trigger = hotplug_status & HOTPLUG_INT_STATUS_I915;
>  
> -	if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) &&
> -	    hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
> -		dp_aux_irq_handler(dev);
> +			intel_hpd_irq_handler(dev, hotplug_trigger, hpd_status_i915);
> +		}
>  
> -	I915_WRITE(PORT_HOTPLUG_STAT, hotplug_status);
> -	/*
> -	 * Make sure hotplug status is cleared before we clear IIR, or else we
> -	 * may miss hotplug events.
> -	 */
> -	POSTING_READ(PORT_HOTPLUG_STAT);
> +		if ((IS_G4X(dev) || IS_VALLEYVIEW(dev)) &&
> +		    hotplug_status & DP_AUX_CHANNEL_MASK_INT_STATUS_G4X)
> +			dp_aux_irq_handler(dev);
> +	}
>  }
>  
>  static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> @@ -1843,29 +1845,36 @@ static irqreturn_t valleyview_irq_handler(int irq, void *arg)
>  	irqreturn_t ret = IRQ_NONE;
>  
>  	while (true) {
> -		iir = I915_READ(VLV_IIR);
>  		gt_iir = I915_READ(GTIIR);
>  		pm_iir = I915_READ(GEN6_PMIIR);
> +		iir = I915_READ(VLV_IIR);
>  
>  		if (gt_iir == 0 && pm_iir == 0 && iir == 0)
>  			goto out;
>  
> -		ret = IRQ_HANDLED;
> +		if (gt_iir)
> +			I915_WRITE(GTIIR, gt_iir);
>  
> -		snb_gt_irq_handler(dev, dev_priv, gt_iir);
> +		if (pm_iir)
> +			I915_WRITE(GEN6_PMIIR, pm_iir);
>  
> -		valleyview_pipestat_irq_handler(dev, iir);
> +		if (iir) {
> +			/* Consume port. Then clear IIR or we'll miss events */
> +			if (iir & I915_DISPLAY_PORT_INTERRUPT)
> +				i9xx_hpd_irq_handler(dev);
> +			I915_WRITE(VLV_IIR, iir);
> +		}
>  
> -		/* Consume port.  Then clear IIR or we'll miss events */
> -		if (iir & I915_DISPLAY_PORT_INTERRUPT)
> -			i9xx_hpd_irq_handler(dev);
> +		ret = IRQ_HANDLED;
> +
> +		if (gt_iir)
> +			snb_gt_irq_handler(dev, dev_priv, gt_iir);
>  
>  		if (pm_iir)
>  			gen6_rps_irq_handler(dev_priv, pm_iir);
>  
> -		I915_WRITE(GTIIR, gt_iir);
> -		I915_WRITE(GEN6_PMIIR, pm_iir);
> -		I915_WRITE(VLV_IIR, iir);
> +		if (iir)
> +			valleyview_pipestat_irq_handler(dev, iir);
>  	}
>  
>  out:
> -- 
> 1.9.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC