[PATCH 1/2] DRI2: Reference count buffers across SwapBuffers
Christopher James Halse Rogers
christopher.halse.rogers at canonical.com
Wed Dec 8 16:55:52 PST 2010
On Wed, 2010-12-08 at 17:47 +0200, Pauli Nieminen wrote:
> 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?
I don't think so. The life cycle of a buffer isn't the same as that of
its parent DRI2Drawable - client requests can cause a buffer attached to
a living DRI2Drawable to be freed.
This patch means we potentially do the unuseful work of swapping buffers
which will immediately be freed if the client schedules a swap then
causes the buffers to be reallocated.
With more invasive API changes we could give drivers an opportunity to
handle this by marking existing swap requests as invalid, so it could
bail early.
>
> 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 <christopher.halse.rogers at canonical.com>
> > ---
> > 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
> >
> > _______________________________________________
> > xorg-devel at lists.x.org: X.Org development
> > Archives: http://lists.x.org/archives/xorg-devel
> > Info: http://lists.x.org/mailman/listinfo/xorg-devel
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
-------------- 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-devel/attachments/20101209/8af95308/attachment.pgp>
More information about the xorg-devel
mailing list