From: Liam Middlebrook Subject: Re: [PATCH v2 3/4] winevulkan: Store a mapping from native handles to wrappers. Message-Id: <5cf524c0-4694-b057-903b-82a2bd993552@nvidia.com> Date: Tue, 22 Sep 2020 14:25:30 -0700 In-Reply-To: <20200922143151.34962-3-dadschoorse@gmail.com> References: <20200922143151.34962-1-dadschoorse@gmail.com> <20200922143151.34962-3-dadschoorse@gmail.com> On 9/22/20 7:31 AM, Georg Lehmann wrote: > Signed-off-by: Georg Lehmann > --- > dlls/winevulkan/vulkan.c | 69 ++++++++++++++++++++++++++++++++ > dlls/winevulkan/vulkan_private.h | 26 ++++++++++++ > 2 files changed, 95 insertions(+) > > diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c > index f730c04923..2747f440ad 100644 > --- a/dlls/winevulkan/vulkan.c > +++ b/dlls/winevulkan/vulkan.c > @@ -66,11 +66,57 @@ static VkResult (*p_vkEnumerateInstanceVersion)(uint32_t *version); > void WINAPI wine_vkGetPhysicalDeviceProperties(VkPhysicalDevice physical_device, > VkPhysicalDeviceProperties *properties); > > +#define WINE_VK_ADD_HANDLE_MAPPING(instance, object, native_handle)\ > + wine_vk_add_handle_mapping((instance), (uint64_t) (uintptr_t) (object), (uint64_t) (native_handle), &(object)->mapping) nit: please put a space before the final '\' here and for other continuations like this. Rather than explicitly casting to (uintptr_t) at almost every call to VK_ADD_HANDLE_MAPPING() would it make more sense to do the same double-cast that you're doing above for the object parameter? > +static void wine_vk_add_handle_mapping(struct VkInstance_T *instance, uint64_t wrapper, > + uint64_t native_handle, struct wine_vk_mapping *mapping) > +{ > + if (instance->enable_wrapper_list) > + { > + mapping->native_handle = native_handle; > + mapping->wine_wrapper = wrapper; > + AcquireSRWLockExclusive(&instance->wrapper_lock); > + list_add_tail(&instance->wrappers, &mapping->link); > + ReleaseSRWLockExclusive(&instance->wrapper_lock); > + } > +} > + > +#define WINE_VK_REMOVE_HANDLE_MAPPING(instance, object)\ > + wine_vk_remove_handle_mapping((instance), &(object)->mapping) > +static void wine_vk_remove_handle_mapping(struct VkInstance_T *instance, struct wine_vk_mapping *mapping) > +{ > + if (instance->enable_wrapper_list) > + { > + AcquireSRWLockExclusive(&instance->wrapper_lock); > + list_remove(&mapping->link); > + ReleaseSRWLockExclusive(&instance->wrapper_lock); > + } > +} > + > +static uint64_t wine_vk_get_wrapper(struct VkInstance_T *instance, uint64_t native_handle) > +{ > + struct wine_vk_mapping *mapping; > + uint64_t result = 0; > + > + AcquireSRWLockShared(&instance->wrapper_lock); > + LIST_FOR_EACH_ENTRY(mapping, &instance->wrappers, struct wine_vk_mapping, link) > + { > + if (mapping->native_handle == native_handle) > + { > + result = mapping->wine_wrapper; > + break; > + } > + } > + ReleaseSRWLockShared(&instance->wrapper_lock); > + return result; > +} > + > static void wine_vk_physical_device_free(struct VkPhysicalDevice_T *phys_dev) > { > if (!phys_dev) > return; > > + WINE_VK_REMOVE_HANDLE_MAPPING(phys_dev->instance, phys_dev); > heap_free(phys_dev->extensions); > heap_free(phys_dev); > } > @@ -91,6 +137,8 @@ static struct VkPhysicalDevice_T *wine_vk_physical_device_alloc(struct VkInstanc > object->instance = instance; > object->phys_dev = phys_dev; > > + WINE_VK_ADD_HANDLE_MAPPING(instance, object, (uintptr_t) phys_dev); > + > res = instance->funcs.p_vkEnumerateDeviceExtensionProperties(phys_dev, > NULL, &num_host_properties, NULL); > if (res != VK_SUCCESS) > @@ -169,6 +217,7 @@ static void wine_vk_free_command_buffers(struct VkDevice_T *device, > > device->funcs.p_vkFreeCommandBuffers(device->device, pool->command_pool, 1, &buffers[i]->command_buffer); > list_remove(&buffers[i]->pool_link); > + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance, buffers[i]); > heap_free(buffers[i]); > } > } > @@ -212,6 +261,8 @@ static struct VkQueue_T *wine_vk_device_alloc_queues(struct VkDevice_T *device, > { > device->funcs.p_vkGetDeviceQueue(device->device, family_index, i, &queue->queue); > } > + > + WINE_VK_ADD_HANDLE_MAPPING(device->phys_dev->instance, queue, (uintptr_t) queue->queue); > } > > return queues; > @@ -294,6 +345,8 @@ static void wine_vk_device_free(struct VkDevice_T *device) > unsigned int i; > for (i = 0; i < device->max_queue_families; i++) > { > + if (device->queues[i] && device->queues[i]->queue) > + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance, device->queues[i]); > heap_free(device->queues[i]); > } > heap_free(device->queues); > @@ -302,6 +355,7 @@ static void wine_vk_device_free(struct VkDevice_T *device) > > if (device->device && device->funcs.p_vkDestroyDevice) > { > + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance, device); > device->funcs.p_vkDestroyDevice(device->device, NULL /* pAllocator */); > } > > @@ -512,6 +566,7 @@ VkResult WINAPI wine_vkAllocateCommandBuffers(VkDevice device, > list_add_tail(&pool->command_buffers, &buffers[i]->pool_link); > res = device->funcs.p_vkAllocateCommandBuffers(device->device, > &allocate_info_host, &buffers[i]->command_buffer); > + WINE_VK_ADD_HANDLE_MAPPING(device->phys_dev->instance, buffers[i], (uintptr_t) buffers[i]->command_buffer); > if (res != VK_SUCCESS) > { > ERR("Failed to allocate command buffer, res=%d.\n", res); > @@ -589,6 +644,7 @@ VkResult WINAPI wine_vkCreateDevice(VkPhysicalDevice phys_dev, > return VK_ERROR_OUT_OF_HOST_MEMORY; > > object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE; > + object->phys_dev = phys_dev; > > res = wine_vk_device_convert_create_info(create_info, &create_info_host); > if (res != VK_SUCCESS) > @@ -597,6 +653,7 @@ VkResult WINAPI wine_vkCreateDevice(VkPhysicalDevice phys_dev, > res = phys_dev->instance->funcs.p_vkCreateDevice(phys_dev->phys_dev, > &create_info_host, NULL /* allocator */, &object->device); > wine_vk_device_free_create_info(&create_info_host); > + WINE_VK_ADD_HANDLE_MAPPING(phys_dev->instance, object, (uintptr_t) object->device); > if (res != VK_SUCCESS) > { > WARN("Failed to create device, res=%d.\n", res); > @@ -678,6 +735,8 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, > return VK_ERROR_OUT_OF_HOST_MEMORY; > } > object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE; > + list_init(&object->wrappers); > + InitializeSRWLock(&object->wrapper_lock); I wasn't able to find cleanup functions for these on instance destruction. > > res = wine_vk_instance_convert_create_info(create_info, &create_info_host, object); > if (res != VK_SUCCESS) > @@ -695,6 +754,8 @@ VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_info, > return res; > } > > + WINE_VK_ADD_HANDLE_MAPPING(object, object, (uintptr_t) object->instance); > + I wasn't able to find a matching WINE_VK_REMOVE_HANDLE_MAPPING call for this. > /* Load all instance functions we are aware of. Note the loader takes care > * of any filtering for extensions which were not requested, but which the > * ICD may support. > @@ -1129,9 +1190,14 @@ VkResult WINAPI wine_vkCreateCommandPool(VkDevice device, const VkCommandPoolCre > res = device->funcs.p_vkCreateCommandPool(device->device, info, NULL, &object->command_pool); > > if (res == VK_SUCCESS) > + { > + WINE_VK_ADD_HANDLE_MAPPING(device->phys_dev->instance, object, object->command_pool); > *command_pool = wine_cmd_pool_to_handle(object); > + } > else > + { > heap_free(object); > + } > > return res; > } > @@ -1156,9 +1222,12 @@ void WINAPI wine_vkDestroyCommandPool(VkDevice device, VkCommandPool handle, > */ > LIST_FOR_EACH_ENTRY_SAFE(buffer, cursor, &pool->command_buffers, struct VkCommandBuffer_T, pool_link) > { > + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance, buffer); > heap_free(buffer); > } > > + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance, pool); > + > device->funcs.p_vkDestroyCommandPool(device->device, pool->command_pool, NULL); > heap_free(pool); > } > diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h > index 4bcc4de440..146bbd8fa4 100644 > --- a/dlls/winevulkan/vulkan_private.h > +++ b/dlls/winevulkan/vulkan_private.h > @@ -60,6 +60,16 @@ struct wine_vk_base > UINT_PTR loader_magic; > }; > > +/* Some extensions have callbacks for those we need to be able to > + * get the wine wrapper for a native handle > + */ > +struct wine_vk_mapping > +{ > + struct list link; > + uint64_t native_handle; > + uint64_t wine_wrapper; It might be good to add a comment here noting the bitsize of Vulkan handles in comparison to native pointers. Also I think it would make sense to include 'handle' in the name for the WINE wrapper handle. Perhaps: wine_wrapped_handle wrapped_handle or something else? Thanks, Liam Middlebrook > +}; > + > struct VkCommandBuffer_T > { > struct wine_vk_base base; > @@ -67,18 +77,22 @@ struct VkCommandBuffer_T > VkCommandBuffer command_buffer; /* native command buffer */ > > struct list pool_link; > + struct wine_vk_mapping mapping; > }; > > struct VkDevice_T > { > struct wine_vk_base base; > struct vulkan_device_funcs funcs; > + struct VkPhysicalDevice_T *phys_dev; /* parent */ > VkDevice device; /* native device */ > > struct VkQueue_T **queues; > uint32_t max_queue_families; > > unsigned int quirks; > + > + struct wine_vk_mapping mapping; > }; > > struct VkInstance_T > @@ -93,7 +107,13 @@ struct VkInstance_T > struct VkPhysicalDevice_T **phys_devs; > uint32_t phys_dev_count; > > + VkBool32 enable_wrapper_list; > + struct list wrappers; > + SRWLOCK wrapper_lock; > + > unsigned int quirks; > + > + struct wine_vk_mapping mapping; > }; > > struct VkPhysicalDevice_T > @@ -104,6 +124,8 @@ struct VkPhysicalDevice_T > > VkExtensionProperties *extensions; > uint32_t extension_count; > + > + struct wine_vk_mapping mapping; > }; > > struct VkQueue_T > @@ -113,6 +135,8 @@ struct VkQueue_T > VkQueue queue; /* native queue */ > > VkDeviceQueueCreateFlags flags; > + > + struct wine_vk_mapping mapping; > }; > > struct wine_cmd_pool > @@ -120,6 +144,8 @@ struct wine_cmd_pool > VkCommandPool command_pool; > > struct list command_buffers; > + > + struct wine_vk_mapping mapping; > }; > > static inline struct wine_cmd_pool *wine_cmd_pool_from_handle(VkCommandPool handle) >