[PATCH] dri2: Reference-count DRI2 buffers.

[Christopher James Halse Rogers]

On Wed, 2010-08-18 at 20:52 +0200, Oldřich Jedlička wrote: 
> On Wednesday 18 August 2010 10:49:20 Michel Dänzer wrote:
> > On Mit, 2010-08-18 at 18:24 +1000, Christopher James Halse Rogers
> > 
> > wrote:
> > > On Tue, 2010-08-17 at 11:41 +0100, Michel Dänzer wrote:
> > > > On Die, 2010-08-17 at 12:19 +1000, Christopher James Halse Rogers
> > > > 
> > > > wrote:
> > > > > When a client calls ScheduleSwap we set up a kernel callback when the
> > > > > relevent vblank event occurs.  However, it's possible for the client
> > > > > to go away between calling ScheduleSwap and the vblank event,
> > > > > resulting in the buffers being destroyed before they're passed to
> > > > > radeon_dri2_frame_event_handler.
> > > > > 
> > > > > Add reference-counting to the buffers and take a reference in
> > > > > radeon_dri2_schedule_swap to ensure the buffers won't be destroyed
> > > > > before the vblank event is dealt with.
> > > > 
> > > > Basically sounds good, but: If the client went away, what does
> > > > event->client point to in radeon_dri2_frame_event_handler()?
> > > 
> > > Well, it points somewhere in the clients array, which will hopefully
> > > have clientGone set.  At worst, it looks like it'll write a
> > > DRI2_BufferSwapComplete event to the unlucky client which claimed the
> > > empty spot.
> > 
> > Maybe that's acceptable, but has it been investigated if the client
> > destruction could be deferred until the event arrival instead / as well?
> 
> Isn't the ClientPtr allocated in dix/dispatch.c in method NextAvailableClient? 
> It uses dixAllocateObjectWithPrivates that calls malloc, so it could happen 
> that your ClientPtr might be freed (method CloseDownClient) when the callback 
> arrives.
> 

Urgh, yes.  I think that we can get notified of clients that go away, so
we could probably hack up a list of clients to not send to.  That's
really ugly though.  I'll see if I can find a cleaner solution.

> The ClientRec doesn't have any reference counting. My opinion is that holding 
> the ClientPtr isn't good (but I'm not xorg-server developper, so I might miss 
> something).
> 
> Oldřich.
> 
> > > > > @@ -267,13 +269,26 @@ radeon_dri2_destroy_buffer(DrawablePtr
> > > > > drawable, BufferPtr buffers)
> > > > > 
> > > > >      if(buffers)
> > > > >      {
> > > > >      
> > > > >          ScreenPtr pScreen = drawable->pScreen;
> > > > > 
> > > > > -        struct dri2_buffer_priv *private;
> > > > > +        struct dri2_buffer_priv *private = buffers->driverPrivate;
> > > > > 
> > > > > -        private = buffers->driverPrivate;
> > > > > -        (*pScreen->DestroyPixmap)(private->pixmap);
> > > > > +        /* Trying to free an already freed buffer is unlikely to end
> > > > > well */ +        if (private->refcnt == 0) {
> > > > > +            ScrnInfoPtr scrn = xf86Screens[pScreen->myNum];
> > > > > 
> > > > > -        free(buffers->driverPrivate);
> > > > > -        free(buffers);
> > > > > +            xf86DrvMsg(scrn->scrnIndex, X_WARNING,
> > > > > +                       "Attempted to destroy previously destroyed
> > > > > buffer.\ + This is a programming error\n");
> > > > > +            return;
> > > > > +	}
> > > > > +
> > > > > +        private->refcnt--;
> > > > > +        if (private->refcnt == 0)
> > > > > +        {
> > > > > +            (*pScreen->DestroyPixmap)(private->pixmap);
> > > > > +
> > > > > +            free(buffers->driverPrivate);
> > > > > +            free(buffers);
> > > > > +        }
> > > > 
> > > > Also, pixmaps already have a reference count, please just use that
> > > > instead of adding another layer.
> > > 
> > > Can we use that?  I originally thought of using that layer, but won't
> > > that interfere with the pixmap reference counting?  It would work for
> > > attachment != DRI2BufferFrontLeft since the pixmap is entirely private
> > > in that case, but with attachment == DRI2BufferFrontLeft the buffer
> > > pixmap is visible to the outside, and so might have a lifespan different
> > > to the DRI2 buffer.
> > 
> > That's what the reference count is for. :) As long as
> > increasing/decreasing the reference count is balanced in every layer
> > using the pixmap it should Just Work™.

It looks to me like we sometimes need to destroy DRI2 buffers which are
associated with a still-valid pixmap, so we can't use the pixmap's
reference count.  Is there some reason why the following can't occur:

* Client calls DRI2GetBuffers with a pixmap Drawable A and including
attachment DRI2BufferFrontLeft.  This wanders through the DRI2 dispatch
code, resulting in radeon_dri2_create_buffer incrementing the refcnt of
A (now at least refcnt 2).

* Client later calls DRI2GetBuffers on A again (with any set of required
attachments).  This calls through do_get_buffers in dri2.c to
update_dri2_drawable_buffers, which calls DestroyBuffer on all existing
buffers.  This calls into radeon_dri2_destroy_buffer, which decrements
the pixmap's refcnt, but doesn't free the buffer, as we've got a
non-zero refcnt.

* Client merrily repeats this dance, creating DRI2 buffers which never
get freed.

There may well be something that I'm missing, but it looks like a single
pixmap can have multiple buffers (with attachment DRI2BufferFrontLeft)
created from it over time so we can't use the pixmap's reference count.

For buffers which create their own pixmap (ie: all buffers without
attachment == DRI2BufferFrontLeft or BufferStencil once that codepath is
fixed) we could safely piggy back on the pixmap's reference count.  I
don't see how that would work with DRI2BufferFrontLeft, though. 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-driver-ati/attachments/20100819/78dbf469/attachment.pgp>