[PATCH v2 1/4] winevulkan: Add is_alias for VkFunctionPointer.

[Georg Lehmann]

Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com>
---
 dlls/winevulkan/make_vulkan | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
index 5261c35360..1b39cbb831 100755
--- a/dlls/winevulkan/make_vulkan
+++ b/dlls/winevulkan/make_vulkan
@@ -865,6 +865,8 @@ class VkFunctionPointer(object):
         text += ");\n"
         return text
 
+    def is_alias(self):
+        return False
 
 class VkHandle(object):
     def __init__(self, name, _type, parent, alias=None):
-- 
2.28.0

[PATCH v2 2/4] winevulkan: Forward declare required structs for function pointer.

[Georg Lehmann]

Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com>
---
 dlls/winevulkan/make_vulkan | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
index 1b39cbb831..f4d223daad 100755
--- a/dlls/winevulkan/make_vulkan
+++ b/dlls/winevulkan/make_vulkan
@@ -805,11 +805,12 @@ class VkFunction(object):
 
 
 class VkFunctionPointer(object):
-    def __init__(self, _type, name, members):
+    def __init__(self, _type, name, members, forward_decls):
         self.name = name
         self.members = members
         self.type = _type
         self.required = False
+        self.forward_decls = forward_decls
 
     @staticmethod
     def from_xml(funcpointer):
@@ -846,10 +847,19 @@ class VkFunctionPointer(object):
 
         _type = funcpointer.text
         name = funcpointer.find("name").text
-        return VkFunctionPointer(_type, name, members)
+        if "requires" in funcpointer.attrib:
+            forward_decls = funcpointer.attrib.get("requires").split(",")
+        else:
+            forward_decls = []
+        return VkFunctionPointer(_type, name, members, forward_decls)
 
     def definition(self):
-        text = "{0} {1})(\n".format(self.type, self.name)
+        text = ""
+        # forward declare required structs
+        for decl in self.forward_decls:
+            text += "typedef struct {0} {0};\n".format(decl)
+
+        text += "{0} {1})(\n".format(self.type, self.name)
 
         first = True
         if len(self.members) > 0:
-- 
2.28.0

[PATCH v2 2/4] winevulkan: Forward declare required structs for function pointer.

[Liam Middlebrook]

Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com>

On 9/22/20 7:31 AM, Georg Lehmann wrote:
> Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com>
> ---
>   dlls/winevulkan/make_vulkan | 16 +++++++++++++---
>   1 file changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> index 1b39cbb831..f4d223daad 100755
> --- a/dlls/winevulkan/make_vulkan
> +++ b/dlls/winevulkan/make_vulkan
> @@ -805,11 +805,12 @@ class VkFunction(object):
>
> class VkFunctionPointer(object):
> -    def __init__(self, _type, name, members):
> +    def __init__(self, _type, name, members, forward_decls):
>           self.name = name
>           self.members = members
>           self.type = _type
>           self.required = False
> +        self.forward_decls = forward_decls
>
> @staticmethod
>       def from_xml(funcpointer):
> @@ -846,10 +847,19 @@ class VkFunctionPointer(object):
>
> _type = funcpointer.text
>           name = funcpointer.find("name").text
> -        return VkFunctionPointer(_type, name, members)
> +        if "requires" in funcpointer.attrib:
> +            forward_decls = funcpointer.attrib.get("requires").split(",")
> +        else:
> +            forward_decls = []
> +        return VkFunctionPointer(_type, name, members, forward_decls)
>
> def definition(self):
> -        text = "{0} {1})(\n".format(self.type, self.name)
> +        text = ""
> +        # forward declare required structs
> +        for decl in self.forward_decls:
> +            text += "typedef struct {0} {0};\n".format(decl)
> +
> +        text += "{0} {1})(\n".format(self.type, self.name)
>
> first = True
>           if len(self.members) > 0:

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

[Georg Lehmann]

Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com>
---
 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)
+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);
 
     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);
+
     /* 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;
+};
+
 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)
-- 
2.28.0

[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 <dadschoorse(a)gmail.com>
> ---
>   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)

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

[Dad Schoorse]

On 22. Sept. 2020, 23:25, Liam Middlebrook wrote:
> On 9/22/20 7:31 AM, Georg Lehmann wrote:
> > Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com>
> > ---
> >   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?

We can't implicitly cast to uintptr_t because non-dispatchable handles are
64bit integers on 32bit systems.

But I think it makes sense to have a macro for dispatchable handles with
the implicit cast and one without it.
> > +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.

There's no cleanup necessary for both of these.
> > 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.

With the current handle mapping implementation that call is not necessary.
VkInstance is going to be the last object in the list anyway and freeing
the instance object will also free the list.
But it's probably a good idea to call WINE_VK_REMOVE_HANDLE_MAPPING anyway,
in case something changes in the future?
> > /* 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.

I'm not sure what the comment should say, generic handles are always
passed as uint64_t in Vulkan.
> 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?

I wasn't sure myself, but wine_wrapped_handle is probably the clearer name.

Thanks,

Georg Lehmann
> 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)
> >

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

[Liam Middlebrook]

On 9/22/20 3:26 PM, Dad Schoorse wrote:
> On 22. Sept. 2020, 23:25, Liam Middlebrook wrote:
> > On 9/22/20 7:31 AM, Georg Lehmann wrote:
> > > Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com 
> > > <mailto:dadschoorse(a)gmail.com>>
> > > ---
> > >    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?
>
> We can't implicitly cast to uintptr_t because non-dispatchable handles are
> 64bit integers on 32bit systems.
>
> But I think it makes sense to have a macro for dispatchable handles with
> the implicit cast and one without it.

Sounds good to me.
> > > +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.
>
> There's no cleanup necessary for both of these.

Ah sorry about that. I didn't realize these weren't treated as distinct 
heap allocations.
> > >        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.
>
> With the current handle mapping implementation that call is not necessary.
> VkInstance is going to be the last object in the list anyway and freeing
> the instance object will also free the list.
> But it's probably a good idea to call WINE_VK_REMOVE_HANDLE_MAPPING anyway,
> in case something changes in the future?

I don't think it'll necessarily change in the future, but I'd prefer 
this to be explicitly cleaned up.
> > >        /* 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.
>
> I'm not sure what the comment should say, generic handles are always
> passed as uint64_t in Vulkan.

Eh, I guess a comment isn't really needed here. Let's just go with the 
suggested renaming below, and that should have a similar effect for clarity.


Thanks,

Liam Middlebrook
> > 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?
>
> I wasn't sure myself, but wine_wrapped_handle is probably the clearer name.
>
> Thanks,
>
> Georg Lehmann
> > 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)
> > >

[PATCH v2 4/4] winevulkan: Implement VK_EXT_debug_utils.

[Georg Lehmann]

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49813

Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com>
---
 dlls/winevulkan/make_vulkan      |   8 +-
 dlls/winevulkan/vulkan.c         | 193 +++++++++++++++++++++++++++++++
 dlls/winevulkan/vulkan_private.h |  31 +++++
 3 files changed, 231 insertions(+), 1 deletion(-)

diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
index f4d223daad..77034cfe36 100755
--- a/dlls/winevulkan/make_vulkan
+++ b/dlls/winevulkan/make_vulkan
@@ -92,7 +92,6 @@ UNSUPPORTED_EXTENSIONS = [
     # plumbing down to the native layer, we will get each message twice as we
     # use 2 loaders (win32+native), but we may get output from the driver.
     # In any case callback conversion is required.
-    "VK_EXT_debug_utils",
     "VK_EXT_validation_features",
     "VK_EXT_validation_flags",
     "VK_KHR_display", # Needs WSI work.
@@ -225,6 +224,11 @@ FUNCTION_OVERRIDES = {
     # VK_EXT_calibrated_timestamps
     "vkGetPhysicalDeviceCalibrateableTimeDomainsEXT" : {"dispatch" : True, "driver" : False, "thunk" : False},
     "vkGetCalibratedTimestampsEXT" : {"dispatch" : True, "driver" : False, "thunk" : False},
+
+    # VK_EXT_debug_utils
+    "vkCreateDebugUtilsMessengerEXT" : {"dispatch": True, "driver" : False, "thunk" : False},
+    "vkDestroyDebugUtilsMessengerEXT" : {"dispatch": True, "driver" : False, "thunk" : False},
+    "vkSubmitDebugUtilsMessageEXT" : {"dispatch": True, "driver" : False, "thunk" : True, "private_thunk" : True},
 }
 
 STRUCT_CHAIN_CONVERSIONS = [
@@ -944,6 +948,8 @@ class VkHandle(object):
 
         if self.name == "VkCommandPool":
             return "wine_cmd_pool_from_handle({0})->command_pool".format(name)
+        if self.name == "VkDebugUtilsMessengerEXT":
+            return "wine_debug_utils_messenger_from_handle({0})->debug_messenger".format(name)
 
         native_handle_name = None
 
diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
index 2747f440ad..11dd0f761e 100644
--- a/dlls/winevulkan/vulkan.c
+++ b/dlls/winevulkan/vulkan.c
@@ -57,6 +57,21 @@ static void *wine_vk_find_struct_(void *s, VkStructureType t)
     return NULL;
 }
 
+#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)
+{
+    VkBaseOutStructure *header;
+    uint32_t result = 0;
+
+    for (header = s; header; header = header->pNext)
+    {
+        if (header->sType == t)
+            result++;
+    }
+
+    return result;
+}
+
 static void *wine_vk_get_global_proc_addr(const char *name);
 
 static HINSTANCE hinstance;
@@ -111,6 +126,76 @@ static uint64_t wine_vk_get_wrapper(struct VkInstance_T *instance, uint64_t nati
     return result;
 }
 
+static VkBool32 is_wrapped(VkObjectType type)
+{
+    return type == VK_OBJECT_TYPE_INSTANCE ||
+           type == VK_OBJECT_TYPE_PHYSICAL_DEVICE ||
+           type == VK_OBJECT_TYPE_DEVICE ||
+           type == VK_OBJECT_TYPE_QUEUE ||
+           type == VK_OBJECT_TYPE_COMMAND_BUFFER ||
+           type == VK_OBJECT_TYPE_COMMAND_POOL ||
+           type == VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT;
+}
+
+static VkBool32 debug_utils_callback_conversion(VkDebugUtilsMessageSeverityFlagBitsEXT severity,
+    VkDebugUtilsMessageTypeFlagsEXT message_types,
+#if defined(USE_STRUCT_CONVERSION)
+    const VkDebugUtilsMessengerCallbackDataEXT_host *callback_data,
+#else
+    const VkDebugUtilsMessengerCallbackDataEXT *callback_data,
+#endif
+    void *user_data)
+{
+    struct VkDebugUtilsMessengerCallbackDataEXT wine_callback_data;
+    VkDebugUtilsObjectNameInfoEXT *object_name_infos;
+    struct wine_debug_utils_messenger *object;
+    int i;
+
+    object = user_data;
+
+    if (!object->instance->instance)
+    {
+        /* instance wasn't yet created, this is a message from the native loader*/
+        return VK_FALSE;
+    }
+
+    wine_callback_data = *((VkDebugUtilsMessengerCallbackDataEXT *) callback_data);
+
+    object_name_infos = heap_calloc(wine_callback_data.objectCount, sizeof(*object_name_infos));
+
+    for (i = 0; i < wine_callback_data.objectCount; i++)
+    {
+        object_name_infos[i].sType = callback_data->pObjects[i].sType;
+        object_name_infos[i].pNext = callback_data->pObjects[i].pNext;
+        object_name_infos[i].objectType = callback_data->pObjects[i].objectType;
+        object_name_infos[i].pObjectName = callback_data->pObjects[i].pObjectName;
+
+        if (is_wrapped(callback_data->pObjects[i].objectType))
+        {
+            object_name_infos[i].objectHandle = wine_vk_get_wrapper(object->instance, callback_data->pObjects[i].objectHandle);
+            if (!object_name_infos[i].objectHandle)
+            {
+                WARN("handle conversion failed 0x%s\n", wine_dbgstr_longlong(callback_data->pObjects[i].objectHandle));
+                heap_free(object_name_infos);
+                return VK_FALSE;
+            }
+        }
+        else
+        {
+            object_name_infos[i].objectHandle = callback_data->pObjects[i].objectHandle;
+        }
+    }
+
+    wine_callback_data.pObjects = object_name_infos;
+
+    /* applications should always return VK_FALSE */
+    object->user_callback(severity, message_types, &wine_callback_data, object->user_data);
+
+    heap_free(object_name_infos);
+
+    return VK_FALSE;
+}
+
 static void wine_vk_physical_device_free(struct VkPhysicalDevice_T *phys_dev)
 {
     if (!phys_dev)
@@ -391,6 +476,8 @@ static void wine_vk_init_once(void)
 static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo *src,
         VkInstanceCreateInfo *dst, struct VkInstance_T *object)
 {
+    VkDebugUtilsMessengerCreateInfoEXT *debug_utils_messenger;
+    VkBaseOutStructure *header;
     unsigned int i;
     VkResult res;
 
@@ -402,6 +489,22 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo
         return res;
     }
 
+    object->utils_messenger_count = wine_vk_count_struct(dst, DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);
+    object->utils_messengers =  heap_calloc(object->utils_messenger_count, sizeof(*object->utils_messengers));
+    header = (VkBaseOutStructure *) dst;
+    for (i = 0; i < object->utils_messenger_count; i++)
+    {
+        header = wine_vk_find_struct(header->pNext, DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);
+        debug_utils_messenger = (VkDebugUtilsMessengerCreateInfoEXT *) header;
+        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;
+
+        debug_utils_messenger->pfnUserCallback = (void *) &debug_utils_callback_conversion;
+        debug_utils_messenger->pUserData = &object->utils_messengers[i];
+    }
+
     /* ICDs don't support any layers, so nothing to copy. Modern versions of the loader
      * filter this data out as well.
      */
@@ -425,6 +528,10 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo
             free_VkInstanceCreateInfo_struct_chain(dst);
             return VK_ERROR_EXTENSION_NOT_PRESENT;
         }
+        if (!strcmp(extension_name, "VK_EXT_debug_utils"))
+        {
+            object->enable_wrapper_list = VK_TRUE;
+        }
     }
 
     return VK_SUCCESS;
@@ -522,6 +629,8 @@ static void wine_vk_instance_free(struct VkInstance_T *instance)
     if (instance->instance)
         vk_funcs->p_vkDestroyInstance(instance->instance, NULL /* allocator */);
 
+    heap_free(instance->utils_messengers);
+
     heap_free(instance);
 }
 
@@ -1607,6 +1716,10 @@ static uint64_t unwrap_object_handle(VkObjectType type, uint64_t handle)
 {
     switch (type)
     {
+        case VK_OBJECT_TYPE_INSTANCE:
+            return (uint64_t) (uintptr_t) ((VkInstance) (uintptr_t) handle)->instance;
+        case VK_OBJECT_TYPE_PHYSICAL_DEVICE:
+            return (uint64_t) (uintptr_t) ((VkPhysicalDevice) (uintptr_t) handle)->phys_dev;
         case VK_OBJECT_TYPE_DEVICE:
             return (uint64_t) (uintptr_t) ((VkDevice) (uintptr_t) handle)->device;
         case VK_OBJECT_TYPE_QUEUE:
@@ -1615,6 +1728,8 @@ static uint64_t unwrap_object_handle(VkObjectType type, uint64_t handle)
             return (uint64_t) (uintptr_t) ((VkCommandBuffer) (uintptr_t) handle)->command_buffer;
         case VK_OBJECT_TYPE_COMMAND_POOL:
             return (uint64_t) wine_cmd_pool_from_handle(handle)->command_pool;
+        case VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT:
+            return (uint64_t) wine_debug_utils_messenger_from_handle(handle)->debug_messenger;
         default:
             return handle;
     }
@@ -1685,6 +1800,84 @@ VkResult WINAPI wine_vkGetPhysicalDeviceSurfaceCapabilities2KHR(VkPhysicalDevice
     return res;
 }
 
+VkResult WINAPI wine_vkCreateDebugUtilsMessengerEXT(VkInstance instance, const VkDebugUtilsMessengerCreateInfoEXT *create_info,
+        const VkAllocationCallbacks *allocator, VkDebugUtilsMessengerEXT *messenger)
+{
+    VkDebugUtilsMessengerCreateInfoEXT wine_create_info;
+    struct wine_debug_utils_messenger *object;
+    VkResult res;
+
+    TRACE("%p, %p, %p, %p\n", instance, create_info, allocator, messenger);
+
+    if (allocator)
+        FIXME("Support for allocation callbacks not implemented yet\n");
+
+    if (!(object = heap_alloc_zero(sizeof(*object))))
+        return VK_ERROR_OUT_OF_HOST_MEMORY;
+
+    object->instance = instance;
+    object->user_callback = create_info->pfnUserCallback;
+    object->user_data = create_info->pUserData;
+
+    wine_create_info = *create_info;
+
+    wine_create_info.pfnUserCallback = (void *) &debug_utils_callback_conversion;
+    wine_create_info.pUserData = object;
+
+    res = instance->funcs.p_vkCreateDebugUtilsMessengerEXT(instance->instance, &wine_create_info, NULL,  &object->debug_messenger);
+
+    if (res != VK_SUCCESS)
+    {
+        heap_free(object);
+        return res;
+    }
+
+    WINE_VK_ADD_HANDLE_MAPPING(instance, object, object->debug_messenger);
+    *messenger = wine_debug_utils_messenger_to_handle(object);
+
+    return VK_SUCCESS;
+}
+
+void WINAPI wine_vkDestroyDebugUtilsMessengerEXT(
+        VkInstance instance, VkDebugUtilsMessengerEXT messenger, const VkAllocationCallbacks *allocator)
+{
+    struct wine_debug_utils_messenger *object;
+
+    TRACE("%p, 0x%s, %p\n", instance, wine_dbgstr_longlong(messenger), allocator);
+
+    object = wine_debug_utils_messenger_from_handle(messenger);
+
+    instance->funcs.p_vkDestroyDebugUtilsMessengerEXT(instance->instance, object->debug_messenger, NULL);
+    WINE_VK_REMOVE_HANDLE_MAPPING(instance, object);
+
+    heap_free(object);
+}
+
+void WINAPI wine_vkSubmitDebugUtilsMessageEXT(VkInstance instance, VkDebugUtilsMessageSeverityFlagBitsEXT severity,
+        VkDebugUtilsMessageTypeFlagsEXT types, const VkDebugUtilsMessengerCallbackDataEXT *callback_data)
+{
+    VkDebugUtilsMessengerCallbackDataEXT native_callback_data;
+    VkDebugUtilsObjectNameInfoEXT *object_names;
+    int i;
+
+    TRACE("%p, %#x, %#x, %p\n", instance, severity, types, callback_data);
+
+    native_callback_data = *callback_data;
+    object_names = heap_calloc(callback_data->objectCount, sizeof(*object_names));
+    memcpy(object_names, callback_data->pObjects, callback_data->objectCount * sizeof(*object_names));
+    native_callback_data.pObjects = object_names;
+
+    for (i = 0; i < callback_data->objectCount; i++)
+    {
+        object_names[i].objectHandle =
+                unwrap_object_handle(callback_data->pObjects[i].objectType, callback_data->pObjects[i].objectHandle);
+    }
+
+    thunk_vkSubmitDebugUtilsMessageEXT(instance->instance, severity, types, &native_callback_data);
+
+    heap_free(object_names);
+}
+
 BOOL WINAPI DllMain(HINSTANCE hinst, DWORD reason, void *reserved)
 {
     TRACE("%p, %u, %p\n", hinst, reason, reserved);
diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h
index 146bbd8fa4..17aa3835cf 100644
--- a/dlls/winevulkan/vulkan_private.h
+++ b/dlls/winevulkan/vulkan_private.h
@@ -95,6 +95,8 @@ struct VkDevice_T
     struct wine_vk_mapping mapping;
 };
 
+struct wine_debug_utils_messenger;
+
 struct VkInstance_T
 {
     struct wine_vk_base base;
@@ -111,6 +113,9 @@ struct VkInstance_T
     struct list wrappers;
     SRWLOCK wrapper_lock;
 
+    struct wine_debug_utils_messenger *utils_messengers;
+    uint32_t utils_messenger_count;
+
     unsigned int quirks;
     
     struct wine_vk_mapping mapping;
@@ -158,6 +163,32 @@ static inline VkCommandPool wine_cmd_pool_to_handle(struct wine_cmd_pool *cmd_po
     return (VkCommandPool)(uintptr_t)cmd_pool;
 }
 
+struct wine_debug_utils_messenger
+{
+    struct VkInstance_T *instance; /* parent */
+    VkDebugUtilsMessengerEXT debug_messenger; /* native messenger */
+
+    /* application callback + data */
+    PFN_vkDebugUtilsMessengerCallbackEXT user_callback;
+    void *user_data;
+
+    struct wine_vk_mapping mapping;
+};
+
+static inline struct wine_debug_utils_messenger *wine_debug_utils_messenger_from_handle(
+        VkDebugUtilsMessengerEXT handle)
+{
+    return (struct wine_debug_utils_messenger *)(uintptr_t)handle;
+}
+
+static inline VkDebugUtilsMessengerEXT wine_debug_utils_messenger_to_handle(
+        struct wine_debug_utils_messenger *debug_messenger)
+{
+    return (VkDebugUtilsMessengerEXT)(uintptr_t)debug_messenger;
+}
+
+
+
 void *wine_vk_get_device_proc_addr(const char *name) DECLSPEC_HIDDEN;
 void *wine_vk_get_instance_proc_addr(const char *name) DECLSPEC_HIDDEN;
 
-- 
2.28.0

[PATCH v2 4/4] winevulkan: Implement VK_EXT_debug_utils.

[Liam Middlebrook]

On 9/22/20 7:31 AM, Georg Lehmann wrote:
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49813
>
> Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com>
> ---
>   dlls/winevulkan/make_vulkan      |   8 +-
>   dlls/winevulkan/vulkan.c         | 193 +++++++++++++++++++++++++++++++
>   dlls/winevulkan/vulkan_private.h |  31 +++++
>   3 files changed, 231 insertions(+), 1 deletion(-)
>
> diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> index f4d223daad..77034cfe36 100755
> --- a/dlls/winevulkan/make_vulkan
> +++ b/dlls/winevulkan/make_vulkan
> @@ -92,7 +92,6 @@ UNSUPPORTED_EXTENSIONS = [
>       # plumbing down to the native layer, we will get each message twice as we
>       # use 2 loaders (win32+native), but we may get output from the driver.
>       # In any case callback conversion is required.
> -    "VK_EXT_debug_utils",
>       "VK_EXT_validation_features",
>       "VK_EXT_validation_flags",
>       "VK_KHR_display", # Needs WSI work.
> @@ -225,6 +224,11 @@ FUNCTION_OVERRIDES = {
>       # VK_EXT_calibrated_timestamps
>       "vkGetPhysicalDeviceCalibrateableTimeDomainsEXT" : {"dispatch" : True, "driver" : False, "thunk" : False},
>       "vkGetCalibratedTimestampsEXT" : {"dispatch" : True, "driver" : False, "thunk" : False},
> +
> +    # VK_EXT_debug_utils
> +    "vkCreateDebugUtilsMessengerEXT" : {"dispatch": True, "driver" : False, "thunk" : False},
> +    "vkDestroyDebugUtilsMessengerEXT" : {"dispatch": True, "driver" : False, "thunk" : False},
> +    "vkSubmitDebugUtilsMessageEXT" : {"dispatch": True, "driver" : False, "thunk" : True, "private_thunk" : True},
>   }
>
> STRUCT_CHAIN_CONVERSIONS = [
> @@ -944,6 +948,8 @@ class VkHandle(object):
>
> if self.name == "VkCommandPool":
>               return "wine_cmd_pool_from_handle({0})->command_pool".format(name)
> +        if self.name == "VkDebugUtilsMessengerEXT":
> +            return "wine_debug_utils_messenger_from_handle({0})->debug_messenger".format(name)
>
> native_handle_name = None
>
> diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> index 2747f440ad..11dd0f761e 100644
> --- a/dlls/winevulkan/vulkan.c
> +++ b/dlls/winevulkan/vulkan.c
> @@ -57,6 +57,21 @@ static void *wine_vk_find_struct_(void *s, VkStructureType t)
>       return NULL;
>   }
>
> +#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)
> +{
> +    VkBaseOutStructure *header;

I think VkBaseInStructure would be more appropriate here for preserving 
const-ness, and given the sole use-case of wine_vk_count_struct that you 
introduce below.
> +    uint32_t result = 0;
> +
> +    for (header = s; header; header = header->pNext)
> +    {
> +        if (header->sType == t)
> +            result++;
> +    }
> +
> +    return result;
> +}
> +
>   static void *wine_vk_get_global_proc_addr(const char *name);
>
> static HINSTANCE hinstance;
> @@ -111,6 +126,76 @@ static uint64_t wine_vk_get_wrapper(struct VkInstance_T *instance, uint64_t nati
>       return result;
>   }
>
> +static VkBool32 is_wrapped(VkObjectType type)
> +{
> +    return type == VK_OBJECT_TYPE_INSTANCE ||
> +           type == VK_OBJECT_TYPE_PHYSICAL_DEVICE ||
> +           type == VK_OBJECT_TYPE_DEVICE ||
> +           type == VK_OBJECT_TYPE_QUEUE ||
> +           type == VK_OBJECT_TYPE_COMMAND_BUFFER ||
> +           type == VK_OBJECT_TYPE_COMMAND_POOL ||
> +           type == VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT;
> +}

Can this be made into an auto-generated function from make_vulkan? It'd 
be nicer to keep the amount of manual-touchups to add a new wrapped 
object type to a minimum.
> +
> +static VkBool32 debug_utils_callback_conversion(VkDebugUtilsMessageSeverityFlagBitsEXT severity,
> +    VkDebugUtilsMessageTypeFlagsEXT message_types,
> +#if defined(USE_STRUCT_CONVERSION)
> +    const VkDebugUtilsMessengerCallbackDataEXT_host *callback_data,
> +#else
> +    const VkDebugUtilsMessengerCallbackDataEXT *callback_data,
> +#endif
> +    void *user_data)
> +{
> +    struct VkDebugUtilsMessengerCallbackDataEXT wine_callback_data;
> +    VkDebugUtilsObjectNameInfoEXT *object_name_infos;
> +    struct wine_debug_utils_messenger *object;
> +    int i;
> +
> +    object = user_data;
> +
> +    if (!object->instance->instance)
> +    {
> +        /* instance wasn't yet created, this is a message from the native loader*/

nit: loader */
> +        return VK_FALSE;
> +    }
> +
> +    wine_callback_data = *((VkDebugUtilsMessengerCallbackDataEXT *) callback_data);
> +
> +    object_name_infos = heap_calloc(wine_callback_data.objectCount, sizeof(*object_name_infos));
> +
> +    for (i = 0; i < wine_callback_data.objectCount; i++)
> +    {
> +        object_name_infos[i].sType = callback_data->pObjects[i].sType;
> +        object_name_infos[i].pNext = callback_data->pObjects[i].pNext;
> +        object_name_infos[i].objectType = callback_data->pObjects[i].objectType;
> +        object_name_infos[i].pObjectName = callback_data->pObjects[i].pObjectName;
> +
> +        if (is_wrapped(callback_data->pObjects[i].objectType))
> +        {
> +            object_name_infos[i].objectHandle = wine_vk_get_wrapper(object->instance, callback_data->pObjects[i].objectHandle);
> +            if (!object_name_infos[i].objectHandle)
> +            {
> +                WARN("handle conversion failed 0x%s\n", wine_dbgstr_longlong(callback_data->pObjects[i].objectHandle));
> +                heap_free(object_name_infos);
> +                return VK_FALSE;

Given that the function will exit early, and a callback will not be 
serviced back to WINE-side listeners, I think this should be an ERROR 
and not a WARN.
> +            }
> +        }
> +        else
> +        {
> +            object_name_infos[i].objectHandle = callback_data->pObjects[i].objectHandle;
> +        }
> +    }
> +
> +    wine_callback_data.pObjects = object_name_infos;
> +
> +    /* applications should always return VK_FALSE */
> +    object->user_callback(severity, message_types, &wine_callback_data, object->user_data);
> +
> +    heap_free(object_name_infos);
> +
> +    return VK_FALSE;

The VK specification states:
> The callback returns a VkBool32, which is interpreted in a layer-specified manner. The application should always return VK_FALSE. The VK_TRUE value is reserved for use in layer development.

So I think we should return the value that user_callback returned to 
remain faithful to whomever registered a callback and what they returned.
> +}
> +
>   static void wine_vk_physical_device_free(struct VkPhysicalDevice_T *phys_dev)
>   {
>       if (!phys_dev)
> @@ -391,6 +476,8 @@ static void wine_vk_init_once(void)
>   static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo *src,
>           VkInstanceCreateInfo *dst, struct VkInstance_T *object)
>   {
> +    VkDebugUtilsMessengerCreateInfoEXT *debug_utils_messenger;
> +    VkBaseOutStructure *header;
>       unsigned int i;
>       VkResult res;
>
> @@ -402,6 +489,22 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo
>           return res;
>       }
>
> +    object->utils_messenger_count = wine_vk_count_struct(dst, DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);
> +    object->utils_messengers =  heap_calloc(object->utils_messenger_count, sizeof(*object->utils_messengers));
> +    header = (VkBaseOutStructure *) dst;
> +    for (i = 0; i < object->utils_messenger_count; i++)
> +    {
> +        header = wine_vk_find_struct(header->pNext, DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);
> +        debug_utils_messenger = (VkDebugUtilsMessengerCreateInfoEXT *) header;
> +        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;
> +
> +        debug_utils_messenger->pfnUserCallback = (void *) &debug_utils_callback_conversion;
> +        debug_utils_messenger->pUserData = &object->utils_messengers[i];

I don't think we should be abusing VkBaseOutStructure like this as a way 
to circumvent const-ness of a read-only pNext chain. When we detect that 
there are VkDebugUtilsMessengerCreateInfoEXT structures in the pNext 
chain I think it would be cleaner to create a local copy and then use 
that however we need to.
> +    }
> +
>       /* ICDs don't support any layers, so nothing to copy. Modern versions of the loader
>        * filter this data out as well.
>        */
> @@ -425,6 +528,10 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo
>               free_VkInstanceCreateInfo_struct_chain(dst);
>               return VK_ERROR_EXTENSION_NOT_PRESENT;
>           }
> +        if (!strcmp(extension_name, "VK_EXT_debug_utils"))
> +        {
> +            object->enable_wrapper_list = VK_TRUE;
> +        }
>       }
>
> return VK_SUCCESS;
> @@ -522,6 +629,8 @@ static void wine_vk_instance_free(struct VkInstance_T *instance)
>       if (instance->instance)
>           vk_funcs->p_vkDestroyInstance(instance->instance, NULL /* allocator */);
>
> +    heap_free(instance->utils_messengers);
> +
>       heap_free(instance);
>   }
>
> @@ -1607,6 +1716,10 @@ static uint64_t unwrap_object_handle(VkObjectType type, uint64_t handle)
>   {
>       switch (type)
>       {
> +        case VK_OBJECT_TYPE_INSTANCE:
> +            return (uint64_t) (uintptr_t) ((VkInstance) (uintptr_t) handle)->instance;
> +        case VK_OBJECT_TYPE_PHYSICAL_DEVICE:
> +            return (uint64_t) (uintptr_t) ((VkPhysicalDevice) (uintptr_t) handle)->phys_dev;

Were these meant to go into this commit? I can see why they're needed 
for debug_utils, but it just feels out of place to see them added in 
this commit.


Thanks,

Liam Middlebrook
> case VK_OBJECT_TYPE_DEVICE:
>               return (uint64_t) (uintptr_t) ((VkDevice) (uintptr_t) handle)->device;
>           case VK_OBJECT_TYPE_QUEUE:
> @@ -1615,6 +1728,8 @@ static uint64_t unwrap_object_handle(VkObjectType type, uint64_t handle)
>               return (uint64_t) (uintptr_t) ((VkCommandBuffer) (uintptr_t) handle)->command_buffer;
>           case VK_OBJECT_TYPE_COMMAND_POOL:
>               return (uint64_t) wine_cmd_pool_from_handle(handle)->command_pool;
> +        case VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT:
> +            return (uint64_t) wine_debug_utils_messenger_from_handle(handle)->debug_messenger;
>           default:
>               return handle;
>       }
> @@ -1685,6 +1800,84 @@ VkResult WINAPI wine_vkGetPhysicalDeviceSurfaceCapabilities2KHR(VkPhysicalDevice
>       return res;
>   }
>
> +VkResult WINAPI wine_vkCreateDebugUtilsMessengerEXT(VkInstance instance, const VkDebugUtilsMessengerCreateInfoEXT *create_info,
> +        const VkAllocationCallbacks *allocator, VkDebugUtilsMessengerEXT *messenger)
> +{
> +    VkDebugUtilsMessengerCreateInfoEXT wine_create_info;
> +    struct wine_debug_utils_messenger *object;
> +    VkResult res;
> +
> +    TRACE("%p, %p, %p, %p\n", instance, create_info, allocator, messenger);
> +
> +    if (allocator)
> +        FIXME("Support for allocation callbacks not implemented yet\n");
> +
> +    if (!(object = heap_alloc_zero(sizeof(*object))))
> +        return VK_ERROR_OUT_OF_HOST_MEMORY;
> +
> +    object->instance = instance;
> +    object->user_callback = create_info->pfnUserCallback;
> +    object->user_data = create_info->pUserData;
> +
> +    wine_create_info = *create_info;
> +
> +    wine_create_info.pfnUserCallback = (void *) &debug_utils_callback_conversion;
> +    wine_create_info.pUserData = object;
> +
> +    res = instance->funcs.p_vkCreateDebugUtilsMessengerEXT(instance->instance, &wine_create_info, NULL,  &object->debug_messenger);
> +
> +    if (res != VK_SUCCESS)
> +    {
> +        heap_free(object);
> +        return res;
> +    }
> +
> +    WINE_VK_ADD_HANDLE_MAPPING(instance, object, object->debug_messenger);
> +    *messenger = wine_debug_utils_messenger_to_handle(object);
> +
> +    return VK_SUCCESS;
> +}
> +
> +void WINAPI wine_vkDestroyDebugUtilsMessengerEXT(
> +        VkInstance instance, VkDebugUtilsMessengerEXT messenger, const VkAllocationCallbacks *allocator)
> +{
> +    struct wine_debug_utils_messenger *object;
> +
> +    TRACE("%p, 0x%s, %p\n", instance, wine_dbgstr_longlong(messenger), allocator);
> +
> +    object = wine_debug_utils_messenger_from_handle(messenger);
> +
> +    instance->funcs.p_vkDestroyDebugUtilsMessengerEXT(instance->instance, object->debug_messenger, NULL);
> +    WINE_VK_REMOVE_HANDLE_MAPPING(instance, object);
> +
> +    heap_free(object);
> +}
> +
> +void WINAPI wine_vkSubmitDebugUtilsMessageEXT(VkInstance instance, VkDebugUtilsMessageSeverityFlagBitsEXT severity,
> +        VkDebugUtilsMessageTypeFlagsEXT types, const VkDebugUtilsMessengerCallbackDataEXT *callback_data)
> +{
> +    VkDebugUtilsMessengerCallbackDataEXT native_callback_data;
> +    VkDebugUtilsObjectNameInfoEXT *object_names;
> +    int i;
> +
> +    TRACE("%p, %#x, %#x, %p\n", instance, severity, types, callback_data);
> +
> +    native_callback_data = *callback_data;
> +    object_names = heap_calloc(callback_data->objectCount, sizeof(*object_names));
> +    memcpy(object_names, callback_data->pObjects, callback_data->objectCount * sizeof(*object_names));
> +    native_callback_data.pObjects = object_names;
> +
> +    for (i = 0; i < callback_data->objectCount; i++)
> +    {
> +        object_names[i].objectHandle =
> +                unwrap_object_handle(callback_data->pObjects[i].objectType, callback_data->pObjects[i].objectHandle);
> +    }
> +
> +    thunk_vkSubmitDebugUtilsMessageEXT(instance->instance, severity, types, &native_callback_data);
> +
> +    heap_free(object_names);
> +}
> +
>   BOOL WINAPI DllMain(HINSTANCE hinst, DWORD reason, void *reserved)
>   {
>       TRACE("%p, %u, %p\n", hinst, reason, reserved);
> diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h
> index 146bbd8fa4..17aa3835cf 100644
> --- a/dlls/winevulkan/vulkan_private.h
> +++ b/dlls/winevulkan/vulkan_private.h
> @@ -95,6 +95,8 @@ struct VkDevice_T
>       struct wine_vk_mapping mapping;
>   };
>
> +struct wine_debug_utils_messenger;
> +
>   struct VkInstance_T
>   {
>       struct wine_vk_base base;
> @@ -111,6 +113,9 @@ struct VkInstance_T
>       struct list wrappers;
>       SRWLOCK wrapper_lock;
>
> +    struct wine_debug_utils_messenger *utils_messengers;
> +    uint32_t utils_messenger_count;
> +
>       unsigned int quirks;
>
> struct wine_vk_mapping mapping;
> @@ -158,6 +163,32 @@ static inline VkCommandPool wine_cmd_pool_to_handle(struct wine_cmd_pool *cmd_po
>       return (VkCommandPool)(uintptr_t)cmd_pool;
>   }
>
> +struct wine_debug_utils_messenger
> +{
> +    struct VkInstance_T *instance; /* parent */
> +    VkDebugUtilsMessengerEXT debug_messenger; /* native messenger */
> +
> +    /* application callback + data */
> +    PFN_vkDebugUtilsMessengerCallbackEXT user_callback;
> +    void *user_data;
> +
> +    struct wine_vk_mapping mapping;
> +};
> +
> +static inline struct wine_debug_utils_messenger *wine_debug_utils_messenger_from_handle(
> +        VkDebugUtilsMessengerEXT handle)
> +{
> +    return (struct wine_debug_utils_messenger *)(uintptr_t)handle;
> +}
> +
> +static inline VkDebugUtilsMessengerEXT wine_debug_utils_messenger_to_handle(
> +        struct wine_debug_utils_messenger *debug_messenger)
> +{
> +    return (VkDebugUtilsMessengerEXT)(uintptr_t)debug_messenger;
> +}
> +
> +
> +
>   void *wine_vk_get_device_proc_addr(const char *name) DECLSPEC_HIDDEN;
>   void *wine_vk_get_instance_proc_addr(const char *name) DECLSPEC_HIDDEN;

[PATCH v2 4/4] winevulkan: Implement VK_EXT_debug_utils.

[Dad Schoorse]

On 23. Sept. 2020, 00:03, Liam Middlebrook wrote:
> On 9/22/20 7:31 AM, Georg Lehmann wrote:
> > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49813
> >
> > Signed-off-by: Georg Lehmann <dadschoorse at gmail.com>
> > ---
> >   dlls/winevulkan/make_vulkan      |   8 +-
> >   dlls/winevulkan/vulkan.c         | 193 +++++++++++++++++++++++++++++++
> >   dlls/winevulkan/vulkan_private.h |  31 +++++
> >   3 files changed, 231 insertions(+), 1 deletion(-)
> >
> > diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> > index f4d223daad..77034cfe36 100755
> > --- a/dlls/winevulkan/make_vulkan
> > +++ b/dlls/winevulkan/make_vulkan
> > @@ -92,7 +92,6 @@ UNSUPPORTED_EXTENSIONS = [
> >       # plumbing down to the native layer, we will get each message
> > twice as we
> >       # use 2 loaders (win32+native), but we may get output from the
> > driver.
> >       # In any case callback conversion is required.
> > -    "VK_EXT_debug_utils",
> >       "VK_EXT_validation_features",
> >       "VK_EXT_validation_flags",
> >       "VK_KHR_display", # Needs WSI work.
> > @@ -225,6 +224,11 @@ FUNCTION_OVERRIDES = {
> >       # VK_EXT_calibrated_timestamps
> >       "vkGetPhysicalDeviceCalibrateableTimeDomainsEXT" : {"dispatch" :
> > True, "driver" : False, "thunk" : False},
> >       "vkGetCalibratedTimestampsEXT" : {"dispatch" : True, "driver" :
> > False, "thunk" : False},
> > +
> > +    # VK_EXT_debug_utils
> > +    "vkCreateDebugUtilsMessengerEXT" : {"dispatch": True, "driver" :
> > False, "thunk" : False},
> > +    "vkDestroyDebugUtilsMessengerEXT" : {"dispatch": True, "driver" :
> > False, "thunk" : False},
> > +    "vkSubmitDebugUtilsMessageEXT" : {"dispatch": True, "driver" :
> > False, "thunk" : True, "private_thunk" : True},
> >   }
> >
> > STRUCT_CHAIN_CONVERSIONS = [
> > @@ -944,6 +948,8 @@ class VkHandle(object):
> >
> > if self.name == "VkCommandPool":
> >               return
> > "wine_cmd_pool_from_handle({0})->command_pool".format(name)
> > +        if self.name == "VkDebugUtilsMessengerEXT":
> > +            return
> > "wine_debug_utils_messenger_from_handle({0})->debug_messenger".format(name)
> >
> > native_handle_name = None
> >
> > diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> > index 2747f440ad..11dd0f761e 100644
> > --- a/dlls/winevulkan/vulkan.c
> > +++ b/dlls/winevulkan/vulkan.c
> > @@ -57,6 +57,21 @@ static void *wine_vk_find_struct_(void *s,
> > VkStructureType t)
> >       return NULL;
> >   }
> >
> > +#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)
> > +{
> > +    VkBaseOutStructure *header;
>
> I think VkBaseInStructure would be more appropriate here for preserving
> const-ness, and given the sole use-case of wine_vk_count_struct that you
> introduce below.
> > +    uint32_t result = 0;
> > +
> > +    for (header = s; header; header = header->pNext)
> > +    {
> > +        if (header->sType == t)
> > +            result++;
> > +    }
> > +
> > +    return result;
> > +}
> > +
> >   static void *wine_vk_get_global_proc_addr(const char *name);
> >
> > static HINSTANCE hinstance;
> > @@ -111,6 +126,76 @@ static uint64_t wine_vk_get_wrapper(struct
> > VkInstance_T *instance, uint64_t nati
> >       return result;
> >   }
> >
> > +static VkBool32 is_wrapped(VkObjectType type)
> > +{
> > +    return type == VK_OBJECT_TYPE_INSTANCE ||
> > +           type == VK_OBJECT_TYPE_PHYSICAL_DEVICE ||
> > +           type == VK_OBJECT_TYPE_DEVICE ||
> > +           type == VK_OBJECT_TYPE_QUEUE ||
> > +           type == VK_OBJECT_TYPE_COMMAND_BUFFER ||
> > +           type == VK_OBJECT_TYPE_COMMAND_POOL ||
> > +           type == VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT;
> > +}
>
> Can this be made into an auto-generated function from make_vulkan? It'd
> be nicer to keep the amount of manual-touchups to add a new wrapped
> object type to a minimum.

I later wanted to auto-generate this and unwrap_object_handle in a separate
patch series, but I can put it in this patch series if you want.
> > +
> > +static VkBool32
> > debug_utils_callback_conversion(VkDebugUtilsMessageSeverityFlagBitsEXT
> > severity,
> > +    VkDebugUtilsMessageTypeFlagsEXT message_types,
> > +#if defined(USE_STRUCT_CONVERSION)
> > +    const VkDebugUtilsMessengerCallbackDataEXT_host *callback_data,
> > +#else
> > +    const VkDebugUtilsMessengerCallbackDataEXT *callback_data,
> > +#endif
> > +    void *user_data)
> > +{
> > +    struct VkDebugUtilsMessengerCallbackDataEXT wine_callback_data;
> > +    VkDebugUtilsObjectNameInfoEXT *object_name_infos;
> > +    struct wine_debug_utils_messenger *object;
> > +    int i;
> > +
> > +    object = user_data;
> > +
> > +    if (!object->instance->instance)
> > +    {
> > +        /* instance wasn't yet created, this is a message from the
> > native loader*/
>
> nit: loader */
> > +        return VK_FALSE;
> > +    }
> > +
> > +    wine_callback_data = *((VkDebugUtilsMessengerCallbackDataEXT *)
> > callback_data);
> > +
> > +    object_name_infos = heap_calloc(wine_callback_data.objectCount,
> > sizeof(*object_name_infos));
> > +
> > +    for (i = 0; i < wine_callback_data.objectCount; i++)
> > +    {
> > +        object_name_infos[i].sType = callback_data->pObjects[i].sType;
> > +        object_name_infos[i].pNext = callback_data->pObjects[i].pNext;
> > +        object_name_infos[i].objectType =
> > callback_data->pObjects[i].objectType;
> > +        object_name_infos[i].pObjectName =
> > callback_data->pObjects[i].pObjectName;
> > +
> > +        if (is_wrapped(callback_data->pObjects[i].objectType))
> > +        {
> > +            object_name_infos[i].objectHandle =
> > wine_vk_get_wrapper(object->instance,
> > callback_data->pObjects[i].objectHandle);
> > +            if (!object_name_infos[i].objectHandle)
> > +            {
> > +                WARN("handle conversion failed 0x%s\n",
> > wine_dbgstr_longlong(callback_data->pObjects[i].objectHandle));
> > +                heap_free(object_name_infos);
> > +                return VK_FALSE;
>
> Given that the function will exit early, and a callback will not be
> serviced back to WINE-side listeners, I think this should be an ERROR
> and not a WARN.

There are some common situations where this occurs. The loader/validation
tools
send messages for a new handle during its creation. In that case winevulkan
doesn't know the handle yet.
I didn't want to introduce too much log spam, that's why it's WARN.
> > +            }
> > +        }
> > +        else
> > +        {
> > +            object_name_infos[i].objectHandle =
> > callback_data->pObjects[i].objectHandle;
> > +        }
> > +    }
> > +
> > +    wine_callback_data.pObjects = object_name_infos;
> > +
> > +    /* applications should always return VK_FALSE */
> > +    object->user_callback(severity, message_types, &wine_callback_data,
> > object->user_data);
> > +
> > +    heap_free(object_name_infos);
> > +
> > +    return VK_FALSE;
>
> The VK specification states:
> > The callback returns a VkBool32, which is interpreted in a
> > layer-specified manner. The application should always return VK_FALSE. The
> > VK_TRUE value is reserved for use in layer development.
>
> So I think we should return the value that user_callback returned to
> remain faithful to whomever registered a callback and what they returned.
> > +}
> > +
> >   static void wine_vk_physical_device_free(struct VkPhysicalDevice_T
> > *phys_dev)
> >   {
> >       if (!phys_dev)
> > @@ -391,6 +476,8 @@ static void wine_vk_init_once(void)
> >   static VkResult wine_vk_instance_convert_create_info(const
> > VkInstanceCreateInfo *src,
> >           VkInstanceCreateInfo *dst, struct VkInstance_T *object)
> >   {
> > +    VkDebugUtilsMessengerCreateInfoEXT *debug_utils_messenger;
> > +    VkBaseOutStructure *header;
> >       unsigned int i;
> >       VkResult res;
> >
> > @@ -402,6 +489,22 @@ static VkResult
> > wine_vk_instance_convert_create_info(const VkInstanceCreateInfo
> >           return res;
> >       }
> >
> > +    object->utils_messenger_count = wine_vk_count_struct(dst,
> > DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);
> > +    object->utils_messengers =
> >  heap_calloc(object->utils_messenger_count,
> > sizeof(*object->utils_messengers));
> > +    header = (VkBaseOutStructure *) dst;
> > +    for (i = 0; i < object->utils_messenger_count; i++)
> > +    {
> > +        header = wine_vk_find_struct(header->pNext,
> > DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);
> > +        debug_utils_messenger = (VkDebugUtilsMessengerCreateInfoEXT *)
> > header;
> > +        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;
> > +
> > +        debug_utils_messenger->pfnUserCallback = (void *)
> > &debug_utils_callback_conversion;
> > +        debug_utils_messenger->pUserData = &object->utils_messengers[i];
>
> I don't think we should be abusing VkBaseOutStructure like this as a way
> to circumvent const-ness of a read-only pNext chain. When we detect that
> there are VkDebugUtilsMessengerCreateInfoEXT structures in the pNext
> chain I think it would be cleaner to create a local copy and then use
> that however we need to.

I'm not really happy with this either, but
convert_VkInstanceCreateInfo_struct_chain already copies the pNext chain.
So nothing changed here will affect the application running in wine.
> > +    }
> > +
> >       /* ICDs don't support any layers, so nothing to copy. Modern
> > versions of the loader
> >        * filter this data out as well.
> >        */
> > @@ -425,6 +528,10 @@ static VkResult
> > wine_vk_instance_convert_create_info(const VkInstanceCreateInfo
> >               free_VkInstanceCreateInfo_struct_chain(dst);
> >               return VK_ERROR_EXTENSION_NOT_PRESENT;
> >           }
> > +        if (!strcmp(extension_name, "VK_EXT_debug_utils"))
> > +        {
> > +            object->enable_wrapper_list = VK_TRUE;
> > +        }
> >       }
> >
> > return VK_SUCCESS;
> > @@ -522,6 +629,8 @@ static void wine_vk_instance_free(struct
> > VkInstance_T *instance)
> >       if (instance->instance)
> >           vk_funcs->p_vkDestroyInstance(instance->instance, NULL /*
> > allocator */);
> >
> > +    heap_free(instance->utils_messengers);
> > +
> >       heap_free(instance);
> >   }
> >
> > @@ -1607,6 +1716,10 @@ static uint64_t unwrap_object_handle(VkObjectType
> > type, uint64_t handle)
> >   {
> >       switch (type)
> >       {
> > +        case VK_OBJECT_TYPE_INSTANCE:
> > +            return (uint64_t) (uintptr_t) ((VkInstance) (uintptr_t)
> > handle)->instance;
> > +        case VK_OBJECT_TYPE_PHYSICAL_DEVICE:
> > +            return (uint64_t) (uintptr_t) ((VkPhysicalDevice)
> > (uintptr_t) handle)->phys_dev;
>
> Were these meant to go into this commit? I can see why they're needed
> for debug_utils, but it just feels out of place to see them added in
> this commit.

I thought it wasn't worth a separate commit. Ideally we should auto-generate
this function anyway to make adding new wrapped handles easier.

Thanks,

Georg Lehmann
> Thanks,
>
> Liam Middlebrook
> > case VK_OBJECT_TYPE_DEVICE:
> >               return (uint64_t) (uintptr_t) ((VkDevice) (uintptr_t)

handle)->device;
> > case VK_OBJECT_TYPE_QUEUE:
> > @@ -1615,6 +1728,8 @@ static uint64_t unwrap_object_handle(VkObjectType
>
> type, uint64_t handle)
> > return (uint64_t) (uintptr_t) ((VkCommandBuffer)
>
> (uintptr_t) handle)->command_buffer;
> > case VK_OBJECT_TYPE_COMMAND_POOL:
> >               return (uint64_t)
>
> wine_cmd_pool_from_handle(handle)->command_pool;
> > +        case VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT:
> > +            return (uint64_t)
> > wine_debug_utils_messenger_from_handle(handle)->debug_messenger;
> >           default:
> >               return handle;
> >       }
> > @@ -1685,6 +1800,84 @@ VkResult WINAPI
> > wine_vkGetPhysicalDeviceSurfaceCapabilities2KHR(VkPhysicalDevice
> >       return res;
> >   }
> >
> > +VkResult WINAPI wine_vkCreateDebugUtilsMessengerEXT(VkInstance
> > instance, const VkDebugUtilsMessengerCreateInfoEXT *create_info,
> > +        const VkAllocationCallbacks *allocator,
> > VkDebugUtilsMessengerEXT *messenger)
> > +{
> > +    VkDebugUtilsMessengerCreateInfoEXT wine_create_info;
> > +    struct wine_debug_utils_messenger *object;
> > +    VkResult res;
> > +
> > +    TRACE("%p, %p, %p, %p\n", instance, create_info, allocator,
> > messenger);
> > +
> > +    if (allocator)
> > +        FIXME("Support for allocation callbacks not implemented yet\n");
> > +
> > +    if (!(object = heap_alloc_zero(sizeof(*object))))
> > +        return VK_ERROR_OUT_OF_HOST_MEMORY;
> > +
> > +    object->instance = instance;
> > +    object->user_callback = create_info->pfnUserCallback;
> > +    object->user_data = create_info->pUserData;
> > +
> > +    wine_create_info = *create_info;
> > +
> > +    wine_create_info.pfnUserCallback = (void *)
> > &debug_utils_callback_conversion;
> > +    wine_create_info.pUserData = object;
> > +
> > +    res =
> > instance->funcs.p_vkCreateDebugUtilsMessengerEXT(instance->instance,
> > &wine_create_info, NULL,  &object->debug_messenger);
> > +
> > +    if (res != VK_SUCCESS)
> > +    {
> > +        heap_free(object);
> > +        return res;
> > +    }
> > +
> > +    WINE_VK_ADD_HANDLE_MAPPING(instance, object,
> > object->debug_messenger);
> > +    *messenger = wine_debug_utils_messenger_to_handle(object);
> > +
> > +    return VK_SUCCESS;
> > +}
> > +
> > +void WINAPI wine_vkDestroyDebugUtilsMessengerEXT(
> > +        VkInstance instance, VkDebugUtilsMessengerEXT messenger, const
> > VkAllocationCallbacks *allocator)
> > +{
> > +    struct wine_debug_utils_messenger *object;
> > +
> > +    TRACE("%p, 0x%s, %p\n", instance, wine_dbgstr_longlong(messenger),
> > allocator);
> > +
> > +    object = wine_debug_utils_messenger_from_handle(messenger);
> > +
> > +
> >  instance->funcs.p_vkDestroyDebugUtilsMessengerEXT(instance->instance,
> > object->debug_messenger, NULL);
> > +    WINE_VK_REMOVE_HANDLE_MAPPING(instance, object);
> > +
> > +    heap_free(object);
> > +}
> > +
> > +void WINAPI wine_vkSubmitDebugUtilsMessageEXT(VkInstance instance,
> > VkDebugUtilsMessageSeverityFlagBitsEXT severity,
> > +        VkDebugUtilsMessageTypeFlagsEXT types, const
> > VkDebugUtilsMessengerCallbackDataEXT *callback_data)
> > +{
> > +    VkDebugUtilsMessengerCallbackDataEXT native_callback_data;
> > +    VkDebugUtilsObjectNameInfoEXT *object_names;
> > +    int i;
> > +
> > +    TRACE("%p, %#x, %#x, %p\n", instance, severity, types,
> > callback_data);
> > +
> > +    native_callback_data = *callback_data;
> > +    object_names = heap_calloc(callback_data->objectCount,
> > sizeof(*object_names));
> > +    memcpy(object_names, callback_data->pObjects,
> > callback_data->objectCount * sizeof(*object_names));
> > +    native_callback_data.pObjects = object_names;
> > +
> > +    for (i = 0; i < callback_data->objectCount; i++)
> > +    {
> > +        object_names[i].objectHandle =
> > +
> >  unwrap_object_handle(callback_data->pObjects[i].objectType,
> > callback_data->pObjects[i].objectHandle);
> > +    }
> > +
> > +    thunk_vkSubmitDebugUtilsMessageEXT(instance->instance, severity,
> > types, &native_callback_data);
> > +
> > +    heap_free(object_names);
> > +}
> > +
> >   BOOL WINAPI DllMain(HINSTANCE hinst, DWORD reason, void *reserved)
> >   {
> >       TRACE("%p, %u, %p\n", hinst, reason, reserved);
> > diff --git a/dlls/winevulkan/vulkan_private.h
> > b/dlls/winevulkan/vulkan_private.h
> > index 146bbd8fa4..17aa3835cf 100644
> > --- a/dlls/winevulkan/vulkan_private.h
> > +++ b/dlls/winevulkan/vulkan_private.h
> > @@ -95,6 +95,8 @@ struct VkDevice_T
> >       struct wine_vk_mapping mapping;
> >   };
> >
> > +struct wine_debug_utils_messenger;
> > +
> >   struct VkInstance_T
> >   {
> >       struct wine_vk_base base;
> > @@ -111,6 +113,9 @@ struct VkInstance_T
> >       struct list wrappers;
> >       SRWLOCK wrapper_lock;
> >
> > +    struct wine_debug_utils_messenger *utils_messengers;
> > +    uint32_t utils_messenger_count;
> > +
> >       unsigned int quirks;
> >
> > struct wine_vk_mapping mapping;
> > @@ -158,6 +163,32 @@ static inline VkCommandPool
> > wine_cmd_pool_to_handle(struct wine_cmd_pool *cmd_po
> >       return (VkCommandPool)(uintptr_t)cmd_pool;
> >   }
> >
> > +struct wine_debug_utils_messenger
> > +{
> > +    struct VkInstance_T *instance; /* parent */
> > +    VkDebugUtilsMessengerEXT debug_messenger; /* native messenger */
> > +
> > +    /* application callback + data */
> > +    PFN_vkDebugUtilsMessengerCallbackEXT user_callback;
> > +    void *user_data;
> > +
> > +    struct wine_vk_mapping mapping;
> > +};
> > +
> > +static inline struct wine_debug_utils_messenger
> > *wine_debug_utils_messenger_from_handle(
> > +        VkDebugUtilsMessengerEXT handle)
> > +{
> > +    return (struct wine_debug_utils_messenger *)(uintptr_t)handle;
> > +}
> > +
> > +static inline VkDebugUtilsMessengerEXT
> > wine_debug_utils_messenger_to_handle(
> > +        struct wine_debug_utils_messenger *debug_messenger)
> > +{
> > +    return (VkDebugUtilsMessengerEXT)(uintptr_t)debug_messenger;
> > +}
> > +
> > +
> > +
> >   void *wine_vk_get_device_proc_addr(const char *name) DECLSPEC_HIDDEN;
> >   void *wine_vk_get_instance_proc_addr(const char *name) DECLSPEC_HIDDEN;

[PATCH v2 4/4] winevulkan: Implement VK_EXT_debug_utils.

[Liam Middlebrook]

On 9/22/20 3:56 PM, Dad Schoorse wrote:
> On 23. Sept. 2020, 00:03, Liam Middlebrook wrote:
> > On 9/22/20 7:31 AM, Georg Lehmann wrote:
> > > Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49813
> > >
> > > Signed-off-by: Georg Lehmann <dadschoorse at gmail.com 
> > > <http://gmail.com>>
> > > ---
> > >   dlls/winevulkan/make_vulkan      |   8 +-
> > >   dlls/winevulkan/vulkan.c         | 193 +++++++++++++++++++++++++++++++
> > >   dlls/winevulkan/vulkan_private.h |  31 +++++
> > >   3 files changed, 231 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> > > index f4d223daad..77034cfe36 100755
> > > --- a/dlls/winevulkan/make_vulkan
> > > +++ b/dlls/winevulkan/make_vulkan
> > > @@ -92,7 +92,6 @@ UNSUPPORTED_EXTENSIONS = [
> > >       # plumbing down to the native layer, we will get each message 
> > > twice as we
> > >       # use 2 loaders (win32+native), but we may get output from the 
> > > driver.
> > >       # In any case callback conversion is required.
> > > -    "VK_EXT_debug_utils",
> > >       "VK_EXT_validation_features",
> > >       "VK_EXT_validation_flags",
> > >       "VK_KHR_display", # Needs WSI work.
> > > @@ -225,6 +224,11 @@ FUNCTION_OVERRIDES = {
> > >       # VK_EXT_calibrated_timestamps
> > >       "vkGetPhysicalDeviceCalibrateableTimeDomainsEXT" : {"dispatch" 
> > > : True, "driver" : False, "thunk" : False},
> > >       "vkGetCalibratedTimestampsEXT" : {"dispatch" : True, "driver" 
> > > : False, "thunk" : False},
> > > +
> > > +    # VK_EXT_debug_utils
> > > +    "vkCreateDebugUtilsMessengerEXT" : {"dispatch": True, "driver" 
> > > : False, "thunk" : False},
> > > +    "vkDestroyDebugUtilsMessengerEXT" : {"dispatch": True, "driver" 
> > > : False, "thunk" : False},
> > > +    "vkSubmitDebugUtilsMessageEXT" : {"dispatch": True, "driver" : 
> > > False, "thunk" : True, "private_thunk" : True},
> > >   }
> > >
> > >   STRUCT_CHAIN_CONVERSIONS = [
> > > @@ -944,6 +948,8 @@ class VkHandle(object):
> > >
> > >           if self.name <http://self.name> == "VkCommandPool":
> > >               return 
> > > "wine_cmd_pool_from_handle({0})->command_pool".format(name)
> > > +        if self.name <http://self.name> == "VkDebugUtilsMessengerEXT":
> > > +            return 
> > > "wine_debug_utils_messenger_from_handle({0})->debug_messenger".format(name)
> > >
> > >           native_handle_name = None
> > >
> > > diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> > > index 2747f440ad..11dd0f761e 100644
> > > --- a/dlls/winevulkan/vulkan.c
> > > +++ b/dlls/winevulkan/vulkan.c
> > > @@ -57,6 +57,21 @@ static void *wine_vk_find_struct_(void *s, 
> > > VkStructureType t)
> > >       return NULL;
> > >   }
> > >
> > > +#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)
> > > +{
> > > +    VkBaseOutStructure *header;
> >
> > I think VkBaseInStructure would be more appropriate here for preserving
> > const-ness, and given the sole use-case of wine_vk_count_struct that you
> > introduce below.
> > > +    uint32_t result = 0;
> > > +
> > > +    for (header = s; header; header = header->pNext)
> > > +    {
> > > +        if (header->sType == t)
> > > +            result++;
> > > +    }
> > > +
> > > +    return result;
> > > +}
> > > +
> > >   static void *wine_vk_get_global_proc_addr(const char *name);
> > >
> > >   static HINSTANCE hinstance;
> > > @@ -111,6 +126,76 @@ static uint64_t wine_vk_get_wrapper(struct 
> > > VkInstance_T *instance, uint64_t nati
> > >       return result;
> > >   }
> > >
> > > +static VkBool32 is_wrapped(VkObjectType type)
> > > +{
> > > +    return type == VK_OBJECT_TYPE_INSTANCE ||
> > > +           type == VK_OBJECT_TYPE_PHYSICAL_DEVICE ||
> > > +           type == VK_OBJECT_TYPE_DEVICE ||
> > > +           type == VK_OBJECT_TYPE_QUEUE ||
> > > +           type == VK_OBJECT_TYPE_COMMAND_BUFFER ||
> > > +           type == VK_OBJECT_TYPE_COMMAND_POOL ||
> > > +           type == VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT;
> > > +}
> >
> > Can this be made into an auto-generated function from make_vulkan? It'd
> > be nicer to keep the amount of manual-touchups to add a new wrapped
> > object type to a minimum.
>
> I later wanted to auto-generate this and unwrap_object_handle in a separate
> patch series, but I can put it in this patch series if you want.

Sure I'd be fine with this in a follow-up in the series if you were 
planning something a bit more complex with it.
> > > +
> > > +static VkBool32 
> > > debug_utils_callback_conversion(VkDebugUtilsMessageSeverityFlagBitsEXT 
> > > severity,
> > > +    VkDebugUtilsMessageTypeFlagsEXT message_types,
> > > +#if defined(USE_STRUCT_CONVERSION)
> > > +    const VkDebugUtilsMessengerCallbackDataEXT_host *callback_data,
> > > +#else
> > > +    const VkDebugUtilsMessengerCallbackDataEXT *callback_data,
> > > +#endif
> > > +    void *user_data)
> > > +{
> > > +    struct VkDebugUtilsMessengerCallbackDataEXT wine_callback_data;
> > > +    VkDebugUtilsObjectNameInfoEXT *object_name_infos;
> > > +    struct wine_debug_utils_messenger *object;
> > > +    int i;
> > > +
> > > +    object = user_data;
> > > +
> > > +    if (!object->instance->instance)
> > > +    {
> > > +        /* instance wasn't yet created, this is a message from the 
> > > native loader*/
> >
> > nit: loader */
> > > +        return VK_FALSE;
> > > +    }
> > > +
> > > +    wine_callback_data = *((VkDebugUtilsMessengerCallbackDataEXT *) 
> > > callback_data);
> > > +
> > > +    object_name_infos = heap_calloc(wine_callback_data.objectCount, 
> > > sizeof(*object_name_infos));
> > > +
> > > +    for (i = 0; i < wine_callback_data.objectCount; i++)
> > > +    {
> > > +        object_name_infos[i].sType = callback_data->pObjects[i].sType;
> > > +        object_name_infos[i].pNext = callback_data->pObjects[i].pNext;
> > > +        object_name_infos[i].objectType = 
> > > callback_data->pObjects[i].objectType;
> > > +        object_name_infos[i].pObjectName = 
> > > callback_data->pObjects[i].pObjectName;
> > > +
> > > +        if (is_wrapped(callback_data->pObjects[i].objectType))
> > > +        {
> > > +            object_name_infos[i].objectHandle = 
> > > wine_vk_get_wrapper(object->instance, 
> > > callback_data->pObjects[i].objectHandle);
> > > +            if (!object_name_infos[i].objectHandle)
> > > +            {
> > > +                WARN("handle conversion failed 0x%s\n", 
> > > wine_dbgstr_longlong(callback_data->pObjects[i].objectHandle));
> > > +                heap_free(object_name_infos);
> > > +                return VK_FALSE;
> >
> > Given that the function will exit early, and a callback will not be
> > serviced back to WINE-side listeners, I think this should be an ERROR
> > and not a WARN.
>
> There are some common situations where this occurs. The 
> loader/validation tools
> send messages for a new handle during its creation. In that case winevulkan
> doesn't know the handle yet.
> I didn't want to introduce too much log spam, that's why it's WARN.

That makes sense. I wasn't sure if this was just theoretical, but if 
we're hitting this in intended use-cases then I agree that ERROR seems a 
bit too spammy.
> > > +            }
> > > +        }
> > > +        else
> > > +        {
> > > +            object_name_infos[i].objectHandle = 
> > > callback_data->pObjects[i].objectHandle;
> > > +        }
> > > +    }
> > > +
> > > +    wine_callback_data.pObjects = object_name_infos;
> > > +
> > > +    /* applications should always return VK_FALSE */
> > > +    object->user_callback(severity, message_types, 
> > > &wine_callback_data, object->user_data);
> > > +
> > > +    heap_free(object_name_infos);
> > > +
> > > +    return VK_FALSE;
> >
> > The VK specification states:
> > > The callback returns a VkBool32, which is interpreted in a 
> > > layer-specified manner. The application should always return VK_FALSE. 
> > > The VK_TRUE value is reserved for use in layer development.
> >
> > So I think we should return the value that user_callback returned to
> > remain faithful to whomever registered a callback and what they returned.
> > > +}
> > > +
> > >   static void wine_vk_physical_device_free(struct VkPhysicalDevice_T 
> > > *phys_dev)
> > >   {
> > >       if (!phys_dev)
> > > @@ -391,6 +476,8 @@ static void wine_vk_init_once(void)
> > >   static VkResult wine_vk_instance_convert_create_info(const 
> > > VkInstanceCreateInfo *src,
> > >           VkInstanceCreateInfo *dst, struct VkInstance_T *object)
> > >   {
> > > +    VkDebugUtilsMessengerCreateInfoEXT *debug_utils_messenger;
> > > +    VkBaseOutStructure *header;
> > >       unsigned int i;
> > >       VkResult res;
> > >
> > > @@ -402,6 +489,22 @@ static VkResult 
> > > wine_vk_instance_convert_create_info(const VkInstanceCreateInfo
> > >           return res;
> > >       }
> > >
> > > +    object->utils_messenger_count = wine_vk_count_struct(dst, 
> > > DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);
> > > +    object->utils_messengers = 
> > >   heap_calloc(object->utils_messenger_count, 
> > > sizeof(*object->utils_messengers));
> > > +    header = (VkBaseOutStructure *) dst;
> > > +    for (i = 0; i < object->utils_messenger_count; i++)
> > > +    {
> > > +        header = wine_vk_find_struct(header->pNext, 
> > > DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);
> > > +        debug_utils_messenger = (VkDebugUtilsMessengerCreateInfoEXT 
> > > *) header;
> > > +        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;
> > > +
> > > +        debug_utils_messenger->pfnUserCallback = (void *) 
> > > &debug_utils_callback_conversion;
> > > +        debug_utils_messenger->pUserData = 
> > > &object->utils_messengers[i];
> >
> > I don't think we should be abusing VkBaseOutStructure like this as a way
> > to circumvent const-ness of a read-only pNext chain. When we detect that
> > there are VkDebugUtilsMessengerCreateInfoEXT structures in the pNext
> > chain I think it would be cleaner to create a local copy and then use
> > that however we need to.
>
> I'm not really happy with this either, but
> convert_VkInstanceCreateInfo_struct_chain already copies the pNext chain.
> So nothing changed here will affect the application running in wine.

Yeah, we're already doing the copy here, I just don't think it makes 
sense to use VkBaseOutStructure here when VkBaseInStructure is more 
appropriate (given the structure is part of an input pNext chain).
> > > +    }
> > > +
> > >       /* ICDs don't support any layers, so nothing to copy. Modern 
> > > versions of the loader
> > >        * filter this data out as well.
> > >        */
> > > @@ -425,6 +528,10 @@ static VkResult 
> > > wine_vk_instance_convert_create_info(const VkInstanceCreateInfo
> > >               free_VkInstanceCreateInfo_struct_chain(dst);
> > >               return VK_ERROR_EXTENSION_NOT_PRESENT;
> > >           }
> > > +        if (!strcmp(extension_name, "VK_EXT_debug_utils"))
> > > +        {
> > > +            object->enable_wrapper_list = VK_TRUE;
> > > +        }
> > >       }
> > >
> > >       return VK_SUCCESS;
> > > @@ -522,6 +629,8 @@ static void wine_vk_instance_free(struct 
> > > VkInstance_T *instance)
> > >       if (instance->instance)
> > >           vk_funcs->p_vkDestroyInstance(instance->instance, NULL /* 
> > > allocator */);
> > >
> > > +    heap_free(instance->utils_messengers);
> > > +
> > >       heap_free(instance);
> > >   }
> > >
> > > @@ -1607,6 +1716,10 @@ static uint64_t 
> > > unwrap_object_handle(VkObjectType type, uint64_t handle)
> > >   {
> > >       switch (type)
> > >       {
> > > +        case VK_OBJECT_TYPE_INSTANCE:
> > > +            return (uint64_t) (uintptr_t) ((VkInstance) (uintptr_t) 
> > > handle)->instance;
> > > +        case VK_OBJECT_TYPE_PHYSICAL_DEVICE:
> > > +            return (uint64_t) (uintptr_t) ((VkPhysicalDevice) 
> > > (uintptr_t) handle)->phys_dev;
> >
> > Were these meant to go into this commit? I can see why they're needed
> > for debug_utils, but it just feels out of place to see them added in
> > this commit.
>
> I thought it wasn't worth a separate commit. Ideally we should auto-generate
> this function anyway to make adding new wrapped handles easier.

Yeah, as above. If you're planning on getting this in a separate commit 
in the series I think that would be fine also.


Thanks,

Liam Middlebrook
> Thanks,
>
> Georg Lehmann
> > Thanks,
> >
> > Liam Middlebrook
> > >           case VK_OBJECT_TYPE_DEVICE:
> > >               return (uint64_t) (uintptr_t) ((VkDevice) (uintptr_t)
>
> handle)->device;
> > >           case VK_OBJECT_TYPE_QUEUE:
> > > @@ -1615,6 +1728,8 @@ static uint64_t 
> > > unwrap_object_handle(VkObjectType type, uint64_t handle)
> > >               return (uint64_t) (uintptr_t) ((VkCommandBuffer) 
> > > (uintptr_t) handle)->command_buffer;
> > >           case VK_OBJECT_TYPE_COMMAND_POOL:
> > >               return (uint64_t) 
> > > wine_cmd_pool_from_handle(handle)->command_pool;
> > > +        case VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT:
> > > +            return (uint64_t) 
> > > wine_debug_utils_messenger_from_handle(handle)->debug_messenger;
> > >           default:
> > >               return handle;
> > >       }
> > > @@ -1685,6 +1800,84 @@ VkResult WINAPI 
> > > wine_vkGetPhysicalDeviceSurfaceCapabilities2KHR(VkPhysicalDevice
> > >       return res;
> > >   }
> > >
> > > +VkResult WINAPI wine_vkCreateDebugUtilsMessengerEXT(VkInstance 
> > > instance, const VkDebugUtilsMessengerCreateInfoEXT *create_info,
> > > +        const VkAllocationCallbacks *allocator, 
> > > VkDebugUtilsMessengerEXT *messenger)
> > > +{
> > > +    VkDebugUtilsMessengerCreateInfoEXT wine_create_info;
> > > +    struct wine_debug_utils_messenger *object;
> > > +    VkResult res;
> > > +
> > > +    TRACE("%p, %p, %p, %p\n", instance, create_info, allocator, 
> > > messenger);
> > > +
> > > +    if (allocator)
> > > +        FIXME("Support for allocation callbacks not implemented 
> > > yet\n");
> > > +
> > > +    if (!(object = heap_alloc_zero(sizeof(*object))))
> > > +        return VK_ERROR_OUT_OF_HOST_MEMORY;
> > > +
> > > +    object->instance = instance;
> > > +    object->user_callback = create_info->pfnUserCallback;
> > > +    object->user_data = create_info->pUserData;
> > > +
> > > +    wine_create_info = *create_info;
> > > +
> > > +    wine_create_info.pfnUserCallback = (void *) 
> > > &debug_utils_callback_conversion;
> > > +    wine_create_info.pUserData = object;
> > > +
> > > +    res = 
> > > instance->funcs.p_vkCreateDebugUtilsMessengerEXT(instance->instance, 
> > > &wine_create_info, NULL,  &object->debug_messenger);
> > > +
> > > +    if (res != VK_SUCCESS)
> > > +    {
> > > +        heap_free(object);
> > > +        return res;
> > > +    }
> > > +
> > > +    WINE_VK_ADD_HANDLE_MAPPING(instance, object, 
> > > object->debug_messenger);
> > > +    *messenger = wine_debug_utils_messenger_to_handle(object);
> > > +
> > > +    return VK_SUCCESS;
> > > +}
> > > +
> > > +void WINAPI wine_vkDestroyDebugUtilsMessengerEXT(
> > > +        VkInstance instance, VkDebugUtilsMessengerEXT messenger, 
> > > const VkAllocationCallbacks *allocator)
> > > +{
> > > +    struct wine_debug_utils_messenger *object;
> > > +
> > > +    TRACE("%p, 0x%s, %p\n", instance, 
> > > wine_dbgstr_longlong(messenger), allocator);
> > > +
> > > +    object = wine_debug_utils_messenger_from_handle(messenger);
> > > +
> > > +   
> > >   instance->funcs.p_vkDestroyDebugUtilsMessengerEXT(instance->instance, 
> > > object->debug_messenger, NULL);
> > > +    WINE_VK_REMOVE_HANDLE_MAPPING(instance, object);
> > > +
> > > +    heap_free(object);
> > > +}
> > > +
> > > +void WINAPI wine_vkSubmitDebugUtilsMessageEXT(VkInstance instance, 
> > > VkDebugUtilsMessageSeverityFlagBitsEXT severity,
> > > +        VkDebugUtilsMessageTypeFlagsEXT types, const 
> > > VkDebugUtilsMessengerCallbackDataEXT *callback_data)
> > > +{
> > > +    VkDebugUtilsMessengerCallbackDataEXT native_callback_data;
> > > +    VkDebugUtilsObjectNameInfoEXT *object_names;
> > > +    int i;
> > > +
> > > +    TRACE("%p, %#x, %#x, %p\n", instance, severity, types, 
> > > callback_data);
> > > +
> > > +    native_callback_data = *callback_data;
> > > +    object_names = heap_calloc(callback_data->objectCount, 
> > > sizeof(*object_names));
> > > +    memcpy(object_names, callback_data->pObjects, 
> > > callback_data->objectCount * sizeof(*object_names));
> > > +    native_callback_data.pObjects = object_names;
> > > +
> > > +    for (i = 0; i < callback_data->objectCount; i++)
> > > +    {
> > > +        object_names[i].objectHandle =
> > > +               
> > >   unwrap_object_handle(callback_data->pObjects[i].objectType, 
> > > callback_data->pObjects[i].objectHandle);
> > > +    }
> > > +
> > > +    thunk_vkSubmitDebugUtilsMessageEXT(instance->instance, 
> > > severity, types, &native_callback_data);
> > > +
> > > +    heap_free(object_names);
> > > +}
> > > +
> > >   BOOL WINAPI DllMain(HINSTANCE hinst, DWORD reason, void *reserved)
> > >   {
> > >       TRACE("%p, %u, %p\n", hinst, reason, reserved);
> > > diff --git a/dlls/winevulkan/vulkan_private.h 
> > > b/dlls/winevulkan/vulkan_private.h
> > > index 146bbd8fa4..17aa3835cf 100644
> > > --- a/dlls/winevulkan/vulkan_private.h
> > > +++ b/dlls/winevulkan/vulkan_private.h
> > > @@ -95,6 +95,8 @@ struct VkDevice_T
> > >       struct wine_vk_mapping mapping;
> > >   };
> > >
> > > +struct wine_debug_utils_messenger;
> > > +
> > >   struct VkInstance_T
> > >   {
> > >       struct wine_vk_base base;
> > > @@ -111,6 +113,9 @@ struct VkInstance_T
> > >       struct list wrappers;
> > >       SRWLOCK wrapper_lock;
> > >
> > > +    struct wine_debug_utils_messenger *utils_messengers;
> > > +    uint32_t utils_messenger_count;
> > > +
> > >       unsigned int quirks;
> > >
> > >       struct wine_vk_mapping mapping;
> > > @@ -158,6 +163,32 @@ static inline VkCommandPool 
> > > wine_cmd_pool_to_handle(struct wine_cmd_pool *cmd_po
> > >       return (VkCommandPool)(uintptr_t)cmd_pool;
> > >   }
> > >
> > > +struct wine_debug_utils_messenger
> > > +{
> > > +    struct VkInstance_T *instance; /* parent */
> > > +    VkDebugUtilsMessengerEXT debug_messenger; /* native messenger */
> > > +
> > > +    /* application callback + data */
> > > +    PFN_vkDebugUtilsMessengerCallbackEXT user_callback;
> > > +    void *user_data;
> > > +
> > > +    struct wine_vk_mapping mapping;
> > > +};
> > > +
> > > +static inline struct wine_debug_utils_messenger 
> > > *wine_debug_utils_messenger_from_handle(
> > > +        VkDebugUtilsMessengerEXT handle)
> > > +{
> > > +    return (struct wine_debug_utils_messenger *)(uintptr_t)handle;
> > > +}
> > > +
> > > +static inline VkDebugUtilsMessengerEXT 
> > > wine_debug_utils_messenger_to_handle(
> > > +        struct wine_debug_utils_messenger *debug_messenger)
> > > +{
> > > +    return (VkDebugUtilsMessengerEXT)(uintptr_t)debug_messenger;
> > > +}
> > > +
> > > +
> > > +
> > >   void *wine_vk_get_device_proc_addr(const char *name) DECLSPEC_HIDDEN;
> > >   void *wine_vk_get_instance_proc_addr(const char *name) 
> > > DECLSPEC_HIDDEN;

[Liam Middlebrook]

I think it would be nice to check if there is an 'alias' attribute when 
parsing funcpointers, and store that alias. But, since there aren't any 
funcpointers which have this attribute set right now, I'm fine with 
deferring that handling until the need arises.

Signed-off-by: Liam Middlebrook <lmiddlebrook(a)nvidia.com>

On 9/22/20 7:31 AM, Georg Lehmann wrote:
> Signed-off-by: Georg Lehmann <dadschoorse(a)gmail.com>
> ---
>   dlls/winevulkan/make_vulkan | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
> index 5261c35360..1b39cbb831 100755
> --- a/dlls/winevulkan/make_vulkan
> +++ b/dlls/winevulkan/make_vulkan
> @@ -865,6 +865,8 @@ class VkFunctionPointer(object):
>           text += ");\n"
>           return text
>
> +    def is_alias(self):
> +        return False
>
> class VkHandle(object):
>       def __init__(self, name, _type, parent, alias=None):