From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 8/8] drm/i915: fix hpd interrupt register locking Date: Thu, 27 Jun 2013 19:44:49 +0200 Message-ID: <20130627174449.GT18285@phenom.ffwll.local> References: <1372333505-7037-1-git-send-email-daniel.vetter@ffwll.ch> <1372333505-7037-8-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f177.google.com (mail-ea0-f177.google.com [209.85.215.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 8377BE5EEA for ; Thu, 27 Jun 2013 10:44:51 -0700 (PDT) Received: by mail-ea0-f177.google.com with SMTP id j14so558777eak.22 for ; Thu, 27 Jun 2013 10:44:50 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: Egbert Eich , Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, Jun 27, 2013 at 11:41:25AM -0300, Paulo Zanoni wrote: > 2013/6/27 Daniel Vetter : > > Our interrupt handler (in hardird context) could race with the timer > > s/hardird/hardirq/ > > > (in softirq context), hence we need to hold the spinlock around the > > call to ->hdp_irq_setup in intel_hpd_irq_handler, too. > > > > But as an optimization (and more so to clarify things) we don't need > > to do the irqsave/restore dance in the hardirq context. > > > > Note also that on ilk+ the race isn't just against the hotplug > > reenable timer, but also against the fifo underrun reporting. That one > > also modifies the SDEIMR register (again protected by the same > > dev_priv->irq_lock). > > > > To lock things down again sprinkle a assert_spin_locked. But exclude > > the functions touching SDEIMR for now, I want to extract them all into > > a new helper function (like we do already for pipestate, display > > interrupts and all the various gt interrupts). > > > > v2: Add the missing 't' Egbert spotted in a comment. > > > > Cc: Egbert Eich > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/i915_irq.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > > index 95999bc..6637575 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -881,15 +881,13 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev, > > const u32 *hpd) > > { > > drm_i915_private_t *dev_priv = dev->dev_private; > > - unsigned long irqflags; > > int i; > > bool storm_detected = false; > > > > if (!hotplug_trigger) > > return; > > > > - spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > > - > > + spin_lock(&dev_priv->irq_lock); > > for (i = 1; i < HPD_NUM_PINS; i++) { > > > > if (!(hpd[i] & hotplug_trigger) || > > @@ -912,10 +910,9 @@ static inline void intel_hpd_irq_handler(struct drm_device *dev, > > } > > } > > > > - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > - > > if (storm_detected) > > dev_priv->display.hpd_irq_setup(dev); > > + spin_unlock(&dev_priv->irq_lock); > > > > queue_work(dev_priv->wq, > > &dev_priv->hotplug_work); > > @@ -2851,7 +2848,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev) > > I915_WRITE(PIPESTAT(1), 0xffff); > > POSTING_READ(VLV_IER); > > > > - /* Interrup setup is already guaranteed to be single-threaded, this is > > + /* Interrupt setup is already guaranteed to be single-threaded, this is > > Here you're fixing an error introduced in patch 03, instead of the > error pointed by Egbert. > > > * just to make the assert_spin_locked check happy. */ > > spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > > i915_enable_pipestat(dev_priv, 0, pipestat_enable); > > @@ -3395,6 +3392,8 @@ static void i915_hpd_irq_setup(struct drm_device *dev) > > struct intel_encoder *intel_encoder; > > u32 hotplug_en; > > > > + assert_spin_locked(&dev_priv->irq_lock); > > + > > if (I915_HAS_HOTPLUG(dev)) { > > hotplug_en = I915_READ(PORT_HOTPLUG_EN); > > hotplug_en &= ~HOTPLUG_INT_EN_MASK; > > @@ -3678,6 +3677,7 @@ void intel_hpd_init(struct drm_device *dev) > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_mode_config *mode_config = &dev->mode_config; > > struct drm_connector *connector; > > + unsigned long irqflags; > > int i; > > > > for (i = 1; i < HPD_NUM_PINS; i++) { > > @@ -3690,6 +3690,11 @@ void intel_hpd_init(struct drm_device *dev) > > if (!connector->polled && I915_HAS_HOTPLUG(dev) && intel_connector->encoder->hpd_pin > HPD_NONE) > > connector->polled = DRM_CONNECTOR_POLL_HPD; > > } > > + > > + /* Interrup setup is already guaranteed to be single-threaded, this is > > s/Interrup/Interrupt/ > > > > > + * just to make the assert_spin_locked checks happy. */ > > + spin_lock_irqsave(&dev_priv->irq_lock, irqflags); > > if (dev_priv->display.hpd_irq_setup) > > dev_priv->display.hpd_irq_setup(dev); > > + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); > > } > > -- > > 1.8.1.4 > > > > So here's the review status for the 8 patches resent today: > - Patches 1 and 2: Reviewed-by: Paulo Zanoni > - Patch 3: in two comments you need to s/Interrup/Interrupt/. Then you > can add Reviewed-by: Paulo Zanoni > - Patches 4-7: You already have my Reviewed-by, but if you look in the > commit messages, some lines contain just my name and email, without > the "Reviewed-by:" text before them. I also wouldn't mind if you > resend patches 6 and 7 based on Chris's comments, but I don't really > care since the problem is actually solved in patch 7. > - Patch 8: due to the change you'll do in patch 3, you'll have to redo > patch 8. If you fix both the "Interrup" and the "hardird" typo you can > also add Reviewed-by: Paulo Zanoni > > I also booted a Haswell with these new patches and after 120 seconds > of uptime I don't see any new WARNs or ERRORs. Ok, I've merged the first 8 patches from this series, thanks for the review. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch