[PATCH] dri2: Reference-count DRI2 buffers.

[Christopher James Halse Rogers]

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.

> 
> > Fixes: http://bugs.freedesktop.org/show_bug.cgi?id=29065
> > Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>
> > ---
> >  src/radeon_dri2.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++----
> >  1 files changed, 49 insertions(+), 5 deletions(-)
> > 
> > diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
> > index 6356711..00b5712 100644
> > --- a/src/radeon_dri2.c
> > +++ b/src/radeon_dri2.c
> > @@ -55,6 +55,7 @@ typedef DRI2Buffer2Ptr BufferPtr;
> >  struct dri2_buffer_priv {
> >      PixmapPtr   pixmap;
> >      unsigned int attachment;
> > +    unsigned int refcnt;
> >  };
> >  
> > 
> > @@ -236,6 +237,7 @@ radeon_dri2_create_buffer(DrawablePtr drawable,
> >      buffers->flags = 0; /* not tiled */
> >      privates->pixmap = pixmap;
> >      privates->attachment = attachment;
> > +    privates->refcnt = 1;
> >  
> >      return buffers;
> >  }
> > @@ -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.