Destroying a wl_buffer that is still attached to a wl_surface is
undefined behavior according to the wayland protocol. We should delay
the destruction until we get the release event.
To achieve this we need to track ownership of wl_buffers, both our own
and the compositor's which occurs from either the first commit request
or the first commit request after a release event until the next
release event.
---
On 12 February 2014 08:54, Pekka Paalanen <[email protected]> wrote:
I assume the code never added a wl_buffer listener before, because if
it did, this patch would be a no-op. "wl_buffer_add_listener" is a
misnomer, there can only ever be one listener, and trying to "add"
another will not actually do anything.
Thanks for the headsup. But, indeed, there was nothing adding a
wl_buffer listener before.
Also, you rely on wl_buffer.release not having arrived before you add
the listener. With weston's gl-renderer, the release comes very soon
after each wl_surface.commit for wl_shm buffers. Maybe it works, maybe
it doesn't, but it seems very fragile. Did you check you don't leak
wl_buffers now?
Right, I was leaking in some cases. So, I came up with the solution
below which, if I'm reading the logs correctly, doesn't leak and works
correctly.
This solution abuses the user_data field to essentially keep two
toggle references, our own and the compositor's, and only destroys the
wl_buffer when both are dropped.
Thanks,
Rui
---
hw/xfree86/xwayland/xwayland-cursor.c | 4 ++-
hw/xfree86/xwayland/xwayland-private.h | 4 +++
hw/xfree86/xwayland/xwayland-window.c | 8 +++---
hw/xfree86/xwayland/xwayland.c | 50 ++++++++++++++++++++++++++++++++++
4 files changed, 61 insertions(+), 5 deletions(-)
diff --git a/hw/xfree86/xwayland/xwayland-cursor.c
b/hw/xfree86/xwayland/xwayland-cursor.c
index 2b3cb5e..232b038 100644
--- a/hw/xfree86/xwayland/xwayland-cursor.c
+++ b/hw/xfree86/xwayland/xwayland-cursor.c
@@ -125,6 +125,7 @@ xwl_realize_cursor(DeviceIntPtr device, ScreenPtr screen,
CursorPtr cursor)
cursor->bits->width, cursor->bits->height,
cursor->bits->width * 4,
WL_SHM_FORMAT_ARGB8888);
+ _buffer_init(buffer);
wl_shm_pool_destroy(pool);
dixSetPrivate(&cursor->devPrivates,
@@ -143,7 +144,7 @@ xwl_unrealize_cursor(DeviceIntPtr device,
xwl_screen = xwl_screen_get(screen);
buffer = dixGetPrivate(&cursor->devPrivates,
&xwl_screen->cursor_private_key);
- wl_buffer_destroy(buffer);
+ _buffer_disown(buffer);
return TRUE;
}
@@ -176,6 +177,7 @@ xwl_seat_set_cursor(struct xwl_seat *xwl_seat)
xwl_seat->x_cursor->bits->width,
xwl_seat->x_cursor->bits->height);
wl_surface_commit(xwl_seat->cursor);
+ _buffer_commited(buffer);
}
static void
diff --git a/hw/xfree86/xwayland/xwayland-private.h
b/hw/xfree86/xwayland/xwayland-private.h
index bdecf8a..41e7e13 100644
--- a/hw/xfree86/xwayland/xwayland-private.h
+++ b/hw/xfree86/xwayland/xwayland-private.h
@@ -137,4 +137,8 @@ void xwl_output_remove(struct xwl_output *output);
extern const struct xserver_listener xwl_server_listener;
+void _buffer_commited(struct wl_buffer *buffer);
+void _buffer_disown(struct wl_buffer *buffer);
+void _buffer_init(struct wl_buffer *buffer);
+
#endif /* _XWAYLAND_PRIVATE_H_ */
diff --git a/hw/xfree86/xwayland/xwayland-window.c
b/hw/xfree86/xwayland/xwayland-window.c
index a2a8206..d18c7f6 100644
--- a/hw/xfree86/xwayland/xwayland-window.c
+++ b/hw/xfree86/xwayland/xwayland-window.c
@@ -62,10 +62,8 @@ xwl_window_attach(struct xwl_window *xwl_window, PixmapPtr
pixmap)
struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
struct wl_callback *callback;
- /* We can safely destroy the buffer because we only use one buffer
- * per surface in xwayland model */
if (xwl_window->buffer)
- wl_buffer_destroy(xwl_window->buffer);
+ _buffer_disown(xwl_window->buffer);
xwl_screen->driver->create_window_buffer(xwl_window, pixmap);
@@ -74,6 +72,8 @@ xwl_window_attach(struct xwl_window *xwl_window, PixmapPtr pixmap)
return;
}
+ _buffer_init(xwl_window->buffer);
+
wl_surface_attach(xwl_window->surface, xwl_window->buffer, 0, 0);
wl_surface_damage(xwl_window->surface, 0, 0,
pixmap->drawable.width,
@@ -185,7 +185,7 @@ xwl_unrealize_window(WindowPtr window)
return ret;
if (xwl_window->buffer)
- wl_buffer_destroy(xwl_window->buffer);
+ _buffer_disown(xwl_window->buffer);
wl_surface_destroy(xwl_window->surface);
xorg_list_del(&xwl_window->link);
if (RegionNotEmpty(DamageRegion(xwl_window->damage)))
diff --git a/hw/xfree86/xwayland/xwayland.c b/hw/xfree86/xwayland/xwayland.c
index c70a52d..c373fcc 100644
--- a/hw/xfree86/xwayland/xwayland.c
+++ b/hw/xfree86/xwayland/xwayland.c
@@ -72,6 +72,55 @@ const struct xserver_listener xwl_server_listener = {
};
static void
+_buffer_add_bits(struct wl_buffer *buffer, unsigned long bits)
+{
+ unsigned long ref = (unsigned long) wl_buffer_get_user_data(buffer);
+ ref |= bits;
+ wl_buffer_set_user_data(buffer, (void *) ref);
+}
+
+static void
+_buffer_clear_bits(struct wl_buffer *buffer, unsigned long bits)
+{
+ unsigned long ref = (unsigned long) wl_buffer_get_user_data(buffer);
+ ref &= ~bits;
+ if (!ref)
+ wl_buffer_destroy(buffer);
+ else
+ wl_buffer_set_user_data(buffer, (void *) ref);
+}
+
+static void
+_buffer_release_handler(void *data, struct wl_buffer *buffer)
+{
+ _buffer_clear_bits(buffer, 1 << 1);
+}
+
+static const struct wl_buffer_listener _buffer_listener = {
+ _buffer_release_handler,
+};
+
+void
+_buffer_commited(struct wl_buffer *buffer)
+{
+ _buffer_add_bits(buffer, 1 << 1);
+}
+
+void
+_buffer_disown(struct wl_buffer *buffer)
+{
+ _buffer_clear_bits(buffer, 1 << 0);
+}
+
+void
+_buffer_init(struct wl_buffer *buffer)
+{
+ /* setup the release listener and add the "owned" reference */
+ wl_buffer_add_listener(buffer, &_buffer_listener, (void *) (1 << 0));
+}
+
+
+static void
xwl_input_delayed_init(void *data, struct wl_callback *callback, uint32_t
time)
{
struct xwl_screen *xwl_screen = data;
@@ -350,6 +399,7 @@ void xwl_screen_post_damage(struct xwl_screen *xwl_screen)
xwl_window->buffer,
0, 0);
wl_surface_commit(xwl_window->surface);
+ _buffer_commited(xwl_window->buffer);
DamageEmpty(xwl_window->damage);
}
