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

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

Open
fluolite opened this issue Oct 10, 2024 · 19 comments

Comments

@fluolite
Copy link

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
Copy link
Contributor

tronical commented Oct 10, 2024

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
Copy link
Member

@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
Copy link
Contributor

@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
Copy link
Author

fluolite commented Oct 10, 2024

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
Copy link
Author

fluolite commented Oct 10, 2024

@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
Copy link
Member

MarijnS95 commented Oct 10, 2024

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:

RawDisplayHandle::Gbm(handle) if extensions.contains("EGL_MESA_platform_gbm") => {
(egl::PLATFORM_GBM_MESA, handle.gbm_device.as_ptr())

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
Copy link
Author

fluolite commented Oct 10, 2024

This is the debug output.

[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")

  1. 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
Copy link
Member

MarijnS95 commented Oct 10, 2024

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

let version = unsafe {
let (mut major, mut minor) = (0, 0);
if egl.Initialize(*display, &mut major, &mut minor) == egl::FALSE {
return Err(super::check_error().expect_err("eglInit failed without a reason"));
}
Version::new(major as u8, minor as u8)
};


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
Copy link
Author

fluolite commented Oct 11, 2024

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

let version = unsafe {
let (mut major, mut minor) = (0, 0);
if egl.Initialize(*display, &mut major, &mut minor) == egl::FALSE {
return Err(super::check_error().expect_err("eglInit failed without a reason"));
}
Version::new(major as u8, minor as u8)
};

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
  1. Scenario 2 working well for me, I don't know why. I'll follow it when I can.

@MarijnS95
Copy link
Member

MarijnS95 commented Oct 11, 2024

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
Copy link
Contributor

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
Copy link
Member

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 MarijnS95 changed the title Egl get_platform_display_ext failed because egl client extensions mismatch EGL_KHR_platform_gbm (but not EGL_MESA_platform_gbm) client extension present on EGL 1.4 Oct 11, 2024
@MarijnS95
Copy link
Member

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
Copy link
Member

MarijnS95 commented Oct 11, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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
Copy link
Member

@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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants