[PATCH v2 0/3] MR5264: winewayland.drv: part 13.2: More OpenGL functionality
This MR adds support for some more OpenGL functionality: * wglCreateContextAttribsARB * wglShareLists * wgl(Get)SwapIntervalEXT I originally intended to also include a fix for the translucency issue seen in some compositors, by using EGL_EXT_present_opaque. However, I'll hold off for now since I am seeing some issues with certain hardware and pixel formats when using this extension. Initial investigation indicates it is a problem in Mesa. For anyone who is affected by the translucency issue and wants to try out the patch in their local build anyway, it is at: https://gitlab.winehq.org/afrantzis/wine/-/commit/0685d1d2cfc126e2bb546fe2cd... -- A question for the upcoming work: In the next OpenGL part I would like to focus on `WGL_ARB_pixel_format`. I was thinking that if a driver implements `wglGetPixelFormatAttribivARB` then it would be possible, in theory at least, to have a default implementation of `wglChoosePixelFormatARB` (and `wglGetPixelFormatAttribfvARB`) in `opengl32`. I am tempted to do this, instead of adding one more driver specific full implementation, although I am not yet sure if there any driver specific considerations or behaviors that would pose issues with such approach. Thoughts? -- v2: winewayland.drv: Implement wgl(Get)SwapIntervalEXT. winewayland.drv: Implement wglShareLists. winewayland.drv: Implement wglCreateContextAttribsARB. https://gitlab.winehq.org/wine/wine/-/merge_requests/5264
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/winewayland.drv/opengl.c | 102 ++++++++++++++++++++++++++++++++-- 1 file changed, 98 insertions(+), 4 deletions(-) diff --git a/dlls/winewayland.drv/opengl.c b/dlls/winewayland.drv/opengl.c index 4692d2f1e15..61c3eae8e40 100644 --- a/dlls/winewayland.drv/opengl.c +++ b/dlls/winewayland.drv/opengl.c @@ -24,6 +24,7 @@ #include "config.h" +#include <assert.h> #include <dlfcn.h> #include <stdlib.h> #include <string.h> @@ -92,6 +93,7 @@ struct wgl_context EGLConfig config; EGLContext context; struct wayland_gl_drawable *draw, *read, *new_draw, *new_read; + EGLint attribs[16]; }; /* lookup the existing drawable for a window, gl_object_mutex must be held */ @@ -320,6 +322,68 @@ static BOOL wgl_context_make_current(struct wgl_context *ctx, HWND draw_hwnd, return ret; } +static BOOL wgl_context_populate_attribs(struct wgl_context *ctx, const int *wgl_attribs) +{ + EGLint *attribs_end = ctx->attribs; + + if (!wgl_attribs) goto out; + + for (; wgl_attribs[0] != 0; wgl_attribs += 2) + { + EGLint name; + + TRACE("%#x %#x\n", wgl_attribs[0], wgl_attribs[1]); + + /* Find the EGL attribute names corresponding to the WGL names. + * For all of the attributes below, the values match between the two + * systems, so we can use them directly. */ + switch (wgl_attribs[0]) + { + case WGL_CONTEXT_MAJOR_VERSION_ARB: + name = EGL_CONTEXT_MAJOR_VERSION_KHR; + break; + case WGL_CONTEXT_MINOR_VERSION_ARB: + name = EGL_CONTEXT_MINOR_VERSION_KHR; + break; + case WGL_CONTEXT_FLAGS_ARB: + name = EGL_CONTEXT_FLAGS_KHR; + break; + case WGL_CONTEXT_OPENGL_NO_ERROR_ARB: + name = EGL_CONTEXT_OPENGL_NO_ERROR_KHR; + break; + case WGL_CONTEXT_PROFILE_MASK_ARB: + if (wgl_attribs[1] & WGL_CONTEXT_ES2_PROFILE_BIT_EXT) + { + ERR("OpenGL ES contexts are not supported\n"); + return FALSE; + } + name = EGL_CONTEXT_OPENGL_PROFILE_MASK_KHR; + break; + default: + name = EGL_NONE; + FIXME("Unhandled attributes: %#x %#x\n", wgl_attribs[0], wgl_attribs[1]); + } + + if (name != EGL_NONE) + { + EGLint *dst = ctx->attribs; + /* Check if we have already set the same attribute and replace it. */ + for (; dst != attribs_end && *dst != name; dst += 2) continue; + /* Our context attribute array should have enough space for all the + * attributes we support (we merge repetitions), plus EGL_NONE. */ + assert(dst - ctx->attribs <= ARRAY_SIZE(ctx->attribs) - 3); + dst[0] = name; + dst[1] = wgl_attribs[1]; + if (dst == attribs_end) attribs_end += 2; + } + } + +out: + *attribs_end = EGL_NONE; + return TRUE; +} + + static void wgl_context_refresh(struct wgl_context *ctx) { BOOL refresh = FALSE; @@ -381,7 +445,8 @@ static BOOL set_pixel_format(HDC hdc, int format, BOOL internal) return TRUE; } -static struct wgl_context *create_context(HDC hdc) +static struct wgl_context *create_context(HDC hdc, struct wgl_context *share, + const int *attribs) { struct wayland_gl_drawable *gl; struct wgl_context *ctx; @@ -394,8 +459,23 @@ static struct wgl_context *create_context(HDC hdc) goto out; } + if (!wgl_context_populate_attribs(ctx, attribs)) + { + ctx->attribs[0] = EGL_NONE; + goto out; + } + + /* For now only OpenGL is supported. It's enough to set the API only for + * context creation, since: + * 1. the default API is EGL_OPENGL_ES_API + * 2. the EGL specification says in section 3.7: + * > EGL_OPENGL_API and EGL_OPENGL_ES_API are interchangeable for all + * > purposes except eglCreateContext. + */ + p_eglBindAPI(EGL_OPENGL_API); ctx->context = p_eglCreateContext(egl_display, EGL_NO_CONFIG_KHR, - EGL_NO_CONTEXT, NULL); + share ? share->context : EGL_NO_CONTEXT, + ctx->attribs); pthread_mutex_lock(&gl_object_mutex); list_add_head(&gl_contexts, &ctx->entry); @@ -427,8 +507,15 @@ static BOOL wayland_wglCopyContext(struct wgl_context *src, static struct wgl_context *wayland_wglCreateContext(HDC hdc) { TRACE("hdc=%p\n", hdc); - p_eglBindAPI(EGL_OPENGL_API); - return create_context(hdc); + return create_context(hdc, NULL, NULL); +} + +static struct wgl_context *wayland_wglCreateContextAttribsARB(HDC hdc, + struct wgl_context *share, + const int *attribs) +{ + TRACE("hdc=%p share=%p attribs=%p\n", hdc, share, attribs); + return create_context(hdc, share, attribs); } static BOOL wayland_wglDeleteContext(struct wgl_context *ctx) @@ -623,6 +710,11 @@ static BOOL init_opengl_funcs(void) opengl_funcs.ext.p_wglGetCurrentReadDCARB = (void *)1; /* never called */ opengl_funcs.ext.p_wglMakeContextCurrentARB = wayland_wglMakeContextCurrentARB; + register_extension("WGL_ARB_create_context"); + register_extension("WGL_ARB_create_context_no_error"); + register_extension("WGL_ARB_create_context_profile"); + opengl_funcs.ext.p_wglCreateContextAttribsARB = wayland_wglCreateContextAttribsARB; + return TRUE; } @@ -751,6 +843,8 @@ static void init_opengl(void) if (!has_extension(egl_exts, #ext)) \ { ERR("Failed to find required extension %s\n", #ext); goto err; } \ } while(0) + REQUIRE_EXT(EGL_KHR_create_context); + REQUIRE_EXT(EGL_KHR_create_context_no_error); REQUIRE_EXT(EGL_KHR_no_config_context); #undef REQUIRE_EXT -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5264
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> EGL works similarly to GLX, in that it only allows sharing on context creation, so we adapt the approach used by winex11. --- dlls/winewayland.drv/opengl.c | 47 +++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/dlls/winewayland.drv/opengl.c b/dlls/winewayland.drv/opengl.c index 61c3eae8e40..361c5fc3302 100644 --- a/dlls/winewayland.drv/opengl.c +++ b/dlls/winewayland.drv/opengl.c @@ -94,6 +94,8 @@ struct wgl_context EGLContext context; struct wayland_gl_drawable *draw, *read, *new_draw, *new_read; EGLint attribs[16]; + BOOL has_been_current; + BOOL sharing; }; /* lookup the existing drawable for a window, gl_object_mutex must be held */ @@ -306,6 +308,7 @@ static BOOL wgl_context_make_current(struct wgl_context *ctx, HWND draw_hwnd, ctx->draw = draw; ctx->read = read; ctx->new_draw = ctx->new_read = NULL; + ctx->has_been_current = TRUE; NtCurrentTeb()->glContext = ctx; } else @@ -641,6 +644,49 @@ static BOOL wayland_wglSetPixelFormatWINE(HDC hdc, int format) return set_pixel_format(hdc, format, TRUE); } +static BOOL wayland_wglShareLists(struct wgl_context *orig, struct wgl_context *dest) +{ + struct wgl_context *keep, *clobber; + + TRACE("(%p, %p)\n", orig, dest); + + /* Sharing of display lists works differently in EGL and WGL. In case of EGL + * it is done at context creation time but in case of WGL it is done using + * wglShareLists. We create an EGL context in wglCreateContext / + * wglCreateContextAttribsARB and when a program requests sharing we + * recreate the destination or source context if it hasn't been made current + * and it hasn't shared display lists before. */ + + if (!dest->has_been_current && !dest->sharing) + { + keep = orig; + clobber = dest; + } + else if (!orig->has_been_current && !orig->sharing) + { + keep = dest; + clobber = orig; + } + else + { + ERR("Could not share display lists because both of the contexts have " + "already been current or shared\n"); + return FALSE; + } + + p_eglDestroyContext(egl_display, clobber->context); + clobber->context = p_eglCreateContext(egl_display, EGL_NO_CONFIG_KHR, + keep->context, clobber->attribs); + TRACE("re-created context (%p) for Wine context %p (%p) " + "sharing lists with ctx %p (%p)\n", + clobber->context, clobber, clobber->config, + keep->context, keep->config); + + orig->sharing = TRUE; + dest->sharing = TRUE; + return TRUE; +} + static BOOL wayland_wglSwapBuffers(HDC hdc) { struct wgl_context *ctx = NtCurrentTeb()->glContext; @@ -876,6 +922,7 @@ static struct opengl_funcs opengl_funcs = .p_wglGetProcAddress = wayland_wglGetProcAddress, .p_wglMakeCurrent = wayland_wglMakeCurrent, .p_wglSetPixelFormat = wayland_wglSetPixelFormat, + .p_wglShareLists = wayland_wglShareLists, .p_wglSwapBuffers = wayland_wglSwapBuffers, } }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5264
From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com> --- dlls/winewayland.drv/opengl.c | 66 +++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/dlls/winewayland.drv/opengl.c b/dlls/winewayland.drv/opengl.c index 361c5fc3302..f072b8063a1 100644 --- a/dlls/winewayland.drv/opengl.c +++ b/dlls/winewayland.drv/opengl.c @@ -69,6 +69,7 @@ DECL_FUNCPTR(eglInitialize); DECL_FUNCPTR(eglMakeCurrent); DECL_FUNCPTR(eglQueryString); DECL_FUNCPTR(eglSwapBuffers); +DECL_FUNCPTR(eglSwapInterval); DECL_FUNCPTR(glClear); #undef DECL_FUNCPTR @@ -85,6 +86,7 @@ struct wayland_gl_drawable struct wl_egl_window *wl_egl_window; EGLSurface surface; LONG resized; + int swap_interval; }; struct wgl_context @@ -157,6 +159,7 @@ static struct wayland_gl_drawable *wayland_gl_drawable_create(HWND hwnd, int for gl->ref = 1; gl->hwnd = hwnd; + gl->swap_interval = 1; /* Get the client surface for the HWND. If don't have a wayland surface * (e.g., HWND_MESSAGE windows) just create a dummy surface to act as the @@ -227,7 +230,11 @@ static void wayland_update_gl_drawable(HWND hwnd, struct wayland_gl_drawable *ne if ((old = find_drawable_for_hwnd(hwnd))) list_remove(&old->entry); if (new) list_add_head(&gl_drawables, &new->entry); - if (old && new) update_context_drawables(new, old); + if (old && new) + { + update_context_drawables(new, old); + new->swap_interval = old->swap_interval; + } pthread_mutex_unlock(&gl_object_mutex); @@ -408,7 +415,11 @@ static void wgl_context_refresh(struct wgl_context *ctx) ctx->new_read = NULL; refresh = TRUE; } - if (refresh) p_eglMakeCurrent(egl_display, ctx->draw, ctx->read, ctx->context); + if (refresh) + { + p_eglMakeCurrent(egl_display, ctx->draw, ctx->read, ctx->context); + if (ctx->draw) p_eglSwapInterval(egl_display, ctx->draw->swap_interval); + } pthread_mutex_unlock(&gl_object_mutex); @@ -608,6 +619,21 @@ static PROC wayland_wglGetProcAddress(LPCSTR name) return (PROC)p_eglGetProcAddress(name); } +static int wayland_wglGetSwapIntervalEXT(void) +{ + struct wgl_context *ctx = NtCurrentTeb()->glContext; + + if (!ctx || !ctx->draw) + { + WARN("No GL drawable found, returning swap interval 0\n"); + return 0; + } + + /* It's safe to read the value without a lock, since only + * the current thread can write to it. */ + return ctx->draw->swap_interval; +} + static BOOL wayland_wglMakeContextCurrentARB(HDC draw_hdc, HDC read_hdc, struct wgl_context *ctx) { @@ -705,6 +731,37 @@ static BOOL wayland_wglSwapBuffers(HDC hdc) return TRUE; } +static BOOL wayland_wglSwapIntervalEXT(int interval) +{ + struct wgl_context *ctx = NtCurrentTeb()->glContext; + BOOL ret; + + TRACE("(%d)\n", interval); + + if (interval < 0) + { + RtlSetLastWin32Error(ERROR_INVALID_DATA); + return FALSE; + } + + if (!ctx || !ctx->draw) + { + RtlSetLastWin32Error(ERROR_DC_NOT_FOUND); + return FALSE; + } + + /* Lock to protect against concurrent access to drawable swap_interval + * from wayland_update_gl_drawable */ + pthread_mutex_lock(&gl_object_mutex); + if ((ret = p_eglSwapInterval(egl_display, interval))) + ctx->draw->swap_interval = interval; + else + RtlSetLastWin32Error(ERROR_DC_NOT_FOUND); + pthread_mutex_unlock(&gl_object_mutex); + + return ret; +} + static BOOL has_extension(const char *list, const char *ext) { size_t len = strlen(ext); @@ -761,6 +818,10 @@ static BOOL init_opengl_funcs(void) register_extension("WGL_ARB_create_context_profile"); opengl_funcs.ext.p_wglCreateContextAttribsARB = wayland_wglCreateContextAttribsARB; + register_extension("WGL_EXT_swap_control"); + opengl_funcs.ext.p_wglGetSwapIntervalEXT = wayland_wglGetSwapIntervalEXT; + opengl_funcs.ext.p_wglSwapIntervalEXT = wayland_wglSwapIntervalEXT; + return TRUE; } @@ -866,6 +927,7 @@ static void init_opengl(void) LOAD_FUNCPTR_EGL(eglInitialize); LOAD_FUNCPTR_EGL(eglMakeCurrent); LOAD_FUNCPTR_EGL(eglSwapBuffers); + LOAD_FUNCPTR_EGL(eglSwapInterval); #undef LOAD_FUNCPTR_EGL egl_display = p_eglGetPlatformDisplay(EGL_PLATFORM_WAYLAND_KHR, -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5264
On Tue Mar 12 11:48:47 2024 +0000, Alexandros Frantzis wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/5264/diffs?diff_id=104580&start_sha=348d665a60f157ea1e7a9192ccffca56a1e8de7d#2fc4f8acb340ee1b9eb3fadf8b3568bd11af1989_673_673) Looking at the codebase there are three different naming schemes for the arguments (I guess thanks to Windows not naming them at all):
`(org, dest)` `(share, source)` `(src, dst)` They all try to convey that the first argument is the one that contains the display lists to share, and the second one will be the recipient of the shared display lists (and also according to MSDN it shouldn't contain any existing display lists).
From the above it seems to me that `d(e)st` is a more fitting name for the second argument. Either `src` or `orig(in)` is a good name for the first one.
I have gone with your first suggestion `(orig, dest)`. WDYT? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64375
On Fri Mar 8 18:22:26 2024 +0000, Rémi Bernon wrote:
Ah yeah I forgot about that before but isn't something that's supposed to be called on every thread that is going to issue GL calls? Shouldn't we do that in wglMakeCurrent? `eglBindAPI` affects the result of certain EGL calls, and only indirectly affects the client APIs (GL etc) themselves (through the current context).
The EGL specifications (>= 1.4) say in section 3.7:
EGL_OPENGL_API and EGL_OPENGL_ES_API are interchangeable for all purposes except eglCreateContext.
This is elaborated further in the documentation of various affected EGL functions listed in the spec. The ones relevant to us at the moment are: * `eglCreateContext`: we call eglBindAPI before this one, so we are covered, i.e., we always create OpenGL contexts * `eglMakeCurrent`: The spec says: > For purposes of eglMakeCurrent, the client API type of all OpenGL ES and OpenGL contexts is considered the same. In other words, if any OpenGL ES context is currently bound and context is an OpenGL context, or if any OpenGL context is currently bound and context is an OpenGL ES context, the currently bound context will be made no longer current and context will be made current. Since the default API is OpenGL ES, it's fine to make an OpenGL context current even if the thread hasn't explicitly called `eglBindAPI(EGL_OPENGL_API)`. * `eglSwapInterval`: This works on the draw surface of the current context, so the considerations of `eglMakeCurrent` apply (i.e., there is a single current context for both OpenGL and OpenGL ES). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64376
On Tue Mar 12 11:48:46 2024 +0000, Alexandros Frantzis wrote:
changed this line in [version 2 of the diff](/wine/wine/-/merge_requests/5264/diffs?diff_id=104580&start_sha=348d665a60f157ea1e7a9192ccffca56a1e8de7d#2fc4f8acb340ee1b9eb3fadf8b3568bd11af1989_342_337) Done, but also renamed the parameter to `wgl_attribs` to avoid confusion.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64377
On Fri Mar 8 18:22:29 2024 +0000, Rémi Bernon wrote:
Any reason for having that flag? Could we just call `p_eglSwapInterval(egl_display, ctx->draw->swap_interval);` withing the `if (refresh)`? The idea was to avoid an unnecessary `eglSwapInterval` call, but it's an inconsequential optimization, so I have removed the flag.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64378
On Fri Mar 8 18:22:30 2024 +0000, Rémi Bernon wrote:
Not sure to see why you need the lock here then if it's indeed safe to read it without a lock. It's only safe to read the `ctx->draw->swap_interval` without a lock from the thread that the context is current in (because that's the only thread that can write to `ctx->draw` and `ctx->draw->swap_interval`).
This lock is to protect the read of `old->swap_interval` in `wayland_update_gl_drawable` which can happen from another thread. That is, one thread updates the drawable for a window, while another does SwapInterval on that soon-to-be-old drawable. This lock was more important in the version with the refresh flag. I guess you could argue that since this is an inherently racy situation anyway (in terms of whether the update or swap interval happens first), we could now skip the lock and just make sure `ctx->swap_interval` is accessed atomically, although I don't think this is much of win in terms of clarity. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64379
On Fri Mar 8 18:22:28 2024 +0000, Rémi Bernon wrote:
What about moving that to update_context_drawables? I chose to leave it out since this is a separate operation on the old/new drawables, not directly related to contexts (`ctx->*`) per-se.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64380
v2: * Remove `refresh_swap_interval` flag and checks. * Rename `wglShareLists` parameter from `org` to `orig`. * Rename variables in `wgl_context_populate_attribs`. * Add comments to clarify some aspects of the code that came up during review. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64381
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5264
participants (3)
-
Alexandros Frantzis -
Alexandros Frantzis (＠afrantzis)
-
Rémi Bernon