Re: [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
>
> 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;