[cairo] [RFC PATCH] xlib-shm: Use XCB for SHM synchronization

Uli Schlachter psychon at znc.in
Sun Dec 1 04:30:41 PST 2013


The old code used XSendEvent() to make sure that LastKnownRequestProcessed()
gets updated eventually and tracked sequence numbers to implement an
asynchronous XSync() through this.

The problems with this is that XSetEventQueueOwner(dpy, XCBOwnsEventQueue)
changes semantics here and _XReadEvents() becomes a no-op. This broke the
implementation and would cause an endless loop in
_cairo_xlib_shm_surface_flush().

Additionally, the SHM event that is generated through XSendEvent() ended up in
the event queue and would need to be fetched by the main loop. Otherwise, the
event queue's size would grow and "some things" got slow.

The new implementation uses xcb_get_input_focus() and xcb_poll_for_reply() to
implement the asynchronous XSync(). This causes more syncs to be done, because
the old optimization of "only call XSendEvent() when actually needed" no longer
applies. Also, this causes ownership of the X11 socket to be passed back and
forth between XCB and Xlib.

TODO: Run the perf suite and check for regressions

TODO: Run the test suite and check if this actually works (so far I only
compile-tested this, sorry!)

TODO: Follow up patch which removes all the checks for broken servers and the
creation of the window that is used for sending events to.

Reference: http://lists.cairographics.org/archives/cairo/2013-November/024818.html
Signed-off-by: Uli Schlachter <psychon at znc.in>
---

I have this (and quite a lot more patches to make the Xlib backend use xcb)
laying around for a while already. I reordered and rewrote some things now to
extract this patch. As the commit message says, this needs more testing.

For now I would like to know your opinion about this. Is this a sensible
approach?

(Also, I just noticed that I left behind two TODOs in the code. I *think* I
handled those, but I am not totally sure about that right now)

Oh and: Of course, with this version of the patch configure.ac should only check
for xcb if SHM is actually enabled. But since I plan to add more code using XCB
into cairo-xlib, I would like to keep it the this way.

 configure.ac                 |   2 +-
 src/cairo-xlib-surface-shm.c | 177 ++++++++++++++++++++-----------------------
 src/cairo-xlib-surface.c     |  13 +++-
 3 files changed, 93 insertions(+), 99 deletions(-)

diff --git a/configure.ac b/configure.ac
index 959ae36..70bae0f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -77,7 +77,7 @@ AM_CONDITIONAL(CAIRO_HAS_DLSYM, test "x$have_dlsym" = "xyes")
 dnl ===========================================================================
 
 CAIRO_ENABLE_SURFACE_BACKEND(xlib, Xlib, auto, [
-  xlib_REQUIRES="x11 xext"
+  xlib_REQUIRES="x11 xext x11-xcb xcb >= 1.6"
   PKG_CHECK_MODULES(xlib, $xlib_REQUIRES, ,
 		    [xlib_REQUIRES=""
 		     AC_PATH_XTRA
diff --git a/src/cairo-xlib-surface-shm.c b/src/cairo-xlib-surface-shm.c
index 84b46d8..8e9ce6b 100644
--- a/src/cairo-xlib-surface-shm.c
+++ b/src/cairo-xlib-surface-shm.c
@@ -141,8 +141,10 @@ void _cairo_xlib_display_fini_shm (cairo_xlib_display_t *display) {}
 #include "cairo-list-inline.h"
 #include "cairo-mempool-private.h"
 
+#include <xcb/xcbext.h>
 #include <X11/Xlibint.h>
 #include <X11/Xproto.h>
+#include <X11/Xlib-xcb.h>
 #include <X11/extensions/XShm.h>
 #if HAVE_X11_EXTENSIONS_SHMPROTO_H
 #include <X11/extensions/shmproto.h>
@@ -170,7 +172,7 @@ struct _cairo_xlib_shm {
 };
 
 struct _cairo_xlib_shm_info {
-    unsigned long last_request;
+    xcb_get_input_focus_cookie_t last_request; /* TODO: Initialize */
     void *mem;
     size_t size;
     cairo_xlib_shm_t *pool;
@@ -182,7 +184,7 @@ struct _cairo_xlib_shm_surface {
     cairo_list_t link;
     cairo_xlib_shm_info_t *info;
     Pixmap pixmap;
-    unsigned long active;
+    xcb_get_input_focus_cookie_t active; /* TODO: Initialize */
     int idle;
 };
 
@@ -206,9 +208,6 @@ struct _cairo_xlib_shm_display {
     int event;
 
     Window window;
-    unsigned long last_request;
-    unsigned long last_event;
-
     cairo_list_t surfaces;
 
     cairo_list_t pool;
@@ -221,18 +220,6 @@ seqno_passed (unsigned long a, unsigned long b)
     return (long)(b - a) >= 0;
 }
 
-static inline cairo_bool_t
-seqno_before (unsigned long a, unsigned long b)
-{
-    return (long)(b - a) > 0;
-}
-
-static inline cairo_bool_t
-seqno_after (unsigned long a, unsigned long b)
-{
-    return (long)(a - b) > 0;
-}
-
 static inline cairo_status_t
 _pqueue_init (struct pqueue *pq)
 {
@@ -306,7 +293,7 @@ _pqueue_push (struct pqueue *pq, cairo_xlib_shm_info_t *info)
 
     for (i = ++pq->size;
 	 i != PQ_FIRST_ENTRY &&
-	 info->last_request < elements[parent = PQ_PARENT_INDEX (i)]->last_request;
+	 info->last_request.sequence < elements[parent = PQ_PARENT_INDEX (i)]->last_request.sequence;
 	 i = parent)
     {
 	elements[i] = elements[parent];
@@ -336,12 +323,12 @@ _pqueue_pop (struct pqueue *pq)
 	 i = child)
     {
 	if (child != pq->size &&
-	    elements[child+1]->last_request < elements[child]->last_request)
+	    elements[child+1]->last_request.sequence < elements[child]->last_request.sequence)
 	{
 	    child++;
 	}
 
-	if (elements[child]->last_request >= tail->last_request)
+	if (elements[child]->last_request.sequence >= tail->last_request.sequence)
 	    break;
 
 	elements[i] = elements[child];
@@ -410,12 +397,68 @@ peek_display (cairo_device_t *device)
     return ((cairo_xlib_display_t *)device)->display;
 }
 
+static inline xcb_connection_t *
+peek_connection (cairo_device_t *device)
+{
+    return XGetXCBConnection (peek_display (device));
+}
+
 static inline unsigned long
 peek_processed (cairo_device_t *device)
 {
     return LastKnownRequestProcessed (peek_display(device));
 }
 
+static inline cairo_bool_t
+sync_pending (xcb_get_input_focus_cookie_t cookie)
+{
+    return cookie.sequence != 0;
+}
+
+static inline void
+sync_discard (cairo_device_t *device, xcb_get_input_focus_cookie_t *cookie)
+{
+    if (!sync_pending (*cookie))
+       return;
+    xcb_discard_reply (peek_connection (device), cookie->sequence);
+    cookie->sequence = 0;
+}
+
+static inline void
+sync_start (cairo_device_t *device, xcb_get_input_focus_cookie_t *cookie)
+{
+    sync_discard (device, cookie);
+    *cookie = xcb_get_input_focus (peek_connection (device));
+}
+
+static inline cairo_bool_t
+sync_done (cairo_device_t *device, xcb_get_input_focus_cookie_t *cookie)
+{
+    xcb_get_input_focus_reply_t *reply;
+    int result;
+
+    if (! sync_pending (*cookie))
+       return TRUE;
+
+    result = xcb_poll_for_reply (peek_connection (device), cookie->sequence, (void **) &reply, NULL);
+    if (! result)
+       return FALSE;
+
+    free (reply);
+    cookie->sequence = 0;
+    return TRUE;
+}
+
+static inline void
+sync_wait (cairo_device_t *device, xcb_get_input_focus_cookie_t *cookie)
+{
+    if (!sync_pending (*cookie))
+       return;
+
+    free (xcb_wait_for_reply (peek_connection (device), cookie->sequence, NULL));
+    cookie->sequence = 0;
+}
+
 static void
 _cairo_xlib_display_shm_pool_destroy (cairo_xlib_display_t *display,
 				      cairo_xlib_shm_t *pool)
@@ -430,29 +473,6 @@ _cairo_xlib_display_shm_pool_destroy (cairo_xlib_display_t *display,
     free (pool);
 }
 
-static void send_event(cairo_xlib_display_t *display,
-		       cairo_xlib_shm_info_t *info,
-		       unsigned long seqno)
-{
-    XShmCompletionEvent ev;
-
-    if (! seqno_after (seqno, display->shm->last_event))
-	return;
-
-    ev.type = display->shm->event;
-    ev.send_event = 1; /* XXX or lie? */
-    ev.serial = NextRequest (display->display);
-    ev.drawable = display->shm->window;
-    ev.major_code = display->shm->opcode;
-    ev.minor_code = X_ShmPutImage;
-    ev.shmseg = info->pool->shm.shmid;
-    ev.offset = (char *)info->mem - (char *)info->pool->shm.shmaddr;
-
-    XSendEvent (display->display, ev.drawable, False, 0, (XEvent *)&ev);
-
-    display->shm->last_event = ev.serial;
-}
-
 static void sync (cairo_xlib_display_t *display)
 {
     cairo_xlib_shm_info_t *info;
@@ -471,22 +491,15 @@ static void
 _cairo_xlib_shm_info_cleanup (cairo_xlib_display_t *display)
 {
     cairo_xlib_shm_info_t *info;
-    Display *dpy = display->display;
     struct pqueue *pq = &display->shm->info;
-    unsigned long processed;
 
     if (PQ_TOP(pq) == NULL)
 	return;
 
-    XEventsQueued (dpy, QueuedAfterReading);
-    processed = LastKnownRequestProcessed (dpy);
-
     info = PQ_TOP(pq);
     do {
-	if (! seqno_passed (info->last_request, processed)) {
-	    send_event (display, info, display->shm->last_request);
+	if (! sync_done (&display->base, &info->last_request))
 	    return;
-	}
 
 	_cairo_mempool_free (&info->pool->mem, info->mem);
 	_pqueue_pop (&display->shm->info);
@@ -496,11 +509,13 @@ _cairo_xlib_shm_info_cleanup (cairo_xlib_display_t *display)
 
 static cairo_xlib_shm_t *
 _cairo_xlib_shm_info_find (cairo_xlib_display_t *display, size_t size,
-			   void **ptr, unsigned long *last_request)
+			   void **ptr, xcb_get_input_focus_cookie_t *last_request)
 {
     cairo_xlib_shm_info_t *info;
     struct pqueue *pq = &display->shm->info;
 
+    last_request->sequence = 0;
+
     if (PQ_TOP(pq) == NULL)
 	return NULL;
 
@@ -508,6 +523,7 @@ _cairo_xlib_shm_info_find (cairo_xlib_display_t *display, size_t size,
     do {
 	cairo_xlib_shm_t *pool = info->pool;
 
+	sync_discard (&display->base, last_request);
 	*last_request = info->last_request;
 
 	_pqueue_pop (&display->shm->info);
@@ -634,7 +650,7 @@ _cairo_xlib_shm_info_create (cairo_xlib_display_t *display,
 {
     cairo_xlib_shm_info_t *info;
     cairo_xlib_shm_t *pool;
-    unsigned long last_request = 0;
+    xcb_get_input_focus_cookie_t last_request = { 0 };
     void *mem = NULL;
 
     _cairo_xlib_shm_info_cleanup (display);
@@ -669,45 +685,28 @@ _cairo_xlib_shm_surface_flush (void *abstract_surface, unsigned flags)
 {
     cairo_xlib_shm_surface_t *shm = abstract_surface;
     cairo_xlib_display_t *display;
-    Display *dpy;
     cairo_status_t status;
 
-    if (shm->active == 0)
+    if (! sync_pending (shm->active))
 	return CAIRO_STATUS_SUCCESS;
 
     if (shm->image.base._finishing)
 	return CAIRO_STATUS_SUCCESS;
 
-    if (seqno_passed (shm->active, peek_processed (shm->image.base.device))) {
-	shm->active = 0;
-	return CAIRO_STATUS_SUCCESS;
-    }
-
     status = _cairo_xlib_display_acquire (shm->image.base.device, &display);
     if (unlikely (status))
 	return status;
 
-    send_event (display, shm->info, shm->active);
-
-    dpy = display->display;
-    XEventsQueued (dpy, QueuedAfterReading);
-    while (! seqno_passed (shm->active, LastKnownRequestProcessed (dpy))) {
-	LockDisplay(dpy);
-	_XReadEvents(dpy);
-	UnlockDisplay(dpy);
-    }
-
+    sync_wait (shm->image.base.device, &shm->active);
     cairo_device_release (&display->base);
-    shm->active = 0;
 
     return CAIRO_STATUS_SUCCESS;
 }
 
 static inline cairo_bool_t
-active (cairo_xlib_shm_surface_t *shm, Display *dpy)
+active (cairo_xlib_shm_surface_t *shm, cairo_device_t *device)
 {
-    return (shm->active &&
-	    ! seqno_passed (shm->active, LastKnownRequestProcessed (dpy)));
+    return ! sync_done (device, &shm->active);
 }
 
 static cairo_status_t
@@ -729,11 +728,10 @@ _cairo_xlib_shm_surface_finish (void *abstract_surface)
     if (shm->pixmap)
 	XFreePixmap (display->display, shm->pixmap);
 
-    if (active (shm, display->display)) {
+    if (active (shm, &display->base)) {
 	shm->info->last_request = shm->active;
+	shm->active.sequence = 0;
 	_pqueue_push (&display->shm->info, shm->info);
-	if (seqno_before (display->shm->last_request, shm->active))
-	    display->shm->last_request = shm->active;
     } else {
 	_cairo_mempool_free (&shm->info->pool->mem, shm->info->mem);
 	free (shm->info);
@@ -848,9 +846,10 @@ _cairo_xlib_shm_surface_create (cairo_xlib_surface_t *other,
 					shm->image.depth);
     }
     shm->active = shm->info->last_request;
+    shm->info->last_request.sequence = 0;
     shm->idle = -5;
 
-    assert (shm->active == 0 || will_sync);
+    assert (! sync_pending (shm->active) || will_sync);
 
     cairo_list_add (&shm->link, &display->shm->surfaces);
 
@@ -950,7 +949,7 @@ _cairo_xlib_surface_update_shm (cairo_xlib_surface_t *surface)
     }
 
     sync (display);
-    shm->active = 0;
+    sync_discard (&display->base, &shm->active);
     shm->idle--;
 
     _cairo_xlib_surface_put_gc (display, surface, gc);
@@ -965,7 +964,7 @@ _cairo_xlib_surface_clear_shm (cairo_xlib_surface_t *surface)
 {
     cairo_xlib_shm_surface_t *shm = (cairo_xlib_shm_surface_t *)surface->shm;
 
-    assert (shm->active == 0);
+    assert (! sync_pending (shm->active));
 
     _cairo_damage_destroy (surface->base.damage);
     surface->base.damage = _cairo_damage_create();
@@ -1183,7 +1182,7 @@ _cairo_xlib_surface_create_similar_shm (void *other,
     if (surface) {
 	if (! surface->is_clear) {
 	    cairo_xlib_shm_surface_t *shm = (cairo_xlib_shm_surface_t *) surface;
-	    assert (shm->active == 0);
+	    assert (! sync_pending (shm->active));
 	    memset (shm->image.data, 0, shm->image.stride * shm->image.height);
 	    shm->image.base.is_clear = TRUE;
 	}
@@ -1199,7 +1198,7 @@ _cairo_xlib_shm_surface_mark_active (cairo_surface_t *_shm)
     cairo_xlib_shm_surface_t *shm = (cairo_xlib_shm_surface_t *) _shm;
     cairo_xlib_display_t *display = (cairo_xlib_display_t *) _shm->device;
 
-    shm->active = NextRequest (display->display);
+    sync_start (&display->base, &shm->active);
 }
 
 void
@@ -1238,10 +1237,8 @@ _cairo_xlib_shm_surface_get_ximage (cairo_surface_t *surface,
 void *
 _cairo_xlib_shm_surface_get_obdata (cairo_surface_t *surface)
 {
-    cairo_xlib_display_t *display = (cairo_xlib_display_t *) surface->device;
     cairo_xlib_shm_surface_t *shm = (cairo_xlib_shm_surface_t *) surface;
 
-    display->shm->last_event = shm->active = NextRequest (display->display);
     return &shm->info->pool->shm;
 }
 
@@ -1274,15 +1271,7 @@ _cairo_xlib_shm_surface_is_active (cairo_surface_t *surface)
     cairo_xlib_shm_surface_t *shm;
 
     shm = (cairo_xlib_shm_surface_t *) surface;
-    if (shm->active == 0)
-	return FALSE;
-
-    if (seqno_passed (shm->active, peek_processed (shm->image.base.device))) {
-	shm->active = 0;
-	return FALSE;
-    }
-
-    return TRUE;
+    return !sync_done (shm->image.base.device, &shm->active);
 }
 
 cairo_bool_t
@@ -1413,8 +1402,6 @@ _cairo_xlib_display_init_shm (cairo_xlib_display_t *display)
 				 InputOutput,
 				 DefaultVisual (display->display, scr),
 				 CWOverrideRedirect, &attr);
-    shm->last_event = 0;
-    shm->last_request = 0;
 
     if (xorg_has_buggy_send_shm_completion_event(display, shm))
 	has_pixmap = 0;
diff --git a/src/cairo-xlib-surface.c b/src/cairo-xlib-surface.c
index 029a542..e46f1ce 100644
--- a/src/cairo-xlib-surface.c
+++ b/src/cairo-xlib-surface.c
@@ -1092,6 +1092,7 @@ _cairo_xlib_surface_draw_image (cairo_xlib_surface_t   *surface,
     cairo_format_masks_t image_masks;
     int native_byte_order = _cairo_is_little_endian () ? LSBFirst : MSBFirst;
     cairo_surface_t *shm_image = NULL;
+    cairo_surface_t *mark_active = NULL;
     pixman_image_t *pixman_image = NULL;
     cairo_status_t status;
     cairo_bool_t own_data = FALSE;
@@ -1152,6 +1153,7 @@ _cairo_xlib_surface_draw_image (cairo_xlib_surface_t   *surface,
 					      0, 0,
 					      0, 0,
 					      width, height);
+		    mark_active = shm_image;
 		    ximage.obdata = _cairo_xlib_shm_surface_get_obdata (shm_image);
 		    ximage.data = (char *)clone->data;
 		    ximage.bytes_per_line = clone->stride;
@@ -1160,8 +1162,10 @@ _cairo_xlib_surface_draw_image (cairo_xlib_surface_t   *surface,
 		    src_x = src_y = 0;
 		}
 	    }
-	} else
+	} else {
+	    mark_active = &image->base;
 	    ximage.obdata = _cairo_xlib_shm_surface_get_obdata (&image->base);
+	}
 
 	ret = XInitImage (&ximage);
 	assert (ret != 0);
@@ -1197,6 +1201,7 @@ _cairo_xlib_surface_draw_image (cairo_xlib_surface_t   *surface,
 	    ximage.data = (char *) clone->data;
 	    ximage.obdata = _cairo_xlib_shm_surface_get_obdata (&clone->base);
 	    ximage.bytes_per_line = clone->stride;
+	    mark_active = &clone->base;
 	} else {
 	    pixman_image = pixman_image_create_bits (intermediate_format,
 						     width, height, NULL, 0);
@@ -1344,10 +1349,12 @@ _cairo_xlib_surface_draw_image (cairo_xlib_surface_t   *surface,
     if (unlikely (status))
 	goto BAIL;
 
-    if (ximage.obdata)
+    if (ximage.obdata) {
+	assert (mark_active != NULL);
 	XShmPutImage (display->display, surface->drawable, gc, &ximage,
 		      src_x, src_y, dst_x, dst_y, width, height, True);
-    else
+	_cairo_xlib_shm_surface_mark_active (mark_active);
+    } else
 	XPutImage (display->display, surface->drawable, gc, &ximage,
 		   src_x, src_y, dst_x, dst_y, width, height);
 
-- 
1.8.4.3

-- 
99 little bugs in the code
99 little bugs in the code
Take one down, patch it around
117 little bugs in the code
  -- @irqed


More information about the cairo mailing list