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 > > <[email protected]> > > --- > > 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. _______________________________________________ xorg-driver-ati mailing list [email protected] http://lists.x.org/mailman/listinfo/xorg-driver-ati
