dri2: Reference count DRI2 buffers

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.

This parallels the approach taken by the Intel DDX in commit
0d2392d44aae95d6b571d98f7ec323cf672a687f.

Fixes: http://bugs.freedesktop.org/show_bug.cgi?id=29065

v2: Don't write completion events to the client if it has quit.
v3: Don't try to unref the NULL buffers from a DRI2_WAITMSC event.
    Take a ref in schedule_swap earlier, so the offscreen fallback
    doesn't incorrectly destroy the buffers.

Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>

1 files changed, 69 insertions, 7 deletions

diff --git a/src/radeon_dri2.c b/src/radeon_dri2.c
index 4ded9dcb..ed7fdd68 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;
 };
 
 
@@ -244,6 +245,7 @@ radeon_dri2_create_buffer(DrawablePtr drawable,
     buffers->flags = 0; /* not tiled */
     privates->pixmap = pixmap;
     privates->attachment = attachment;
+    privates->refcnt = 1;
 
     return buffers;
 }
@@ -275,13 +277,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);
+        }
     }
 }
 #endif
@@ -361,6 +376,7 @@ enum DRI2FrameEventType {
 typedef struct _DRI2FrameEvent {
     XID drawable_id;
     ClientPtr client;
+    int client_index;
     enum DRI2FrameEventType type;
     int frame;
 
@@ -371,11 +387,28 @@ typedef struct _DRI2FrameEvent {
     DRI2BufferPtr back;
 } DRI2FrameEventRec, *DRI2FrameEventPtr;
 
+static void
+radeon_dri2_ref_buffer(BufferPtr buffer)
+{
+    struct dri2_buffer_priv *private = buffer->driverPrivate;
+    private->refcnt++;
+}
+
+static void
+radeon_dri2_unref_buffer(BufferPtr buffer)
+{
+    if (buffer) {
+        struct dri2_buffer_priv *private = buffer->driverPrivate;
+        radeon_dri2_destroy_buffer(&(private->pixmap->drawable), buffer);
+    }
+}
+
 void radeon_dri2_frame_event_handler(unsigned int frame, unsigned int tv_sec,
                                      unsigned int tv_usec, void *event_data)
 {
     DRI2FrameEventPtr event = event_data;
     DrawablePtr drawable;
+    ClientPtr client;
     ScreenPtr screen;
     ScrnInfoPtr scrn;
     int status;
@@ -386,6 +419,8 @@ void radeon_dri2_frame_event_handler(unsigned int frame, unsigned int tv_sec,
     status = dixLookupDrawable(&drawable, event->drawable_id, serverClient,
                                M_ANY, DixWriteAccess);
     if (status != Success) {
+        radeon_dri2_unref_buffer(event->front);
+        radeon_dri2_unref_buffer(event->back);
         free(event);
         return;
     }
@@ -393,6 +428,17 @@ void radeon_dri2_frame_event_handler(unsigned int frame, unsigned int tv_sec,
     screen = drawable->pScreen;
     scrn = xf86Screens[screen->myNum];
 
+    /* event->client may have quit between submitting a request
+     * and this callback being triggered.
+     *
+     * Check our saved client pointer against the client in the saved client
+     * slot.  This will catch almost all cases where the client that requested
+     * SwapBuffers has gone away, and will guarantee that there is at least a 
+     * valid client to write the BufferSwapComplete event to.
+     */
+    client = event->client == clients[event->client_index] ? 
+            event->client : NULL;
+
     switch (event->type) {
     case DRI2_FLIP:
     case DRI2_SWAP:
@@ -404,11 +450,14 @@ void radeon_dri2_frame_event_handler(unsigned int frame, unsigned int tv_sec,
         radeon_dri2_copy_region(drawable, &region, event->front, event->back);
         swap_type = DRI2_BLIT_COMPLETE;
 
-        DRI2SwapComplete(event->client, drawable, frame, tv_sec, tv_usec,
+        DRI2SwapComplete(client, drawable, frame, tv_sec, tv_usec,
                 swap_type, event->event_complete, event->event_data);
+
+        radeon_dri2_unref_buffer(event->front);
+        radeon_dri2_unref_buffer(event->back);
         break;
     case DRI2_WAITMSC:
-        DRI2WaitMSCComplete(event->client, drawable, frame, tv_sec, tv_usec);
+        DRI2WaitMSCComplete(client, drawable, frame, tv_sec, tv_usec);
         break;
     default:
         /* Unknown type */
@@ -511,6 +560,7 @@ static int radeon_dri2_schedule_wait_msc(ClientPtr client, DrawablePtr draw,
 
     wait_info->drawable_id = draw->id;
     wait_info->client = client;
+    wait_info->client_index = client->index;
     wait_info->type = DRI2_WAITMSC;
 
     /* Get current count */
@@ -641,12 +691,20 @@ static int radeon_dri2_schedule_swap(ClientPtr client, DrawablePtr draw,
 
     swap_info = calloc(1, sizeof(DRI2FrameEventRec));
 
+    /* radeon_dri2_frame_event_handler will get called some unknown time in the
+     * future with these buffers.  Take a reference to ensure that they won't
+     * get destroyed before then. 
+     */
+    radeon_dri2_ref_buffer(front);
+    radeon_dri2_ref_buffer(back);
+
     /* Drawable not displayed... just complete the swap */
     if (crtc == -1 || !swap_info)
         goto blit_fallback;
 
     swap_info->drawable_id = draw->id;
     swap_info->client = client;
+    swap_info->client_index = client->index;
     swap_info->event_complete = func;
     swap_info->event_data = data;
     swap_info->front = front;
@@ -775,6 +833,10 @@ blit_fallback:
     DRI2SwapComplete(client, draw, 0, 0, 0, DRI2_BLIT_COMPLETE, func, data);
     if (swap_info)
         free(swap_info);
+
+    radeon_dri2_unref_buffer(front);
+    radeon_dri2_unref_buffer(back);
+
     *target_msc = 0; /* offscreen, so zero out target vblank count */
     return TRUE;
 }