[PATCH 1/2] DRI2: Reference count buffers across SwapBuffers
Christopher James Halse Rogers
christopher.halse.rogers at canonical.com
Tue Dec 7 22:56:33 PST 2010
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.
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
More information about the xorg-devel
mailing list