[Intel-gfx] [PATCH] [drm/i915] Protect vblank IRQ reg access with spinlock

[Eric Anholt]

On Thu, 2008-10-16 at 11:31 -0700, Keith Packard wrote:
> From d70b6b672bc4a1ac4356d46d03c328c689aa76a7 Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp at keithp.com>
> Date: Thu, 16 Oct 2008 11:28:46 -0700
> Subject: [PATCH] [drm/i915] Protect vblank IRQ reg access with spinlock
> 
> This uses the same spinlock as the user_irq code as it shares the same
> register, ensuring that interrupt registers are updated atomically.

This looks good, and I've pushed it.

I've still got a concern about our synchronization of the register
control, though:  For the MSI path, you've got the IMR=0xffffffff from
entry until events have been handled and we're ready for interrupts to
be queued at us again (IMR=dev_priv->irq_mask_reg).  If somebody comes
along during that time and, say, triggers a wait on rendering to a BO,
then they'll i915_enable_irq(USER_INTERRUPT) and set
IMR=dev_priv->irq_mask_reg before interrupt events have been handled and
you're ready for it to be set back to that.

Perhaps suppress interrupt_enable/disable IMR setting between the
IMR=0xffffffff and ending IMR=dev_priv->irq_mask_reg?

> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |   57 ++++++++++++++++++++++-----------------
>  1 files changed, 32 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index baae511..8332c12 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -584,33 +584,36 @@ int i915_enable_vblank(struct drm_device *dev, int plane)
>  	int pipe = i915_get_pipe(dev, plane);
>  	u32	pipestat_reg = 0;
>  	u32	pipestat;
> +	u32	interrupt = 0;
>  
>  	switch (pipe) {
>  	case 0:
>  		pipestat_reg = PIPEASTAT;
> -		i915_enable_irq(dev_priv, I915_DISPLAY_PIPE_A_EVENT_INTERRUPT);
> +		interrupt = I915_DISPLAY_PIPE_A_EVENT_INTERRUPT;
>  		break;
>  	case 1:
>  		pipestat_reg = PIPEBSTAT;
> -		i915_enable_irq(dev_priv, I915_DISPLAY_PIPE_B_EVENT_INTERRUPT);
> +		interrupt = I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
>  		break;
>  	default:
>  		DRM_ERROR("tried to enable vblank on non-existent pipe %d\n",
>  			  pipe);
> -		break;
> +		return 0;
>  	}
>  
> -	if (pipestat_reg) {
> -		pipestat = I915_READ(pipestat_reg);
> -		if (IS_I965G(dev))
> -			pipestat |= PIPE_START_VBLANK_INTERRUPT_ENABLE;
> -		else
> -			pipestat |= PIPE_VBLANK_INTERRUPT_ENABLE;
> -		/* Clear any stale interrupt status */
> -		pipestat |= (PIPE_START_VBLANK_INTERRUPT_STATUS |
> -			     PIPE_VBLANK_INTERRUPT_STATUS);
> -		I915_WRITE(pipestat_reg, pipestat);
> -	}
> +	spin_lock(&dev_priv->user_irq_lock);
> +	pipestat = I915_READ(pipestat_reg);
> +	if (IS_I965G(dev))
> +		pipestat |= PIPE_START_VBLANK_INTERRUPT_ENABLE;
> +	else
> +		pipestat |= PIPE_VBLANK_INTERRUPT_ENABLE;
> +	/* Clear any stale interrupt status */
> +	pipestat |= (PIPE_START_VBLANK_INTERRUPT_STATUS |
> +		     PIPE_VBLANK_INTERRUPT_STATUS);
> +	I915_WRITE(pipestat_reg, pipestat);
> +	(void) I915_READ(pipestat_reg);	/* Posting read */
> +	i915_enable_irq(dev_priv, interrupt);
> +	spin_unlock(&dev_priv->user_irq_lock);
>  
>  	return 0;
>  }
> @@ -621,31 +624,35 @@ void i915_disable_vblank(struct drm_device *dev, int plane)
>  	int pipe = i915_get_pipe(dev, plane);
>  	u32	pipestat_reg = 0;
>  	u32	pipestat;
> +	u32	interrupt = 0;
>  
>  	switch (pipe) {
>  	case 0:
>  		pipestat_reg = PIPEASTAT;
> -		i915_disable_irq(dev_priv, I915_DISPLAY_PIPE_A_EVENT_INTERRUPT);
> +		interrupt = I915_DISPLAY_PIPE_A_EVENT_INTERRUPT;
>  		break;
>  	case 1:
>  		pipestat_reg = PIPEBSTAT;
> -		i915_disable_irq(dev_priv, I915_DISPLAY_PIPE_B_EVENT_INTERRUPT);
> +		interrupt = I915_DISPLAY_PIPE_B_EVENT_INTERRUPT;
>  		break;
>  	default:
>  		DRM_ERROR("tried to disable vblank on non-existent pipe %d\n",
>  			  pipe);
> +		return;
>  		break;
>  	}
>  
> -	if (pipestat_reg) {
> -		pipestat = I915_READ(pipestat_reg);
> -		pipestat &= ~(PIPE_START_VBLANK_INTERRUPT_ENABLE |
> -			      PIPE_VBLANK_INTERRUPT_ENABLE);
> -		/* Clear any stale interrupt status */
> -		pipestat |= (PIPE_START_VBLANK_INTERRUPT_STATUS |
> -			     PIPE_VBLANK_INTERRUPT_STATUS);
> -		I915_WRITE(pipestat_reg, pipestat);
> -	}
> +	spin_lock(&dev_priv->user_irq_lock);
> +	i915_disable_irq(dev_priv, interrupt);
> +	pipestat = I915_READ(pipestat_reg);
> +	pipestat &= ~(PIPE_START_VBLANK_INTERRUPT_ENABLE |
> +		      PIPE_VBLANK_INTERRUPT_ENABLE);
> +	/* Clear any stale interrupt status */
> +	pipestat |= (PIPE_START_VBLANK_INTERRUPT_STATUS |
> +		     PIPE_VBLANK_INTERRUPT_STATUS);
> +	I915_WRITE(pipestat_reg, pipestat);
> +	(void) I915_READ(pipestat_reg);	/* Posting read */
> +	spin_unlock(&dev_priv->user_irq_lock);
>  }
>  
>  /* Set the vblank monitor pipe
> -- 
> 1.5.6.5
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
-- 
Eric Anholt
eric at anholt.net                         eric.anholt at intel.com


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20081016/57b1b9b4/attachment.sig>