[PATCH] xwayland: Destroy wl_buffers only after they are released

Axel Davy axel.davy at ens.fr
Thu Feb 20 05:18:06 PST 2014


Hi,

I've checked how it would behave when X closes properly,
and I think xwl_screen_close would need a new flush after the roundtrip:
. first flush: the server knows the wl_surface was destroyed
. dispatch: X gets the release
. missing flush: the server knows the wl_buffer has been destroyed.

Moreover I figured out that every windows and pixmaps should have been 
destroyed
before xwl_screen_close, making xwl_screen->window_list always empty,
so there is some useless code here.

I'm going to write a patch to do what you want on top of my tree with 
XWayland Present implementation, since
my XWayland Present patches are incompatible with yours.

Axel Davy

On 20/02/2014, Rui Matos 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 <ppaalanen at gmail.com> 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);
>       }
>   



More information about the wayland-devel mailing list