From 408a93a1ec9766ff8c527d9ed2d61bcee565e1cb Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Sat, 23 Oct 2021 15:43:04 -0500 Subject: [PATCH] wayland: Use multi-thread event reading APIs Wayland provides the prepare_read()/read_events() family of APIs for reading from the display fd in a deadlock-free manner across multiple threads in a multi-threaded application. Let's use those instead of trying to roll our own solution using a mutex. This fixes an issue where a call to SDL_GL_SwapWindow() doesn't swap buffers if it happens to collide with SDL_PumpEvents() in the main thread. It also allows coexistence with other code or toolkits in our process that may want read and dispatch events themselves. --- src/video/wayland/SDL_waylandevents.c | 22 +++++++++---------- src/video/wayland/SDL_waylandopengles.c | 29 +++++++++++++------------ src/video/wayland/SDL_waylandsym.h | 4 ++++ src/video/wayland/SDL_waylandvideo.c | 3 --- src/video/wayland/SDL_waylandvideo.h | 1 - 5 files changed, 29 insertions(+), 30 deletions(-) diff --git a/src/video/wayland/SDL_waylandevents.c b/src/video/wayland/SDL_waylandevents.c index 11a4ebdbc..7a03039f2 100644 --- a/src/video/wayland/SDL_waylandevents.c +++ b/src/video/wayland/SDL_waylandevents.c @@ -231,20 +231,18 @@ Wayland_PumpEvents(_THIS) keyboard_repeat_handle(&input->keyboard_repeat, now); } - /* If we're trying to dispatch the display in another thread, - * we could trigger a race condition and end up blocking - * in wl_display_dispatch() */ - if (SDL_TryLockMutex(d->display_dispatch_lock) != 0) { - return; + /* wl_display_prepare_read() will return -1 if the default queue is not empty. + * If the default queue is empty, it will prepare us for our SDL_IOReady() call. */ + if (WAYLAND_wl_display_prepare_read(d->display) == 0) { + if (SDL_IOReady(WAYLAND_wl_display_get_fd(d->display), SDL_FALSE, 0) > 0) { + WAYLAND_wl_display_read_events(d->display); + } else { + WAYLAND_wl_display_cancel_read(d->display); + } } - if (SDL_IOReady(WAYLAND_wl_display_get_fd(d->display), SDL_FALSE, 0)) { - err = WAYLAND_wl_display_dispatch(d->display); - } else { - err = WAYLAND_wl_display_dispatch_pending(d->display); - } - - SDL_UnlockMutex(d->display_dispatch_lock); + /* Dispatch any pre-existing pending events or new events we may have read */ + err = WAYLAND_wl_display_dispatch_pending(d->display); if (err == -1 && !d->display_disconnected) { /* Something has failed with the Wayland connection -- for example, diff --git a/src/video/wayland/SDL_waylandopengles.c b/src/video/wayland/SDL_waylandopengles.c index c6d565910..934406f43 100644 --- a/src/video/wayland/SDL_waylandopengles.c +++ b/src/video/wayland/SDL_waylandopengles.c @@ -136,33 +136,34 @@ Wayland_GLES_SwapWindow(_THIS, SDL_Window *window) while (SDL_AtomicGet(&data->swap_interval_ready) == 0) { Uint32 now; - /* !!! FIXME: this is just the crucial piece of Wayland_PumpEvents */ WAYLAND_wl_display_flush(display); - if (WAYLAND_wl_display_dispatch_queue_pending(display, data->frame_event_queue) > 0) { - /* We dispatched some pending events. Check if the frame callback happened. */ + + /* wl_display_prepare_read_queue() will return -1 if the event queue is not empty. + * If the event queue is empty, it will prepare us for our SDL_IOReady() call. */ + if (WAYLAND_wl_display_prepare_read_queue(display, data->frame_event_queue) != 0) { + /* We have some pending events. Check if the frame callback happened. */ + WAYLAND_wl_display_dispatch_queue_pending(display, data->frame_event_queue); continue; } + /* Beyond this point, we must either call wl_display_cancel_read() or wl_display_read_events() */ + now = SDL_GetTicks(); if (SDL_TICKS_PASSED(now, max_wait)) { - /* Timeout expired */ + /* Timeout expired. Cancel the read. */ + WAYLAND_wl_display_cancel_read(display); break; } - /* Make sure we're not competing with SDL_PumpEvents() for any new - * events, or one of us may end up blocking in wl_display_dispatch */ - if (SDL_TryLockMutex(videodata->display_dispatch_lock) != 0) { - continue; - } - if (SDL_IOReady(WAYLAND_wl_display_get_fd(display), SDL_FALSE, max_wait - now) <= 0) { - /* Error or timeout expired without any events for us */ - SDL_UnlockMutex(videodata->display_dispatch_lock); + /* Error or timeout expired without any events for us. Cancel the read. */ + WAYLAND_wl_display_cancel_read(display); break; } - WAYLAND_wl_display_dispatch_queue(display, data->frame_event_queue); - SDL_UnlockMutex(videodata->display_dispatch_lock); + /* We have events. Read and dispatch them. */ + WAYLAND_wl_display_read_events(display); + WAYLAND_wl_display_dispatch_queue_pending(display, data->frame_event_queue); } SDL_AtomicSet(&data->swap_interval_ready, 0); } diff --git a/src/video/wayland/SDL_waylandsym.h b/src/video/wayland/SDL_waylandsym.h index 37c4b42dd..d6e6a761d 100644 --- a/src/video/wayland/SDL_waylandsym.h +++ b/src/video/wayland/SDL_waylandsym.h @@ -56,6 +56,10 @@ SDL_WAYLAND_SYM(int, wl_display_dispatch, (struct wl_display *)) SDL_WAYLAND_SYM(int, wl_display_dispatch_queue, (struct wl_display *, struct wl_event_queue *)) SDL_WAYLAND_SYM(int, wl_display_dispatch_queue_pending, (struct wl_display *, struct wl_event_queue *)) SDL_WAYLAND_SYM(int, wl_display_dispatch_pending, (struct wl_display *)) +SDL_WAYLAND_SYM(int, wl_display_prepare_read, (struct wl_display *)) +SDL_WAYLAND_SYM(int, wl_display_prepare_read_queue, (struct wl_display *, struct wl_event_queue *)) +SDL_WAYLAND_SYM(int, wl_display_read_events, (struct wl_display *)) +SDL_WAYLAND_SYM(void, wl_display_cancel_read, (struct wl_display *)) SDL_WAYLAND_SYM(int, wl_display_get_error, (struct wl_display *)) SDL_WAYLAND_SYM(int, wl_display_flush, (struct wl_display *)) SDL_WAYLAND_SYM(int, wl_display_roundtrip, (struct wl_display *)) diff --git a/src/video/wayland/SDL_waylandvideo.c b/src/video/wayland/SDL_waylandvideo.c index df915eea6..03464d193 100644 --- a/src/video/wayland/SDL_waylandvideo.c +++ b/src/video/wayland/SDL_waylandvideo.c @@ -169,7 +169,6 @@ Wayland_DeleteDevice(SDL_VideoDevice *device) WAYLAND_wl_display_flush(data->display); WAYLAND_wl_display_disconnect(data->display); } - SDL_DestroyMutex(data->display_dispatch_lock); SDL_free(data); SDL_free(device); SDL_WAYLAND_UnloadSymbols(); @@ -202,8 +201,6 @@ Wayland_CreateDevice(int devindex) data->display = display; - data->display_dispatch_lock = SDL_CreateMutex(); - /* Initialize all variables that we clean on shutdown */ device = SDL_calloc(1, sizeof(SDL_VideoDevice)); if (!device) { diff --git a/src/video/wayland/SDL_waylandvideo.h b/src/video/wayland/SDL_waylandvideo.h index 42da52e0e..2d8b6323a 100644 --- a/src/video/wayland/SDL_waylandvideo.h +++ b/src/video/wayland/SDL_waylandvideo.h @@ -86,7 +86,6 @@ typedef struct { char *classname; int relative_mouse_mode; - SDL_mutex *display_dispatch_lock; } SDL_VideoData; typedef struct {