On 08/12/10 07:56 +0100, ext Christopher James Halse Rogers wrote: > The SwapBuffers request requires that we trigger the swap at some point > in the future. Sane drivers implement this by passing this request to > something that will trigger a callback with the buffer pointers > at the appropriate time. > > The client can cause those buffers to be freed in X before the trigger > occurs, most easily by quitting. This leads to a server crash in > the driver when trying to do the swap. > > See http://bugs.freedesktop.org/show_bug.cgi?id=29065 for Radeon and > https://bugs.freedesktop.org/show_bug.cgi?id=28080 for Intel. >
Could this reference counting be applied to DRI2Drawable instead of per buffer? But in any case this looks like good idea. I didn't yet have to have detailed look what is inside the patch. > Signed-off-by: Christopher James Halse Rogers > <[email protected]> > --- > hw/xfree86/dri2/dri2.c | 98 ++++++++++++++++++++++++++++++++++++++--------- > hw/xfree86/dri2/dri2.h | 1 + > 2 files changed, 80 insertions(+), 19 deletions(-) > > diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c > index e4693d9..6da2e17 100644 > --- a/hw/xfree86/dri2/dri2.c > +++ b/hw/xfree86/dri2/dri2.c > @@ -107,12 +107,41 @@ typedef struct _DRI2Screen { > ConfigNotifyProcPtr ConfigNotify; > } DRI2ScreenRec; > > +typedef struct _DRI2SwapCompleteDataRec { > + DRI2BufferPtr src; > + DRI2BufferPtr dest; > + void * data; > +} DRI2SwapCompleteDataRec, *DRI2SwapCompleteDataPtr; > + > static DRI2ScreenPtr > DRI2GetScreen(ScreenPtr pScreen) > { > return dixLookupPrivate(&pScreen->devPrivates, dri2ScreenPrivateKey); > } > > +static void > +buffer_ref(DRI2BufferPtr buffer) > +{ > + buffer->refcnt++; > +} > + > +static void > +buffer_unref(DrawablePtr pDraw, DRI2BufferPtr buffer) > +{ > + DRI2ScreenPtr ds; > + if (buffer->refcnt == 0) { > + xf86DrvMsg(pDraw->pScreen->myNum, X_ERROR, > + "[DRI2] Attempt to unreference already freed buffer ignored\n"); > + return; > + } > + > + buffer->refcnt--; > + if (buffer->refcnt == 0) { > + ds = DRI2GetScreen(pDraw->pScreen); > + (*ds->DestroyBuffer)(pDraw, buffer); > + } > +} > + > static DRI2DrawablePtr > DRI2GetDrawable(DrawablePtr pDraw) > { > @@ -261,7 +290,6 @@ DRI2CreateDrawable(ClientPtr client, DrawablePtr pDraw, > XID id, > static int DRI2DrawableGone(pointer p, XID id) > { > DRI2DrawablePtr pPriv = p; > - DRI2ScreenPtr ds = pPriv->dri2_screen; > DRI2DrawableRefPtr ref, next; > WindowPtr pWin; > PixmapPtr pPixmap; > @@ -300,7 +328,7 @@ static int DRI2DrawableGone(pointer p, XID id) > > if (pPriv->buffers != NULL) { > for (i = 0; i < pPriv->bufferCount; i++) > - (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]); > + buffer_unref(pDraw, pPriv->buffers[i]); > > free(pPriv->buffers); > } > @@ -341,6 +369,7 @@ allocate_or_reuse_buffer(DrawablePtr pDraw, DRI2ScreenPtr > ds, > || !dimensions_match > || (pPriv->buffers[old_buf]->format != format)) { > *buffer = (*ds->CreateBuffer)(pDraw, attachment, format); > + buffer_ref(*buffer); > pPriv->serialNumber = DRI2DrawableSerial(pDraw); > return TRUE; > > @@ -355,13 +384,12 @@ static void > update_dri2_drawable_buffers(DRI2DrawablePtr pPriv, DrawablePtr pDraw, > DRI2BufferPtr *buffers, int *out_count, int > *width, int *height) > { > - DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen); > int i; > > if (pPriv->buffers != NULL) { > for (i = 0; i < pPriv->bufferCount; i++) { > if (pPriv->buffers[i] != NULL) { > - (*ds->DestroyBuffer)(pDraw, pPriv->buffers[i]); > + buffer_unref(pDraw, pPriv->buffers[i]); > } > } > > @@ -498,7 +526,7 @@ err_out: > > for (i = 0; i < count; i++) { > if (buffers[i] != NULL) > - (*ds->DestroyBuffer)(pDraw, buffers[i]); > + buffer_unref(pDraw, buffers[i]); > } > > free(buffers); > @@ -708,21 +736,31 @@ DRI2WakeClient(ClientPtr client, DrawablePtr pDraw, int > frame, > } > } > > +static void > +free_swap_complete_data (DrawablePtr pDraw, DRI2SwapCompleteDataPtr > pSwapData) > +{ > + buffer_unref(pDraw, pSwapData->src); > + buffer_unref(pDraw, pSwapData->dest); > + free(pSwapData); > +} > + > void > DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, int frame, > unsigned int tv_sec, unsigned int tv_usec, int type, > DRI2SwapEventPtr swap_complete, void *swap_data) > { > - ScreenPtr pScreen = pDraw->pScreen; > - DRI2DrawablePtr pPriv; > - CARD64 ust = 0; > - BoxRec box; > - RegionRec region; > + ScreenPtr pScreen = pDraw->pScreen; > + DRI2DrawablePtr pPriv; > + CARD64 ust = 0; > + BoxRec box; > + RegionRec region; > + DRI2SwapCompleteDataPtr pSwapData = swap_data; > > pPriv = DRI2GetDrawable(pDraw); > if (pPriv == NULL) { > xf86DrvMsg(pScreen->myNum, X_ERROR, > "[DRI2] %s: bad drawable\n", __func__); > + free_swap_complete_data(pDraw, pSwapData); > return; > } > > @@ -739,12 +777,15 @@ DRI2SwapComplete(ClientPtr client, DrawablePtr pDraw, > int frame, > > ust = ((CARD64)tv_sec * 1000000) + tv_usec; > if (swap_complete) > - swap_complete(client, swap_data, type, ust, frame, pPriv->swap_count); > + swap_complete(client, pSwapData->data, type, ust, frame, > + pPriv->swap_count); > > pPriv->last_swap_msc = frame; > pPriv->last_swap_ust = ust; > > DRI2WakeClient(client, pDraw, frame, tv_sec, tv_usec); > + > + free_swap_complete_data(pDraw, pSwapData); > } > > Bool > @@ -771,12 +812,13 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, > CARD64 target_msc, > CARD64 divisor, CARD64 remainder, CARD64 *swap_target, > DRI2SwapEventPtr func, void *data) > { > - ScreenPtr pScreen = pDraw->pScreen; > - DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen); > - DRI2DrawablePtr pPriv; > - DRI2BufferPtr pDestBuffer = NULL, pSrcBuffer = NULL; > - int ret, i; > - CARD64 ust, current_msc; > + ScreenPtr pScreen = pDraw->pScreen; > + DRI2ScreenPtr ds = DRI2GetScreen(pDraw->pScreen); > + DRI2DrawablePtr pPriv; > + DRI2BufferPtr pDestBuffer = NULL, pSrcBuffer = NULL; > + DRI2SwapCompleteDataPtr pSwapData; > + int ret, i; > + CARD64 ust, current_msc; > > pPriv = DRI2GetDrawable(pDraw); > if (pPriv == NULL) { > @@ -796,6 +838,23 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, > CARD64 target_msc, > "[DRI2] %s: drawable has no back or front?\n", __func__); > return BadDrawable; > } > + > + pSwapData = malloc(sizeof *pSwapData); > + if (pSwapData == NULL) > + return BadAlloc; > + > + /* > + * Take a reference to the buffers we're exchanging. > + * This ensures that these buffers will remain valid until the swap > + * is completed. > + * > + * DRI2SwapComplete is required to unref these buffers. > + */ > + buffer_ref(pSrcBuffer); > + buffer_ref(pDestBuffer); > + pSwapData->src = pSrcBuffer; > + pSwapData->dest = pDestBuffer; > + pSwapData->data = data; > > /* Old DDX or no swap interval, just blit */ > if (!ds->ScheduleSwap || !pPriv->swap_interval) { > @@ -812,7 +871,7 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, > CARD64 target_msc, > > (*ds->CopyRegion)(pDraw, ®ion, pDestBuffer, pSrcBuffer); > DRI2SwapComplete(client, pDraw, target_msc, 0, 0, DRI2_BLIT_COMPLETE, > - func, data); > + func, pSwapData); > return Success; > } > > @@ -851,11 +910,12 @@ DRI2SwapBuffers(ClientPtr client, DrawablePtr pDraw, > CARD64 target_msc, > > pPriv->swapsPending++; > ret = (*ds->ScheduleSwap)(client, pDraw, pDestBuffer, pSrcBuffer, > - swap_target, divisor, remainder, func, data); > + swap_target, divisor, remainder, func, pSwapData); > if (!ret) { > pPriv->swapsPending--; /* didn't schedule */ > xf86DrvMsg(pScreen->myNum, X_ERROR, > "[DRI2] %s: driver failed to schedule swap\n", __func__); > + free_swap_complete_data(pDraw, pSwapData); > return BadDrawable; > } > > diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h > index fe0bf6c..12343fd 100644 > --- a/hw/xfree86/dri2/dri2.h > +++ b/hw/xfree86/dri2/dri2.h > @@ -42,6 +42,7 @@ typedef struct { > unsigned int pitch; > unsigned int cpp; > unsigned int flags; > + unsigned int refcnt; > unsigned int format; > void *driverPrivate; > } DRI2BufferRec, *DRI2BufferPtr; > -- > 1.7.2.3 > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
