Re: [PATCH 8/8] drm/i915: fix hpd interrupt register locking

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Jun 27, 2013 at 11:41:25AM -0300, Paulo Zanoni wrote:
> 2013/6/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > 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 <eich@suse.de>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  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 <paulo.r.zanoni@intel.com>
> - Patch 3: in two comments you need to s/Interrup/Interrupt/. Then you
> can add Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> - 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 <paulo.r.zanoni@intel.com>
> 
> 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