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

[Mateo Lozano, Oscar]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel
> Vetter
> Sent: Monday, June 16, 2014 7:07 PM
> To: Deak, Imre
> Cc: Mateo Lozano, Oscar; intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 2/4] drm/i915/vlv: Ack interrupts before
> handling them (VLV)
> 
> On Mon, Jun 16, 2014 at 04:19:30PM +0300, Imre Deak wrote:
> > On Mon, 2014-06-16 at 12:30 +0100, oscar.mateo at intel.com wrote:
> > > From: Oscar Mateo <oscar.mateo at 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.
> > >
> > > Spotted by Bob Beckett <robert.beckett at intel.com>.
> > >
> > > Signed-off-by: Oscar Mateo <oscar.mateo at 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);
> >
> > Afaik the pipe underrun flag handled in
> > valleyview_pipestat_irq_handler() is not signaled in IIR, although
> > bspec is rather unclear about this.
> 
> Pipe underrun isn't signalling the top-level interrupt and we also can't mask it
> in any way. Hence the funny logic.
> -Daniel

In v3 of the patch I call valleyview_pipestat_irq_handler() regardless, with a comment explaining why. AFAICT, we don´t need to do the same thing for other handlers, but a thorough review of all the patches would be greatly appreciated (this is my first encounter with the interrupt code, other than the pretty clean GEN8 gt sources handler).