[Intel-gfx] [PATCH] drm/i915: don't queue flips during a flip pending event

[Eric Anholt]

On Mon, 3 May 2010 12:40:30 -0700, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> On Thu, 29 Apr 2010 15:52:36 -0700
> Eric Anholt <eric at anholt.net> wrote:
> > > +	if (intel_crtc->plane)
> > > +		flip_mask =
> > > I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT;
> > > +	else
> > > +		flip_mask =
> > > I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT; +
> > > +	/* Wait for any previous flip to finish */
> > > +	while (I915_READ(ISR) & flip_mask)
> > > +		;
> > > +
> > 
> > I'm confused about how this patch works.  It looks like there's a race
> > where the ISR bit could get set at this point after the loop checking
> > it, and you would lose.  The whole thing seems very dubious -- why
> > does the state of the ISR at the time you queue a flip matter?  I
> > could imagine there being an issue with the state of the ISR at the
> > point that a flip packet is processed, but this doesn't handle that
> > (seems like only throttling flips to avoid outstanding flips would)
> 
> [We talked about this in person; this is for posterity]
> 
> We have mutexes and spinlocks in place to make sure only one flip is
> queued at once, so checking the ISR should be safe here.
> 
> The reason we need to check is that hw could have queued a previous
> flip, and we may even have completed the first half of it (due to the
> flip pending interrupt).  But until the flip actually completes at
> vblank time, queuing another one will be "undefined" according to the
> docs.  Turns out in this case "undefined" means hangs and other bad
> behavior.
> 
> That said, I think this needs an Ironlake check too, since the bits
> have moved recently.

status update: taking this one off my todo list, since it needs to be
updated for ironlake
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20100701/15d4431d/attachment.sig>