[PATCH v2 0/2] MR7094: winex11.drv: Flush before blitting offscreen drawable in wglFlush() and wglFinish().
-- v2: winex11.drv: Call glFinish() when presenting offscreen drawable from wglFlush. winex11.drv: Flush display when presenting offcreen drawable from wglFlush / wglFinish.
From: Paul Gofman pgofman@codeweavers.com
--- dlls/winex11.drv/opengl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c index 594ffaea2dd..1ac3d117aab 100644 --- a/dlls/winex11.drv/opengl.c +++ b/dlls/winex11.drv/opengl.c @@ -1929,7 +1929,7 @@ static void wglFinish(void) { sync_context(ctx); pglFinish(); - present_gl_drawable( hwnd, ctx->hdc, gl, FALSE ); + present_gl_drawable( hwnd, ctx->hdc, gl, TRUE ); release_gl_drawable( gl ); } } @@ -1945,7 +1945,7 @@ static void wglFlush(void) { sync_context(ctx); pglFlush(); - present_gl_drawable( hwnd, ctx->hdc, gl, FALSE ); + present_gl_drawable( hwnd, ctx->hdc, gl, TRUE ); release_gl_drawable( gl ); } }
From: Paul Gofman pgofman@codeweavers.com
--- dlls/winex11.drv/opengl.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c index 1ac3d117aab..f6e8def0ade 100644 --- a/dlls/winex11.drv/opengl.c +++ b/dlls/winex11.drv/opengl.c @@ -1877,7 +1877,7 @@ static BOOL glxdrv_wglShareLists(struct wgl_context *org, struct wgl_context *de return TRUE; }
-static void present_gl_drawable( HWND hwnd, HDC hdc, struct gl_drawable *gl, BOOL flush ) +static void present_gl_drawable( HWND hwnd, HDC hdc, struct gl_drawable *gl, BOOL flush, BOOL gl_finish ) { HWND toplevel = NtUserGetAncestor( hwnd, GA_ROOT ); struct x11drv_win_data *data; @@ -1896,6 +1896,7 @@ static void present_gl_drawable( HWND hwnd, HDC hdc, struct gl_drawable *gl, BOO window = get_dc_drawable( hdc, &rect ); region = get_dc_monitor_region( hwnd, hdc );
+ if (gl_finish) pglFinish(); if (flush) XFlush( gdi_display );
NtUserGetClientRect( hwnd, &rect_dst, NtUserGetWinMonitorDpi( hwnd, MDT_RAW_DPI ) ); @@ -1929,7 +1930,7 @@ static void wglFinish(void) { sync_context(ctx); pglFinish(); - present_gl_drawable( hwnd, ctx->hdc, gl, TRUE ); + present_gl_drawable( hwnd, ctx->hdc, gl, TRUE, FALSE ); release_gl_drawable( gl ); } } @@ -1945,7 +1946,7 @@ static void wglFlush(void) { sync_context(ctx); pglFlush(); - present_gl_drawable( hwnd, ctx->hdc, gl, TRUE ); + present_gl_drawable( hwnd, ctx->hdc, gl, TRUE, TRUE ); release_gl_drawable( gl ); } } @@ -2867,7 +2868,7 @@ static BOOL glxdrv_wglSwapBuffers( HDC hdc ) if (ctx && drawable && pglXWaitForSbcOML) pglXWaitForSbcOML( gdi_display, gl->drawable, target_sbc, &ust, &msc, &sbc );
- present_gl_drawable( hwnd, ctx ? ctx->hdc : hdc, gl, !pglXWaitForSbcOML ); + present_gl_drawable( hwnd, ctx ? ctx->hdc : hdc, gl, !pglXWaitForSbcOML, FALSE ); update_gl_drawable_size( gl ); release_gl_drawable( gl ); return TRUE;
Hi,
It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated.
The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details:
The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=150754
Your paranoid android.
=== debian11b (64 bit WoW report) ===
mf: mf.c:6536: Test failed: Test 3: Unexpected hr 0xc00d36b2.
user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 0000000003DF00F4, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
v2: - split the patch in two.
On Sun Jan 5 21:49:36 2025 +0000, Elizabeth Figura wrote:
Maybe I should split that in two, one for ensuring XFlush(
gdi_display) for both wglFinish and wglFlush, and another for forcing glFinish when offscreen blit is used in glFlush? That would probably help.
glFinish() should only be called from wglFlush() if we need to blit
from offscreen drawable, and all the logic which detects / processes that is now inside present_gl_drawable(). Sorry, I don't understand? You're passing a flag to present_gl_drawable() to signal whether glFinish() should be called, so the logic is pretty clearly outside present_gl_drawable(). And as far as I can tell, the sequence isn't important; present_gl_drawable() doesn't do any GL work before glFinish() is called.
Well, the sequence is important: glFinish() should be called before NtGdiStretchBlt() in present_gl_drawable (so the GL really completes its part before we are trying to blit from its result). At the same time, calling before present_gl_drawable would inject unneeded and performance impacting glFinish on the normal path without offscreen blit.
On Mon Jan 6 16:11:53 2025 +0000, Paul Gofman wrote:
Well, the sequence is important: glFinish() should be called before NtGdiStretchBlt() in present_gl_drawable (so the GL really completes its part before we are trying to blit from its result). At the same time, calling before present_gl_drawable would inject unneeded and performance impacting glFinish on the normal path without offscreen blit.
... or otherwise it needs this logic from present_gl_drawable() to be factored out in a separate function: ``` if (!gl) return; switch (gl->type) { case DC_GL_PIXMAP_WIN: drawable = gl->pixmap; break; case DC_GL_CHILD_WIN: drawable = gl->window; break; default: drawable = 0; break; } if (!drawable) return; ```
Which would essentially mean reverting 73453e9442fea9fffcbba6bde49c0931f05579df , but it seems to me even with the extra flag avoiding repeating all of that in 3 functions is better?
On Mon Jan 6 16:14:33 2025 +0000, Paul Gofman wrote:
... or otherwise it needs this logic from present_gl_drawable() to be factored out in a separate function:
if (!gl) return; switch (gl->type) { case DC_GL_PIXMAP_WIN: drawable = gl->pixmap; break; case DC_GL_CHILD_WIN: drawable = gl->window; break; default: drawable = 0; break; } if (!drawable) return;Which would essentially mean reverting 73453e9442fea9fffcbba6bde49c0931f05579df , but it seems to me even with the extra flag avoiding repeating all of that in 3 functions is better?
Ah sorry, I missed the early return. This is why I really don't like the style of putting the body on the same line as the if...
Commit 9d95bd5f4b54a392d97f0b58db8f78d675495544 (which introduced multiple d3d devices per ddraw support) regressed Master Magistrate. The game tries to create a separate d3d device for its popup dialog windows. Previously when it was failing it worked with some fallback path. Now when that succeeds one of the issues that it hits is that while drawing on the child window updating popup dialog it does the draw only once something changed and doesn't present repeatedly. That is done by blitting to primary surface in ddraw, ddraw / wined3d update GL frontbuffer in that case and call glFlush(). The image gets blitted from offscreen client window before the actual image gets there.
Why is the correct solution to modify wglFlush()/wglFinish(), rather than modifying ddraw to do a full present?
GLX_OML_sync_control which is used in wglSwapBuffers is not that useful here because it only allows to wait for swap happen and not for onscreen present resulting from glFinish() or Flush (it allows to wait for the next monitor refresh but it doesn't guarantee that the image is actually presented). Besides, GLX_OML_sync_control is not available on Nvidia. XFlush( gdi_display ) is the fallback path already used in wglSwapBuffers when GLX_OML_sync_control is not available. Besides, we need glFinish() to wait for GL operation complete before blitting in wglFinish().
Why do we need the full glFinish()? glXWaitForSbcOML() is in theory enough—we don't actually need to wait for the whole present to be done; we just need to wait for X11 to be aware of it, so that the following blit works, which is what I understand that our usage of glXWaitForSbcOML() does. I don't have a perfect grasp on this code, but it seems suspicious that we need the full glFinish() in wglFlush() and wglFinish() but not in wglSwapBuffers()—why can't we use the same logic as we use in the latter?
Why is the correct solution to modify wglFlush()/wglFinish(), rather than modifying ddraw to do a full present?
I am not sure how that can be changed in ddraw at all, only in wined3d in theory as in this case ddraw correctly hits wined3d swapchain present path? It seems to me there is nothing wrong in ddraw or wined3d. The issue comes from child window rendering implementation in winex11.drv and is specific to winex11, I am not sure how and why that should be detected and worked around in d3d side?
Why do we need the full glFinish()? glXWaitForSbcOML() is in theory enough—we don't actually need to wait for the whole present to be done; we just need to wait for X11 to be aware of it, so that the following blit works, which is what I understand that our usage of glXWaitForSbcOML() does. I don't have a perfect grasp on this code, but it seems suspicious that we need the full glFinish() in wglFlush() and wglFinish() but not in wglSwapBuffers()—why can't we use the same logic as we use in the latter?
glXWaitForSbcOML() waits for sbc value change (Swap Buffer Counter, see [1]). Swap doesn't happen on glFlush / glFinish and this counter is not incremented. So for wglSwapBuffers() glXWaitForSbcOML() waits until the buffer is actually swapped (implying that preceding GL drawing is complete), which is not possible on glFlush/Finish. The fallback code (working on Nvidia) in wglSwapBuffer doesn't do neither glFlush nor glFinish, I am not immediately sure if that is and oversight or glxSwapBuffer actually does glFlush and waits for current GL drawing to backbuffer to finish (likely the latter although I don't see it in spec), but we do need to GL pipeline to complete before blitting in wglFlush if we want the latest image to appear on screen, otherwise nothing is going to wait for that (and removing that glFinish in the problematic game reintroduces the issue).
1. https://registry.khronos.org/OpenGL/extensions/OML/GLX_OML_sync_control.txt
WRT wglSwapBuffers, I recall now, the same problem is actually there in practice unless GL swap_interval is set to 0, maybe we should do glFinish there as well before blitting but that looks not fully related and so far I don't know that it is regressive.
If by full presentation in ddraw you mean swapping instead of drawing to frontbuffer it seems to me it is not quite trivial at least, if possible. Such a present (by blitting to primary surface) often updates only the part of the surface (vs ddraw Flip which swaps the backbuffer). It is possible that primary surface doesn't have backbuffer at all, also such an added swap should care about preserving untouched parts of frontbuffer. I. e., it looks to me there are good reasons why it is done this way in ddraw / wined3d now.
Why is the correct solution to modify wglFlush()/wglFinish(), rather than modifying ddraw to do a full present?
I am not sure how that can be changed in ddraw at all, only in wined3d in theory as in this case ddraw correctly hits wined3d swapchain present path? It seems to me there is nothing wrong in ddraw or wined3d. The issue comes from child window rendering implementation in winex11.drv and is specific to winex11, I am not sure how and why that should be detected and worked around in d3d side?
Yes, or wined3d, I'm using metonymy to some degree. The point is, if what we need is for the image to get on screen, should we be actually presenting it, rather than just flushing?
E.g. have we tested that wglFlush() is sufficient to get an image on screen with a bare GL application?
E.g. have we tested that wglFlush() is sufficient to get an image on screen with a bare GL application?
Yes, it certainly is sufficient with bare GL when blitting / drawing to front buffer, it is just it doesn't happen immediately / before glFlush return. And it seems to me (see the message above) it is quite non trivial at least (even if possible) to replace ddraw blits to frontbuffer with a swap (present).
E.g. have we tested that wglFlush() is sufficient to get an image on screen with a bare GL application?
Yes, it certainly is sufficient with bare GL when blitting / drawing to front buffer, it is just it doesn't happen immediately / before glFlush return. And it seems to me (see the message above) it is quite non trivial at least (even if possible) to replace ddraw blits to frontbuffer with a swap (present).
Okay, I suppose that's good enough then. I'd feel more comfortable with Henri's blessing but I guess this looks correct to me as well as I understand the code.
This merge request was approved by Elizabeth Figura.
-
Elizabeth Figura (＠zfigura) -
Marvin -
Paul Gofman
-
Paul Gofman (＠gofman)