Re: [PATCH v2 3/4] winevulkan: Store a mapping from native handles to wrappers.

[Liam Middlebrook]

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)