From: Dad Schoorse Subject: Re: [PATCH v2 4/4] winevulkan: Implement VK_EXT_debug_utils. Message-Id: Date: Wed, 23 Sep 2020 00:56:16 +0200 In-Reply-To: <187aca41-5be0-455a-b910-6560f9bc2c39@nvidia.com> References: <20200922143151.34962-1-dadschoorse@gmail.com> <20200922143151.34962-4-dadschoorse@gmail.com> <187aca41-5be0-455a-b910-6560f9bc2c39@nvidia.com> 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 >> --- >> 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; >> >> >
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;
>>  
>>
>