[Intel-gfx] [PATCH] drm/i915: untangle page flip completion

[Jesse Barnes]

On Wed, 27 Jan 2010 09:08:24 +0000
Chris Wilson <chris at chris-wilson.co.uk> wrote:

> On Tue, 26 Jan 2010 14:40:05 -0800, Jesse Barnes <jbarnes at virtuousgeek.org> wrote:
> > When a new page flip is requested, we need to both queue an unpin for
> > the current framebuffer, and also increment the flip pending count on
> > the newly submitted buffer.
> > 
> > At flip finish time, we need to unpin the old fb and decrement the flip
> > pending count on the new buffer.
> > 
> > The old code was conflating the two, and led to hangs when new direct
> > rendered apps were started, replacing the existing frame buffer.  This
> > patch splits out the buffers and prevents the hangs.
> 
> I'm not seeing where old_fb_obj != pending_flip_obj. Indeed, you only take
> a reference on old_fb_obj.

In intel_crtc_page_flip we save off the current intel_fb->obj for later
unpinning.  Later on in the function, we reset intel_fb to the fb that
was passed into the ioctl, a different object (the new fb).

I think that was the core of this confusion.  But maybe you're seeing
something I'm not, I've been staring at this for a long time and it's
all just a blur to me now.

> >  	/* Initial scanout buffer will have a 0 pending flip count */
> >  	if ((atomic_read(&obj_priv->pending_flip) == 0) ||
> 
> I'm still a little skeptical about this one - I keep trying to work how we
> end up with the old scanout buffer on the flip path. It seems like this
> patch is trying to tell me something...

I don't like this either.  But see above.  We pass the old fb object in
the work struct, and that's what the finish handler operates on.  It
both tries to unpin it *and* decrement its flip count (at least before
this patch).

With this patch added I think the 0 check could be removed or turned
into a BUG_ON, since we should never be decrementing the flip count on
a buffer that wasn't previously incremented.  I'll give that a try just
to confirm.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center