From: Liam Middlebrook Subject: Re: [PATCH v2 4/4] winevulkan: Implement VK_EXT_debug_utils. Message-Id: <187aca41-5be0-455a-b910-6560f9bc2c39@nvidia.com> Date: Tue, 22 Sep 2020 15:02:27 -0700 In-Reply-To: <20200922143151.34962-4-dadschoorse@gmail.com> References: <20200922143151.34962-1-dadschoorse@gmail.com> <20200922143151.34962-4-dadschoorse@gmail.com> 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; > >