From 9f0e9f36be99461ed9076f5e0c9fcc33251d6b7d Mon Sep 17 00:00:00 2001 From: Campbell Barton Date: Wed, 16 Nov 2022 10:09:39 +1100 Subject: [PATCH] GHOST/Wayland: extend on comments regarding locking --- intern/ghost/intern/GHOST_WindowWayland.cpp | 8 ++++--- intern/ghost/intern/GHOST_WindowWayland.h | 23 +++++++++++++++++++-- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/intern/ghost/intern/GHOST_WindowWayland.cpp b/intern/ghost/intern/GHOST_WindowWayland.cpp index 909304577c3..9a916f9bd1b 100644 --- a/intern/ghost/intern/GHOST_WindowWayland.cpp +++ b/intern/ghost/intern/GHOST_WindowWayland.cpp @@ -115,7 +115,10 @@ struct GWL_Window { #endif WGL_XDG_Decor_Window *xdg_decor = nullptr; - /** The current value of frame, copied from `frame_pending` when applying updates. */ + /** + * The current value of frame, copied from `frame_pending` when applying updates. + * This avoids the need for locking when reading from `frame`. + */ GWL_WindowFrame frame; GWL_WindowFrame frame_pending; @@ -543,14 +546,13 @@ static void frame_handle_configure(struct libdecor_frame *frame, win->libdecor->configured = true; } + /* Apply the changes. */ { GWL_Window *win = static_cast(data); # ifdef USE_EVENT_BACKGROUND_THREAD GHOST_SystemWayland *system = win->ghost_system; const bool is_main_thread = system->main_thread_id == std::this_thread::get_id(); if (!is_main_thread) { - /* NOTE(@campbellbarton): this only gets one redraw, - * I could not find a case where this causes problems. */ gwl_window_pending_actions_tag(win, PENDING_FRAME_CONFIGURE); } else diff --git a/intern/ghost/intern/GHOST_WindowWayland.h b/intern/ghost/intern/GHOST_WindowWayland.h index e530d29f6d6..c43f7ca008c 100644 --- a/intern/ghost/intern/GHOST_WindowWayland.h +++ b/intern/ghost/intern/GHOST_WindowWayland.h @@ -23,13 +23,32 @@ * * Solve this using a thread that handles events, locking must be performed as follows: * - * - Lock #GWL_Display.server_mutex to prevent wl_display_dispatch / wl_display_roundtrip + * - Lock #GWL_Display.server_mutex to prevent #wl_display_dispatch / #wl_display_roundtrip * running from multiple threads at once. - * GHOST functions that communicate with WAYLAND must use this lock to to be thread safe. + * + * Even though WAYLAND functions that communicate with `wl_display_*` have their own locks, + * GHOST functions that communicate with WAYLAND must use this lock to be thread safe. + * + * Without this reading/writing values such as changing the cursor or setting the window size + * could conflict with WAYLAND callbacks running in a separate thread. + * So the `server_mutex` ensures either GHOST or WAYLAND is manipulating this data, + * having two WAYLAND callbacks accessing the data at once isn't a problem. + * + * \warning Some operations such as #ContextEGL creation/deletion & swap-buffers may call + * #wl_display_dispatch indirectly, so it's important to take care to lock the `server_mutex`, + * before accessing these functions too. + * + * \warning An unfortunate side-effect of this is care needs to be taken not to call public + * GHOST functions from each other in situations that would otherwise be supported. + * As both using a `server_mutex` results in a dead-lock. In some cases this means the + * implementation and the public function may need to be split. * * - Lock #GWL_Display.timer_mutex when WAYLAND callbacks manipulate timers. * * - Lock #GWL_Display.events_pending_mutex before manipulating #GWL_Display.events_pending. + * + * - Lock #GWL_Window.frame_pending_mutex before changing window size & frame settings, + * this is flushed in #GHOST_WindowWayland::pending_actions_handle. */ #define USE_EVENT_BACKGROUND_THREAD