From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 2/4] drm/i915/vlv: Ack interrupts before handling them (VLV) Date: Mon, 16 Jun 2014 20:07:01 +0200 Message-ID: <20140616180701.GG5821@phenom.ffwll.local> References: <1402918229-7246-1-git-send-email-oscar.mateo@intel.com> <1402918229-7246-2-git-send-email-oscar.mateo@intel.com> <1402924770.22308.2.camel@intelbox> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-we0-f177.google.com (mail-we0-f177.google.com [74.125.82.177]) by gabe.freedesktop.org (Postfix) with ESMTP id C54396E5BA for ; Mon, 16 Jun 2014 11:07:08 -0700 (PDT) Received: by mail-we0-f177.google.com with SMTP id u56so5850746wes.8 for ; Mon, 16 Jun 2014 11:07:08 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1402924770.22308.2.camel@intelbox> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Imre Deak Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Mon, Jun 16, 2014 at 04:19:30PM +0300, Imre Deak wrote: > On Mon, 2014-06-16 at 12:30 +0100, oscar.mateo@intel.com wrote: > > From: Oscar Mateo > > > > 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 . > > > > Signed-off-by: Oscar Mateo > > --- > > 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch