[PATCH v2 06/10] DRI2: Reference count buffers across SwapBuffers

Pauli ext-pauli.nieminen at nokia.com
Tue Feb 8 13:42:52 PST 2011


From: Christopher James Halse Rogers <christopher.halse.rogers at canonical.com>

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>
Signed-off-by: Pauli Nieminen <ext-pauli.nieminen at nokia.com>
---
 hw/xfree86/dri2/dri2.c |   68 ++++++++++++++++++++++++++++++++++++------------
 hw/xfree86/dri2/dri2.h |    1 +
 2 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/hw/xfree86/dri2/dri2.c b/hw/xfree86/dri2/dri2.c
index 1693546..5868fb4 100644
--- a/hw/xfree86/dri2/dri2.c
+++ b/hw/xfree86/dri2/dri2.c
@@ -109,8 +109,10 @@ typedef struct _DRI2Screen {
 } DRI2ScreenRec;
 
 typedef struct _DRI2SwapCompleteDataRec {
-    ClientPtr client;
-    void *data;
+    DRI2BufferPtr	src;
+    DRI2BufferPtr	dest;
+    ClientPtr		client;
+    void *		data;
 } DRI2SwapCompleteDataRec, *DRI2SwapCompleteDataPtr;
 
 static DRI2ScreenPtr
@@ -119,6 +121,23 @@ DRI2GetScreen(ScreenPtr pScreen)
     return dixLookupPrivate(&pScreen->devPrivates, dri2ScreenPrivateKey);
 }
 
+static void
+buffer_ref(DRI2BufferPtr buffer)
+{
+    buffer->refcnt++;
+}
+
+static void
+buffer_unref(DRI2DrawablePtr pPriv, DRI2BufferPtr buffer)
+{
+    DRI2ScreenPtr ds = pPriv->dri2_screen;
+
+    buffer->refcnt--;
+    if (buffer->refcnt == 0) {
+	(*ds->DestroyBuffer)(pPriv, buffer);
+    }
+}
+
 DRI2DrawablePtr
 DRI2GetDrawable(DrawablePtr pDraw)
 {
@@ -330,7 +349,6 @@ DRI2DestroyDrawable(ClientPtr client, DRI2DrawablePtr pPriv, XID id)
 static int DRI2DrawableGone(pointer p, XID id)
 {
     DRI2DrawablePtr pPriv = p;
-    DRI2ScreenPtr   ds = pPriv->dri2_screen;
     DRI2DrawableRefPtr ref, next;
     DrawablePtr pDraw = pPriv->drawable;
     int i;
@@ -376,7 +394,7 @@ static int DRI2DrawableGone(pointer p, XID id)
 
     if (pPriv->buffers != NULL) {
 	for (i = 0; i < pPriv->bufferCount; i++)
-	    (*ds->DestroyBuffer)(pPriv, pPriv->buffers[i]);
+	    buffer_unref(pPriv, pPriv->buffers[i]);
 
 	free(pPriv->buffers);
     }
@@ -417,6 +435,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;
 
@@ -431,14 +450,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)(pPriv, pPriv->buffers[i]);
-	    }
+	    if (pPriv->buffers[i] != NULL)
+		buffer_unref(pPriv, pPriv->buffers[i]);
 	}
 
 	free(pPriv->buffers);
@@ -574,7 +591,7 @@ err_out:
 
     for (i = 0; i < count; i++) {
 	    if (buffers[i] != NULL)
-		    (*ds->DestroyBuffer)(pPriv, buffers[i]);
+		    buffer_unref(pPriv, buffers[i]);
     }
 
     free(buffers);
@@ -786,6 +803,14 @@ DRI2WakeClient(ClientPtr client, DRI2DrawablePtr pPriv, int frame,
     }
 }
 
+static void
+free_swap_complete_data (DRI2DrawablePtr pPriv, DRI2SwapCompleteDataPtr pSwapData)
+{
+    buffer_unref(pPriv, pSwapData->src);
+    buffer_unref(pPriv, pSwapData->dest);
+    free(pSwapData);
+}
+
 void
 DRI2SwapComplete2(DRI2DrawablePtr pPriv, int frame,
 		   unsigned int tv_sec, unsigned int tv_usec, int type,
@@ -800,11 +825,6 @@ DRI2SwapComplete2(DRI2DrawablePtr pPriv, int frame,
     pPriv->swap_count++;
     pPriv->refcnt--;
 
-    if (pPriv->refcnt <= 0) {
-	DRI2DrawableGone(pPriv, 0);
-	return;
-    }
-
     if (pDraw) {
 	BoxRec          box;
 	RegionRec       region;
@@ -828,7 +848,10 @@ DRI2SwapComplete2(DRI2DrawablePtr pPriv, int frame,
 
     DRI2WakeClient(client, pPriv, frame, tv_sec, tv_usec);
 
-    free(pSwapData);
+    free_swap_complete_data(pPriv, pSwapData);
+
+    if (pPriv->refcnt <= 0)
+	DRI2DrawableGone(pPriv, 0);
 }
 
 Bool
@@ -883,6 +906,17 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
     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->client = client;
     pSwapData->data = data;
 
@@ -942,11 +976,11 @@ DRI2SwapBuffers(ClientPtr client, DRI2DrawablePtr pPriv, CARD64 target_msc,
     pPriv->swapsPending++;
     pPriv->refcnt++;
     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 */
 	pPriv->refcnt--;
-	free(pSwapData);
+	free_swap_complete_data(pPriv, pSwapData);
         xf86DrvMsg(pScreen->myNum, X_ERROR,
 		   "[DRI2] %s: driver failed to schedule swap\n", __func__);
 	return BadDrawable;
diff --git a/hw/xfree86/dri2/dri2.h b/hw/xfree86/dri2/dri2.h
index 4616dc5..c062a43 100644
--- a/hw/xfree86/dri2/dri2.h
+++ b/hw/xfree86/dri2/dri2.h
@@ -44,6 +44,7 @@ typedef struct {
     unsigned int flags;
     unsigned int format;
     void *driverPrivate;
+    unsigned int refcnt;
 } DRI2BufferRec, *DRI2BufferPtr;
 
 struct _DRI2Drawable;
-- 
1.7.0.4



More information about the xorg-devel mailing list