[Intel-gfx] [PATCH] drm/i915: fix locking around ironlake_enable|disable_display_irq

[Paulo Zanoni]

2013/6/25 Daniel Vetter <daniel.vetter at 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.


>
> Signed-off-by: Daniel Vetter <daniel.vetter at 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.


> +                       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()?


>                 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni