[PATCH 0/4] MR7219: win32u, winex11.drv: Fix multiple Vulkan surfaces handling.
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/win32u/vulkan.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/win32u/vulkan.c b/dlls/win32u/vulkan.c index 84657e4c16f..ba6a7f604b4 100644 --- a/dlls/win32u/vulkan.c +++ b/dlls/win32u/vulkan.c @@ -701,6 +701,7 @@ void vulkan_detach_surfaces( struct list *surfaces ) driver_funcs->p_vulkan_surface_detach( surface->hwnd, surface->driver_private ); list_remove( &surface->entry ); list_init( &surface->entry ); + surface->hwnd = NULL; } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7219
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winex11.drv/vulkan.c | 1 + 1 file changed, 1 insertion(+) diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index 14711537636..1804ba85287 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -220,6 +220,7 @@ static void X11DRV_vulkan_surface_presented( HWND hwnd, void *private, VkResult HRGN region; HDC hdc; + if (!hwnd) return; vulkan_surface_update_size( hwnd, surface ); vulkan_surface_update_offscreen( hwnd, surface ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7219
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/win32u/vulkan.c | 41 ----------------------------------------- 1 file changed, 41 deletions(-) diff --git a/dlls/win32u/vulkan.c b/dlls/win32u/vulkan.c index ba6a7f604b4..0b0e71f5564 100644 --- a/dlls/win32u/vulkan.c +++ b/dlls/win32u/vulkan.c @@ -75,45 +75,6 @@ static struct swapchain *swapchain_from_handle( VkSwapchainKHR handle ) return CONTAINING_RECORD( obj, struct swapchain, obj ); } -static int window_surface_compare( const void *key, const struct rb_entry *entry ) -{ - const struct surface *surface = RB_ENTRY_VALUE( entry, struct surface, window_entry ); - HWND key_hwnd = (HWND)key; - - if (key_hwnd < surface->hwnd) return -1; - if (key_hwnd > surface->hwnd) return 1; - return 0; -} - -static pthread_mutex_t window_surfaces_lock = PTHREAD_MUTEX_INITIALIZER; -static struct rb_tree window_surfaces = {.compare = window_surface_compare}; - -static void window_surfaces_insert( struct surface *surface ) -{ - struct surface *previous; - struct rb_entry *ptr; - - pthread_mutex_lock( &window_surfaces_lock ); - - if (!(ptr = rb_get( &window_surfaces, surface->hwnd ))) - rb_put( &window_surfaces, surface->hwnd, &surface->window_entry ); - else - { - previous = RB_ENTRY_VALUE( ptr, struct surface, window_entry ); - rb_replace( &window_surfaces, &previous->window_entry, &surface->window_entry ); - previous->hwnd = 0; /* make sure previous surface becomes invalid */ - } - - pthread_mutex_unlock( &window_surfaces_lock ); -} - -static void window_surfaces_remove( struct surface *surface ) -{ - pthread_mutex_lock( &window_surfaces_lock ); - if (surface->hwnd) rb_remove( &window_surfaces, &surface->window_entry ); - pthread_mutex_unlock( &window_surfaces_lock ); -} - static VkResult win32u_vkCreateWin32SurfaceKHR( VkInstance client_instance, const VkWin32SurfaceCreateInfoKHR *create_info, const VkAllocationCallbacks *allocator, VkSurfaceKHR *ret ) { @@ -161,7 +122,6 @@ static VkResult win32u_vkCreateWin32SurfaceKHR( VkInstance client_instance, cons instance->p_insert_object( instance, &surface->obj.obj ); if (dummy) NtUserDestroyWindow( dummy ); - window_surfaces_insert( surface ); *ret = surface->obj.client.surface; return VK_SUCCESS; @@ -189,7 +149,6 @@ static void win32u_vkDestroySurfaceKHR( VkInstance client_instance, VkSurfaceKHR driver_funcs->p_vulkan_surface_destroy( surface->hwnd, surface->driver_private ); instance->p_remove_object( instance, &surface->obj.obj ); - window_surfaces_remove( surface ); free( surface ); } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7219
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/winex11.drv/vulkan.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/dlls/winex11.drv/vulkan.c b/dlls/winex11.drv/vulkan.c index 1804ba85287..3b303b5ad5a 100644 --- a/dlls/winex11.drv/vulkan.c +++ b/dlls/winex11.drv/vulkan.c @@ -161,7 +161,15 @@ static void vulkan_surface_update_offscreen( HWND hwnd, struct x11drv_vulkan_sur BOOL offscreen = needs_offscreen_rendering( hwnd, FALSE ); struct x11drv_win_data *data; - if (offscreen == surface->offscreen) return; + if (offscreen == surface->offscreen) + { + if (!offscreen && (data = get_win_data( hwnd ))) + { + attach_client_window( data, surface->window ); + release_win_data( data ); + } + return; + } surface->offscreen = offscreen; if (!surface->offscreen) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/7219
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=151099 Your paranoid android. === debian11b (64 bit WoW report) === mf: mf.c:6785: Test failed: Unexpected hr 0. user32: win.c:4070: Test failed: Expected active window 0000000002F60190, got 0000000000000000. win.c:4071: Test failed: Expected focus window 0000000002F60190, got 0000000000000000.
There are a few different issues concerned here: 1. Addressed by first two patches. That fixes a random X11 fatal error (reproducible with vulkan-1 tests here). The vulkan surface is detached upon deleting the window with existing Vulkan swapchain but then when presenting to the surface handling in X11DRV_vulkan_surface_presented() has the invalid hwnd and vulkan_surface_update_size() tries to configue window based on random data. 2. Addressed by patch 3, a use after free / access violation when a new swapchain is created before previous one is destroyed. The crash is X11DRV_vulkan_surface_detach called with NULL driver surface. Previously the surface was 'invalidated' in win32u.window_surfaces_insert() when adding another surface (surface->hwnd is set to 0) but it is not deleted from widnow's surface list; then win32u_vkDestroySurfaceKHR deletes the surface but it is still linked into window's surface list (it is only deleted if get_win_ptr( surface->hwnd ) is not NULL which can't happen as surface->hwnd is 0 already). 3. Addressed by patch 4. Allows presentation to multiple swapchain to be actually presented onscreen (that would already work if those surfaces happened to be offscreen after patch 3 although making them offscreen is not needed). That allows d3d over Vulkan to work with multiple dxgi swapchains created for one window as well as, e. g, multiple d3d9 devices on the same window. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7219#note_93088
Rémi Bernon (@rbernon) commented about dlls/winex11.drv/vulkan.c:
HRGN region; HDC hdc;
+ if (!hwnd) return;
This should probably be moved to win32u instead, and avoid calling into drivers with detached surfaces / NULL hwnd? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/7219#note_93095
On Tue Jan 28 20:47:55 2025 +0000, Rémi Bernon wrote:
This should probably be moved to win32u instead, and avoid calling into drivers with detached surfaces / NULL hwnd? Well, I thought it is driver business to decide whether it wants to do anything with detached surface after present or not? Not like I am really against of moving it.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7219#note_93096
On Tue Jan 28 20:49:09 2025 +0000, Paul Gofman wrote:
Well, I thought it is driver business to decide whether it wants to do anything with detached surface after present or not? Not like I am really against of moving it. IMO it would be cleaner to avoid bothering drivers with surfaces for windows that don't exist anymore, that's also true for the `vulkan_surface_update` callback.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7219#note_93097
On Tue Jan 28 20:54:24 2025 +0000, Rémi Bernon wrote:
IMO it would be cleaner to avoid bothering drivers with surfaces for windows that don't exist anymore, that's also true for the `vulkan_surface_update` callback. Looks like vulkan_surface_update callback is only called from win32u_vkCreateSwapchainKHR() where surface->hwnd is currently checked for being a valid window before that?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7219#note_93098
On Tue Jan 28 20:57:46 2025 +0000, Paul Gofman wrote:
Looks like vulkan_surface_update callback is only called from win32u_vkCreateSwapchainKHR() where surface->hwnd is currently checked for being a valid window before that? Eh, right, just that presented callback then I guess.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/7219#note_93099
participants (4)
-
Marvin -
Paul Gofman -
Paul Gofman (＠gofman)
-
Rémi Bernon