Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Manually load glXSwapInterval function pointers #68714

Closed
wants to merge 1 commit into from

Conversation

clayjohn
Copy link
Member

@clayjohn clayjohn commented Nov 16, 2022

Fixes issue reported in #68700 (comment)

This copies the code from 3.x for initializing the swap interval function with GLX. By actually calling the vsync code I had exposed an issue that has been around since the early GLES3 port. For some reason the old code was working fine on my system (intel iGPU) My guess is my system was able to pull the MESA version while the NVidia drivers crashed in one of the latter two options.

This works fine on my system, but my system wasn't crashing before. Will need testing from someone who can reproduce.

cc @Lateasusual

@Lateasusual
Copy link
Contributor

Tested just now, no crash this time :)

@alcomposer
Copy link
Contributor

alcomposer commented Nov 16, 2022

Apologies for double posting, I was looking at this a few hours ago, and didn't notice this fix.

I can confirm that this patch fixes the crash for Nvidia cards on Linux, I'm using driver: 520.56.06

Alternatively can't we just skip setting the extensions to nullptr's and use what GLAD has already loaded?

This also works on my system:

diff --git a/platform/linuxbsd/x11/gl_manager_x11.cpp b/platform/linuxbsd/x11/gl_manager_x11.cpp
index 4d8d63c64a..bcefcf9695 100644
--- a/platform/linuxbsd/x11/gl_manager_x11.cpp
+++ b/platform/linuxbsd/x11/gl_manager_x11.cpp
@@ -330,10 +330,6 @@ Error GLManager_X11::initialize() {
 }
 
 void GLManager_X11::set_use_vsync(bool p_use) {
-       static PFNGLXSWAPINTERVALEXTPROC glXSwapIntervalEXT = nullptr;
-       static PFNGLXSWAPINTERVALSGIPROC glXSwapIntervalMESA = nullptr;
-       static PFNGLXSWAPINTERVALSGIPROC glXSwapIntervalSGI = nullptr;
-
        // force vsync in the editor for now, as a safety measure
        bool is_editor = Engine::get_singleton()->is_editor_hint();
        if (is_editor) {

GLAD SwapInterval extension loaders be here:

godot/thirdparty/glad/glx.c

Lines 149 to 161 in 95a85c9

static void glad_glx_load_GLX_EXT_swap_control( GLADuserptrloadfunc load, void* userptr) {
if(!GLAD_GLX_EXT_swap_control) return;
glad_glXSwapIntervalEXT = (PFNGLXSWAPINTERVALEXTPROC) load(userptr, "glXSwapIntervalEXT");
}
static void glad_glx_load_GLX_MESA_swap_control( GLADuserptrloadfunc load, void* userptr) {
if(!GLAD_GLX_MESA_swap_control) return;
glad_glXGetSwapIntervalMESA = (PFNGLXGETSWAPINTERVALMESAPROC) load(userptr, "glXGetSwapIntervalMESA");
glad_glXSwapIntervalMESA = (PFNGLXSWAPINTERVALMESAPROC) load(userptr, "glXSwapIntervalMESA");
}
static void glad_glx_load_GLX_SGI_swap_control( GLADuserptrloadfunc load, void* userptr) {
if(!GLAD_GLX_SGI_swap_control) return;
glad_glXSwapIntervalSGI = (PFNGLXSWAPINTERVALSGIPROC) load(userptr, "glXSwapIntervalSGI");
}

@akien-mga
Copy link
Member

CC @Riteo

@akien-mga
Copy link
Member

Superseded by #68727, @alcomposer's patch does sound like a cleaner solution (and seems to work in my tests).

@alcomposer
Copy link
Contributor

I'm a bit concerned why @clayjohn 's iGPU didn't crash before. Possibly Intel don't have these extensions?

@akien-mga
Copy link
Member

akien-mga commented Nov 16, 2022

It didn't crash for me either on Intel and AMD, it seems to be a Nvidia specific issue. But yeah, I don't know why.

@Riteo
Copy link
Contributor

Riteo commented Nov 16, 2022

Sorry guys, this was my fault. I removed that exact code without removing the pointer references.

See #68727 (comment).

@alcomposer
Copy link
Contributor

Well, I'm just thinking I can't see anything calling a nullptr and recovering. So possibly that if/else block just returns on those platforms? We have confirmed that V-Sync gets set right?

Nvidia glXSwapIntervalSGI(1) returns 0. So I think that is working, at least it thinks it's working.

@Riteo
Copy link
Contributor

Riteo commented Nov 16, 2022

@alcomposer what are you talking about? If you mean that it should've had crashed with my wrong change, it did. If you're wondering whether VSync is set right, I don't know.

The Intel iGPU didn't crash before because in the meantime my PR was merged. At least if "before" doesn't mean "with the same commit".

@akien-mga
Copy link
Member

The Intel iGPU didn't crash before because in the meantime my PR was merged. At least if "before" doesn't mean "with the same commit".

Ah indeed, I reverted #68727 locally and I can reproduce the crash with Intel and AMD on Mesa now.
Mystery solved :)

@clayjohn clayjohn deleted the GLES3-vsync-fix branch November 16, 2022 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash GLManager_X11::set_use_vsync(bool)
5 participants