Re: [PATCH] drm/i915: fix locking around ironlake_enable|disable_display_irq

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Jun 26, 2013 at 06:15:42PM -0300, Paulo Zanoni wrote:
> 2013/6/25 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > The haswell unclaimed register handling code forgot to take the
> > spinlock. Since this is in the context of the non-rentrant interupt
> > handler and we only have one interrupt handler it is sufficient to
> > just grab the spinlock - we do not need to exclude any other
> > interrupts from running on the same cpu.
> >
> > To prevent such gaffles in the future sprinkle assert_spin_locked over
> > these functions. Unfornately this requires us to hold the spinlock in
> > the ironlake postinstall hook where it is not strictly required:
> > Currently that is run in single-threaded context and with userspace
> > exlcuded from running concurrent ioctls. Add a comment explaining
> > this.
> >
> > v2: ivb_can_enable_err_int also needs to be protected by the spinlock.
> > To ensure this won't happen in the future again also sprinkle a
> > spinlock assert in there.
> 
> Why does ivb_can_enable_err_int need it? Maybe we should add a comment
> inside the function explaining why it needs it.

It does access ->cpu_fifo_underrun_disabled. Just reading that won't race,
but it's important that the subsequent state change is consistent with the
state we've read (i.e. no one must be able to sneak in). Hence we need to
hold the lock around the entire if (can_enable)  do_enable; sequence.
> 
> 
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 27 ++++++++++++++++++++++++---
> >  1 file changed, 24 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index c482e8a..ff1fed4 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -95,6 +95,8 @@ static void i915_hpd_irq_setup(struct drm_device *dev);
> >  static void
> >  ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
> >  {
> > +       assert_spin_locked(&dev_priv->irq_lock);
> > +
> >         if ((dev_priv->irq_mask & mask) != 0) {
> >                 dev_priv->irq_mask &= ~mask;
> >                 I915_WRITE(DEIMR, dev_priv->irq_mask);
> > @@ -105,6 +107,8 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
> >  static void
> >  ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
> >  {
> > +       assert_spin_locked(&dev_priv->irq_lock);
> > +
> >         if ((dev_priv->irq_mask & mask) != mask) {
> >                 dev_priv->irq_mask |= mask;
> >                 I915_WRITE(DEIMR, dev_priv->irq_mask);
> > @@ -118,6 +122,8 @@ static bool ivb_can_enable_err_int(struct drm_device *dev)
> >         struct intel_crtc *crtc;
> >         enum pipe pipe;
> >
> > +       assert_spin_locked(&dev_priv->irq_lock);
> > +
> >         for_each_pipe(pipe) {
> >                 crtc = to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]);
> >
> > @@ -1218,8 +1224,11 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
> >         /* On Haswell, also mask ERR_INT because we don't want to risk
> >          * generating "unclaimed register" interrupts from inside the interrupt
> >          * handler. */
> > -       if (IS_HASWELL(dev))
> > +       if (IS_HASWELL(dev)) {
> > +               spin_lock(&dev_priv->irq_lock);
> >                 ironlake_disable_display_irq(dev_priv, DE_ERR_INT_IVB);
> > +               spin_unlock(&dev_priv->irq_lock);
> > +       }
> >
> >         gt_iir = I915_READ(GTIIR);
> >         if (gt_iir) {
> > @@ -1272,8 +1281,12 @@ static irqreturn_t ivybridge_irq_handler(int irq, void *arg)
> >                 ret = IRQ_HANDLED;
> >         }
> >
> > -       if (IS_HASWELL(dev) && ivb_can_enable_err_int(dev))
> > -               ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
> > +       if (IS_HASWELL(dev) && ivb_can_enable_err_int(dev)) {
> > +               spin_lock(&dev_priv->irq_lock);
> > +               if (ivb_can_enable_err_int(dev))
> 
> You're calling ivb_can_enable_err_int twice here.

Oops, will fix.

> 
> 
> > +                       ironlake_enable_display_irq(dev_priv, DE_ERR_INT_IVB);
> > +               spin_unlock(&dev_priv->irq_lock);
> > +       }
> >
> >         I915_WRITE(DEIER, de_ier);
> >         POSTING_READ(DEIER);
> > @@ -2633,6 +2646,8 @@ static void ibx_irq_postinstall(struct drm_device *dev)
> >
> >  static int ironlake_irq_postinstall(struct drm_device *dev)
> >  {
> > +       unsigned long irqflags;
> > +
> >         drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> >         /* enable kind of interrupts always enabled */
> >         u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
> > @@ -2671,7 +2686,13 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
> >                 /* Clear & enable PCU event interrupts */
> >                 I915_WRITE(DEIIR, DE_PCU_EVENT);
> >                 I915_WRITE(DEIER, I915_READ(DEIER) | DE_PCU_EVENT);
> > +
> > +               /* spinlocking not required here for correctness since interrupt
> > +                * setup is guaranteed to run in single-threaded context. But we
> > +                * need it to make the assert_spin_locked happy. */
> > +               spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> 
> If spinlocking is not even required, why take the irqsave one instead
> of just spin_lock()?

lockdep will complain about the inconsistent locking usage, since just
doing a spin_lock while the interrupt handler could also do a spin_lock
call can deadlock. Ofc lockdep doesn't now that at this point our
interrupt handler can't run, but it's imo established procedure to not
care about such details. So imo this doesn't warrant an extended comment.

> 
> 
> >                 ironlake_enable_display_irq(dev_priv, DE_PCU_EVENT);
> > +               spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> >         }
> >
> >         return 0;
> > --
> > 1.8.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Paulo Zanoni

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch