[PATCH 0/2] MR5835: win32u, winex11: Avoid using the win32u vulkan driver interface.
It might be hooking some functions when we want to call into the host vulkan instead. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/d3dkmt.c | 9 ++++--- dlls/win32u/ntuser_private.h | 5 ++++ dlls/win32u/vulkan.c | 51 +++++++++++++++++++----------------- 3 files changed, 37 insertions(+), 28 deletions(-) diff --git a/dlls/win32u/d3dkmt.c b/dlls/win32u/d3dkmt.c index 5f3804c165d..a97ab5d3c33 100644 --- a/dlls/win32u/d3dkmt.c +++ b/dlls/win32u/d3dkmt.c @@ -28,6 +28,7 @@ #define WIN32_NO_STATUS #include "ntgdi_private.h" #include "win32u_private.h" +#include "ntuser_private.h" #include "wine/vulkan.h" #include "wine/vulkan_driver.h" @@ -83,13 +84,13 @@ static void d3dkmt_init_vulkan(void) PFN_vkCreateInstance p_vkCreateInstance; VkResult vr; - if (!(vulkan_funcs = __wine_get_vulkan_driver( WINE_VULKAN_DRIVER_VERSION ))) + if (!vulkan_init()) { WARN( "Failed to open the Vulkan driver\n" ); return; } - p_vkCreateInstance = vulkan_funcs->p_vkGetInstanceProcAddr( NULL, "vkCreateInstance" ); + p_vkCreateInstance = p_vkGetInstanceProcAddr( NULL, "vkCreateInstance" ); if ((vr = p_vkCreateInstance( &create_info, NULL, &d3dkmt_vk_instance ))) { WARN( "Failed to create a Vulkan instance, vr %d.\n", vr ); @@ -97,9 +98,9 @@ static void d3dkmt_init_vulkan(void) return; } - p_vkDestroyInstance = vulkan_funcs->p_vkGetInstanceProcAddr( d3dkmt_vk_instance, "vkDestroyInstance" ); + p_vkDestroyInstance = p_vkGetInstanceProcAddr( d3dkmt_vk_instance, "vkDestroyInstance" ); #define LOAD_VK_FUNC( f ) \ - if (!(p##f = (void *)vulkan_funcs->p_vkGetInstanceProcAddr( d3dkmt_vk_instance, #f ))) \ + if (!(p##f = (void *)p_vkGetInstanceProcAddr( d3dkmt_vk_instance, #f ))) \ { \ WARN( "Failed to load " #f ".\n" ); \ p_vkDestroyInstance( d3dkmt_vk_instance, NULL ); \ diff --git a/dlls/win32u/ntuser_private.h b/dlls/win32u/ntuser_private.h index 020c98005df..62e65d71d31 100644 --- a/dlls/win32u/ntuser_private.h +++ b/dlls/win32u/ntuser_private.h @@ -25,6 +25,7 @@ #include "ntuser.h" #include "shellapi.h" #include "wine/list.h" +#include "wine/vulkan.h" #define WM_POPUPSYSTEMMENU 0x0313 @@ -250,6 +251,10 @@ extern int peek_message( MSG *msg, const struct peek_message_filter *filter ); extern LRESULT system_tray_call( HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam, void *data ); /* vulkan.c */ +extern void *(*p_vkGetDeviceProcAddr)(VkDevice, const char *); +extern void *(*p_vkGetInstanceProcAddr)(VkInstance, const char *); + +extern BOOL vulkan_init(void); extern void vulkan_detach_surfaces( struct list *surfaces ); /* window.c */ diff --git a/dlls/win32u/vulkan.c b/dlls/win32u/vulkan.c index 5b128afe3cb..471a4459ec3 100644 --- a/dlls/win32u/vulkan.c +++ b/dlls/win32u/vulkan.c @@ -39,16 +39,18 @@ WINE_DEFAULT_DEBUG_CHANNEL(vulkan); -#ifdef SONAME_LIBVULKAN +void *(*p_vkGetDeviceProcAddr)(VkDevice, const char *) = NULL; +void *(*p_vkGetInstanceProcAddr)(VkInstance, const char *) = NULL; static void *vulkan_handle; -static const struct vulkan_driver_funcs *driver_funcs; static struct vulkan_funcs vulkan_funcs; +#ifdef SONAME_LIBVULKAN + +static const struct vulkan_driver_funcs *driver_funcs; + static void (*p_vkDestroySurfaceKHR)(VkInstance, VkSurfaceKHR, const VkAllocationCallbacks *); static VkResult (*p_vkQueuePresentKHR)(VkQueue, const VkPresentInfoKHR *); -static void *(*p_vkGetDeviceProcAddr)(VkDevice, const char *); -static void *(*p_vkGetInstanceProcAddr)(VkInstance, const char *); struct surface { @@ -283,7 +285,7 @@ static const struct vulkan_driver_funcs lazydrv_funcs = .p_vulkan_surface_presented = lazydrv_vulkan_surface_presented, }; -static void vulkan_init(void) +static void vulkan_init_once(void) { if (!(vulkan_handle = dlopen( SONAME_LIBVULKAN, RTLD_NOW ))) { @@ -323,27 +325,24 @@ void vulkan_detach_surfaces( struct list *surfaces ) } } -/*********************************************************************** - * __wine_get_vulkan_driver (win32u.so) - */ -const struct vulkan_funcs *__wine_get_vulkan_driver( UINT version ) -{ - static pthread_once_t init_once = PTHREAD_ONCE_INIT; +#else /* SONAME_LIBVULKAN */ - if (version != WINE_VULKAN_DRIVER_VERSION) - { - ERR( "version mismatch, vulkan wants %u but win32u has %u\n", version, WINE_VULKAN_DRIVER_VERSION ); - return NULL; - } +void vulkan_detach_surfaces( struct list *surfaces ) +{ +} - pthread_once( &init_once, vulkan_init ); - return vulkan_handle ? &vulkan_funcs : NULL; +static void vulkan_init_once(void) +{ + ERR( "Wine was built without Vulkan support.\n" ); } -#else /* SONAME_LIBVULKAN */ +#endif /* SONAME_LIBVULKAN */ -void vulkan_detach_surfaces( struct list *surfaces ) +BOOL vulkan_init(void) { + static pthread_once_t init_once = PTHREAD_ONCE_INIT; + pthread_once( &init_once, vulkan_init_once ); + return !!vulkan_handle; } /*********************************************************************** @@ -351,8 +350,12 @@ void vulkan_detach_surfaces( struct list *surfaces ) */ const struct vulkan_funcs *__wine_get_vulkan_driver( UINT version ) { - ERR("Wine was built without Vulkan support.\n"); - return NULL; -} + if (version != WINE_VULKAN_DRIVER_VERSION) + { + ERR( "version mismatch, vulkan wants %u but win32u has %u\n", version, WINE_VULKAN_DRIVER_VERSION ); + return NULL; + } -#endif /* SONAME_LIBVULKAN */ + if (!vulkan_init()) return NULL; + return &vulkan_funcs; +} -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5835
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/xrandr.c | 56 ++++++++++++++++++++++++++++++++------- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/dlls/winex11.drv/xrandr.c b/dlls/winex11.drv/xrandr.c index 74b975e8671..edad0712927 100644 --- a/dlls/winex11.drv/xrandr.c +++ b/dlls/winex11.drv/xrandr.c @@ -49,6 +49,8 @@ WINE_DECLARE_DEBUG_CHANNEL(winediag); #include "wine/vulkan_driver.h" static void *xrandr_handle; +static void *vulkan_handle; +static void *(*p_vkGetInstanceProcAddr)(VkInstance, const char *); #define MAKE_FUNCPTR(f) static typeof(f) * p##f; MAKE_FUNCPTR(XRRConfigCurrentConfiguration) @@ -136,6 +138,45 @@ sym_not_found: return r; } +#ifdef SONAME_LIBVULKAN + +static void vulkan_init_once(void) +{ + if (!(vulkan_handle = dlopen( SONAME_LIBVULKAN, RTLD_NOW ))) + { + ERR( "Failed to load %s\n", SONAME_LIBVULKAN ); + return; + } + +#define LOAD_FUNCPTR( f ) \ + if (!(p_##f = dlsym( vulkan_handle, #f ))) \ + { \ + ERR( "Failed to find " #f "\n" ); \ + dlclose( vulkan_handle ); \ + vulkan_handle = NULL; \ + return; \ + } + + LOAD_FUNCPTR( vkGetInstanceProcAddr ); +#undef LOAD_FUNCPTR +} + +#else /* SONAME_LIBVULKAN */ + +static void vulkan_init_once(void) +{ + ERR( "Wine was built without Vulkan support.\n" ); +} + +#endif /* SONAME_LIBVULKAN */ + +static BOOL vulkan_init(void) +{ + static pthread_once_t init_once = PTHREAD_ONCE_INIT; + pthread_once( &init_once, vulkan_init_once ); + return !!vulkan_handle; +} + static int XRandRErrorHandler(Display *dpy, XErrorEvent *event, void *arg) { return 1; @@ -641,11 +682,11 @@ static BOOL get_gpu_properties_from_vulkan( struct x11drv_gpu *gpu, const XRRPro "VK_KHR_display", VK_KHR_SURFACE_EXTENSION_NAME, }; - const struct vulkan_funcs *vulkan_funcs = __wine_get_vulkan_driver( WINE_VULKAN_DRIVER_VERSION ); VkResult (*pvkGetRandROutputDisplayEXT)( VkPhysicalDevice, Display *, RROutput, VkDisplayKHR * ); PFN_vkGetPhysicalDeviceProperties2KHR pvkGetPhysicalDeviceProperties2KHR; PFN_vkEnumeratePhysicalDevices pvkEnumeratePhysicalDevices; uint32_t device_count, device_idx, output_idx, i; + PFN_vkDestroyInstance pvkDestroyInstance = NULL; VkPhysicalDevice *vk_physical_devices = NULL; VkPhysicalDeviceProperties2 properties2; PFN_vkCreateInstance pvkCreateInstance; @@ -656,8 +697,7 @@ static BOOL get_gpu_properties_from_vulkan( struct x11drv_gpu *gpu, const XRRPro BOOL ret = FALSE; VkResult vr; - if (!vulkan_funcs) - goto done; + if (!vulkan_init()) goto done; memset( &create_info, 0, sizeof(create_info) ); create_info.sType = VK_STRUCTURE_TYPE_INSTANCE_CREATE_INFO; @@ -665,7 +705,7 @@ static BOOL get_gpu_properties_from_vulkan( struct x11drv_gpu *gpu, const XRRPro create_info.ppEnabledExtensionNames = extensions; #define LOAD_VK_FUNC(f) \ - if (!(p##f = (void *)vulkan_funcs->p_vkGetInstanceProcAddr( vk_instance, #f ))) \ + if (!(p##f = (void *)p_vkGetInstanceProcAddr( vk_instance, #f ))) \ { \ WARN("Failed to load " #f ".\n"); \ goto done; \ @@ -683,6 +723,7 @@ static BOOL get_gpu_properties_from_vulkan( struct x11drv_gpu *gpu, const XRRPro LOAD_VK_FUNC(vkEnumeratePhysicalDevices) LOAD_VK_FUNC(vkGetPhysicalDeviceProperties2KHR) LOAD_VK_FUNC(vkGetRandROutputDisplayEXT) + LOAD_VK_FUNC(vkDestroyInstance) #undef LOAD_VK_FUNC vr = pvkEnumeratePhysicalDevices( vk_instance, &device_count, NULL ); @@ -747,12 +788,7 @@ static BOOL get_gpu_properties_from_vulkan( struct x11drv_gpu *gpu, const XRRPro done: free( vk_physical_devices ); - if (vk_instance) - { - PFN_vkDestroyInstance p_vkDestroyInstance; - p_vkDestroyInstance = vulkan_funcs->p_vkGetInstanceProcAddr( vk_instance, "vkDestroyInstance" ); - p_vkDestroyInstance( vk_instance, NULL ); - } + if (vk_instance && pvkDestroyInstance) pvkDestroyInstance( vk_instance, NULL ); return ret; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5835
Fwiw this would help fixing https://gitlab.winehq.org/wine/wine/-/merge_requests/5787, although it's not the only issue. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835#note_73502
Emil Velikov (@xexaxo) commented about dlls/win32u/d3dkmt.c:
PFN_vkCreateInstance p_vkCreateInstance; VkResult vr;
- if (!(vulkan_funcs = __wine_get_vulkan_driver( WINE_VULKAN_DRIVER_VERSION ))) + if (!vulkan_init()) { WARN( "Failed to open the Vulkan driver\n" ); return; }
- p_vkCreateInstance = vulkan_funcs->p_vkGetInstanceProcAddr( NULL, "vkCreateInstance" ); + p_vkCreateInstance = p_vkGetInstanceProcAddr( NULL, "vkCreateInstance" ); if ((vr = p_vkCreateInstance( &create_info, NULL, &d3dkmt_vk_instance ))) { WARN( "Failed to create a Vulkan instance, vr %d.\n", vr ); vulkan_funcs = NULL;
The vulkan_funcs seems to be no longer set/needed. The helper using it should also some polish. Would this commit/allow us to drop the lazy loading introduced with commit 70cf97a59c0e48370ab0ff719a82aed015ad3195 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835#note_73576
Emil Velikov (@xexaxo) commented about dlls/winex11.drv/xrandr.c:
return r; }
+#ifdef SONAME_LIBVULKAN + +static void vulkan_init_once(void) +{ + if (!(vulkan_handle = dlopen( SONAME_LIBVULKAN, RTLD_NOW )))
Does wine (usually) close() the libraries at some point? I'm wondering if anyone runs valgrind (with or without your MR) and they would see extra "leaks" in the report. Somewhat heading in the opposite direction - one could cache the vulkan instance, instead of creating/destroying (the exact same one?) multiple times. Just my final question, no more or less. Promise I won't interrogate you further 😅 Massive thanks for all the enlightening answers 🙇 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835#note_73580
The vulkan_funcs seems to be no longer set/needed.
Right.
Would this commit/allow us to drop the lazy loading introduced with commit 70cf97a59c0e48370ab0ff719a82aed015ad3195
Could be, is it causing any trouble? I think it can avoid loading the win32u drivers (although right now they probably get loaded elsewhere) when offscreen vulkan is used, which might be useful? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835#note_73584
Does wine (usually) close() the libraries at some point? I'm wondering if anyone runs valgrind (with or without your MR) and they would see extra "leaks" in the report.
I don't think we bother with that very much. I haven't seen valgrind complain about it, although valgrind has a lot of problems to overcome to be usable with Wine anyway :).
Somewhat heading in the opposite direction - one could cache the vulkan instance, instead of creating/destroying (the exact same one?) multiple times.
That's a good idea, thanks. No worries, thanks for taking interest and having a look :) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835#note_73585
On Tue Jun 18 14:49:32 2024 +0000, Rémi Bernon wrote:
Does wine (usually) close() the libraries at some point? I'm wondering if anyone runs valgrind (with or without your MR) and they would see extra "leaks" in the report. I don't think we bother with that very much. I haven't seen valgrind complain about it, although valgrind has a lot of problems to overcome to be usable with Wine anyway :). Somewhat heading in the opposite direction - one could cache the vulkan instance, instead of creating/destroying (the exact same one?) multiple times. That's a good idea, thanks. No worries, thanks for taking interest and having a look :) (I mean, we unload PE DLLs when they should, and call dlclose on unix libraries when they get unused, but in this particular case I don't think user32/win32u/user drivers can be truly unloaded at any point after it's been loaded)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835#note_73588
On Tue Jun 18 14:49:32 2024 +0000, Rémi Bernon wrote:
The vulkan_funcs seems to be no longer set/needed. Right. Would this commit/allow us to drop the lazy loading introduced with commit 70cf97a59c0e48370ab0ff719a82aed015ad3195 Could be, is it causing any trouble? I think it can avoid loading the win32u drivers (although right now they probably get loaded elsewhere) when offscreen vulkan is used, which might be useful? AFAICT there is a small bug in the lazy loading/offscreen logic. Plus personally not a huge fan of extra indirection, unless needed.
I will be away for a couple of weeks and will send a MR if needed when I'm back. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835#note_73605
On Tue Jun 18 14:56:46 2024 +0000, Rémi Bernon wrote:
(I mean, we unload PE DLLs when they should, and call dlclose on unix libraries when they get unused, but in this particular case I don't think user32/win32u/user drivers can be truly unloaded at any point after it's been loaded) Does the wine (pre)loader unload things on `exit()` or perhaps the dependency on ntdll.so and(?) win32u.so, makes that impossible?
Either way, as you said there are bigger concerns than this one dlhandle. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5835#note_73608
participants (2)
-
Emil Velikov (＠xexaxo) -
Rémi Bernon