[PATCH] dri2: Reference-count DRI2 buffers.

[Michel Dänzer]

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?


> > > @@ -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™.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer