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

[Steven J Newbury]

On Tue, 2008-11-04 at 16:04 -0800, Eric Anholt wrote:
> 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+.
> 
I'm on 965GM and I'm having a serious interrupt problem since this patch
went into for-review:

Nov  7 04:20:22 infinity irq 16: nobody cared (try booting with the
"irqpoll" option)
Nov  7 04:20:22 infinity Pid: 0, comm: swapper Not tainted
2.6.28-rc3-00236-g1d7eff8 #23
Nov  7 04:20:22 infinity Call Trace:
Nov  7 04:20:22 infinity <IRQ>  [<ffffffff80491a25>] ?
i915_driver_irq_handler+0x53/0x186
Nov  7 04:20:22 infinity [<ffffffff80270b55>] __report_bad_irq+0x3d/0x8c
Nov  7 04:20:22 infinity [<ffffffff80270cb7>] note_interrupt+0x113/0x178
Nov  7 04:20:22 infinity [<ffffffff802713db>] handle_fasteoi_irq
+0x99/0xc3
Nov  7 04:20:22 infinity [<ffffffff8020ee5f>] do_IRQ+0x9c/0x11d
Nov  7 04:20:22 infinity [<ffffffff8020c826>] ret_from_intr+0x0/0xa
Nov  7 04:20:22 infinity <EOI>  [<ffffffff804572c0>] ?
acpi_idle_enter_simple+0x175/0x1a8
Nov  7 04:20:22 infinity [<ffffffff804572b6>] ? acpi_idle_enter_simple
+0x16b/0x1a8
Nov  7 04:20:22 infinity [<ffffffff8052af56>] ? cpuidle_idle_call
+0xa6/0xe0
Nov  7 04:20:22 infinity [<ffffffff8020b47a>] ? cpu_idle+0x4c/0xb0
Nov  7 04:20:22 infinity [<ffffffff80614551>] ? rest_init+0x75/0x77
Nov  7 04:20:22 infinity handlers:
Nov  7 04:20:22 infinity [<ffffffff804919d2>] (i915_driver_irq_handler
+0x0/0x186)
Nov  7 04:20:22 infinity Disabling IRQ #16

This happens after a random amount of time in X, athough never very
long.  From this point on there are no interrupts generated unless I
switch vts away from X and back again.  This gets interrupts working
again for a short while.

This is possibly also related to the massive slowdown I get X uses 20%+
CPU constantly and continually probes DDC, when I switch to battery,
this I had expected to be fixed by the recent patch removing ACPI event
handling, but strangely it still occurs.