egl: Create sRGB platform surfaces
Needs RevisionPublic

Authored by fredrik on Wed, Jun 19, 11:52 PM.

Details

Reviewers
romangg
Group Reviewers
KWin
Summary

Set EGL_GL_COLORSPACE to EGL_GL_COLORSPACE_SRGB in the
eglCreatePlatformWindowSurface() attributes.

We only do this when we are using OpenGL for now, because
GL_FRAMEBUFFER_SRGB is enabled by default in GLES, and it cannot be
disabled without GL_EXT_sRGB_write_control.

Diff Detail

Repository
R108 KWin
Lint
Lint Skipped
Unit
Unit Tests Skipped
fredrik created this revision.Wed, Jun 19, 11:52 PM
Restricted Application added a project: KWin. · View Herald TranscriptWed, Jun 19, 11:52 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
fredrik requested review of this revision.Wed, Jun 19, 11:52 PM
zzag added a subscriber: zzag.Thu, Jun 20, 11:02 AM
zzag added inline comments.
plugins/platforms/drm/egl_gbm_backend.cpp
188

eglCreatePlatformWindowSurfaceEXT fails with EGL_BAD_ATTRIBUTE when EGL_GL_COLORSPACE is in attrib_list. (EGL_KHR_gl_colorspace is supported)

zzag added inline comments.Thu, Jun 20, 11:07 AM
plugins/platforms/x11/common/eglonxbackend.cpp
281–286

There's another way to implement this, e.g.

int nattrs = 0; // or int cursor = 0;
int attrs[] = {
    EGL_NONE, EGL_NONE,
    EGL_NONE
};

if (!isOpenGLES() && hasEglColorspace) {
    attrs[nattrs++] = EGL_GL_COLORSPACE;
    attrs[nattrs++] = EGL_GL_COLORSPACE_SRGB;
}

// ...

perhaps that's a matter of personal taste. :-)

fredrik updated this revision to Diff 60117.Thu, Jun 20, 12:51 PM
fredrik edited the summary of this revision. (Show Details)
fredrik edited the test plan for this revision. (Show Details)

So this is kind of embarrassing, but it turns out that EGL_NONE is not defined to 0.

fredrik marked an inline comment as done.Thu, Jun 20, 12:52 PM
zzag added a comment.EditedThu, Jun 20, 1:28 PM

Now eglCreatePlatformWindowSurface fails with EGL_BAD_MATCH.

In D21916#482461, @zzag wrote:

Now eglCreatePlatformWindowSurface fails with EGL_BAD_MATCH.

That means that there are no sRGB capable configurations, so this is not going to work.
Does this happen on all platforms with Intel, or just GBM?

zzag added a comment.EditedThu, Jun 20, 2:28 PM

This happens on all platforms with Intel.

In D21916#482561, @zzag wrote:

This happens on all platforms with Intel.

It does seem to work on radeonsi, though I haven't tested all platforms either.
But regardless, this means we have to go to plan B: Disable sRGB rendering on Wayland for the time being.

zzag added a comment.Thu, Jun 20, 3:48 PM

It does seem to work on radeonsi, though I haven't tested all platforms either.

Yep, it works fine on radeonsi.

romangg requested changes to this revision.EditedMon, Jul 1, 8:54 AM
romangg added a subscriber: romangg.

There are now two open reviews and a merged patch to 5.16 that wasn't merged back to master (now merged back by Vlad) and apparently induces a regression on openQA. The merged patch didn't reference its review. All open reviews don't reference each other.

What I mean by that: It's difficult for me and probably other people as well to have an overview of which patch does what and how they are all connected with each other. Please create an overview task where you describe the overall goal of these patches and the intend of each patch singularly. Reference all patches in the task and in between each other when it makes sense.

This revision now requires changes to proceed.Mon, Jul 1, 8:54 AM

There are now two open reviews and a merged patch to 5.16 that wasn't merged back to master (now merged back by Vlad) and apparently induces a regression on openQA. The merged patch didn't reference its review. All open reviews don't reference each other.

What I mean by that: It's difficult for me and probably other people as well to have an overview of which patch does what and how they are all connected with each other. Please create an overview task where you describe the overall goal of these patches and the intend of each patch singularly. Reference all patches in the task and in between each other when it makes sense.

Sorry for the confusion.

This patch is indefinitely postponed, because it doesn't work on the one driver where we need it to work for it to make a difference.

But this patch and the GLX equivalent D21908 explicitly request an sRGB encoded rendering surface.
D22153 disables sRGB rendering in the blur effect if for whatever reason the rendering surface we end up with is not sRGB encoded.

So these patches are related, but also kind of independent.

I think creating a task is a bit overkill at this point, because aside from the regression fix for D21908, D22153 is the only patch that needs to tested and reviewed.