[PATCH v4 0/5] MR4488: winevulkan: Allocate create info host extensions with malloc.
I don't think there's any reason to use the conversion context to allocate this memory, it's briefly used to build the host extension list and released right away. -- v4: winevulkan: Fix python linter warnings. winevulkan: Drop the WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS quirk. winevulkan: Use a local instance variable instead of phys_dev->instance. winevulkan: Restore utils messengers when vkCreateInstance needs more physical devices. winevulkan: Restore debug callbacks when vkCreateInstance needs more physical devices. https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winevulkan/vulkan.c | 54 +++++++++++++++++++++++++------- dlls/winevulkan/vulkan_private.h | 27 ++++++++-------- 2 files changed, 57 insertions(+), 24 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index df109e5c84a..308d3a546ca 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -488,7 +488,7 @@ static VkResult wine_vk_instance_convert_create_info(struct conversion_context * const VkInstanceCreateInfo *src, VkInstanceCreateInfo *dst, struct wine_instance *object) { VkDebugUtilsMessengerCreateInfoEXT *debug_utils_messenger; - VkDebugReportCallbackCreateInfoEXT *debug_report_callback; + VkDebugReportCallbackCreateInfoEXT *callback_info; VkBaseInStructure *header; unsigned int i; @@ -512,17 +512,22 @@ static VkResult wine_vk_instance_convert_create_info(struct conversion_context * debug_utils_messenger->pUserData = &object->utils_messengers[i]; } - debug_report_callback = find_next_struct(header->pNext, - VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT); - if (debug_report_callback) + callback_info = (VkDebugReportCallbackCreateInfoEXT *)dst; + while ((callback_info = find_next_struct(callback_info->pNext, VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT))) { - object->default_callback.instance = object; - object->default_callback.debug_callback = VK_NULL_HANDLE; - object->default_callback.user_callback = debug_report_callback->pfnCallback; - object->default_callback.user_data = debug_report_callback->pUserData; + struct wine_debug_report_callback *callback; - debug_report_callback->pfnCallback = (void *) &debug_report_callback_conversion; - debug_report_callback->pUserData = &object->default_callback; + if (!(callback = calloc(1, sizeof(*callback)))) return VK_ERROR_OUT_OF_HOST_MEMORY; + callback->instance = object; + callback->debug_callback = VK_NULL_HANDLE; + callback->user_callback = callback_info->pfnCallback; + callback->user_data = callback_info->pUserData; + + /* convert_VkInstanceCreateInfo_* already copied the chain, so we can modify it in-place. */ + callback_info->pfnCallback = debug_report_callback_conversion; + callback_info->pUserData = callback; + + list_add_tail(&object->debug_report_callbacks, &callback->entry); } /* ICDs don't support any layers, so nothing to copy. Modern versions of the loader @@ -569,6 +574,23 @@ static VkResult wine_vk_instance_convert_create_info(struct conversion_context * return VK_SUCCESS; } +static void cleanup_instance_create_info(struct conversion_context *ctx, VkInstanceCreateInfo *info) +{ + VkDebugReportCallbackCreateInfoEXT *callback_info = (VkDebugReportCallbackCreateInfoEXT *)info; + + while ((callback_info = find_next_struct(callback_info->pNext, VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT))) + { + struct wine_debug_report_callback *callback = (struct wine_debug_report_callback *)callback_info->pUserData; + + if (callback_info->pfnCallback != debug_report_callback_conversion) break; + /* restore previous create info values, if VkCreateInstance needs to be called again */ + callback_info->pfnCallback = callback->user_callback; + callback_info->pUserData = callback->user_data; + } + + free_conversion_context(ctx); +} + /* Helper function which stores wrapped physical devices in the instance object. */ static VkResult wine_vk_instance_load_physical_devices(struct wine_instance *instance) { @@ -652,6 +674,9 @@ static struct wine_phys_dev *wine_vk_instance_wrap_physical_device(struct wine_i */ static void wine_vk_instance_free(struct wine_instance *instance) { + struct wine_debug_report_callback *callback; + void *next; + if (!instance) return; @@ -675,6 +700,12 @@ static void wine_vk_instance_free(struct wine_instance *instance) pthread_rwlock_destroy(&instance->wrapper_lock); free(instance->utils_messengers); + LIST_FOR_EACH_ENTRY_SAFE(callback, next, &instance->debug_report_callbacks, struct wine_debug_report_callback, entry) + { + list_remove(&callback->entry); + free(callback); + } + free(instance); } @@ -846,13 +877,14 @@ VkResult wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, return VK_ERROR_OUT_OF_HOST_MEMORY; } list_init(&object->wrappers); + list_init(&object->debug_report_callbacks); pthread_rwlock_init(&object->wrapper_lock, NULL); init_conversion_context(&ctx); res = wine_vk_instance_convert_create_info(&ctx, create_info, &create_info_host, object); if (res == VK_SUCCESS) res = vk_funcs->p_vkCreateInstance(&create_info_host, NULL /* allocator */, &object->instance); - free_conversion_context(&ctx); + cleanup_instance_create_info(&ctx, &create_info_host); if (res != VK_SUCCESS) { ERR("Failed to create instance, res=%d\n", res); diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h index c8e234e4c8d..d2280f0e734 100644 --- a/dlls/winevulkan/vulkan_private.h +++ b/dlls/winevulkan/vulkan_private.h @@ -74,18 +74,6 @@ static inline struct wine_device *wine_device_from_handle(VkDevice handle) struct wine_debug_utils_messenger; -struct wine_debug_report_callback -{ - struct wine_instance *instance; /* parent */ - VkDebugReportCallbackEXT debug_callback; /* native callback object */ - - /* application callback + data */ - PFN_vkDebugReportCallbackEXT user_callback; - void *user_data; - - struct wine_vk_mapping mapping; -}; - struct wine_instance { struct vulkan_instance_funcs funcs; @@ -106,7 +94,7 @@ struct wine_instance struct wine_debug_utils_messenger *utils_messengers; uint32_t utils_messenger_count; - struct wine_debug_report_callback default_callback; + struct list debug_report_callbacks; unsigned int quirks; @@ -207,6 +195,19 @@ static inline VkDebugUtilsMessengerEXT wine_debug_utils_messenger_to_handle( return (VkDebugUtilsMessengerEXT)(uintptr_t)debug_messenger; } +struct wine_debug_report_callback +{ + struct wine_instance *instance; /* parent */ + VkDebugReportCallbackEXT debug_callback; /* native callback object */ + + /* application callback + data */ + PFN_vkDebugReportCallbackEXT user_callback; + void *user_data; + + struct wine_vk_mapping mapping; + struct list entry; /* entry in wine_instance debug_report_callbacks */ +}; + static inline struct wine_debug_report_callback *wine_debug_report_callback_from_handle( VkDebugReportCallbackEXT handle) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winevulkan/vulkan.c | 62 ++++++++++++++++---------------- dlls/winevulkan/vulkan_private.h | 7 ++-- 2 files changed, 34 insertions(+), 35 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index 308d3a546ca..d690bb6a322 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -44,21 +44,6 @@ static BOOL use_external_memory(void) static ULONG_PTR zero_bits = 0; -#define wine_vk_count_struct(s, t) wine_vk_count_struct_((void *)s, VK_STRUCTURE_TYPE_##t) -static uint32_t wine_vk_count_struct_(void *s, VkStructureType t) -{ - const VkBaseInStructure *header; - uint32_t result = 0; - - for (header = s; header; header = header->pNext) - { - if (header->sType == t) - result++; - } - - return result; -} - static const struct vulkan_funcs *vk_funcs; #define WINE_VK_ADD_DISPATCHABLE_MAPPING(instance, client_handle, native_handle, object) \ @@ -487,29 +472,28 @@ NTSTATUS init_vulkan(void *args) static VkResult wine_vk_instance_convert_create_info(struct conversion_context *ctx, const VkInstanceCreateInfo *src, VkInstanceCreateInfo *dst, struct wine_instance *object) { - VkDebugUtilsMessengerCreateInfoEXT *debug_utils_messenger; + VkDebugUtilsMessengerCreateInfoEXT *messenger_info; VkDebugReportCallbackCreateInfoEXT *callback_info; - VkBaseInStructure *header; unsigned int i; *dst = *src; - object->utils_messenger_count = wine_vk_count_struct(dst, DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT); - object->utils_messengers = calloc(object->utils_messenger_count, sizeof(*object->utils_messengers)); - header = (VkBaseInStructure *) dst; - for (i = 0; i < object->utils_messenger_count; i++) + messenger_info = (VkDebugUtilsMessengerCreateInfoEXT *)dst; + while ((messenger_info = find_next_struct(messenger_info->pNext, VK_STRUCTURE_TYPE_DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT))) { - header = find_next_struct(header->pNext, VK_STRUCTURE_TYPE_DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT); - debug_utils_messenger = (VkDebugUtilsMessengerCreateInfoEXT *) header; + struct wine_debug_utils_messenger *messenger; - object->utils_messengers[i].instance = object; - object->utils_messengers[i].debug_messenger = VK_NULL_HANDLE; - object->utils_messengers[i].user_callback = debug_utils_messenger->pfnUserCallback; - object->utils_messengers[i].user_data = debug_utils_messenger->pUserData; + if (!(messenger = calloc(1, sizeof(*messenger)))) return VK_ERROR_OUT_OF_HOST_MEMORY; + messenger->instance = object; + messenger->debug_messenger = VK_NULL_HANDLE; + messenger->user_callback = messenger_info->pfnUserCallback; + messenger->user_data = messenger_info->pUserData; /* convert_VkInstanceCreateInfo_* already copied the chain, so we can modify it in-place. */ - debug_utils_messenger->pfnUserCallback = (void *) &debug_utils_callback_conversion; - debug_utils_messenger->pUserData = &object->utils_messengers[i]; + messenger_info->pfnUserCallback = debug_utils_callback_conversion; + messenger_info->pUserData = messenger; + + list_add_tail(&object->debug_utils_messengers, &messenger->entry); } callback_info = (VkDebugReportCallbackCreateInfoEXT *)dst; @@ -576,8 +560,19 @@ static VkResult wine_vk_instance_convert_create_info(struct conversion_context * static void cleanup_instance_create_info(struct conversion_context *ctx, VkInstanceCreateInfo *info) { + VkDebugUtilsMessengerCreateInfoEXT *messenger_info = (VkDebugUtilsMessengerCreateInfoEXT *)info; VkDebugReportCallbackCreateInfoEXT *callback_info = (VkDebugReportCallbackCreateInfoEXT *)info; + while ((messenger_info = find_next_struct(messenger_info->pNext, VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT))) + { + struct wine_debug_utils_messenger *messenger = (struct wine_debug_utils_messenger *)messenger_info->pUserData; + + if (messenger_info->pfnUserCallback != debug_utils_callback_conversion) break; + /* restore previous create info values, if VkCreateInstance needs to be called again */ + messenger_info->pfnUserCallback = messenger->user_callback; + messenger_info->pUserData = messenger->user_data; + } + while ((callback_info = find_next_struct(callback_info->pNext, VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT))) { struct wine_debug_report_callback *callback = (struct wine_debug_report_callback *)callback_info->pUserData; @@ -674,6 +669,7 @@ static struct wine_phys_dev *wine_vk_instance_wrap_physical_device(struct wine_i */ static void wine_vk_instance_free(struct wine_instance *instance) { + struct wine_debug_utils_messenger *messenger; struct wine_debug_report_callback *callback; void *next; @@ -698,7 +694,12 @@ static void wine_vk_instance_free(struct wine_instance *instance) } pthread_rwlock_destroy(&instance->wrapper_lock); - free(instance->utils_messengers); + + LIST_FOR_EACH_ENTRY_SAFE(messenger, next, &instance->debug_utils_messengers, struct wine_debug_utils_messenger, entry) + { + list_remove(&messenger->entry); + free(messenger); + } LIST_FOR_EACH_ENTRY_SAFE(callback, next, &instance->debug_report_callbacks, struct wine_debug_report_callback, entry) { @@ -877,6 +878,7 @@ VkResult wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, return VK_ERROR_OUT_OF_HOST_MEMORY; } list_init(&object->wrappers); + list_init(&object->debug_utils_messengers); list_init(&object->debug_report_callbacks); pthread_rwlock_init(&object->wrapper_lock, NULL); diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h index d2280f0e734..0c9fef43551 100644 --- a/dlls/winevulkan/vulkan_private.h +++ b/dlls/winevulkan/vulkan_private.h @@ -72,8 +72,6 @@ static inline struct wine_device *wine_device_from_handle(VkDevice handle) return (struct wine_device *)(uintptr_t)handle->base.unix_handle; } -struct wine_debug_utils_messenger; - struct wine_instance { struct vulkan_instance_funcs funcs; @@ -91,9 +89,7 @@ struct wine_instance struct list wrappers; pthread_rwlock_t wrapper_lock; - struct wine_debug_utils_messenger *utils_messengers; - uint32_t utils_messenger_count; - + struct list debug_utils_messengers; struct list debug_report_callbacks; unsigned int quirks; @@ -181,6 +177,7 @@ struct wine_debug_utils_messenger void *user_data; struct wine_vk_mapping mapping; + struct list entry; /* entry in wine_instance debug_utils_messengers */ }; static inline struct wine_debug_utils_messenger *wine_debug_utils_messenger_from_handle( -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winevulkan/vulkan.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index d690bb6a322..c076efcbeda 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -767,6 +767,7 @@ VkResult wine_vkCreateDevice(VkPhysicalDevice phys_dev_handle, const VkDeviceCre void *client_ptr) { struct wine_phys_dev *phys_dev = wine_phys_dev_from_handle(phys_dev_handle); + struct wine_instance *instance = phys_dev->instance; VkDevice device_handle = client_ptr; VkDeviceCreateInfo create_info_host; struct VkQueue_T *queue_handles; @@ -783,7 +784,7 @@ VkResult wine_vkCreateDevice(VkPhysicalDevice phys_dev_handle, const VkDeviceCre { VkPhysicalDeviceProperties properties; - phys_dev->instance->funcs.p_vkGetPhysicalDeviceProperties(phys_dev->phys_dev, &properties); + instance->funcs.p_vkGetPhysicalDeviceProperties(phys_dev->phys_dev, &properties); TRACE("Device name: %s.\n", debugstr_a(properties.deviceName)); TRACE("Vendor ID: %#x, Device ID: %#x.\n", properties.vendorID, properties.deviceID); @@ -798,10 +799,10 @@ VkResult wine_vkCreateDevice(VkPhysicalDevice phys_dev_handle, const VkDeviceCre init_conversion_context(&ctx); res = wine_vk_device_convert_create_info(phys_dev, &ctx, create_info, &create_info_host); if (res == VK_SUCCESS) - res = phys_dev->instance->funcs.p_vkCreateDevice(phys_dev->phys_dev, - &create_info_host, NULL /* allocator */, &object->device); + res = instance->funcs.p_vkCreateDevice(phys_dev->phys_dev, &create_info_host, + NULL /* allocator */, &object->device); free_conversion_context(&ctx); - WINE_VK_ADD_DISPATCHABLE_MAPPING(phys_dev->instance, device_handle, object->device, object); + WINE_VK_ADD_DISPATCHABLE_MAPPING(instance, device_handle, object->device, object); if (res != VK_SUCCESS) { WARN("Failed to create device, res=%d.\n", res); @@ -847,7 +848,7 @@ VkResult wine_vkCreateDevice(VkPhysicalDevice phys_dev_handle, const VkDeviceCre next_queue += queue_count; } - device_handle->quirks = phys_dev->instance->quirks; + device_handle->quirks = instance->quirks; device_handle->base.unix_handle = (uintptr_t)object; *ret_device = device_handle; TRACE("Created device %p (native device %p).\n", object, object->device); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
From: Rémi Bernon <rbernon(a)codeweavers.com> It's not set anywhere. --- dlls/winevulkan/vulkan.c | 7 ++----- dlls/winevulkan/vulkan_loader.h | 1 - 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index c076efcbeda..35da93872e3 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -517,11 +517,8 @@ static VkResult wine_vk_instance_convert_create_info(struct conversion_context * /* ICDs don't support any layers, so nothing to copy. Modern versions of the loader * filter this data out as well. */ - if (object->quirks & WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS) { - dst->enabledLayerCount = 0; - dst->ppEnabledLayerNames = NULL; - WARN("Ignoring explicit layers!\n"); - } else if (dst->enabledLayerCount) { + if (dst->enabledLayerCount) + { FIXME("Loading explicit layers is not supported by winevulkan!\n"); return VK_ERROR_LAYER_NOT_PRESENT; } diff --git a/dlls/winevulkan/vulkan_loader.h b/dlls/winevulkan/vulkan_loader.h index 4e606624819..6f62201f503 100644 --- a/dlls/winevulkan/vulkan_loader.h +++ b/dlls/winevulkan/vulkan_loader.h @@ -40,7 +40,6 @@ #define WINEVULKAN_QUIRK_GET_DEVICE_PROC_ADDR 0x00000001 #define WINEVULKAN_QUIRK_ADJUST_MAX_IMAGE_COUNT 0x00000002 -#define WINEVULKAN_QUIRK_IGNORE_EXPLICIT_LAYERS 0x00000004 /* Base 'class' for our Vulkan dispatchable objects such as VkDevice and VkInstance. * This structure MUST be the first element of a dispatchable object as the ICD -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winevulkan/make_vulkan | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan index 9163f543c15..314b642bc83 100755 --- a/dlls/winevulkan/make_vulkan +++ b/dlls/winevulkan/make_vulkan @@ -23,7 +23,6 @@ import argparse import logging import os import re -import sys import urllib.request import xml.etree.ElementTree as ET from collections import OrderedDict @@ -2124,7 +2123,6 @@ class VkStruct(Sequence): if self.name in ["VkOpticalFlowSessionCreateInfoNV", "VkDescriptorBufferBindingInfoEXT"]: is_const = True - needs_output_copy = False for e in self.struct_extensions: if not e.required: @@ -2574,7 +2572,7 @@ class ArrayConversionFunction(object): body += " else\n" body += " out[i] = NULL;\n" else: - body += " out[i] = UlongToPtr(in[i]);\n".format(win_type) + body += " out[i] = UlongToPtr(in[i]);\n" elif self.array.is_handle(): if self.array.pointer_array: LOGGER.error("Unhandled handle pointer arrays") @@ -3701,7 +3699,7 @@ class VkRegistry(object): # Since we already parsed the enum before, just link it in. try: type_info["data"] = self.enums[name] - except KeyError as e: + except KeyError: # Not all enums seem to be defined yet, typically that's for # ones ending in 'FlagBits' where future extensions may add # definitions. -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
Repurposed the MR to the create info cleanup. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53919
On Mon Nov 27 15:00:39 2023 +0000, Jacek Caban wrote:
That's not true, conversion context is also used for deferred operations where it lives as long as the operation lives. Depends on what you call briefly, but fine, it's still true for vast majority of use cases and it's still meaningless as a reason to change the code. IMHO both using conversion context and `malloc` is fine in this case. I just don't see any reason to change between them. The code also doesn't check for memory allocation failures winevulkan was never consistent in memory allocation failures handling and changing that is more tricky than that. For this particular case, we don't need `malloc` to handle allocation failure. I still believe we should get rid of unnecessary indirections. I've dropped this for now but I'm sure I'll come back to it ;)
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53920
Jacek Caban (@jacek) commented about dlls/winevulkan/vulkan.c:
return VK_SUCCESS; }
+static void cleanup_instance_create_info(struct conversion_context *ctx, VkInstanceCreateInfo *info) +{ + VkDebugReportCallbackCreateInfoEXT *callback_info = (VkDebugReportCallbackCreateInfoEXT *)info; + + while ((callback_info = find_next_struct(callback_info->pNext, VK_STRUCTURE_TYPE_DEBUG_REPORT_CALLBACK_CREATE_INFO_EXT))) + { + struct wine_debug_report_callback *callback = (struct wine_debug_report_callback *)callback_info->pUserData; + + if (callback_info->pfnCallback != debug_report_callback_conversion) break; + /* restore previous create info values, if VkCreateInstance needs to be called again */
This is not needed, we always copy the chain in the generated thunk anyway. That's why it's fine to modify it in-place in the first place (and we need that anyway due to `VK_STRUCTURE_TYPE_LOADER_INSTANCE_CREATE_INFO` filtering). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53924
I see. Okay, I'll open a separate MR for the cleanups. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53927
This merge request was closed by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
On Mon Nov 27 16:23:35 2023 +0000, Rémi Bernon wrote:
I see. Okay, I'll open a separate MR for the cleanups. FWIW, the part allowing multiple debug callbacks in the chain was probably fine.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53928
On Mon Nov 27 16:23:35 2023 +0000, Jacek Caban wrote:
FWIW, the part allowing multiple debug callbacks in the chain was probably fine. Well, looking at the spec a bit more in detail it seems like only one debug callback is allowed. So it all ended up truly with 1) fixing the lookup to start at the begin of the chain (if messengers were found), and 2) using dynamic allocations instead, which given the discussion above didn't feel like a very useful change.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53930
participants (2)
-
Jacek Caban (＠jacek) -
Rémi Bernon