Wayland event polling crash when VSync is disabled, with message "explicit sync is used, but no release point is set" about godot HOT 3 CLOSED

All right, I think that, finally, the PR is ready! Note that it was a bit more complicated than I expected due to low processor usage mode but according to my tests, it fixes the problem!

Here it is: #93684.

from godot.

All right, so, after a short panic, I think I got it.

Funnily enough, another user reported this to me in a more exotic usecase and I, out of experience, shrugged it off as a driver bug (oh was I naive). Note that they used vsync, so this is certainly not a vsync issue.

It looks like putting wl_surface_commit inside some asynchronous methods (namely WaylandThread::_frame_callback_on_done) made a threading issue evident, after the huge "explicit sync revolution" (apparently also firefox has this issue¹).

If, due to multithreading, the command gets queued while an explicit sync is getting set up and before a buffer is set, the no_buffer error rightfully shows up and it explodes.

This apparently got also noticed while developing the new protocol² and it has been concluded as misuse of the protocol by clients like us, so we ought to fix that by limiting commits to at least the same thread (ideally once by the renderer). I already tried reducing commits before, but wasn't able to do so because I forgot about low-consumption mode (my archnemesis).

I think I got a quick patch that should remove even more unneeded commits and sync the only one needed for frame timeout to the main thread . It's not pretty, but if it works we can look to perhaps only spam-commit if in low-usage mode or only when checking, not sure. (this should get only called each frame anyways so whatever).

What?

If all of that flew over the reader's head, don't worry, I'm still a bit panicking :gdsweat: . Please try this patch:

UPDATE: There's a PR now which is a bit more complex: #93684

diff --git a/platform/linuxbsd/wayland/display_server_wayland.cpp b/platform/linuxbsd/wayland/display_server_wayland.cpp
index 3fad8c2987..363c14bbcd 100644
--- a/platform/linuxbsd/wayland/display_server_wayland.cpp
+++ b/platform/linuxbsd/wayland/display_server_wayland.cpp
@@ -1192,6 +1192,7 @@ void DisplayServerWayland::process_events() {
 
 	wayland_thread.keyboard_echo_keys();
 
+	wl_surface_commit(wayland_thread.window_get_wl_surface(MAIN_WINDOW_ID));
 	if (!suspended) {
 		if (emulate_vsync) {
 			// Due to various reasons, we manually handle display synchronization by
diff --git a/platform/linuxbsd/wayland/wayland_thread.cpp b/platform/linuxbsd/wayland/wayland_thread.cpp
index 63a8db07df..02b204f6be 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
@@ -3241,10 +3240,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);

Note: I've not tested the above as I don't have an explicit sync setup (yet), I'm working on getting one ASAP.

See also: NVIDIA/egl-wayland#110

Edit: made wording a little bit clearer, it sounded like the protocol was wrong or something. We're in the wrong and have always been, it popped up just now.

from godot.

Ok, update, I managed to set up an environment with explicit-sync and I can indeed replicate this bug.

It seems indeed that the aforementioned patch fixes the issue! I'd be tempted to make a PR as-is, but I'll look at it harder to make sure whether committing each process frame is a problem (I don't think) and if so whether I can avoid doing that (detecting low-usage mode?)

from godot.