[PATCH v3 0/9] MR4533: winex11: Detach / attach vulkan window surfaces as needed.

List overview All Threads
Download

newer

older

[PATCH v2 0/1] MR5157: evr/dshow:...

[PATCH v4 0/1] MR2274: ntdll: Add...

Rémi Bernon (＠rbernon)

28 Nov 2023 28 Nov '23

2:41 p.m.

This is some preparatory work for vulkan child window and other features which currently live in Proton. I think this could be upstream already, and it will make rebasing patches on top of it simpler. To get vulkan child window upstream we'd probably still need to rework it a bit and maybe try to move the wait and blit to `vkQueuePresent` somehow. The idea here is also that although Vulkan spec does not allow multiple active swapchains for the same window, Windows tends to allow it especially with D3D, and we also need to support the case where multiple APIs are use to draw onto the same window surface (as we implement D3D on top of GL/Vk for instance). -- v3: winex11: Rename X11DRV_FLUSH_GL_DRAWABLE to X11DRV_PRESENT_DRAWABLE. winex11: Put detached client windows surfaces offscreen. winex11: Re-attach vulkan surfaces when images are acquired. winevulkan: Add vkAcquireNextImage(2)KHR driver entries. winex11: Resize detached vulkan surfaces when window is resized. winex11: Detach the vulkan surfaces when their HWND is destroyed. winex11: Detach vulkan client windows before destroying them. winex11: Introduce a new detach_client_window helper. winex11: Add traces to vulkan surface grab / release. https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Show replies by date

Rémi Bernon

28 Nov 28 Nov

2:41 p.m.

New subject: [PATCH v3 1/9] winex11: Add traces to vulkan surface grab / release.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/vulkan.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index 101504a7887..f8987e2c49d 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -196,16 +196,18 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo return VK_SUCCESS; } -static struct wine_vk_surface *wine_vk_surface_grab(struct wine_vk_surface *surface) +static struct wine_vk_surface *wine_vk_surface_grab( struct wine_vk_surface *surface ) { - InterlockedIncrement(&surface->ref); + int refcount = InterlockedIncrement( &surface->ref ); + TRACE( "surface %p, refcount %d.\n", surface, refcount ); return surface; } -static void wine_vk_surface_release(struct wine_vk_surface *surface) +static void wine_vk_surface_release( struct wine_vk_surface *surface ) { - if (InterlockedDecrement(&surface->ref)) - return; + int refcount = InterlockedDecrement( &surface->ref ); + TRACE( "surface %p, refcount %d.\n", surface, refcount ); + if (refcount) return; if (surface->entry.next) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon

2:41 p.m.

New subject: [PATCH v3 2/9] winex11: Introduce a new detach_client_window helper.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/window.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 53982bb8c3b..4237e92990e 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1549,6 +1549,24 @@ Window get_dummy_parent(void) } +/********************************************************************** + * detach_client_window + */ +static void detach_client_window( struct x11drv_win_data *data, Window window ) +{ + if (!data->client_window || data->client_window != window) return; + + XSelectInput( data->display, data->client_window, 0 ); + XFlush( data->display ); /* make sure XSelectInput is disabled for client_window after this point */ + XDeleteContext( data->display, data->client_window, winContext ); + + XReparentWindow( gdi_display, data->client_window, get_dummy_parent(), 0, 0 ); + TRACE( "%p/%lx detached client window %lx\n", data->hwnd, data->whole_window, data->client_window ); + + data->client_window = 0; +} + + /********************************************************************** * create_dummy_client_window */ @@ -1588,12 +1606,7 @@ Window create_client_window( HWND hwnd, const XVisualInfo *visual ) data->window_rect = data->whole_rect = data->client_rect; } - if (data->client_window) - { - XDeleteContext( data->display, data->client_window, winContext ); - XReparentWindow( gdi_display, data->client_window, dummy_parent, 0, 0 ); - TRACE( "%p reparent xwin %lx/%lx\n", data->hwnd, data->whole_window, data->client_window ); - } + detach_client_window( data, data->client_window ); if (data->client_colormap) XFreeColormap( gdi_display, data->client_colormap ); data->client_colormap = XCreateColormap( gdi_display, dummy_parent, visual->visual, @@ -1731,12 +1744,7 @@ static void destroy_whole_window( struct x11drv_win_data *data, BOOL already_des } else { - if (data->client_window && !already_destroyed) - { - XSelectInput( data->display, data->client_window, 0 ); - XFlush( data->display ); /* make sure XSelectInput doesn't use client_window after this point */ - XReparentWindow( gdi_display, data->client_window, get_dummy_parent(), 0, 0 ); - } + if (!already_destroyed) detach_client_window( data, data->client_window ); XDeleteContext( data->display, data->whole_window, winContext ); if (!already_destroyed) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon

2:41 p.m.

New subject: [PATCH v3 3/9] winex11: Detach vulkan client windows before destroying them.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/vulkan.c | 12 +++++++++++- dlls/winex11.drv/window.c | 4 ++-- dlls/winex11.drv/x11drv.h | 1 + 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index f8987e2c49d..6597771c4c6 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -217,7 +217,17 @@ static void wine_vk_surface_release( struct wine_vk_surface *surface ) } if (surface->window) - XDestroyWindow(gdi_display, surface->window); + { + struct x11drv_win_data *data; + + if ((data = get_win_data( surface->hwnd ))) + { + detach_client_window( data, surface->window ); + release_win_data( data ); + } + + XDestroyWindow( gdi_display, surface->window ); + } free(surface); } diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 4237e92990e..d38035e2042 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1552,9 +1552,9 @@ Window get_dummy_parent(void) /********************************************************************** * detach_client_window */ -static void detach_client_window( struct x11drv_win_data *data, Window window ) +void detach_client_window( struct x11drv_win_data *data, Window window ) { - if (!data->client_window || data->client_window != window) return; + if (data->client_window != window || !data->client_window) return; XSelectInput( data->display, data->client_window, 0 ); XFlush( data->display ); /* make sure XSelectInput is disabled for client_window after this point */ diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 2c15a95dfab..dc14d5ff372 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -657,6 +657,7 @@ extern void update_net_wm_states( struct x11drv_win_data *data ) DECLSPEC_HIDDEN extern void make_window_embedded( struct x11drv_win_data *data ) DECLSPEC_HIDDEN; extern Window create_dummy_client_window(void) DECLSPEC_HIDDEN; extern Window create_client_window( HWND hwnd, const XVisualInfo *visual ) DECLSPEC_HIDDEN; +extern void detach_client_window( struct x11drv_win_data *data, Window window ); extern void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BOOL use_alpha ) DECLSPEC_HIDDEN; extern void change_systray_owner( Display *display, Window systray_window ) DECLSPEC_HIDDEN; extern HWND create_foreign_window( Display *display, Window window ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon

2:41 p.m.

New subject: [PATCH v3 4/9] winex11: Detach the vulkan surfaces when their HWND is destroyed.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/vulkan.c | 59 ++++++++++++++++++--------------------- dlls/winex11.drv/window.c | 2 +- dlls/winex11.drv/x11drv.h | 2 +- 3 files changed, 29 insertions(+), 34 deletions(-) diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index 6597771c4c6..005a429f3df 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -49,8 +49,6 @@ WINE_DECLARE_DEBUG_CHANNEL(fps); static pthread_mutex_t vulkan_mutex; -static XContext vulkan_hwnd_context; - #define VK_STRUCTURE_TYPE_XLIB_SURFACE_CREATE_INFO_KHR 1000004000 static struct list surface_list = LIST_INIT( surface_list ); @@ -140,7 +138,6 @@ static void wine_vk_init(void) #undef LOAD_FUNCPTR #undef LOAD_OPTIONAL_FUNCPTR - vulkan_hwnd_context = XUniqueContext(); return; fail: @@ -196,13 +193,6 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo return VK_SUCCESS; } -static struct wine_vk_surface *wine_vk_surface_grab( struct wine_vk_surface *surface ) -{ - int refcount = InterlockedIncrement( &surface->ref ); - TRACE( "surface %p, refcount %d.\n", surface, refcount ); - return surface; -} - static void wine_vk_surface_release( struct wine_vk_surface *surface ) { int refcount = InterlockedDecrement( &surface->ref ); @@ -232,18 +222,33 @@ static void wine_vk_surface_release( struct wine_vk_surface *surface ) free(surface); } -void wine_vk_surface_destroy(HWND hwnd) +static void wine_vk_surface_detach( struct wine_vk_surface *surface ) { - struct wine_vk_surface *surface; - pthread_mutex_lock(&vulkan_mutex); - if (!XFindContext(gdi_display, (XID)hwnd, vulkan_hwnd_context, (char **)&surface)) + struct x11drv_win_data *data; + + TRACE( "Detaching surface %p, hwnd %p.\n", surface, surface->hwnd ); + + if (surface->window && (data = get_win_data( surface->hwnd ))) { - surface->hwnd_thread_id = 0; - surface->hwnd = NULL; - wine_vk_surface_release(surface); + detach_client_window( data, surface->window ); + release_win_data( data ); } - XDeleteContext(gdi_display, (XID)hwnd, vulkan_hwnd_context); - pthread_mutex_unlock(&vulkan_mutex); + + surface->hwnd_thread_id = 0; + surface->hwnd = 0; +} + +void destroy_vk_surface( HWND hwnd ) +{ + struct wine_vk_surface *surface, *next; + + pthread_mutex_lock( &vulkan_mutex ); + LIST_FOR_EACH_ENTRY_SAFE( surface, next, &surface_list, struct wine_vk_surface, entry ) + { + if (surface->hwnd != hwnd) continue; + wine_vk_surface_detach( surface ); + } + pthread_mutex_unlock( &vulkan_mutex ); } void vulkan_thread_detach(void) @@ -254,13 +259,8 @@ void vulkan_thread_detach(void) pthread_mutex_lock(&vulkan_mutex); LIST_FOR_EACH_ENTRY_SAFE(surface, next, &surface_list, struct wine_vk_surface, entry) { - if (surface->hwnd_thread_id != thread_id) - continue; - - TRACE("Detaching surface %p, hwnd %p.\n", surface, surface->hwnd); - XReparentWindow(gdi_display, surface->window, get_dummy_parent(), 0, 0); - XSync(gdi_display, False); - wine_vk_surface_destroy(surface->hwnd); + if (surface->hwnd_thread_id != thread_id) continue; + wine_vk_surface_detach( surface ); } pthread_mutex_unlock(&vulkan_mutex); } @@ -371,11 +371,6 @@ static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance, } pthread_mutex_lock(&vulkan_mutex); - if (x11_surface->hwnd) - { - wine_vk_surface_destroy( x11_surface->hwnd ); - XSaveContext(gdi_display, (XID)create_info->hwnd, vulkan_hwnd_context, (char *)wine_vk_surface_grab(x11_surface)); - } list_add_tail(&surface_list, &x11_surface->entry); pthread_mutex_unlock(&vulkan_mutex); @@ -754,7 +749,7 @@ const struct vulkan_funcs *get_vulkan_driver(UINT version) return NULL; } -void wine_vk_surface_destroy(HWND hwnd) +void destroy_vk_surface( HWND hwnd ) { } diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index d38035e2042..96fb90e0cec 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1867,7 +1867,7 @@ void X11DRV_DestroyWindow( HWND hwnd ) release_win_data( data ); free( data ); destroy_gl_drawable( hwnd ); - wine_vk_surface_destroy( hwnd ); + destroy_vk_surface( hwnd ); } diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index dc14d5ff372..caebb115e74 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -646,7 +646,7 @@ extern Window get_dummy_parent(void) DECLSPEC_HIDDEN; extern void sync_gl_drawable( HWND hwnd, BOOL known_child ) DECLSPEC_HIDDEN; extern void set_gl_drawable_parent( HWND hwnd, HWND parent ) DECLSPEC_HIDDEN; extern void destroy_gl_drawable( HWND hwnd ) DECLSPEC_HIDDEN; -extern void wine_vk_surface_destroy( HWND hwnd ) DECLSPEC_HIDDEN; +extern void destroy_vk_surface( HWND hwnd ) DECLSPEC_HIDDEN; extern void vulkan_thread_detach(void) DECLSPEC_HIDDEN; extern void wait_for_withdrawn_state( HWND hwnd, BOOL set ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon

2:41 p.m.

New subject: [PATCH v3 5/9] winex11: Resize detached vulkan surfaces when window is resized.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/vulkan.c | 18 ++++++++++++++++++ dlls/winex11.drv/window.c | 1 + dlls/winex11.drv/x11drv.h | 1 + 3 files changed, 20 insertions(+) diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index 005a429f3df..635a90d9689 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -251,6 +251,20 @@ void destroy_vk_surface( HWND hwnd ) pthread_mutex_unlock( &vulkan_mutex ); } +void resize_vk_surfaces( HWND hwnd, Window active, int mask, XWindowChanges *changes ) +{ + struct wine_vk_surface *surface; + + pthread_mutex_lock( &vulkan_mutex ); + LIST_FOR_EACH_ENTRY( surface, &surface_list, struct wine_vk_surface, entry ) + { + if (surface->hwnd != hwnd) continue; + if (!surface->window || surface->window == active) continue; + XConfigureWindow( gdi_display, surface->window, mask, changes ); + } + pthread_mutex_unlock( &vulkan_mutex ); +} + void vulkan_thread_detach(void) { struct wine_vk_surface *surface, *next; @@ -753,6 +767,10 @@ void destroy_vk_surface( HWND hwnd ) { } +void resize_vk_surfaces( HWND hwnd, Window active, int mask, XWindowChanges changes ) +{ +} + void vulkan_thread_detach(void) { } diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index 96fb90e0cec..b709db79b68 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1432,6 +1432,7 @@ static void sync_client_position( struct x11drv_win_data *data, TRACE( "setting client win %lx pos %d,%d,%dx%d changes=%x\n", data->client_window, changes.x, changes.y, changes.width, changes.height, mask ); XConfigureWindow( gdi_display, data->client_window, mask, &changes ); + resize_vk_surfaces( data->hwnd, data->client_window, mask, &changes ); } } diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index caebb115e74..a6406099771 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -647,6 +647,7 @@ extern void sync_gl_drawable( HWND hwnd, BOOL known_child ) DECLSPEC_HIDDEN; extern void set_gl_drawable_parent( HWND hwnd, HWND parent ) DECLSPEC_HIDDEN; extern void destroy_gl_drawable( HWND hwnd ) DECLSPEC_HIDDEN; extern void destroy_vk_surface( HWND hwnd ) DECLSPEC_HIDDEN; +extern void resize_vk_surfaces( HWND hwnd, Window active, int mask, XWindowChanges *changes ); extern void vulkan_thread_detach(void) DECLSPEC_HIDDEN; extern void wait_for_withdrawn_state( HWND hwnd, BOOL set ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon

2:41 p.m.

New subject: [PATCH v3 6/9] winevulkan: Add vkAcquireNextImage(2)KHR driver entries.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winemac.drv/vulkan.c | 2 ++ dlls/winevulkan/make_vulkan | 2 ++ dlls/winex11.drv/vulkan.c | 2 ++ include/wine/vulkan_driver.h | 6 ++++++ 4 files changed, 12 insertions(+) diff --git a/dlls/winemac.drv/vulkan.c b/dlls/winemac.drv/vulkan.c index 00f5e8465ab..0c1a11af63d 100644 --- a/dlls/winemac.drv/vulkan.c +++ b/dlls/winemac.drv/vulkan.c @@ -591,6 +591,8 @@ static VkSurfaceKHR macdrv_wine_get_native_surface(VkSurfaceKHR surface) static const struct vulkan_funcs vulkan_funcs = { + NULL, + NULL, macdrv_vkCreateInstance, macdrv_vkCreateSwapchainKHR, macdrv_vkCreateWin32SurfaceKHR, diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan index 9163f543c15..17a410f81bf 100755 --- a/dlls/winevulkan/make_vulkan +++ b/dlls/winevulkan/make_vulkan @@ -236,6 +236,8 @@ FUNCTION_OVERRIDES = { "vkGetPhysicalDeviceWin32PresentationSupportKHR" : {"dispatch" : True, "driver" : True, "thunk" : ThunkType.PUBLIC}, # VK_KHR_swapchain + "vkAcquireNextImageKHR" : {"dispatch" : True, "driver" : True, "thunk" : ThunkType.PUBLIC}, + "vkAcquireNextImage2KHR": {"dispatch" : True, "driver" : True, "thunk" : ThunkType.PUBLIC}, "vkCreateSwapchainKHR" : {"dispatch" : True, "driver" : True, "thunk" : ThunkType.PUBLIC}, "vkDestroySwapchainKHR" : {"dispatch" : True, "driver" : True, "thunk" : ThunkType.PUBLIC}, "vkGetSwapchainImagesKHR": {"dispatch" : True, "driver" : True, "thunk" : ThunkType.PUBLIC}, diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index 635a90d9689..fd4c5168901 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -704,6 +704,8 @@ static VkSurfaceKHR X11DRV_wine_get_native_surface(VkSurfaceKHR surface) static const struct vulkan_funcs vulkan_funcs = { + NULL, + NULL, X11DRV_vkCreateInstance, X11DRV_vkCreateSwapchainKHR, X11DRV_vkCreateWin32SurfaceKHR, diff --git a/include/wine/vulkan_driver.h b/include/wine/vulkan_driver.h index c9ac98b252b..61cdf7c69fe 100644 --- a/include/wine/vulkan_driver.h +++ b/include/wine/vulkan_driver.h @@ -21,6 +21,8 @@ struct vulkan_funcs * needs to provide. Other function calls will be provided indirectly by dispatch * tables part of dispatchable Vulkan objects such as VkInstance or vkDevice. */ + VkResult (*p_vkAcquireNextImage2KHR)(VkDevice, const VkAcquireNextImageInfoKHR *, uint32_t *); + VkResult (*p_vkAcquireNextImageKHR)(VkDevice, VkSwapchainKHR, uint64_t, VkSemaphore, VkFence, uint32_t *); VkResult (*p_vkCreateInstance)(const VkInstanceCreateInfo *, const VkAllocationCallbacks *, VkInstance *); VkResult (*p_vkCreateSwapchainKHR)(VkDevice, const VkSwapchainCreateInfoKHR *, const VkAllocationCallbacks *, VkSwapchainKHR *); VkResult (*p_vkCreateWin32SurfaceKHR)(VkInstance, const VkWin32SurfaceCreateInfoKHR *, const VkAllocationCallbacks *, VkSurfaceKHR *); @@ -53,6 +55,10 @@ static inline void *get_vulkan_driver_device_proc_addr( name += 2; + if (!strcmp(name, "AcquireNextImage2KHR")) + return vulkan_funcs->p_vkAcquireNextImage2KHR; + if (!strcmp(name, "AcquireNextImageKHR")) + return vulkan_funcs->p_vkAcquireNextImageKHR; if (!strcmp(name, "CreateSwapchainKHR")) return vulkan_funcs->p_vkCreateSwapchainKHR; if (!strcmp(name, "DestroySwapchainKHR")) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon

2:41 p.m.

New subject: [PATCH v3 7/9] winex11: Re-attach vulkan surfaces when images are acquired.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/vulkan.c | 59 +++++++++++++++++++++++++++++++++++++-- dlls/winex11.drv/window.c | 20 +++++++++++++ dlls/winex11.drv/x11drv.h | 1 + 3 files changed, 77 insertions(+), 3 deletions(-) diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index fd4c5168901..d8ffc201afe 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -49,6 +49,8 @@ WINE_DECLARE_DEBUG_CHANNEL(fps); static pthread_mutex_t vulkan_mutex; +static XContext vulkan_swapchain_context; + #define VK_STRUCTURE_TYPE_XLIB_SURFACE_CREATE_INFO_KHR 1000004000 static struct list surface_list = LIST_INIT( surface_list ); @@ -72,6 +74,7 @@ typedef struct VkXlibSurfaceCreateInfoKHR Window window; } VkXlibSurfaceCreateInfoKHR; +static VkResult (*pvkAcquireNextImageKHR)(VkDevice, VkSwapchainKHR, uint64_t, VkSemaphore, VkFence, uint32_t *); static VkResult (*pvkCreateInstance)(const VkInstanceCreateInfo *, const VkAllocationCallbacks *, VkInstance *); static VkResult (*pvkCreateSwapchainKHR)(VkDevice, const VkSwapchainCreateInfoKHR *, const VkAllocationCallbacks *, VkSwapchainKHR *); static VkResult (*pvkCreateXlibSurfaceKHR)(VkInstance, const VkXlibSurfaceCreateInfoKHR *, const VkAllocationCallbacks *, VkSurfaceKHR *); @@ -115,6 +118,7 @@ static void wine_vk_init(void) #define LOAD_FUNCPTR(f) if (!(p##f = dlsym(vulkan_handle, #f))) goto fail #define LOAD_OPTIONAL_FUNCPTR(f) p##f = dlsym(vulkan_handle, #f) + LOAD_FUNCPTR(vkAcquireNextImageKHR); LOAD_FUNCPTR(vkCreateInstance); LOAD_FUNCPTR(vkCreateSwapchainKHR); LOAD_FUNCPTR(vkCreateXlibSurfaceKHR); @@ -138,6 +142,7 @@ static void wine_vk_init(void) #undef LOAD_FUNCPTR #undef LOAD_OPTIONAL_FUNCPTR + vulkan_swapchain_context = XUniqueContext(); return; fail: @@ -193,6 +198,13 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo return VK_SUCCESS; } +static struct wine_vk_surface *wine_vk_surface_grab( struct wine_vk_surface *surface ) +{ + int refcount = InterlockedIncrement( &surface->ref ); + TRACE( "surface %p, refcount %d.\n", surface, refcount ); + return surface; +} + static void wine_vk_surface_release( struct wine_vk_surface *surface ) { int refcount = InterlockedDecrement( &surface->ref ); @@ -312,6 +324,8 @@ static VkResult X11DRV_vkCreateSwapchainKHR(VkDevice device, { struct wine_vk_surface *x11_surface = surface_from_handle(create_info->surface); VkSwapchainCreateInfoKHR create_info_host; + VkResult result; + TRACE("%p %p %p %p\n", device, create_info, allocator, swapchain); if (allocator) @@ -323,7 +337,14 @@ static VkResult X11DRV_vkCreateSwapchainKHR(VkDevice device, create_info_host = *create_info; create_info_host.surface = x11_surface->surface; - return pvkCreateSwapchainKHR(device, &create_info_host, NULL /* allocator */, swapchain); + if ((result = pvkCreateSwapchainKHR( device, &create_info_host, NULL /* allocator */, + swapchain )) == VK_SUCCESS) + { + XSaveContext( gdi_display, (XID)(*swapchain), vulkan_swapchain_context, + (char *)wine_vk_surface_grab( x11_surface ) ); + } + + return result; } static VkResult X11DRV_vkCreateWin32SurfaceKHR(VkInstance instance, @@ -430,12 +451,17 @@ static void X11DRV_vkDestroySurfaceKHR(VkInstance instance, VkSurfaceKHR surface static void X11DRV_vkDestroySwapchainKHR(VkDevice device, VkSwapchainKHR swapchain, const VkAllocationCallbacks *allocator) { + struct wine_vk_surface *surface; + TRACE("%p, 0x%s %p\n", device, wine_dbgstr_longlong(swapchain), allocator); if (allocator) FIXME("Support for allocation callbacks not implemented yet\n"); pvkDestroySwapchainKHR(device, swapchain, NULL /* allocator */); + + if (!XFindContext( gdi_display, (XID)swapchain, vulkan_swapchain_context, (char **)&surface )) + wine_vk_surface_release( surface ); } static VkResult X11DRV_vkEnumerateInstanceExtensionProperties(const char *layer_name, @@ -661,6 +687,33 @@ static VkResult X11DRV_vkGetSwapchainImagesKHR(VkDevice device, return pvkGetSwapchainImagesKHR(device, swapchain, count, images); } +static VkResult X11DRV_vkAcquireNextImageKHR( VkDevice device, VkSwapchainKHR swapchain, uint64_t timeout, + VkSemaphore semaphore, VkFence fence, uint32_t *image_index ) +{ + struct wine_vk_surface *surface; + struct x11drv_win_data *data; + + if (XFindContext( gdi_display, (XID)swapchain, vulkan_swapchain_context, (char **)&surface )) + return VK_ERROR_SURFACE_LOST_KHR; + + if ((data = get_win_data( surface->hwnd ))) + { + attach_client_window( data, surface->window ); + release_win_data( data ); + } + + return pvkAcquireNextImageKHR( device, swapchain, timeout, semaphore, fence, image_index ); +} + +static VkResult X11DRV_vkAcquireNextImage2KHR( VkDevice device, const VkAcquireNextImageInfoKHR *acquire_info, + uint32_t *image_index ) +{ + static int once; + if (!once++) FIXME( "Emulating vkAcquireNextImage2KHR, ignoring pNext.\n" ); + return X11DRV_vkAcquireNextImageKHR( device, acquire_info->swapchain, acquire_info->timeout, + acquire_info->semaphore, acquire_info->fence, image_index ); +} + static VkResult X11DRV_vkQueuePresentKHR(VkQueue queue, const VkPresentInfoKHR *present_info) { VkResult res; @@ -704,8 +757,8 @@ static VkSurfaceKHR X11DRV_wine_get_native_surface(VkSurfaceKHR surface) static const struct vulkan_funcs vulkan_funcs = { - NULL, - NULL, + X11DRV_vkAcquireNextImage2KHR, + X11DRV_vkAcquireNextImageKHR, X11DRV_vkCreateInstance, X11DRV_vkCreateSwapchainKHR, X11DRV_vkCreateWin32SurfaceKHR, diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index b709db79b68..c968b9dff24 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -1549,6 +1549,26 @@ Window get_dummy_parent(void) return dummy_parent; } +/********************************************************************** + * attach_client_window + */ +void attach_client_window( struct x11drv_win_data *data, Window window ) +{ + if (data->client_window == window || !data->whole_window) return; + detach_client_window( data, data->client_window ); + + data->client_window = window; + + XSaveContext( data->display, data->client_window, winContext, (char *)data->hwnd ); + XSelectInput( data->display, data->client_window, ExposureMask ); + XFlush( data->display ); /* make sure XSelectInput is enabled for client_window after this point */ + + XReparentWindow( gdi_display, data->client_window, data->whole_window, + data->client_rect.left - data->whole_rect.left, + data->client_rect.top - data->whole_rect.top ); + TRACE( "%p/%lx attached client window %lx\n", data->hwnd, data->whole_window, data->client_window ); +} + /********************************************************************** * detach_client_window diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index a6406099771..82a1fb062ea 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -659,6 +659,7 @@ extern void make_window_embedded( struct x11drv_win_data *data ) DECLSPEC_HIDDEN extern Window create_dummy_client_window(void) DECLSPEC_HIDDEN; extern Window create_client_window( HWND hwnd, const XVisualInfo *visual ) DECLSPEC_HIDDEN; extern void detach_client_window( struct x11drv_win_data *data, Window window ); +extern void attach_client_window( struct x11drv_win_data *data, Window window ); extern void set_window_visual( struct x11drv_win_data *data, const XVisualInfo *vis, BOOL use_alpha ) DECLSPEC_HIDDEN; extern void change_systray_owner( Display *display, Window systray_window ) DECLSPEC_HIDDEN; extern HWND create_foreign_window( Display *display, Window window ) DECLSPEC_HIDDEN; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon

2:41 p.m.

New subject: [PATCH v3 8/9] winex11: Put detached client windows surfaces offscreen.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/window.c | 7 +++++++ dlls/winex11.drv/x11drv_main.c | 18 +++++++++++++----- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/dlls/winex11.drv/window.c b/dlls/winex11.drv/window.c index c968b9dff24..010dacd35bb 100644 --- a/dlls/winex11.drv/window.c +++ b/dlls/winex11.drv/window.c @@ -51,6 +51,7 @@ #include "wingdi.h" #include "winuser.h" +#include "xcomposite.h" #include "wine/debug.h" #include "wine/server.h" #include "mwm.h" @@ -1566,6 +1567,9 @@ void attach_client_window( struct x11drv_win_data *data, Window window ) XReparentWindow( gdi_display, data->client_window, data->whole_window, data->client_rect.left - data->whole_rect.left, data->client_rect.top - data->whole_rect.top ); +#ifdef SONAME_LIBXCOMPOSITE + if (usexcomposite) pXCompositeUnredirectWindow( gdi_display, data->client_window, CompositeRedirectManual ); +#endif TRACE( "%p/%lx attached client window %lx\n", data->hwnd, data->whole_window, data->client_window ); } @@ -1582,6 +1586,9 @@ void detach_client_window( struct x11drv_win_data *data, Window window ) XDeleteContext( data->display, data->client_window, winContext ); XReparentWindow( gdi_display, data->client_window, get_dummy_parent(), 0, 0 ); +#ifdef SONAME_LIBXCOMPOSITE + if (usexcomposite) pXCompositeRedirectWindow( gdi_display, data->client_window, CompositeRedirectManual ); +#endif TRACE( "%p/%lx detached client window %lx\n", data->hwnd, data->whole_window, data->client_window ); data->client_window = 0; diff --git a/dlls/winex11.drv/x11drv_main.c b/dlls/winex11.drv/x11drv_main.c index 32a20e0e4f2..6daa2f459b8 100644 --- a/dlls/winex11.drv/x11drv_main.c +++ b/dlls/winex11.drv/x11drv_main.c @@ -96,6 +96,7 @@ static unsigned long err_serial; /* serial number of first request * static int (*old_error_handler)( Display *, XErrorEvent * ); static BOOL use_xim = TRUE; static WCHAR input_style[20]; +static int xcomp_opcode; static pthread_mutex_t d3dkmt_mutex = PTHREAD_MUTEX_INITIALIZER; static pthread_mutex_t error_mutex = PTHREAD_MUTEX_INITIALIZER; @@ -247,6 +248,12 @@ static inline BOOL ignore_error( Display *display, XErrorEvent *event ) { if (event->error_code == xrender_error_base + BadPicture) return TRUE; } +#endif +#ifdef SONAME_LIBXCOMPOSITE + if (xcomp_opcode && event->request_code == xcomp_opcode && + (event->minor_code == X_CompositeRedirectWindow || + event->minor_code == X_CompositeUnredirectWindow)) + return TRUE; #endif } return FALSE; @@ -573,15 +580,16 @@ static void X11DRV_XComposite_Init(void) LOAD_FUNCPTR(XCompositeNameWindowPixmap); #undef LOAD_FUNCPTR - if(!pXCompositeQueryExtension(gdi_display, &xcomp_event_base, - &xcomp_error_base)) { - TRACE("XComposite extension could not be queried; disabled\n"); - dlclose(xcomposite_handle); + if (!XQueryExtension( gdi_display, "Composite", &xcomp_opcode, &xcomp_event_base, &xcomp_error_base )) + { + WARN( "XComposite extension could not be queried; disabled\n" ); + dlclose( xcomposite_handle ); xcomposite_handle = NULL; usexcomposite = FALSE; return; } - TRACE("XComposite is up and running error_base = %d\n", xcomp_error_base); + TRACE( "XComposite is up and running opcode = %d, error_base = %d, event_base %d\n", + xcomp_opcode, xcomp_error_base, xcomp_event_base ); return; sym_not_found: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon

2:41 p.m.

New subject: [PATCH v3 9/9] winex11: Rename X11DRV_FLUSH_GL_DRAWABLE to X11DRV_PRESENT_DRAWABLE.

From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/init.c | 8 ++++---- dlls/winex11.drv/opengl.c | 40 +++++++++++++++++++-------------------- dlls/winex11.drv/x11drv.h | 8 ++++---- 3 files changed, 28 insertions(+), 28 deletions(-) diff --git a/dlls/winex11.drv/init.c b/dlls/winex11.drv/init.c index c3d54da1d4d..35364884e50 100644 --- a/dlls/winex11.drv/init.c +++ b/dlls/winex11.drv/init.c @@ -243,16 +243,16 @@ static INT X11DRV_ExtEscape( PHYSDEV dev, INT escape, INT in_count, LPCVOID in_d return TRUE; } break; - case X11DRV_FLUSH_GL_DRAWABLE: - if (in_count >= sizeof(struct x11drv_escape_flush_gl_drawable)) + case X11DRV_PRESENT_DRAWABLE: + if (in_count >= sizeof(struct x11drv_escape_present_drawable)) { - const struct x11drv_escape_flush_gl_drawable *data = in_data; + const struct x11drv_escape_present_drawable *data = in_data; RECT rect = physDev->dc_rect; OffsetRect( &rect, -physDev->dc_rect.left, -physDev->dc_rect.top ); if (data->flush) XFlush( gdi_display ); XSetFunction( gdi_display, physDev->gc, GXcopy ); - XCopyArea( gdi_display, data->gl_drawable, physDev->drawable, physDev->gc, + XCopyArea( gdi_display, data->drawable, physDev->drawable, physDev->gc, 0, 0, rect.right, rect.bottom, physDev->dc_rect.left, physDev->dc_rect.top ); add_device_bounds( physDev, &rect ); diff --git a/dlls/winex11.drv/opengl.c b/dlls/winex11.drv/opengl.c index bb8f13f78b9..dd0753a624a 100644 --- a/dlls/winex11.drv/opengl.c +++ b/dlls/winex11.drv/opengl.c @@ -1980,20 +1980,20 @@ static BOOL glxdrv_wglShareLists(struct wgl_context *org, struct wgl_context *de static void wglFinish(void) { - struct x11drv_escape_flush_gl_drawable escape; + struct x11drv_escape_present_drawable escape; struct gl_drawable *gl; struct wgl_context *ctx = NtCurrentTeb()->glContext; - escape.code = X11DRV_FLUSH_GL_DRAWABLE; - escape.gl_drawable = 0; + escape.code = X11DRV_PRESENT_DRAWABLE; + escape.drawable = 0; escape.flush = FALSE; if ((gl = get_gl_drawable( NtUserWindowFromDC( ctx->hdc ), 0 ))) { switch (gl->type) { - case DC_GL_PIXMAP_WIN: escape.gl_drawable = gl->pixmap; break; - case DC_GL_CHILD_WIN: escape.gl_drawable = gl->window; break; + case DC_GL_PIXMAP_WIN: escape.drawable = gl->pixmap; break; + case DC_GL_CHILD_WIN: escape.drawable = gl->window; break; default: break; } sync_context(ctx); @@ -2001,26 +2001,26 @@ static void wglFinish(void) } pglFinish(); - if (escape.gl_drawable) + if (escape.drawable) NtGdiExtEscape( ctx->hdc, NULL, 0, X11DRV_ESCAPE, sizeof(escape), (LPSTR)&escape, 0, NULL ); } static void wglFlush(void) { - struct x11drv_escape_flush_gl_drawable escape; + struct x11drv_escape_present_drawable escape; struct gl_drawable *gl; struct wgl_context *ctx = NtCurrentTeb()->glContext; - escape.code = X11DRV_FLUSH_GL_DRAWABLE; - escape.gl_drawable = 0; + escape.code = X11DRV_PRESENT_DRAWABLE; + escape.drawable = 0; escape.flush = FALSE; if ((gl = get_gl_drawable( NtUserWindowFromDC( ctx->hdc ), 0 ))) { switch (gl->type) { - case DC_GL_PIXMAP_WIN: escape.gl_drawable = gl->pixmap; break; - case DC_GL_CHILD_WIN: escape.gl_drawable = gl->window; break; + case DC_GL_PIXMAP_WIN: escape.drawable = gl->pixmap; break; + case DC_GL_CHILD_WIN: escape.drawable = gl->window; break; default: break; } sync_context(ctx); @@ -2028,7 +2028,7 @@ static void wglFlush(void) } pglFlush(); - if (escape.gl_drawable) + if (escape.drawable) NtGdiExtEscape( ctx->hdc, NULL, 0, X11DRV_ESCAPE, sizeof(escape), (LPSTR)&escape, 0, NULL ); } @@ -3348,15 +3348,15 @@ static void X11DRV_WineGL_LoadExtensions(void) */ static BOOL glxdrv_wglSwapBuffers( HDC hdc ) { - struct x11drv_escape_flush_gl_drawable escape; + struct x11drv_escape_present_drawable escape; struct gl_drawable *gl; struct wgl_context *ctx = NtCurrentTeb()->glContext; INT64 ust, msc, sbc, target_sbc = 0; TRACE("(%p)\n", hdc); - escape.code = X11DRV_FLUSH_GL_DRAWABLE; - escape.gl_drawable = 0; + escape.code = X11DRV_PRESENT_DRAWABLE; + escape.drawable = 0; escape.flush = !pglXWaitForSbcOML; if (!(gl = get_gl_drawable( NtUserWindowFromDC( hdc ), hdc ))) @@ -3377,7 +3377,7 @@ static BOOL glxdrv_wglSwapBuffers( HDC hdc ) { case DC_GL_PIXMAP_WIN: if (ctx) sync_context( ctx ); - escape.gl_drawable = gl->pixmap; + escape.drawable = gl->pixmap; if (pglXCopySubBufferMESA) { /* (glX)SwapBuffers has an implicit glFlush effect, however * GLX_MESA_copy_sub_buffer doesn't. Make sure GL is flushed before @@ -3398,10 +3398,10 @@ static BOOL glxdrv_wglSwapBuffers( HDC hdc ) case DC_GL_WINDOW: case DC_GL_CHILD_WIN: if (ctx) sync_context( ctx ); - if (gl->type == DC_GL_CHILD_WIN) escape.gl_drawable = gl->window; + if (gl->type == DC_GL_CHILD_WIN) escape.drawable = gl->window; /* fall through */ default: - if (escape.gl_drawable && pglXSwapBuffersMscOML) + if (escape.drawable && pglXSwapBuffersMscOML) { pglFlush(); target_sbc = pglXSwapBuffersMscOML( gdi_display, gl->drawable, 0, 0, 0 ); @@ -3411,12 +3411,12 @@ static BOOL glxdrv_wglSwapBuffers( HDC hdc ) break; } - if (escape.gl_drawable && pglXWaitForSbcOML) + if (escape.drawable && pglXWaitForSbcOML) pglXWaitForSbcOML( gdi_display, gl->drawable, target_sbc, &ust, &msc, &sbc ); release_gl_drawable( gl ); - if (escape.gl_drawable) + if (escape.drawable) NtGdiExtEscape( ctx->hdc, NULL, 0, X11DRV_ESCAPE, sizeof(escape), (LPSTR)&escape, 0, NULL ); return TRUE; } diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 82a1fb062ea..b7d09dad779 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -346,7 +346,7 @@ enum x11drv_escape_codes X11DRV_GET_DRAWABLE, /* get current drawable for a DC */ X11DRV_START_EXPOSURES, /* start graphics exposures */ X11DRV_END_EXPOSURES, /* end graphics exposures */ - X11DRV_FLUSH_GL_DRAWABLE /* flush changes made to the gl drawable */ + X11DRV_PRESENT_DRAWABLE, /* present the drawable on screen */ }; struct x11drv_escape_set_drawable @@ -365,10 +365,10 @@ struct x11drv_escape_get_drawable int pixel_format; /* internal GL pixel format */ }; -struct x11drv_escape_flush_gl_drawable +struct x11drv_escape_present_drawable { - enum x11drv_escape_codes code; /* escape code (X11DRV_FLUSH_GL_DRAWABLE) */ - Drawable gl_drawable; /* GL drawable */ + enum x11drv_escape_codes code; /* escape code (X11DRV_PRESENT_DRAWABLE) */ + Drawable drawable; /* GL / VK drawable */ BOOL flush; /* flush X11 before copying */ }; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon (＠rbernon)

2:41 p.m.

v3: Remove unnecessary if. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54105

Paul Gofman (＠gofman)

9:12 p.m.

Paul Gofman (@gofman) commented about dlls/winex11.drv/window.c:

...

+ * detach_client_window + */ +static void detach_client_window( struct x11drv_win_data *data, Window window ) +{ + if (!data->client_window || data->client_window != window) return; + + XSelectInput( data->display, data->client_window, 0 ); + XFlush( data->display ); /* make sure XSelectInput is disabled for client_window after this point */ + XDeleteContext( data->display, data->client_window, winContext ); + + XReparentWindow( gdi_display, data->client_window, get_dummy_parent(), 0, 0 ); + TRACE( "%p/%lx detached client window %lx\n", data->hwnd, data->whole_window, data->client_window ); + + data->client_window = 0; +} +

I think after 'XReparentWindow( gdi_display, ...)' we always need XSync(gdi_display, False). I know that as far as this patch is concerned all the uses of the helper do that XSync after anyway. But it looks like the following patches which use detach_client_window (directly or through attach_client_window added later) miss that. Maybe worth considering putting XSync() here and rearranging the calling code so it doesn't call it the second time (doing the other ops on gdi_context before this call)? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54141

Paul Gofman (＠gofman)

9:12 p.m.

Paul Gofman (@gofman) commented about dlls/winex11.drv/vulkan.c:

...

} if (surface->window) - XDestroyWindow(gdi_display, surface->window); + { + struct x11drv_win_data *data; + + if ((data = get_win_data( surface->hwnd ))) + { + detach_client_window( data, surface->window );

We are destoying the window right after and only want to update client window data here. Reparenting the window before looks excessive. It is not exactly cheap and triggers more WM events opening up a way to more races, so I guess it is important to avoid that. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54142

Paul Gofman (＠gofman)

9:12 p.m.

Paul Gofman (@gofman) commented about dlls/winex11.drv/vulkan.c:

...

-void wine_vk_surface_destroy(HWND hwnd) +static void wine_vk_surface_detach( struct wine_vk_surface *surface ) { - struct wine_vk_surface *surface; - pthread_mutex_lock(&vulkan_mutex); - if (!XFindContext(gdi_display, (XID)hwnd, vulkan_hwnd_context, (char **)&surface)) + struct x11drv_win_data *data; + + TRACE( "Detaching surface %p, hwnd %p.\n", surface, surface->hwnd ); + + if (surface->window && (data = get_win_data( surface->hwnd ))) { - surface->hwnd_thread_id = 0; - surface->hwnd = NULL; - wine_vk_surface_release(surface);

I think this is important refcounting change but a bit separate from what the rest of the patch is doing. Before the patchset the code assumes that window holds the reference to the surface (while tracking the release is a bit convoluted). The idea of the change (which I believe is correct) is to get rid of this reference and make only app creating surface (and swapchain later in this patchset) hold the reference. Surface may be deleted without deleting window (currently doing so will effectively leak surfaces if the app creates and destroy surfaces). Do you think it maybe worth splitting the refcounting change from this patch to a separate one? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54143

Paul Gofman (＠gofman)

9:12 p.m.

Paul Gofman (@gofman) commented about dlls/winex11.drv/vulkan.c:

...

Unless we are adding XSync to detach_client_window() as I suggested earlier, XSync(gdi_display) is probably needed here. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54144

Paul Gofman (＠gofman)

9:12 p.m.

Paul Gofman (@gofman) commented about dlls/winex11.drv/vulkan.c:

...

return pvkGetSwapchainImagesKHR(device, swapchain, count, images); } +static VkResult X11DRV_vkAcquireNextImageKHR( VkDevice device, VkSwapchainKHR swapchain, uint64_t timeout,

I think this really belongs to vkQueuePresent rather than vkAquireNextImage. I know why the functionality related to blitting offscreen surfaces was done in Aquire in Proton initially: it was an attempt to workaround tearing and make sure that the image already presented to the (offscreen) surface when performing blit. But the workaround doesn't really work anymore (at least on AMD) after some Mesa WSI changes. There is VK_KHR_present_wait extenstion now which I believe we might be using for such purposes. It is not universally there yet (e. g, not on Xwayland where it is probably need that as most), but hopefully it will come to fully working state. We also know that bundling presentation in Acquire leads to bugs in apps which don't present continuously and expect the image to appear on screen after calling vkQueuePresent. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54145

Paul Gofman (＠gofman)

9:12 p.m.

Paul Gofman (@gofman) commented about dlls/winex11.drv/vulkan.c:

...

return pvkGetSwapchainImagesKHR(device, swapchain, count, images); } +static VkResult X11DRV_vkAcquireNextImageKHR( VkDevice device, VkSwapchainKHR swapchain, uint64_t timeout, + VkSemaphore semaphore, VkFence fence, uint32_t *image_index ) +{ + struct wine_vk_surface *surface; + struct x11drv_win_data *data; + + if (XFindContext( gdi_display, (XID)swapchain, vulkan_swapchain_context, (char **)&surface )) + return VK_ERROR_SURFACE_LOST_KHR; + + if ((data = get_win_data( surface->hwnd ))) + { + attach_client_window( data, surface->window );

Here we also probably need XSync(gdi_display...) unless doing that inside the function. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54146

Paul Gofman (＠gofman)

9:12 p.m.

Paul Gofman (@gofman) commented about dlls/winex11.drv/window.c:

...

XDeleteContext( data->display, data->client_window, winContext ); XReparentWindow( gdi_display, data->client_window, get_dummy_parent(), 0, 0 ); +#ifdef SONAME_LIBXCOMPOSITE + if (usexcomposite) pXCompositeRedirectWindow( gdi_display, data->client_window, CompositeRedirectManual );

This is actually a workaround for Xwayland bug. When a Vulkan window has fullscreen size and position, Xwayland tends to fully flip it onscreen even regardless of it being a child of 1x1 size parent. It does so not only over some same app other windows but over the whole desktop breaking it completely. This workaround helps in some cases but only partially, there were still cases when it will flip not minding composite redirect as well. So I am not entirely certain if bringing in this part now is justified. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54147

Paul Gofman (＠gofman)

9:12 p.m.

I added my observations inline. Probably worth noting that this patchset, in particular, enables suport for multiple surfaces per single window. While looking at the present patches I tested that on Windows (AMD) with a standalone interactive sample and confirmed that creation and presenting on multiple window surfaces created for single hwnd works (contents is displayed on screen), both on Windows (at least AMD) and with Wine after this patchset. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54148

Rémi Bernon (＠rbernon)

29 Nov 29 Nov

3:20 p.m.

On Tue Nov 28 21:12:43 2023 +0000, Paul Gofman wrote:

...

I don't really see why it would need an additional `XSync` if the current synchronization is correct. The only synchronization points are the same as before or after this MR: 1) when client_window is created using the gdi_display, we need to make sure gdi_display knows about its whole_window parent beforehand. After the creation we also need a XFlush so to make sure the creation requests will be seen by other displays. 2) before XSelectInput is called with thread display, we need to make sure thread display know about the client window. 3) when whole_window is destroyed, through the thread display, we need to make sure any client window has been fully reparented to a different window. The reparenting request is made on the gdi_display, so we could do `XFlush( gdi_display ); XSync( thread->display )` but as this is only causing errors after the whole window request is sent and processed, a `XSync( gdi_display )` should be enough. Re-parenting alone should not need any synchronization as whole_window is still valid at that point, and you can reparent windows as many times as you like, as long as both windows stay valid. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54252

Rémi Bernon (＠rbernon)

3:20 p.m.

On Tue Nov 28 21:12:45 2023 +0000, Paul Gofman wrote:

...

Sure. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54253

Rémi Bernon (＠rbernon)

3:20 p.m.

On Tue Nov 28 21:12:46 2023 +0000, Paul Gofman wrote:

...

Unless we are adding XSync to detach_client_window() as I suggested earlier, XSync(gdi_display) is probably needed here.

As said above, I don't see why we'd need one. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54254

Rémi Bernon (＠rbernon)

3:20 p.m.

On Tue Nov 28 21:12:49 2023 +0000, Paul Gofman wrote:

...

Here we also probably need XSync(gdi_display...) unless doing that inside the function.

Ditto. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54255

Rémi Bernon (＠rbernon)

3:20 p.m.

On Tue Nov 28 21:12:48 2023 +0000, Paul Gofman wrote:

...

Yeah, I didn't want to change that too much from Proton, but I agree that it probably belongs in Present instead. I'll put it there. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54256

Rémi Bernon (＠rbernon)

3:20 p.m.

On Tue Nov 28 21:12:50 2023 +0000, Paul Gofman wrote:

...

Sure, I'll leave it aside. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54257

Paul Gofman (＠gofman)

4:41 p.m.

On Wed Nov 29 15:20:34 2023 +0000, Rémi Bernon wrote:

...

Are you sure that deleting parent whole_window is the only case for which synchronization is needed? E. g., X11DRV_Expose() handles events for client window, don't we want to make sure the current X window tree is synced as soon as client window is detached? Also, if we have, e. g., some resize or iconify of the whole window, while X still thinks that client window is its child, what happen to it? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54279

Rémi Bernon (＠rbernon)

4:50 p.m.

On Wed Nov 29 16:41:34 2023 +0000, Paul Gofman wrote:

...

I'm not sure because pretty much nothing is certain in this area, but if there's any need for additional XSync, it's not because of this MR and it is the case already, and should be added separately. As I don't have evidence that they are required I'm not going to add them now. Regarding the events, this is why there also are XFlush before / after we call XSelectInput, making sure that we only register for Expose events on attached client windows. Of course, there may be pending events that we might receive later but is that even the case after XSelectInput deselected a window? There again I haven't yet seen any evidence it's happening. Regarding resize / iconify of the top-level window, afaik it doesn't have any effect on the child. Resize of the client window is now done using the gdi_display as it should, since a6bc5f34b87393e04ff46659f518f2e7094cc7e4, and shouldn't need additional synchronization. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54280

Paul Gofman (＠gofman)

5:01 p.m.

...

I'm not sure because pretty much nothing is certain in this area, but if there's any need for additional XSync, it's not because of this MR and it is the case already, and should be added separately.

I think now whenever reparent happens (in create_client_window, destroy_whole_window and set_window_visual) there is XSync(gdi_display ) after. After this MR (not exactly the patch introducing detach_client_window, later in the MR when new uses of the function are added) there are paths with XReparentWindow without XSync, so unless I am missing something MR does change things in this regard?

...

As I don't have evidence that they are required I'm not going to add them now

I'd rather suggest maybe then to keep XSync always after the XReparentWindow (effectively, probably not exactly in detach_client_window or attach_client_window but in the added paths), and then remove those in a separate commit if there is a certainty that it is not needed? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54282

Rémi Bernon (＠rbernon)

5:07 p.m.

On Wed Nov 29 17:01:15 2023 +0000, Paul Gofman wrote:

...

I'm not sure because pretty much nothing is certain in this area, but

if there's any need for additional XSync, it's not because of this MR and it is the case already, and should be added separately. I think now whenever reparent happens (in create_client_window, destroy_whole_window and set_window_visual) there is XSync(gdi_display ) after. After this MR (not exactly the patch introducing detach_client_window, later in the MR when new uses of the function are added) there are paths with XReparentWindow without XSync, so unless I am missing something MR does change things in this regard?

As I don't have evidence that they are required I'm not going to add

them now I'd rather suggest maybe then to keep XSync always after the XReparentWindow (effectively, probably not exactly in detach_client_window or attach_client_window but in the added paths), and then remove those in a separate commit if there is a certainty that it is not needed?

But the current XSync is only there to synchronize between XReparentWindow and XDestroyWindow, which definitely caused crashes (and only because of XDestroyWindow, which still believed client window was a child and destroyed it as well, later causing some GL code using gdi_display to access an invalid client window), not between XReparentWindow and other calls. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54284

Paul Gofman (＠gofman)

5:18 p.m.

On Wed Nov 29 17:07:41 2023 +0000, Rémi Bernon wrote:

...

Maybe, but regardless of the motivation, it is de-facto there after each XReparentWindow now. We may have other desync problems besides fatal crashes with XBadWindow. If you ask me now for exact bugs without XSync on the added paths it would be hard to figure out, there were some in Proton version but that was without the referenced earlier upstream changes fixing the sync elsewhere, it might be that those are resolved this way. Usually if we have reasonable doubts if synchronization is needed somewhere we tend to do it and then think on whether / how we can relax it, or if extra sync is especially harmful for performance we want to be sure if it is needed, do we not? Any reason this should be different approach here: in case of doubts assume that it is not needed and then add once the evidence proves otherwise? We are not talking here about any X server operation, only X window tree changes, which are expensive anyway and also potentially sensitive to desync. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54287

Rémi Bernon (＠rbernon)

5:23 p.m.

On Wed Nov 29 17:18:12 2023 +0000, Paul Gofman wrote:

...

I don't think this has ever been like that on winex11 and it is actually the opposite: XSync and XFlush have always been frowned upon and avoided unless explicitly required. The XSync addition here was meant only to synchronize *before* calling XDestroyWindow, and I'd keep it like that until we have an evidence that it's needed *after* XReparentWindow (which should be confirmed soon enough and be a trivial change). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54288

Paul Gofman (＠gofman)

5:30 p.m.

On Wed Nov 29 17:23:08 2023 +0000, Rémi Bernon wrote:

...

Not sure about soon enough, given it obviously won't affect every app, given how fun is to debug X sync issues, especially when you can't reproduce that because your setup is just a bit different and issues are random, when it doesn't crash with XBadWindow but rather misbehaves with some wrong size when going fullscreen or out due to some intermediate resize, and as best it will be blamed to a commit which does other thing not directly concerning such synchronization. But if there are reasons to think XSync on reparenting is expensive enough, well, ok. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54289

Rémi Bernon (＠rbernon)

30 Nov 30 Nov

5:10 p.m.

I've split the client window helpers to a separate MR. I think they are useful on their own to make reasoning easier. I actually found an instance of a missing XSync btw (in `set_window_visual`, when `whole_window` is re-created and the new one used in a `XReparentWindow` call). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_54414

Rémi Bernon (＠rbernon)

29 Feb 29 Feb

8:57 a.m.

New subject: [PATCH v3 0/9] MR4533: winex11: Detach / attach vulkan window surfaces as needed. - closed

This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533

Rémi Bernon (＠rbernon)

8:57 a.m.

Will probably try to move more things in win32u instead of doing all this surface management in winex11. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4533#note_63035

days inactive

171

days old

[email protected]

Manage subscription

34 comments

3 participants

tags (0)

participants (3)

Paul Gofman (＠gofman)
Rémi Bernon
Rémi Bernon (＠rbernon)