On Thu, 20 Feb 2014 11:43:05 +0100 Rui Matos <[email protected]> wrote:
> 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); > } > The basic algorithm looks fine to me now, FWIW. Thanks, pq _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
