From f27471fbd806c9e2ee48f0e49f9d4f8f4e6af0a8 Mon Sep 17 00:00:00 2001 From: Riteo Date: Thu, 27 Jun 2024 23:28:29 +0200 Subject: [PATCH] Wayland: minimize surface commits and limit them to the main thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before of this patch, as explained in the usual commented-wall-of-text-longer-than-the-actual-patch-itself™, due to the multithreaded nature of the Wayland thread, it was possible to commit a surface while the renderer was doing stuff, which was _very_ wrong. Initially the consequences of such a sin weren't obvious but, now that explicit synchronization is becoming more and more common, we can't commit a buffer randomly without basically guaranteeing a nasty, nasty crash (and we should have avoided commits altogether in the first place to ensure atomic surface updates). We now only trigger a commit _in the main thread_ when low processor usage mode is on _and_ if we know that we won't be rendering anything as, due to its intermittent nature, it makes "legacy" (pre xdg_wm_base v6) frame callback based suspension quite annoying. --- .../wayland/display_server_wayland.cpp | 66 +++++++++++++------ .../linuxbsd/wayland/display_server_wayland.h | 2 + platform/linuxbsd/wayland/wayland_thread.cpp | 14 ++-- platform/linuxbsd/wayland/wayland_thread.h | 2 + 4 files changed, 53 insertions(+), 31 deletions(-) diff --git a/platform/linuxbsd/wayland/display_server_wayland.cpp b/platform/linuxbsd/wayland/display_server_wayland.cpp index 3fad8c2987c29..c05ad93f42068 100644 --- a/platform/linuxbsd/wayland/display_server_wayland.cpp +++ b/platform/linuxbsd/wayland/display_server_wayland.cpp @@ -1111,6 +1111,28 @@ Key DisplayServerWayland::keyboard_get_keycode_from_physical(Key p_keycode) cons return key; } +void DisplayServerWayland::try_suspend() { + // Due to various reasons, we manually handle display synchronization by + // waiting for a frame event (request to draw) or, if available, the actual + // window's suspend status. When a window is suspended, we can avoid drawing + // altogether, either because the compositor told us that we don't need to or + // because the pace of the frame events became unreliable. + if (emulate_vsync) { + bool frame = wayland_thread.wait_frame_suspend_ms(1000); + if (!frame) { + suspended = true; + } + } else { + if (wayland_thread.is_suspended()) { + suspended = true; + } + } + + if (suspended) { + DEBUG_LOG_WAYLAND("Window suspended."); + } +} + void DisplayServerWayland::process_events() { wayland_thread.mutex.lock(); @@ -1193,30 +1215,32 @@ void DisplayServerWayland::process_events() { wayland_thread.keyboard_echo_keys(); if (!suspended) { - if (emulate_vsync) { - // Due to various reasons, we manually handle display synchronization by - // waiting for a frame event (request to draw) or, if available, the actual - // window's suspend status. When a window is suspended, we can avoid drawing - // altogether, either because the compositor told us that we don't need to or - // because the pace of the frame events became unreliable. - bool frame = wayland_thread.wait_frame_suspend_ms(1000); - if (!frame) { - suspended = true; + // Due to the way legacy suspension works, we have to treat low processor + // usage mode very differently than the regular one. + if (OS::get_singleton()->is_in_low_processor_usage_mode()) { + // NOTE: We must avoid committing a surface if we expect a new frame, as we + // might otherwise commit some inconsistent data (e.g. buffer scale). Note + // that if a new frame is expected it's going to be committed by the renderer + // soon anyways. + if (!RenderingServer::get_singleton()->has_changed()) { + // We _can't_ commit in a different thread (such as in the frame callback + // itself) because we would risk to step on the renderer's feet, which would + // cause subtle but severe issues, such as crashes on setups with explicit + // sync. This isn't normally a problem, as the renderer commits at every + // frame (which is what we need for atomic surface updates anyways), but in + // low processor usage mode that expectation is broken. When it's on, our + // frame rate stops being constant. This also reflects in the frame + // information we use for legacy suspension. In order to avoid issues, let's + // manually commit all surfaces, so that we can get fresh frame data. + wayland_thread.commit_surfaces(); + try_suspend(); } } else { - if (wayland_thread.is_suspended()) { - suspended = true; - } - } - - if (suspended) { - DEBUG_LOG_WAYLAND("Window suspended."); - } - } else { - if (wayland_thread.get_reset_frame()) { - // At last, a sign of life! We're no longer suspended. - suspended = false; + try_suspend(); } + } else if (wayland_thread.get_reset_frame()) { + // At last, a sign of life! We're no longer suspended. + suspended = false; } #ifdef DBUS_ENABLED diff --git a/platform/linuxbsd/wayland/display_server_wayland.h b/platform/linuxbsd/wayland/display_server_wayland.h index f02640a12eb82..e611533664216 100644 --- a/platform/linuxbsd/wayland/display_server_wayland.h +++ b/platform/linuxbsd/wayland/display_server_wayland.h @@ -154,6 +154,8 @@ class DisplayServerWayland : public DisplayServer { virtual void _show_window(); + void try_suspend(); + public: virtual bool has_feature(Feature p_feature) const override; diff --git a/platform/linuxbsd/wayland/wayland_thread.cpp b/platform/linuxbsd/wayland/wayland_thread.cpp index 63a8db07df51f..341cc517e3b39 100644 --- a/platform/linuxbsd/wayland/wayland_thread.cpp +++ b/platform/linuxbsd/wayland/wayland_thread.cpp @@ -968,7 +968,6 @@ void WaylandThread::_frame_wl_callback_on_done(void *data, struct wl_callback *w ws->frame_callback = wl_surface_frame(ws->wl_surface), wl_callback_add_listener(ws->frame_callback, &frame_wl_callback_listener, ws); - wl_surface_commit(ws->wl_surface); if (ws->wl_surface && ws->buffer_scale_changed) { // NOTE: We're only now setting the buffer scale as the idea is to get this @@ -980,11 +979,6 @@ void WaylandThread::_frame_wl_callback_on_done(void *data, struct wl_callback *w // rendering if needed. wl_surface_set_buffer_scale(ws->wl_surface, window_state_get_preferred_buffer_scale(ws)); } - - // NOTE: Remember to set here also other buffer-dependent states (e.g. opaque - // region) if used, to be as close as possible to an atomic surface update. - // Ideally we'd only have one surface commit, but it's not really doable given - // the current state of things. } void WaylandThread::_wl_surface_on_leave(void *data, struct wl_surface *wl_surface, struct wl_output *wl_output) { @@ -3241,10 +3235,6 @@ void WaylandThread::window_create(DisplayServer::WindowID p_window_id, int p_wid ws.frame_callback = wl_surface_frame(ws.wl_surface); wl_callback_add_listener(ws.frame_callback, &frame_wl_callback_listener, &ws); - // NOTE: This commit is only called once to start the whole frame callback - // "loop". - wl_surface_commit(ws.wl_surface); - if (registry.xdg_exporter) { ws.xdg_exported = zxdg_exporter_v1_export(registry.xdg_exporter, ws.wl_surface); zxdg_exported_v1_add_listener(ws.xdg_exported, &xdg_exported_listener, &ws); @@ -4120,6 +4110,10 @@ void WaylandThread::primary_set_text(const String &p_text) { wl_display_roundtrip(wl_display); } +void WaylandThread::commit_surfaces() { + wl_surface_commit(main_window.wl_surface); +} + void WaylandThread::set_frame() { frame = true; } diff --git a/platform/linuxbsd/wayland/wayland_thread.h b/platform/linuxbsd/wayland/wayland_thread.h index 75d03181e270d..775ca71346b8d 100644 --- a/platform/linuxbsd/wayland/wayland_thread.h +++ b/platform/linuxbsd/wayland/wayland_thread.h @@ -992,6 +992,8 @@ class WaylandThread { void primary_set_text(const String &p_text); + void commit_surfaces(); + void set_frame(); bool get_reset_frame(); bool wait_frame_suspend_ms(int p_timeout);