> -----Original Message----- > From: Daniel Vetter [mailto:daniel.vet...@ffwll.ch] On Behalf Of Daniel > Vetter > Sent: Monday, June 16, 2014 7:07 PM > To: Deak, Imre > Cc: Mateo Lozano, Oscar; intel-gfx@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.ma...@intel.com wrote: > > > From: Oscar Mateo <oscar.ma...@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.beck...@intel.com>. > > > > > > Signed-off-by: Oscar Mateo <oscar.ma...@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). _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
