[Intel-gfx] [PATCH] Manage PIPESTAT pending interrupt values to unblock vblank interrupts

[Eric Anholt]

On Tue, 2008-11-04 at 02:03 -0800, Keith Packard wrote:
> The pipestat fields affect reporting of all vblank-related interrupts, so we
> have to reset them during the irq_handler, and while enabling vblank
> interrupts.
> 
> This patch adds i915_enable_pipestat and i915_disable_pipestat to abstract
> out the steps needed to change the reported interrupts.

This looks pretty good, and covers a lot of my concerns with interrupt
handling.  See comments, and my additional MSI patch that I required for
stability against wiggling a window over vblank-synced glxgears
(reproducible in <30s before, and I've now spent a couple of minutes
wiggling windows successfully).

> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b2ba2b6..f7ac581 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -34,28 +34,69 @@
>  #define MAX_NOPID ((u32)~0)
>  
>  /** These are the interrupts used by the driver */
> -#define I915_INTERRUPT_ENABLE_MASK (I915_USER_INTERRUPT |		\
> -				    I915_ASLE_INTERRUPT |		\
> -				    I915_DISPLAY_PIPE_A_EVENT_INTERRUPT | \
> -				    I915_DISPLAY_PIPE_B_EVENT_INTERRUPT)
> +#define I915_INTERRUPT_ENABLE_FIX (I915_ASLE_INTERRUPT | \
> +				   I915_DISPLAY_PIPE_A_EVENT_INTERRUPT |  \
> +				   I915_DISPLAY_PIPE_B_EVENT_INTERRUPT)
> +
> +#define I915_INTERRUPT_ENABLE_VAR (I915_USER_INTERRUPT)
> +
> +#define I915_INTERRUPT_ENABLE_MASK (I915_INTERRUPT_ENABLE_FIX | \
> +				    I915_INTERRUPT_ENABLE_VAR)
>  
>  void
>  i915_enable_irq(drm_i915_private_t *dev_priv, u32 mask)
>  {
> -	if ((dev_priv->irq_mask_reg & mask) != 0) {
> -		dev_priv->irq_mask_reg &= ~mask;
> -		I915_WRITE(IMR, dev_priv->irq_mask_reg);
> -		(void) I915_READ(IMR);
> +	mask &= I915_INTERRUPT_ENABLE_VAR;
> +	if ((dev_priv->irq_enable_reg & mask) != mask) {
> +		dev_priv->irq_enable_reg |= mask;
> +		I915_WRITE(IER, dev_priv->irq_enable_reg);
> +		(void) I915_READ(IER);
>  	}
>  }

IER versus IMR would be worthwhile to mention in the commit message.

>  	spin_lock_irqsave(&dev_priv->user_irq_lock, irqflags);
> -	/* Enabling vblank events in IMR comes before PIPESTAT write, or
> -	 * there's a race where the PIPESTAT vblank bit gets set to 1, so
> -	 * the OR of enabled PIPESTAT bits goes to 1, so the PIPExEVENT in
> -	 * ISR flashes to 1, but the IIR bit doesn't get set to 1 because
> -	 * IMR masks it.  It doesn't ever get set after we clear the masking
> -	 * in IMR because the ISR bit is edge, not level-triggered, on the
> -	 * OR of PIPESTAT bits.
> -	 */
> -	i915_enable_irq(dev_priv, interrupt);
> -	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_pipestat(dev_priv, pipe, I915_VBLANK_INTERRUPT_ENABLE);

I'm uncomfortable with the dropping of the START_VBLANK versus VBLANK,
as I'm pretty sure people had figured out that the other bit was bad
news on 965+.

>  int i915_driver_irq_postinstall(struct drm_device *dev)
> @@ -834,23 +805,25 @@ int i915_driver_irq_postinstall(struct drm_device *dev)
>  	INIT_WORK(&dev_priv->vblank_work, i915_vblank_work_handler);
>  	dev_priv->swaps_pending = 0;
>  
> -	/* Set initial unmasked IRQs to just the selected vblank pipes. */
> -	dev_priv->irq_mask_reg = ~0;
> -
>  	ret = drm_vblank_init(dev, num_pipes);
>  	if (ret)
>  		return ret;
>  
>  	dev_priv->vblank_pipe = DRM_I915_VBLANK_PIPE_A | DRM_I915_VBLANK_PIPE_B;
> -	dev_priv->irq_mask_reg &= ~I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT;
> -	dev_priv->irq_mask_reg &= ~I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
>  
>  	dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
>  
> -	dev_priv->irq_mask_reg &= I915_INTERRUPT_ENABLE_MASK;
> +	dev_priv->irq_enable_reg = I915_INTERRUPT_ENABLE_FIX;
> +
> +	dev_priv->pipestat[0] = 0;
> +	dev_priv->pipestat[1] = 0;
> +
> +	/* Disable pipe interrupt enables, clear pending pipe status */
> +	I915_WRITE(PIPEASTAT, I915_READ(PIPEASTAT) & 0x8000ffff);
> +	I915_WRITE(PIPEBSTAT, I915_READ(PIPEBSTAT) & 0x8000ffff);

Could we get a PIPESTAT_STATUS_MASK instead of 0x800ffff all over?

-- 
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/20081104/060f433c/attachment.sig>