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
[EMAIL PROTECTED] [EMAIL PROTECTED]
signature.asc
Description: This is a digitally signed message part
------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/
-- _______________________________________________ Dri-devel mailing list Dri-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/dri-devel
