[Mesa-dev] [PATCH v2] egl/wayland: Avoid race conditions when on non-main thread

Emil Velikov emil.l.velikov at gmail.com
Sat Oct 15 00:49:13 UTC 2016


[Cc-ing Daniel/Pekka]

Hi all,

On 24 June 2016 at 03:46, Jonas Ådahl <jadahl at gmail.com> wrote:
> When EGL is used on some other thread than the thread that drives the
> main wl_display queue, the Wayland EGL dri2 implementation is
> vulnerable to a race condition related to display round trips and global
> object advertisements.
>
> The race that may happen is that after after a proxy is created, but
> before the queue is set, events meant to be emitted via the yet to be
> set queue may already have been queued on the wrong queue.
>
> In order to make it possible to avoid this race, wayland 1.11
> introduced new API that allows creating a proxy wrapper that may be used
> as the factory proxy when creating new proxies via Wayland requests. The
> queue of a proxy wrapper can be changed without effecting what queue
> events emitted by the actual proxy will be queued on, while still
> effecting what default queue proxies created from it will have.
>
> By introducing a wl_display proxy wrapper and using this when performing
> round trips (via wl_display_sync()) and retrieving the global objects (via
> wl_display_get_registry()), the mentioned race condition is avoided.
>
> Signed-off-by: Jonas Ådahl <jadahl at gmail.com>
> ---
>
> Changes since v1:
>
>  - Added proxy clean up on tear down
>  - Changed required version to the stable wayland release (1.11)
>
>
>  configure.ac                            |  2 +-
>  src/egl/drivers/dri2/egl_dri2.c         |  1 +
>  src/egl/drivers/dri2/egl_dri2.h         |  1 +
>  src/egl/drivers/dri2/platform_wayland.c | 30 ++++++++++++++++--------------
>  4 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 023110e..d73af83 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -83,7 +83,7 @@ GLPROTO_REQUIRED=1.4.14
>  LIBOMXIL_BELLAGIO_REQUIRED=0.0
>  LIBVA_REQUIRED=0.38.0
>  VDPAU_REQUIRED=1.1
> -WAYLAND_REQUIRED=1.2.0
> +WAYLAND_REQUIRED=1.11
>  XCB_REQUIRED=1.9.3
>  XCBDRI2_REQUIRED=1.8
>  XCBGLX_REQUIRED=1.8.1
> diff --git a/src/egl/drivers/dri2/egl_dri2.c b/src/egl/drivers/dri2/egl_dri2.c
> index d8448f4..88191b4 100644
> --- a/src/egl/drivers/dri2/egl_dri2.c
> +++ b/src/egl/drivers/dri2/egl_dri2.c
> @@ -849,6 +849,7 @@ dri2_terminate(_EGLDriver *drv, _EGLDisplay *disp)
>            wl_shm_destroy(dri2_dpy->wl_shm);
>        wl_registry_destroy(dri2_dpy->wl_registry);
>        wl_event_queue_destroy(dri2_dpy->wl_queue);
> +      wl_proxy_wrapper_destroy(dri2_dpy->wl_dpy_wrapper);
>        if (dri2_dpy->own_device) {
>           wl_display_disconnect(dri2_dpy->wl_dpy);
>        }
> diff --git a/src/egl/drivers/dri2/egl_dri2.h b/src/egl/drivers/dri2/egl_dri2.h
> index ddb5f39..075c2a4 100644
> --- a/src/egl/drivers/dri2/egl_dri2.h
> +++ b/src/egl/drivers/dri2/egl_dri2.h
> @@ -205,6 +205,7 @@ struct dri2_egl_display
>
>  #ifdef HAVE_WAYLAND_PLATFORM
>     struct wl_display        *wl_dpy;
> +   struct wl_display        *wl_dpy_wrapper;
>     struct wl_registry       *wl_registry;
>     struct wl_drm            *wl_server_drm;
>     struct wl_drm            *wl_drm;
> diff --git a/src/egl/drivers/dri2/platform_wayland.c b/src/egl/drivers/dri2/platform_wayland.c
> index ff0d5c8..519469e 100644
> --- a/src/egl/drivers/dri2/platform_wayland.c
> +++ b/src/egl/drivers/dri2/platform_wayland.c
> @@ -74,9 +74,8 @@ roundtrip(struct dri2_egl_display *dri2_dpy)
>     struct wl_callback *callback;
>     int done = 0, ret = 0;
>
> -   callback = wl_display_sync(dri2_dpy->wl_dpy);
> +   callback = wl_display_sync(dri2_dpy->wl_dpy_wrapper);
>     wl_callback_add_listener(callback, &sync_listener, &done);
> -   wl_proxy_set_queue((struct wl_proxy *) callback, dri2_dpy->wl_queue);
>     while (ret != -1 && !done)
>        ret = wl_display_dispatch_queue(dri2_dpy->wl_dpy, dri2_dpy->wl_queue);
>
> @@ -765,11 +764,9 @@ dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
>      * handle the commit and send a release event before checking for a free
>      * buffer */
>     if (dri2_surf->throttle_callback == NULL) {
> -      dri2_surf->throttle_callback = wl_display_sync(dri2_dpy->wl_dpy);
> +      dri2_surf->throttle_callback = wl_display_sync(dri2_dpy->wl_dpy_wrapper);
>        wl_callback_add_listener(dri2_surf->throttle_callback,
>                                 &throttle_listener, dri2_surf);
> -      wl_proxy_set_queue((struct wl_proxy *) dri2_surf->throttle_callback,
> -                         dri2_dpy->wl_queue);
>     }
>
>     wl_display_flush(dri2_dpy->wl_dpy);
> @@ -1093,12 +1090,17 @@ dri2_initialize_wayland_drm(_EGLDriver *drv, _EGLDisplay *disp)
>
>     dri2_dpy->wl_queue = wl_display_create_queue(dri2_dpy->wl_dpy);
>
> +   dri2_dpy->wl_dpy_wrapper = wl_proxy_create_wrapper(dri2_dpy->wl_dpy);
> +   if (dri2_dpy->wl_dpy_wrapper == NULL)
> +      goto cleanup_dpy_wrapper;
> +
> +   wl_proxy_set_queue((struct wl_proxy *) dri2_dpy->wl_dpy_wrapper,
> +                      dri2_dpy->wl_queue);
> +
>     if (dri2_dpy->own_device)
>        wl_display_dispatch_pending(dri2_dpy->wl_dpy);
>
> -   dri2_dpy->wl_registry = wl_display_get_registry(dri2_dpy->wl_dpy);
> -   wl_proxy_set_queue((struct wl_proxy *) dri2_dpy->wl_registry,
> -                      dri2_dpy->wl_queue);
> +   dri2_dpy->wl_registry = wl_display_get_registry(dri2_dpy->wl_dpy_wrapper);
>     wl_registry_add_listener(dri2_dpy->wl_registry,
>                              &registry_listener_drm, dri2_dpy);
>     if (roundtrip(dri2_dpy) < 0 || dri2_dpy->wl_drm == NULL)
> @@ -1235,6 +1237,10 @@ dri2_initialize_wayland_drm(_EGLDriver *drv, _EGLDisplay *disp)
>     wl_drm_destroy(dri2_dpy->wl_drm);
>   cleanup_registry:
>     wl_registry_destroy(dri2_dpy->wl_registry);
> +   wl_proxy_wrapper_destroy(dri2_dpy->wl_dpy_wrapper);
> + cleanup_dpy_wrapper:
> +   if (dri2_dpy->wl_dpy != disp->PlatformDisplay)
> +      wl_display_disconnect(dri2_dpy->wl_dpy);
>     wl_event_queue_destroy(dri2_dpy->wl_queue);
>   cleanup_dpy:
>     free(dri2_dpy);
> @@ -1546,11 +1552,9 @@ dri2_wl_swrast_commit_backbuffer(struct dri2_egl_surface *dri2_surf)
>      * handle the commit and send a release event before checking for a free
>      * buffer */
>     if (dri2_surf->throttle_callback == NULL) {
> -      dri2_surf->throttle_callback = wl_display_sync(dri2_dpy->wl_dpy);
> +      dri2_surf->throttle_callback = wl_display_sync(dri2_dpy->wl_dpy_wrapper);
>        wl_callback_add_listener(dri2_surf->throttle_callback,
>                                 &throttle_listener, dri2_surf);
> -      wl_proxy_set_queue((struct wl_proxy *) dri2_surf->throttle_callback,
> -                         dri2_dpy->wl_queue);
>     }
>
>     wl_display_flush(dri2_dpy->wl_dpy);
> @@ -1824,9 +1828,7 @@ dri2_initialize_wayland_swrast(_EGLDriver *drv, _EGLDisplay *disp)
>     if (dri2_dpy->own_device)
>        wl_display_dispatch_pending(dri2_dpy->wl_dpy);
>
> -   dri2_dpy->wl_registry = wl_display_get_registry(dri2_dpy->wl_dpy);
> -   wl_proxy_set_queue((struct wl_proxy *) dri2_dpy->wl_registry,
> -                      dri2_dpy->wl_queue);
> +   dri2_dpy->wl_registry = wl_display_get_registry(dri2_dpy->wl_dpy_wrapper);
>     wl_registry_add_listener(dri2_dpy->wl_registry,
>                              &registry_listener_swrast, dri2_dpy);
>

Can we have someone with more wayland experience than me look into the
above patch ?

Thanks
Emil


More information about the mesa-dev mailing list