rust-windowing / glutin

A low-level library for OpenGL context creation
Apache License 2.0
2k stars 477 forks source link

`EGL_KHR_platform_gbm` (but not `EGL_MESA_platform_gbm`) client extension present on EGL 1.4 #1708

Closed fluolite closed 1 day ago

fluolite commented 1 month ago

In my project, get_platform_display_ext get failed because egl client extensions mismatch。

egl client extensions: 
EGL_EXT_client_extensions EGL_EXT_platform_base EGL_KHR_client_get_all_proc_addresses EGL_KHR_platform_gbm EGL_KHR_platform_wayland EGL_EXT_platform_wayland

Screenshot_select-area_20241010115958

tronical commented 1 month ago

Hmm, this is in get_platform_display_ext, but before that function is called, get_platform_display should've matched:

            RawDisplayHandle::Gbm(handle) if extensions.contains("EGL_KHR_platform_gbm") => {
                (egl::PLATFORM_GBM_KHR, handle.gbm_device.as_ptr())
            },

Differently put: EGL_KHR_platform_gbm should patch to (egl::PLATFORM_GBM_KHR, handle.gbm_device.as_ptr()) and not (egl::PLATFORM_GBM_MESA, handle.gbm_device.as_ptr()).

Could you try to see why that didn't work?

MarijnS95 commented 1 month ago

@fluolite can you clarify what the issue is? You provided a list of extensions including EGL_KHR_platform_gbm and a screenshot showing that that's part of the match, but didn't detail what is meant by In my project, get_platform_display_ext get failed.

Specifically, can you clarify if you actually have a RawDisplayHandle::Gbm(..) variant? How are you calling into glutin, and what's the result?

tronical commented 1 month ago

@fluolite is using Slint IIRC and there we do create a RawDisplayHandle::Gbm and eventually call glutin::display::Display::new(display_handle.as_raw(), glutin::display::DisplayApiPreference::Egl).

fluolite commented 1 month ago

Hmm, this is in get_platform_display_ext, but before that function is called, get_platform_display should've matched:

            RawDisplayHandle::Gbm(handle) if extensions.contains("EGL_KHR_platform_gbm") => {
                (egl::PLATFORM_GBM_KHR, handle.gbm_device.as_ptr())
            },

Differently put: EGL_KHR_platform_gbm should patch to (egl::PLATFORM_GBM_KHR, handle.gbm_device.as_ptr()) and not (egl::PLATFORM_GBM_MESA, handle.gbm_device.as_ptr()).

Could you try to see why that didn't work?

The get_platform_display did not matched! I guess is some platform implementation did not abide by EGL extensions. And I review qt source code, it look like always use eglGetProcAddress("eglGetPlatformDisplayEXT")

Screenshot_select-area_20241010180015 Screenshot_select-area_20241010181956

Screenshot_select-area_20241010180145

fluolite commented 1 month ago

@fluolite can you clarify what the issue is? You provided a list of extensions including EGL_KHR_platform_gbm and a screenshot showing that that's part of the match, but didn't detail what is meant by In my project, get_platform_display_ext get failed.

Specifically, can you clarify if you actually have a RawDisplayHandle::Gbm(..) variant? How are you calling into glutin, and what's the result?

I didn't use it directly but use slint ui, and slint backend is linuxkms-skia-opengl. The following is EGL extensions from glutin get_platform_display_ext println!. Screenshot_select-area_20241010180015 Screenshot_select-area_20241010181956

Screenshot_select-area_20241010180145

MarijnS95 commented 1 month ago

Thanks @tronical, I see no way how I should have inferred that from @fluolite's minimal description 😅.


@fluolite please share snippets of code in backticks instead of screenshots, they are unnecessarily hard to read and digest.

Please debug-print the value of display: RawDisplayHandle. I want to confirm if you truly have a Gbm handle and not something else.

EDIT: Not to mention, @tronical is correct here that EGL_KHR_platform_gbm should only be used with eglGetPlatformDisplay() and not with eglGetPlatformDisplayExt() (even if the constants PLATFORM_GBM_KHR and PLATFORM_GBM_MESA are the same value) so a subsequent failure that you did not provide in any message thus far is expected:

https://github.com/rust-windowing/glutin/blob/76dac27b51a9f1619d1098bed777ece7cd2b8c73/glutin/src/api/egl/display.rs#L365-L366

We should instead debug why egl.GetPlatformDisplay (not the Ext version) isn't being loaded, which is available since EGL 1.5 and a requirement for EGL_KHR_platform_gbm to exist.

fluolite commented 1 month ago

This is the debug output.

  1. 
    [root@TW02:/userdata/alien/bin]# export SLINT_BACKEND=linuxkms-skia-opengl
    [root@TW02:/userdata/alien/bin]# ./slintprimer 
    ============================I am fix
    =====================Display new
    =====================get_platform_display
    =====================get_platform_display is not supported
    =====================get_platform_display_ext
    =====================get_platform_display_ext, extensions: {"EGL_EXT_platform_wayland", "EGL_EXT_client_extensions", "EGL_EXT_platform_base", "EGL_KHR_platform_gbm", "EGL_KHR_platform_wayland", "EGL_KHR_client_get_all_proc_addresses"}
    =====================get_display
    Error: Other("Error creating glutin display for native display Gbm(\n    GbmDisplayHandle {\n        gbm_device: 0x000000557db98990,\n    },\n): failed to create EGLDisplay without a reason")

2. fix "extensions.contains("EGL_KHR_platform_gbm")" in get_platform_display_ext

[root@TW02:/userdata/alien/bin]# export SLINT_BACKEND=linuxkms-skia-opengl [root@TW02:/userdata/alien/bin]# ./slintprimer ============================I am fix =====================Display new =====================get_platform_display =====================get_platform_display is not supported =====================get_platform_display_ext =====================get_platform_display_ext, extensions: {"EGL_KHR_client_get_all_proc_addresses", "EGL_EXT_platform_base", "EGL_KHR_platform_wayland", "EGL_EXT_platform_wayland", "EGL_EXT_client_extensions", "EGL_KHR_platform_gbm"} =====================initialize_display Using Skia OpenGL renderer Rendering at 1920x1080 Slint: Build config: debug; Backend: Skia renderer (skia backend opengl; surface: 24 bpp)

MarijnS95 commented 1 month ago

Thanks! In your initialize_display logs, can you debug-print the version that is returned by egl.Initialize()?

https://github.com/rust-windowing/glutin/blob/d8e7569b1493b91d50af68156ec6cee34816fcb9/glutin/src/api/egl/display.rs#L506-L513


Is scenario 2 working for you? I'm curious if eglGetPlatformDisplayExt() is allowed to work if EGL_KHR_platform_gbm is available, which inidcates EGL 1.5 and support/requirement for eglGetPlatformDisplay().

fluolite commented 1 month ago

Thanks! In your initialize_display logs, can you debug-print the version that is returned by egl.Initialize()?

https://github.com/rust-windowing/glutin/blob/d8e7569b1493b91d50af68156ec6cee34816fcb9/glutin/src/api/egl/display.rs#L506-L513

Is scenario 2 working for you? I'm curious if eglGetPlatformDisplayExt() is allowed to work if EGL_KHR_platform_gbm is available, which inidcates EGL 1.5 and support/requirement for eglGetPlatformDisplay().

  1. The logs

    [root@TW02:/userdata/alien/bin]# ./slintprimer 
    ============================I am fix
    =====================Display new
    =====================get_platform_display
    =====================get_platform_display is not supported
    =====================get_platform_display_ext
    =====================get_platform_display_ext, extensions: {"EGL_KHR_platform_wayland", "EGL_EXT_platform_base", "EGL_EXT_platform_wayland", "EGL_EXT_client_extensions", "EGL_KHR_client_get_all_proc_addresses", "EGL_KHR_platform_gbm"}
    =====================initialize_display
    =====================initialize_display success, version major: 1, minor: 4
    Using FemtoVG OpenGL renderer
    Rendering at 1920x1080
    Slint: Build config: debug; Backend: FemtoVG renderer
  2. Scenario 2 working well for me, I don't know why. I'll follow it when I can.

MarijnS95 commented 1 month ago

So that's a broken EGL driver, if it advertises EGL 1.4 but also EGL_KHR_platform_gbm which requires 1.5. You should probably report this to the respective driver vendor - they should instead advertise EGL_MESA_platform_gbm.

tronical commented 1 month ago

AFAIK this is in the context of a Rockchip RK3568 with a Mali-G52 GPU. That's not too exotic. Would you be open to a workaround in glutin?

I think @fluolite is correct in his observation that Qt treats EGL_KHR_platform_gbm and EGL_MESA_platform_gbm the same.

MarijnS95 commented 1 month ago

That doesn't use the Mesa driver stack, right? I found that it should expose both MESA and KHR otherwise:

https://gitlab.freedesktop.org/mesa/mesa/-/blob/ce2eedd13e8163185a69070bc1fb8abcd602c87d/src/egl/main/eglglobals.c#L99-102

The commit that added the KHR variant also explains that presence of the extension determines whether there is support for EGL 1.5 and the non-EXT eglGetPlatformDisplay function can be loaded:

https://gitlab.freedesktop.org/mesa/mesa/-/commit/a3fb064e000a8706319dc996788159bf84a13f0f

Neither of which is the case here, again detailing that this driver should've exposed MESA.


I'm not the one to sign off on having workarounds in glutin and it'd be hard to have one if there is no upstream bug report to link to in the first place. In any case this issue feels similar to the one in #1689 where libglvnd was the culprit: is libglvnd used here?


And as detailed above, while the value of egl::PLATFORM_GBM_KHR is equal to that of egl::PLATFORM_GBM_MESA, the extensions technically only specify that its values are valid for eglGetPlatformDisplay() and eglGetPlatformDisplayEXT() respectively so they are theoretically allowed to reject it because the combination of platform extension and function is not "allowed"?

Maybe if you link the Qt source code (which would anyway be useful when referencing something...) that does this, we could reverse-search whether that followed up with an upstream report and justification of this workaround.

MarijnS95 commented 1 month ago

I'm trying to come up with a title that is more descriptive about the actual issue at hand, since get_platform_display_ext failed is counter what you said that get_platform_display_ext actually works (because get_platform_display cannot be loaded) when we allow it to handle EGL_KHR_platform_gbm, even though the client driver should've exposed EGL_MESA_platform_gbm. (and/or switched to EGL 1.5 which is a requirement for EGL_KHR_platform_gbm, but that's a much bigger change to make)

MarijnS95 commented 1 month ago

Note that I am now reading from https://gitlab.freedesktop.org/glvnd/libglvnd/-/issues/251#note_2553412:

By the spec, it is allowed to support EGL 1.5 at the client level (so you can use eglGetPlatformDisplay to get your EGLDisplay), but for that to still return an EGLDisplay that only supports EGL 1.4 or earlier. So, checking the EGLDisplay's version (as returned by eglInitialize) is technically the correct thing for an application to do.

Not that that changes much. While we can't say from the result of eglInitialize() that the client implementation should have behaved as 1.4 (that only applies to the returned EGLDisplay), the original findings still hold that by exposing EGL_KHR_platform_gbm we can expect the client to minimally support 1.5, yet it doesn't behave like that by not providing the eglGetPlatformDisplay() symbol.

kchibisov commented 1 month ago

And as detailed above, while the value of egl::PLATFORM_GBM_KHR is equal to that of egl::PLATFORM_GBM_MESA, the extensions technically only specify that its values are valid for eglGetPlatformDisplay() and eglGetPlatformDisplayEXT() respectively so they are theoretically allowed to reject it because the combination of platform extension and function is not "allowed"?

Given that they are all the same and that EGLDisplay is the same no matter which path you take, it'll probably be fine to workaround here.

And yes, KHR without eglGetPlatformDisplay doesn't make any sense at all...

@fluolite I'd assume that

diff --git a/glutin/src/api/egl/display.rs b/glutin/src/api/egl/display.rs
index 4c2454d..ffe7cb0 100644
--- a/glutin/src/api/egl/display.rs
+++ b/glutin/src/api/egl/display.rs
@@ -362,7 +362,10 @@ impl Display {
                     handle.connection.map_or(egl::DEFAULT_DISPLAY as *mut _, |c| c.as_ptr()),
                 )
             },
-            RawDisplayHandle::Gbm(handle) if extensions.contains("EGL_MESA_platform_gbm") => {
+            RawDisplayHandle::Gbm(handle)
+                if extensions.contains("EGL_MESA_platform_gbm")
+                    || extensions.contains("EGL_KHR_platform_gbm") =>
+            {
                 (egl::PLATFORM_GBM_MESA, handle.gbm_device.as_ptr())
             },
             RawDisplayHandle::Drm(_) => {

works?

kchibisov commented 1 month ago

I generally don't mind working around stuff like that, since usually nothing can be done in some cases other than that, and it's not like we can fail here, since if KHR worked, we'd used it, and if e.g. mesa not present but khr present, using khr will be fine, since it's the same constant.

Though, a link with justification for stuff like that would be nice.

tronical commented 1 month ago

Maybe if you link the Qt source code (which would anyway be useful when referencing something...) that does this, we could reverse-search whether that followed up with an upstream report and justification of this workaround.

These are the two places I was looking at earlier:

https://github.com/qt/qtbase/blob/b429da48bed8cfa58f361ae85503c412919c3dd6/src/plugins/platforms/eglfs/deviceintegration/eglfs_kms/qeglfskmsgbmintegration.cpp#L41 https://github.com/qt/qtbase/blob/b429da48bed8cfa58f361ae85503c412919c3dd6/src/plugins/platforms/eglfs/deviceintegration/eglfs_kms/qeglfskmsgbmwindow.cpp#L28

kchibisov commented 1 month ago

Hm, just to make it clear, does creating surface actually work with EXT function here? Or you'd need a KHR as well for it, if that's the case it could be a bit more weird?

MarijnS95 commented 1 month ago

@kchibisov it seems yes, per the answer to my question 2 here.

(After all the enum value is the same, and the underlying function implementation likely behaves the same, even it the combination of extension version and "inferred EGL client version" is wrong)

kchibisov commented 2 days ago

One can try this https://github.com/rust-windowing/glutin/pull/1716 , from what I've understood in QT code it should be enough...