<div dir="ltr"><div dir="ltr"></div>On 23. Sept. 2020, 00:03, Liam Middlebrook wrote:<br>>On 9/22/20 7:31 AM, Georg Lehmann wrote:<br>>> Wine-Bug: <a href="https://bugs.winehq.org/show_bug.cgi?id=49813">https://bugs.winehq.org/show_bug.cgi?id=49813</a><br>>> <br>>> Signed-off-by: Georg Lehmann <dadschoorse at <a href="http://gmail.com">gmail.com</a>><br>>> ---<br>>>   dlls/winevulkan/make_vulkan      |   8 +-<br>>>   dlls/winevulkan/vulkan.c         | 193 +++++++++++++++++++++++++++++++<br>>>   dlls/winevulkan/vulkan_private.h |  31 +++++<br>>>   3 files changed, 231 insertions(+), 1 deletion(-)<br>>> <br>>> diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan<br>>> index f4d223daad..77034cfe36 100755<br>>> --- a/dlls/winevulkan/make_vulkan<br>>> +++ b/dlls/winevulkan/make_vulkan<br>>> @@ -92,7 +92,6 @@ UNSUPPORTED_EXTENSIONS = [<br>>>       # plumbing down to the native layer, we will get each message twice as we<br>>>       # use 2 loaders (win32+native), but we may get output from the driver.<br>>>       # In any case callback conversion is required.<br>>> -    "VK_EXT_debug_utils",<br>>>       "VK_EXT_validation_features",<br>>>       "VK_EXT_validation_flags",<br>>>       "VK_KHR_display", # Needs WSI work.<br>>> @@ -225,6 +224,11 @@ FUNCTION_OVERRIDES = {<br>>>       # VK_EXT_calibrated_timestamps<br>>>       "vkGetPhysicalDeviceCalibrateableTimeDomainsEXT" : {"dispatch" : True, "driver" : False, "thunk" : False},<br>>>       "vkGetCalibratedTimestampsEXT" : {"dispatch" : True, "driver" : False, "thunk" : False},<br>>> +<br>>> +    # VK_EXT_debug_utils<br>>> +    "vkCreateDebugUtilsMessengerEXT" : {"dispatch": True, "driver" : False, "thunk" : False},<br>>> +    "vkDestroyDebugUtilsMessengerEXT" : {"dispatch": True, "driver" : False, "thunk" : False},<br>>> +    "vkSubmitDebugUtilsMessageEXT" : {"dispatch": True, "driver" : False, "thunk" : True, "private_thunk" : True},<br>>>   }<br>>>   <br>>>   STRUCT_CHAIN_CONVERSIONS = [<br>>> @@ -944,6 +948,8 @@ class VkHandle(object):<br>>>   <br>>>           if <a href="http://self.name">self.name</a> == "VkCommandPool":<br>>>               return "wine_cmd_pool_from_handle({0})->command_pool".format(name)<br>>> +        if <a href="http://self.name">self.name</a> == "VkDebugUtilsMessengerEXT":<br>>> +            return "wine_debug_utils_messenger_from_handle({0})->debug_messenger".format(name)<br>>>   <br>>>           native_handle_name = None<br>>>   <br>>> diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c<br>>> index 2747f440ad..11dd0f761e 100644<br>>> --- a/dlls/winevulkan/vulkan.c<br>>> +++ b/dlls/winevulkan/vulkan.c<br>>> @@ -57,6 +57,21 @@ static void *wine_vk_find_struct_(void *s, VkStructureType t)<br>>>       return NULL;<br>>>   }<br>>>   <br>>> +#define wine_vk_count_struct(s, t) wine_vk_count_struct_((void *)s, VK_STRUCTURE_TYPE_##t)<br>>> +static uint32_t wine_vk_count_struct_(void *s, VkStructureType t)<br>>> +{<br>>> +    VkBaseOutStructure *header;<br>><br>> I think VkBaseInStructure would be more appropriate here for preserving <br>> const-ness, and given the sole use-case of wine_vk_count_struct that you <br>> introduce below.<br>><br>>> +    uint32_t result = 0;<br>>> +<br>>> +    for (header = s; header; header = header->pNext)<br>>> +    {<br>>> +        if (header->sType == t)<br>>> +            result++;<br>>> +    }<br>>> +<br>>> +    return result;<br>>> +}<br>>> +<br>>>   static void *wine_vk_get_global_proc_addr(const char *name);<br>>>   <br>>>   static HINSTANCE hinstance;<br>>> @@ -111,6 +126,76 @@ static uint64_t wine_vk_get_wrapper(struct VkInstance_T *instance, uint64_t nati<br>>>       return result;<br>>>   }<br>>>   <br>>> +static VkBool32 is_wrapped(VkObjectType type)<br>>> +{<br>>> +    return type == VK_OBJECT_TYPE_INSTANCE ||<br>>> +           type == VK_OBJECT_TYPE_PHYSICAL_DEVICE ||<br>>> +           type == VK_OBJECT_TYPE_DEVICE ||<br>>> +           type == VK_OBJECT_TYPE_QUEUE ||<br>>> +           type == VK_OBJECT_TYPE_COMMAND_BUFFER ||<br>>> +           type == VK_OBJECT_TYPE_COMMAND_POOL ||<br>>> +           type == VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT;<br>>> +}<br>><br>>Can this be made into an auto-generated function from make_vulkan? It'd <br>>be nicer to keep the amount of manual-touchups to add a new wrapped <br>>object type to a minimum.<br>><br><br>I later wanted to auto-generate this and unwrap_object_handle in a separate<br>patch series, but I can put it in this patch series if you want.<br><br>>> +<br>>> +static VkBool32 debug_utils_callback_conversion(VkDebugUtilsMessageSeverityFlagBitsEXT severity,<br>>> +    VkDebugUtilsMessageTypeFlagsEXT message_types,<br>>> +#if defined(USE_STRUCT_CONVERSION)<br>>> +    const VkDebugUtilsMessengerCallbackDataEXT_host *callback_data,<br>>> +#else<br>>> +    const VkDebugUtilsMessengerCallbackDataEXT *callback_data,<br>>> +#endif<br>>> +    void *user_data)<br>>> +{<br>>> +    struct VkDebugUtilsMessengerCallbackDataEXT wine_callback_data;<br>>> +    VkDebugUtilsObjectNameInfoEXT *object_name_infos;<br>>> +    struct wine_debug_utils_messenger *object;<br>>> +    int i;<br>>> +<br>>> +    object = user_data;<br>>> +<br>>> +    if (!object->instance->instance)<br>>> +    {<br>>> +        /* instance wasn't yet created, this is a message from the native loader*/<br>><br>> nit: loader */<br>><br>>> +        return VK_FALSE;<br>>> +    }<br>>> +<br>>> +    wine_callback_data = *((VkDebugUtilsMessengerCallbackDataEXT *) callback_data);<br>>> +<br>>> +    object_name_infos = heap_calloc(wine_callback_data.objectCount, sizeof(*object_name_infos));<br>>> +<br>>> +    for (i = 0; i < wine_callback_data.objectCount; i++)<br>>> +    {<br>>> +        object_name_infos[i].sType = callback_data->pObjects[i].sType;<br>>> +        object_name_infos[i].pNext = callback_data->pObjects[i].pNext;<br>>> +        object_name_infos[i].objectType = callback_data->pObjects[i].objectType;<br>>> +        object_name_infos[i].pObjectName = callback_data->pObjects[i].pObjectName;<br>>> +<br>>> +        if (is_wrapped(callback_data->pObjects[i].objectType))<br>>> +        {<br>>> +            object_name_infos[i].objectHandle = wine_vk_get_wrapper(object->instance, callback_data->pObjects[i].objectHandle);<br>>> +            if (!object_name_infos[i].objectHandle)<br>>> +            {<br>>> +                WARN("handle conversion failed 0x%s\n", wine_dbgstr_longlong(callback_data->pObjects[i].objectHandle));<br>>> +                heap_free(object_name_infos);<br>>> +                return VK_FALSE;<br>><br>> Given that the function will exit early, and a callback will not be <br>> serviced back to WINE-side listeners, I think this should be an ERROR <br>> and not a WARN.<br>><br><br>There are some common situations where this occurs. The loader/validation tools<br>send messages for a new handle during its creation. In that case winevulkan<br>doesn't know the handle yet.<br>I didn't want to introduce too much log spam, that's why it's WARN.<br><br>>> +            }<br>>> +        }<br>>> +        else<br>>> +        {<br>>> +            object_name_infos[i].objectHandle = callback_data->pObjects[i].objectHandle;<br>>> +        }<br>>> +    }<br>>> +<br>>> +    wine_callback_data.pObjects = object_name_infos;<br>>> +<br>>> +    /* applications should always return VK_FALSE */<br>>> +    object->user_callback(severity, message_types, &wine_callback_data, object->user_data);<br>>> +<br>>> +    heap_free(object_name_infos);<br>>> +<br>>> +    return VK_FALSE;<br>><br>> The VK specification states:<br>><br>>> 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.<br>><br>> So I think we should return the value that user_callback returned to <br>> remain faithful to whomever registered a callback and what they returned.<br>><br>>> +}<br>>> +<br>>>   static void wine_vk_physical_device_free(struct VkPhysicalDevice_T *phys_dev)<br>>>   {<br>>>       if (!phys_dev)<br>>> @@ -391,6 +476,8 @@ static void wine_vk_init_once(void)<br>>>   static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo *src,<br>>>           VkInstanceCreateInfo *dst, struct VkInstance_T *object)<br>>>   {<br>>> +    VkDebugUtilsMessengerCreateInfoEXT *debug_utils_messenger;<br>>> +    VkBaseOutStructure *header;<br>>>       unsigned int i;<br>>>       VkResult res;<br>>>   <br>>> @@ -402,6 +489,22 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo<br>>>           return res;<br>>>       }<br>>>   <br>>> +    object->utils_messenger_count = wine_vk_count_struct(dst, DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);<br>>> +    object->utils_messengers =  heap_calloc(object->utils_messenger_count, sizeof(*object->utils_messengers));<br>>> +    header = (VkBaseOutStructure *) dst;<br>>> +    for (i = 0; i < object->utils_messenger_count; i++)<br>>> +    {<br>>> +        header = wine_vk_find_struct(header->pNext, DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT);<br>>> +        debug_utils_messenger = (VkDebugUtilsMessengerCreateInfoEXT *) header;<br>>> +        object->utils_messengers[i].instance = object;<br>>> +        object->utils_messengers[i].debug_messenger = VK_NULL_HANDLE;<br>>> +        object->utils_messengers[i].user_callback = debug_utils_messenger->pfnUserCallback;<br>>> +        object->utils_messengers[i].user_data = debug_utils_messenger->pUserData;<br>>> +<br>>> +        debug_utils_messenger->pfnUserCallback = (void *) &debug_utils_callback_conversion;<br>>> +        debug_utils_messenger->pUserData = &object->utils_messengers[i];<br>><br>> I don't think we should be abusing VkBaseOutStructure like this as a way <br>> to circumvent const-ness of a read-only pNext chain. When we detect that <br>> there are VkDebugUtilsMessengerCreateInfoEXT structures in the pNext <br>> chain I think it would be cleaner to create a local copy and then use <br>> that however we need to.<br>><br><br>I'm not really happy with this either, but <br>convert_VkInstanceCreateInfo_struct_chain already copies the pNext chain.<br>So nothing changed here will affect the application running in wine.<br><br>>> +    }<br>>> +<br>>>       /* ICDs don't support any layers, so nothing to copy. Modern versions of the loader<br>>>        * filter this data out as well.<br>>>        */<br>>> @@ -425,6 +528,10 @@ static VkResult wine_vk_instance_convert_create_info(const VkInstanceCreateInfo<br>>>               free_VkInstanceCreateInfo_struct_chain(dst);<br>>>               return VK_ERROR_EXTENSION_NOT_PRESENT;<br>>>           }<br>>> +        if (!strcmp(extension_name, "VK_EXT_debug_utils"))<br>>> +        {<br>>> +            object->enable_wrapper_list = VK_TRUE;<br>>> +        }<br>>>       }<br>>>   <br>>>       return VK_SUCCESS;<br>>> @@ -522,6 +629,8 @@ static void wine_vk_instance_free(struct VkInstance_T *instance)<br>>>       if (instance->instance)<br>>>           vk_funcs->p_vkDestroyInstance(instance->instance, NULL /* allocator */);<br>>>   <br>>> +    heap_free(instance->utils_messengers);<br>>> +<br>>>       heap_free(instance);<br>>>   }<br>>>   <br>>> @@ -1607,6 +1716,10 @@ static uint64_t unwrap_object_handle(VkObjectType type, uint64_t handle)<br>>>   {<br>>>       switch (type)<br>>>       {<br>>> +        case VK_OBJECT_TYPE_INSTANCE:<br>>> +            return (uint64_t) (uintptr_t) ((VkInstance) (uintptr_t) handle)->instance;<br>>> +        case VK_OBJECT_TYPE_PHYSICAL_DEVICE:<br>>> +            return (uint64_t) (uintptr_t) ((VkPhysicalDevice) (uintptr_t) handle)->phys_dev;<br>><br>> Were these meant to go into this commit? I can see why they're needed <br>> for debug_utils, but it just feels out of place to see them added in <br>> this commit.<br>><br><br>I thought it wasn't worth a separate commit. Ideally we should auto-generate<br>this function anyway to make adding new wrapped handles easier.<br><br>Thanks,<br><br>Georg Lehmann<br><br>><br>> Thanks,<br>><br>> Liam Middlebrook<br>><br>>>           case VK_OBJECT_TYPE_DEVICE:<br>>>               return (uint64_t) (uintptr_t) ((VkDevice) (uintptr_t) handle)->device;<br>>>           case VK_OBJECT_TYPE_QUEUE:<br>>> @@ -1615,6 +1728,8 @@ static uint64_t unwrap_object_handle(VkObjectType type, uint64_t handle)<br>>>               return (uint64_t) (uintptr_t) ((VkCommandBuffer) (uintptr_t) handle)->command_buffer;<br>>>           case VK_OBJECT_TYPE_COMMAND_POOL:<br>>>               return (uint64_t) wine_cmd_pool_from_handle(handle)->command_pool;<br>>> +        case VK_OBJECT_TYPE_DEBUG_UTILS_MESSENGER_EXT:<br>>> +            return (uint64_t) wine_debug_utils_messenger_from_handle(handle)->debug_messenger;<br>>>           default:<br>>>               return handle;<br>>>       }<br>>> @@ -1685,6 +1800,84 @@ VkResult WINAPI wine_vkGetPhysicalDeviceSurfaceCapabilities2KHR(VkPhysicalDevice<br>>>       return res;<br>>>   }<br>>>   <br>>> +VkResult WINAPI wine_vkCreateDebugUtilsMessengerEXT(VkInstance instance, const VkDebugUtilsMessengerCreateInfoEXT *create_info,<br>>> +        const VkAllocationCallbacks *allocator, VkDebugUtilsMessengerEXT *messenger)<br>>> +{<br>>> +    VkDebugUtilsMessengerCreateInfoEXT wine_create_info;<br>>> +    struct wine_debug_utils_messenger *object;<br>>> +    VkResult res;<br>>> +<br>>> +    TRACE("%p, %p, %p, %p\n", instance, create_info, allocator, messenger);<br>>> +<br>>> +    if (allocator)<br>>> +        FIXME("Support for allocation callbacks not implemented yet\n");<br>>> +<br>>> +    if (!(object = heap_alloc_zero(sizeof(*object))))<br>>> +        return VK_ERROR_OUT_OF_HOST_MEMORY;<br>>> +<br>>> +    object->instance = instance;<br>>> +    object->user_callback = create_info->pfnUserCallback;<br>>> +    object->user_data = create_info->pUserData;<br>>> +<br>>> +    wine_create_info = *create_info;<br>>> +<br>>> +    wine_create_info.pfnUserCallback = (void *) &debug_utils_callback_conversion;<br>>> +    wine_create_info.pUserData = object;<br>>> +<br>>> +    res = instance->funcs.p_vkCreateDebugUtilsMessengerEXT(instance->instance, &wine_create_info, NULL,  &object->debug_messenger);<br>>> +<br>>> +    if (res != VK_SUCCESS)<br>>> +    {<br>>> +        heap_free(object);<br>>> +        return res;<br>>> +    }<br>>> +<br>>> +    WINE_VK_ADD_HANDLE_MAPPING(instance, object, object->debug_messenger);<br>>> +    *messenger = wine_debug_utils_messenger_to_handle(object);<br>>> +<br>>> +    return VK_SUCCESS;<br>>> +}<br>>> +<br>>> +void WINAPI wine_vkDestroyDebugUtilsMessengerEXT(<br>>> +        VkInstance instance, VkDebugUtilsMessengerEXT messenger, const VkAllocationCallbacks *allocator)<br>>> +{<br>>> +    struct wine_debug_utils_messenger *object;<br>>> +<br>>> +    TRACE("%p, 0x%s, %p\n", instance, wine_dbgstr_longlong(messenger), allocator);<br>>> +<br>>> +    object = wine_debug_utils_messenger_from_handle(messenger);<br>>> +<br>>> +    instance->funcs.p_vkDestroyDebugUtilsMessengerEXT(instance->instance, object->debug_messenger, NULL);<br>>> +    WINE_VK_REMOVE_HANDLE_MAPPING(instance, object);<br>>> +<br>>> +    heap_free(object);<br>>> +}<br>>> +<br>>> +void WINAPI wine_vkSubmitDebugUtilsMessageEXT(VkInstance instance, VkDebugUtilsMessageSeverityFlagBitsEXT severity,<br>>> +        VkDebugUtilsMessageTypeFlagsEXT types, const VkDebugUtilsMessengerCallbackDataEXT *callback_data)<br>>> +{<br>>> +    VkDebugUtilsMessengerCallbackDataEXT native_callback_data;<br>>> +    VkDebugUtilsObjectNameInfoEXT *object_names;<br>>> +    int i;<br>>> +<br>>> +    TRACE("%p, %#x, %#x, %p\n", instance, severity, types, callback_data);<br>>> +<br>>> +    native_callback_data = *callback_data;<br>>> +    object_names = heap_calloc(callback_data->objectCount, sizeof(*object_names));<br>>> +    memcpy(object_names, callback_data->pObjects, callback_data->objectCount * sizeof(*object_names));<br>>> +    native_callback_data.pObjects = object_names;<br>>> +<br>>> +    for (i = 0; i < callback_data->objectCount; i++)<br>>> +    {<br>>> +        object_names[i].objectHandle =<br>>> +                unwrap_object_handle(callback_data->pObjects[i].objectType, callback_data->pObjects[i].objectHandle);<br>>> +    }<br>>> +<br>>> +    thunk_vkSubmitDebugUtilsMessageEXT(instance->instance, severity, types, &native_callback_data);<br>>> +<br>>> +    heap_free(object_names);<br>>> +}<br>>> +<br>>>   BOOL WINAPI DllMain(HINSTANCE hinst, DWORD reason, void *reserved)<br>>>   {<br>>>       TRACE("%p, %u, %p\n", hinst, reason, reserved);<br>>> diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h<br>>> index 146bbd8fa4..17aa3835cf 100644<br>>> --- a/dlls/winevulkan/vulkan_private.h<br>>> +++ b/dlls/winevulkan/vulkan_private.h<br>>> @@ -95,6 +95,8 @@ struct VkDevice_T<br>>>       struct wine_vk_mapping mapping;<br>>>   };<br>>>   <br>>> +struct wine_debug_utils_messenger;<br>>> +<br>>>   struct VkInstance_T<br>>>   {<br>>>       struct wine_vk_base base;<br>>> @@ -111,6 +113,9 @@ struct VkInstance_T<br>>>       struct list wrappers;<br>>>       SRWLOCK wrapper_lock;<br>>>   <br>>> +    struct wine_debug_utils_messenger *utils_messengers;<br>>> +    uint32_t utils_messenger_count;<br>>> +<br>>>       unsigned int quirks;<br>>>       <br>>>       struct wine_vk_mapping mapping;<br>>> @@ -158,6 +163,32 @@ static inline VkCommandPool wine_cmd_pool_to_handle(struct wine_cmd_pool *cmd_po<br>>>       return (VkCommandPool)(uintptr_t)cmd_pool;<br>>>   }<br>>>   <br>>> +struct wine_debug_utils_messenger<br>>> +{<br>>> +    struct VkInstance_T *instance; /* parent */<br>>> +    VkDebugUtilsMessengerEXT debug_messenger; /* native messenger */<br>>> +<br>>> +    /* application callback + data */<br>>> +    PFN_vkDebugUtilsMessengerCallbackEXT user_callback;<br>>> +    void *user_data;<br>>> +<br>>> +    struct wine_vk_mapping mapping;<br>>> +};<br>>> +<br>>> +static inline struct wine_debug_utils_messenger *wine_debug_utils_messenger_from_handle(<br>>> +        VkDebugUtilsMessengerEXT handle)<br>>> +{<br>>> +    return (struct wine_debug_utils_messenger *)(uintptr_t)handle;<br>>> +}<br>>> +<br>>> +static inline VkDebugUtilsMessengerEXT wine_debug_utils_messenger_to_handle(<br>>> +        struct wine_debug_utils_messenger *debug_messenger)<br>>> +{<br>>> +    return (VkDebugUtilsMessengerEXT)(uintptr_t)debug_messenger;<br>>> +}<br>>> +<br>>> +<br>>> +<br>>>   void *wine_vk_get_device_proc_addr(const char *name) DECLSPEC_HIDDEN;<br>>>   void *wine_vk_get_instance_proc_addr(const char *name) DECLSPEC_HIDDEN;<br>>>   <br>>> <br>></div>