drm backend: choose correct EGL config with mesa-18
ClosedPublic

Authored by alexeymin on Mar 27 2018, 8:24 PM.

Details

Summary

Do not blindly select first EGL config from returned list, but choose the one that matches GBM surfaces, that will be created later.
GBM surfaces are created with GBM_FORMAT_XRGB8888 format, so choose the config that matches it.
With wrong format EglGbmBackend::resetOutput() will later fail with error EGL_BAD_MATCH.

Test Plan

Compile, run startplasmacompositor. Verify that OpenGL compositing is used, either by kwin debug console, or by kwin support information.

Diff Detail

Repository
R108 KWin
Branch
fix_egl_configs
Lint
No Linters Available
Unit
No Unit Test Coverage
alexeymin created this revision.Mar 27 2018, 8:24 PM
Restricted Application added a project: KWin. · View Herald TranscriptMar 27 2018, 8:24 PM
Restricted Application added a subscriber: KWin. · View Herald Transcript
alexeymin requested review of this revision.Mar 27 2018, 8:24 PM
alexeymin added a comment.EditedMar 27 2018, 8:56 PM

Some prehistory for this: recently I finally got a new video card, based on AMD RX 560 specifically to test Plasma on Wayland, and was very disappointed by the fact that kwin_wayland falls back to QPainter compositing.
On my 4k screen this compositing type can be very laggy.
I use amdgpu opensource driver and mesa-18 from git, also xf86-video-amdgpu-18.0.1.

Original error looked like this, eglCreatePlatformWindowSurfaceEXT() failed:

Then I noticed, that if I first start weston, and from weston kwin_wayland --windowed, I can even run full plasma session in that kwin_wayland window running smoothly with OpenGL compositing. So it became clear to me that drm backend was the problem. According to EGL_MESA_platform_gbm spec:

... call eglCreatePlatformWindowSurfaceEXT ...
If <native_window> was created without the GBM_BO_USE_RENDERING flag, or if
the color format of <native_window> differs from the EGL_NATIVE_VISUAL_ID
of <config>, then the function fails and generates EGL_BAD_MATCH.

I supposed that color format can be a problem, so I wrote a small test program to enumerate configs and formats ( github ) that outputs formats FOURCC and number of bits per color channel; its output is:

num egl configs = 10
  0-th GBM format: AR30;  sizes(RGBA) = 10,10,10,2,
  1-th GBM format: AR24;  sizes(RGBA) = 8,8,8,8,
  2-th GBM format: AR30;  sizes(RGBA) = 10,10,10,2,
  3-th GBM format: AR24;  sizes(RGBA) = 8,8,8,8,
  4-th GBM format: AR30;  sizes(RGBA) = 10,10,10,2,
  5-th GBM format: AR24;  sizes(RGBA) = 8,8,8,8,
  6-th GBM format: AR30;  sizes(RGBA) = 10,10,10,2,
  7-th GBM format: AR24;  sizes(RGBA) = 8,8,8,8,
  8-th GBM format: AR30;  sizes(RGBA) = 10,10,10,2,
  9-th GBM format: AR24;  sizes(RGBA) = 8,8,8,8,

Wow, some formats have 10 bits per red, green, blue, and a first one is the one with AR30 format.
Then I tried to patch kwin to be more clever when choosing config, and... it worked. I did that 2 days ago, and since that time I'm running plasma wayland session with hardware accelerated OpenGL compositing with no problems.
I'm currently submitting this diff from this configuration:

At least it fixed the problem for me. Output from kwin_wayland when starting plasma session as usual (startplasmacompositor) is: https://paste.kde.org/p88rlpvso
I used following env vars to get more debug output: EGL_LOG_LEVEL=debug QT_LOGGING_RULES="kwin*.debug=true"
As you can see, more EGL configs match kwin's demand here (80), and first one which is not AR30 - XR24 - matched and is chosen.

...
kwin_wayland_drm: 31 th config GBM format: AR30 ; color sizes (RGBA order): 10 10 10 2
kwin_wayland_drm: 32 th config GBM format: XR24 ; color sizes (RGBA order): 8 8 8 0

Hopefully this doesn't break on intel, If someone has intel hardware, please test :)

alexeymin edited the test plan for this revision. (Show Details)Mar 27 2018, 9:19 PM
alexeymin edited reviewers, added: KWin, Plasma on Wayland, bshah; removed: kwin.
alexeymin added a project: Plasma on Wayland.
Restricted Application added a subscriber: kwin. · View Herald TranscriptMar 27 2018, 9:19 PM
zzag added a subscriber: zzag.Mar 27 2018, 9:40 PM
zzag added inline comments.
plugins/platforms/drm/egl_gbm_backend.cpp
270

Coding style nitpick: use camelCase instead of snake_case.

286

(a) Maybe, I'm wrong but that's not how arrays are compared.
(b) GBM_FORMAT_XRGB8888 is an integer(uint32_t).

zzag added inline comments.Mar 27 2018, 9:42 PM
plugins/platforms/drm/egl_gbm_backend.cpp
286

Stop! Misinterpreted it with gbm_format_str

alexeymin added inline comments.Mar 27 2018, 9:51 PM
plugins/platforms/drm/egl_gbm_backend.cpp
286

Yeah, since GBM_FORMAT_XRGB8888 and others are defined as __gbm_fourcc_code('X', 'R', '2', '4'), which shifts ASCII chars into different offsets in integer, we can directly print this integer as string of chars, if followed by zero bytes.
And it was only used for debug output, guess this part of code can be removed at all. Especially part which gets EGL_RED_SIZE etc, they are not used for anything except debug output. But it was important to understand waht is happening here.

zzag added inline comments.Mar 27 2018, 10:10 PM
plugins/platforms/drm/egl_gbm_backend.cpp
286

Sorry for that.

Anyways, you could merge these [this and one below] two if statements.

alexeymin added inline comments.Mar 28 2018, 7:25 AM
plugins/platforms/drm/egl_gbm_backend.cpp
286

NP!
About two ifs, initially I tested configs with GBM_FORMAT_XRGB8888 and GBM_FORMAT_ARGB8888 separately, that's why two ifs left. For me variant with ARGB also worked. Note: gbm surface is created with XRGB8888 format (link) , but I guess it is somehow compatible with config ARGB8888. I'm not sure if this is right thing to do; probably the if (gbm_format == GBM_FORMAT_ARGB8888) { block should be removed, if it causes problems for someone. It needs testing on different GPUs/drivers, I guess...

And now I'm reading news about mesa-18 new 10-bit color feature and realizing that maybe mesa-18 was my initial problem :)

alexeymin updated this revision to Diff 30971.Mar 30 2018, 8:11 PM
  • Use camelCase; merge two "if" statements.
alexeymin marked 6 inline comments as done.Mar 30 2018, 8:14 PM
alexeymin retitled this revision from drm backend: choose correct EGL config to drm backend: choose correct EGL config with mesa-18.
alexeymin added a comment.EditedApr 3 2018, 10:50 AM

I looked into Qt sources (QtWayland specifically), and found out that Qt uses similar approach - it loops over received configs and chooses the one that matches user's requested bit depths, if they were specified.
bool QEglConfigChooser::filterConfig(EGLConfig config) const
which is used from QWaylandGLContext class during context creation.

The whole EGLConfig selection part is great.

plugins/platforms/drm/egl_gbm_backend.cpp
281

May as well wrap this bit in QLoggingCategory::isDebugEnabled()

If EGLInt is 8 bytes long you need to be 9 to include the '\0' character.
I can imagine this works if EGLInt is four bytes long, but then why 8? Are you adding padding?

283

th -> the

alexeymin updated this revision to Diff 31337.Apr 4 2018, 10:21 PM
  • Move extra debug output under isDebugEnabled() block
davidedmundson accepted this revision.Apr 4 2018, 10:25 PM
This revision is now accepted and ready to land.Apr 4 2018, 10:25 PM
This revision was automatically updated to reflect the committed changes.
alexeymin marked 2 inline comments as done.
cfeck added a subscriber: cfeck.Apr 4 2018, 11:00 PM

Is https://bugs.kde.org/show_bug.cgi?id=392716 related or even fixed with this commit?

Is https://bugs.kde.org/show_bug.cgi?id=392716 related or even fixed with this commit?

I didn't know about this bug.
It definitely looks related, though I don't have Intel hardware to test. And in my case I've seen problems only with kwin_wayland, x11 session worked fine...

Could you please backport to Plasma/5.12 branch?

Could you please backport to Plasma/5.12 branch?

Done 0ccecbc4275783e217d3c1ab1d3acd6988368757, it actually applied to Plasma/5.12 branch without modifications.