[PATCH v2 0/2] 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. -- v2: winevulkan: Allocate memory for VkInstanceCreateInfo with malloc. winevulkan: Allocate memory for VkDeviceCreateInfo with malloc. https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winevulkan/vulkan.c | 60 +++++++++++++++++++--------------------- 1 file changed, 29 insertions(+), 31 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index df109e5c84a..99fdeaa8f1a 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -390,20 +390,23 @@ static void wine_vk_device_get_queues(struct wine_device *device, } static VkResult wine_vk_device_convert_create_info(struct wine_phys_dev *phys_dev, - struct conversion_context *ctx, const VkDeviceCreateInfo *src, VkDeviceCreateInfo *dst) + const VkDeviceCreateInfo *src, VkDeviceCreateInfo *dst) { + const char **extensions; unsigned int i; *dst = *src; + dst->ppEnabledExtensionNames = NULL; + dst->enabledExtensionCount = 0; /* Should be filtered out by loader as ICDs don't support layers. */ dst->enabledLayerCount = 0; dst->ppEnabledLayerNames = NULL; - TRACE("Enabled %u extensions.\n", dst->enabledExtensionCount); - for (i = 0; i < dst->enabledExtensionCount; i++) + TRACE("Enabled %u extensions.\n", src->enabledExtensionCount); + for (i = 0; i < src->enabledExtensionCount; i++) { - const char *extension_name = dst->ppEnabledExtensionNames[i]; + const char *extension_name = src->ppEnabledExtensionNames[i]; TRACE("Extension %u: %s.\n", i, debugstr_a(extension_name)); if (!wine_vk_device_extension_supported(extension_name)) { @@ -412,19 +415,21 @@ static VkResult wine_vk_device_convert_create_info(struct wine_phys_dev *phys_de } } - if (phys_dev->external_memory_align) + if (!src->enabledExtensionCount) extensions = NULL; + else { - const char **new_extensions; + if (!(extensions = malloc((src->enabledExtensionCount + 2) * sizeof(*extensions)))) return VK_ERROR_OUT_OF_HOST_MEMORY; + memcpy(extensions, src->ppEnabledExtensionNames, src->enabledExtensionCount * sizeof(*extensions)); + dst->enabledExtensionCount = src->enabledExtensionCount; + } - new_extensions = conversion_context_alloc(ctx, (dst->enabledExtensionCount + 2) * - sizeof(*dst->ppEnabledExtensionNames)); - memcpy(new_extensions, src->ppEnabledExtensionNames, - dst->enabledExtensionCount * sizeof(*dst->ppEnabledExtensionNames)); - new_extensions[dst->enabledExtensionCount++] = "VK_KHR_external_memory"; - new_extensions[dst->enabledExtensionCount++] = "VK_EXT_external_memory_host"; - dst->ppEnabledExtensionNames = new_extensions; + if (phys_dev->external_memory_align) + { + extensions[dst->enabledExtensionCount++] = "VK_KHR_external_memory"; + extensions[dst->enabledExtensionCount++] = "VK_EXT_external_memory_host"; } + dst->ppEnabledExtensionNames = extensions; return VK_SUCCESS; } @@ -735,12 +740,13 @@ 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; struct wine_queue *next_queue; - struct conversion_context ctx; struct wine_device *object; + VkDevice native_device; unsigned int i; VkResult res; @@ -751,30 +757,22 @@ 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); TRACE("Driver version: %#x.\n", properties.driverVersion); } - if (!(object = calloc(1, sizeof(*object)))) - return VK_ERROR_OUT_OF_HOST_MEMORY; + if ((res = wine_vk_device_convert_create_info(phys_dev, create_info, &create_info_host)) != VK_SUCCESS) return res; + res = instance->funcs.p_vkCreateDevice(phys_dev->phys_dev, &create_info_host, NULL /* allocator */, &native_device); + free((void *)create_info_host.ppEnabledExtensionNames); + if (res != VK_SUCCESS) return res; + if (!(object = calloc(1, sizeof(*object)))) return VK_ERROR_OUT_OF_HOST_MEMORY; object->phys_dev = phys_dev; - - 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); - free_conversion_context(&ctx); - WINE_VK_ADD_DISPATCHABLE_MAPPING(phys_dev->instance, device_handle, object->device, object); - if (res != VK_SUCCESS) - { - WARN("Failed to create device, res=%d.\n", res); - goto fail; - } + object->device = native_device; + WINE_VK_ADD_DISPATCHABLE_MAPPING(instance, device_handle, object->device, object); /* Just load all function pointers we are aware off. The loader takes care of filtering. * We use vkGetDeviceProcAddr as opposed to vkGetInstanceProcAddr for efficiency reasons @@ -815,7 +813,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> --- dlls/winevulkan/vulkan.c | 48 +++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 20 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index 99fdeaa8f1a..2d9978cccb6 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -489,15 +489,18 @@ NTSTATUS init_vulkan(void *args) * This function takes care of extensions handled at winevulkan layer, a Wine graphics * driver is responsible for handling e.g. surface extensions. */ -static VkResult wine_vk_instance_convert_create_info(struct conversion_context *ctx, - const VkInstanceCreateInfo *src, VkInstanceCreateInfo *dst, struct wine_instance *object) +static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo *src, + VkInstanceCreateInfo *dst, struct wine_instance *object) { VkDebugUtilsMessengerCreateInfoEXT *debug_utils_messenger; VkDebugReportCallbackCreateInfoEXT *debug_report_callback; VkBaseInStructure *header; + const char **extensions; unsigned int i; *dst = *src; + dst->ppEnabledExtensionNames = NULL; + dst->enabledExtensionCount = 0; 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)); @@ -542,10 +545,10 @@ static VkResult wine_vk_instance_convert_create_info(struct conversion_context * return VK_ERROR_LAYER_NOT_PRESENT; } - TRACE("Enabled %u instance extensions.\n", dst->enabledExtensionCount); - for (i = 0; i < dst->enabledExtensionCount; i++) + TRACE("Enabled %u instance extensions.\n", src->enabledExtensionCount); + for (i = 0; i < src->enabledExtensionCount; i++) { - const char *extension_name = dst->ppEnabledExtensionNames[i]; + const char *extension_name = src->ppEnabledExtensionNames[i]; TRACE("Extension %u: %s.\n", i, debugstr_a(extension_name)); if (!wine_vk_instance_extension_supported(extension_name)) { @@ -558,19 +561,21 @@ static VkResult wine_vk_instance_convert_create_info(struct conversion_context * } } - if (use_external_memory()) + if (!src->enabledExtensionCount) extensions = NULL; + else { - const char **new_extensions; + if (!(extensions = malloc((src->enabledExtensionCount + 2) * sizeof(*extensions)))) return VK_ERROR_OUT_OF_HOST_MEMORY; + memcpy(extensions, src->ppEnabledExtensionNames, src->enabledExtensionCount * sizeof(*extensions)); + dst->enabledExtensionCount = src->enabledExtensionCount; + } - new_extensions = conversion_context_alloc(ctx, (dst->enabledExtensionCount + 2) * - sizeof(*dst->ppEnabledExtensionNames)); - memcpy(new_extensions, src->ppEnabledExtensionNames, - dst->enabledExtensionCount * sizeof(*dst->ppEnabledExtensionNames)); - new_extensions[dst->enabledExtensionCount++] = "VK_KHR_get_physical_device_properties2"; - new_extensions[dst->enabledExtensionCount++] = "VK_KHR_external_memory_capabilities"; - dst->ppEnabledExtensionNames = new_extensions; + if (use_external_memory()) + { + extensions[dst->enabledExtensionCount++] = "VK_KHR_get_physical_device_properties2"; + extensions[dst->enabledExtensionCount++] = "VK_KHR_external_memory_capabilities"; } + dst->ppEnabledExtensionNames = extensions; return VK_SUCCESS; } @@ -831,7 +836,6 @@ VkResult wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, VkInstance client_instance = client_ptr; VkInstanceCreateInfo create_info_host; const VkApplicationInfo *app_info; - struct conversion_context ctx; struct wine_instance *object; VkResult res; @@ -846,11 +850,15 @@ VkResult wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, list_init(&object->wrappers); 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); + if ((res = wine_vk_instance_convert_create_info(create_info, &create_info_host, object)) != VK_SUCCESS) + { + wine_vk_instance_free(object); + return res; + } + + res = vk_funcs->p_vkCreateInstance(&create_info_host, NULL /* allocator */, &object->instance); + free((void *)create_info_host.ppEnabledExtensionNames); + if (res != VK_SUCCESS) { ERR("Failed to create instance, res=%d\n", res); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4488
v2: Free the extension array as soon as we're done with it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53700
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.
All usages of conversion context and briefly used and released right away, I don't see how it's relevant. The main point of conversion context is for the caller to not need to worry about details of conversion memory management (and as a bonus, it should be more efficient). It was more important for generated conversion functions, where we previously needed to generate a set of function just to free converted structs. Still, if we have the infrastructure, why not use it here? The MR adds somewhat unpleasant casts in `free` calls and performs memory allocations when they are not needed. Granted, it's not a big deal, but I don't see how it's an improvement. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53701
Actually, I think I'll rework `wine_vk_instance_convert_create_info` a bit more, I believe it's bogus when `vkCreateInstance` wants more than the default physical devices allow. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53703
All usages of conversion context and briefly used and released right away
That's not true, conversion context is also used for deferred operations where it lives as long as the operation lives. Here we use it for a single allocation, which is absolutely not performance critical so efficiency does not matter, and it only adds unnecessary indirection and complexity. The code also doesn't check for memory allocation failures and uses large amount of stack space for no good reason. Sure, the free cast is a bit ugly. I'm fine keeping a separate pointer if that matters. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4488#note_53782
participants (2)
-
Jacek Caban (＠jacek) -
Rémi Bernon