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

Fix KMS with OpenGL. #7708

Merged
merged 1 commit into from
Dec 6, 2018
Merged

Fix KMS with OpenGL. #7708

merged 1 commit into from
Dec 6, 2018

Conversation

orbea
Copy link
Contributor

@orbea orbea commented Dec 6, 2018

Description

Fixes KMS with OpenGL.

All credit for this patch goes to @dtsarr.

Related Issues

Fixes #7119

All credit for this patch goes to dtsarr.

Fixes #7119
@inactive123 inactive123 merged commit e036470 into libretro:master Dec 6, 2018
@orbea orbea deleted the kms branch December 7, 2018 00:10
@5schatten
Copy link

This commit breaks the build for me on ARM / S905X

gfx/common/egl_common.c: In function 'egl_init_context':
gfx/common/egl_common.c:360:17: error: 'GBM_FORMAT_XRGB8888' undeclared (first use in this function)
       if (id == GBM_FORMAT_XRGB8888) break;
                 ^~~~~~~~~~~~~~~~~~~
gfx/common/egl_common.c:360:17: note: each undeclared identifier is reported only once for each function it appears in
CC deps/7zip/7zIn.c
make: *** [Makefile:193: obj-unix/release/gfx/common/egl_common.o] Error 1
make: *** Waiting for unfinished jobs....

@orbea
Copy link
Contributor Author

orbea commented Dec 7, 2018

Well that is unfortunate... What OS is running on this device? Can you please share the contents of config.mk after ./configure? I'd guess this needs to be hidden behind #ifdef GBM, but its hard to test that without the right device.

Herre GBM_FORMAT_XRGB8888 is provided by gbm.h from the mesa package.

@inactive123
Copy link
Contributor

Yes. @orbea There really shouldn't be any GBM dependencies added to egl_common.c. It should be fairly decoupled so that egl_common.c won't have any GBM dependencies. Not sure how to fix it, from the logic that is being shown here, you deliberately look for a GBM_FORMAT_XRGB8888 format and if you can't find it, then it will just not attempt to setup EGL? Is there a more portable way of doing this? I'd imagine this wouldn't work on EGLStreams.

I believe they faced a similar issue here -

https://gitlab.gnome.org/mvicomoya/mutter/commit/54246c75f57ac889c0d433756f3ddf1b280d2dd5

@orbea
Copy link
Contributor Author

orbea commented Dec 7, 2018

I didn't make the patch only the PR, @dtsarr may know better. However this might be a bandaid even if the example you linked looks better...

diff --git a/gfx/common/egl_common.c b/gfx/common/egl_common.c
index 8ae8fee90c..a8f7380568 100644
--- a/gfx/common/egl_common.c
+++ b/gfx/common/egl_common.c
@@ -334,6 +334,7 @@ bool egl_init_context(egl_ctx_data_t *egl,
 
    RARCH_LOG("[EGL]: EGL version: %d.%d
", *major, *minor);
 
+#ifdef HAVE_GBM
    if (!eglGetConfigs(egl->dpy, NULL, 0, &count) || count < 1)
    {
       RARCH_ERR("[EGL]: No cofigs to choose from.
");
@@ -374,6 +375,11 @@ bool egl_init_context(egl_ctx_data_t *egl,
       egl->config = configs[config_index];
 
    free(configs);
+#else
+   if (!eglChooseConfig(egl->dpy,
+            attrib_ptr, &egl->config, 1, n) || *n != 1)
+      return false;
+#endif
 
    egl->major = g_egl_major;
    egl->minor = g_egl_minor;

@inactive123
Copy link
Contributor

Yeah, we can go with that for now.

@5schatten
Copy link

@twinaphex @orbea
I'm building this package https://github.com/5schatten/LibreELEC.tv/blob/libreelec-9.0-rr/packages/5schatten/emulation-frontends/retroarch/package.mk for my LE fork. It builds fine for generic/OpenGL target.

Your patch was a bit malformed but this one fix compiling for S905X -> building a test image now
https://hastebin.com/nexucinuji.cpp

@orbea
Copy link
Contributor Author

orbea commented Dec 7, 2018

@natinusala Have you tried the band-aid patch above? Does it work?

@inactive123
Copy link
Contributor

@natinusala Try if this works (I assume you are talking about Switch Lakka here) -

bfd9141

@natinusala
Copy link
Contributor

All good

@Sunderland93
Copy link
Contributor

Broke Wayland EGL #7721

@inactive123
Copy link
Contributor

inactive123 commented Dec 10, 2018

@Sunderland93 can you fix this up without breaking the other targets?

@Sunderland93
Copy link
Contributor

@twinaphex No, I have no idea how to resolve this

@inactive123
Copy link
Contributor

@orbea I'd like to ask you to fix this then. It's pretty important that Wayland EGL continues to work.

@orbea
Copy link
Contributor Author

orbea commented Dec 10, 2018

@twinaphex I didn't make this patch, @dtsarr did. I've never even used wayland before and unfortunately I'm not sure how to fix this either...

@inactive123
Copy link
Contributor

@dtsarr Can you make sure EGL Wayland still works and send a patch for it?

@inactive123
Copy link
Contributor

inactive123 commented Dec 11, 2018

You guys need to come together here and figure out a solution of some sorts, a release is nearing soon and it would be very bad form if we fixed KMS for one particular usecase and broke it for say EGL Wayland. I might have to revert this completely for now if you guys can't work together to come up with some kind of collaborative solution unfortunately.

I really don't want to have to do that, so please work with me here to ensure we have a nice good release prepared before Christmas.

@orbea
Copy link
Contributor Author

orbea commented Dec 11, 2018

Isn't there anyone involved in this project anymore with knowledge of EGL or wayland that can help?

A naive guess at a solution would be to find what EGL format wayland wants and then give it that format under a wayland context while keeping the current format otherwise. I tried researching the first step, but my lack of knowledge on wayland made finding these details less than fruitful with a search engine.

Its unfortunate it turned out like this, but what do you expect when no one knowledgeable enough actually reviews the code? ... Of course it seems the only way to get testing done here is to merge first and hope for the best. :\

Edit: Just to clarify, I am not blaming anyone, just I wish there was a better way?

@inactive123
Copy link
Contributor

@orbea I'm asking around right now. There might be a solution coming.

@orbea
Copy link
Contributor Author

orbea commented Dec 11, 2018

Thank you! I really appreciate the help rectifying this issue. :)

@inactive123
Copy link
Contributor

I have just merged a PR by @Themaister. I ask the people in this thread to please test again and see if everything works now for them.

@orbea
Copy link
Contributor Author

orbea commented Dec 11, 2018

KMS still works here with both OpenGL and Vulkan, we'll have to wait for wayland users to test if its fixed there too. @Themaister thank you very much for your help!

@Themaister
Copy link
Contributor

I tested EGL Wayland myself as well as DRM, works for me (tm).

@orbea
Copy link
Contributor Author

orbea commented Dec 11, 2018

@Sunderland93 still seems to be having problems with wayland and gnome3 here.

#7721

Maybe its specific to gnome though?

@Sunderland93
Copy link
Contributor

Maybe its specific to gnome though?

Same in rootston, mir and weston

@Sunderland93
Copy link
Contributor

Upd. Fixed after update my distro :)

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

Successfully merging this pull request may close these issues.

6 participants