[PATCH v2 4/4] winevulkan: Implement VK_EXT_debug_utils.
Liam Middlebrook
lmiddlebrook at nvidia.com
Fri Sep 25 05:26:27 CDT 2020
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;
> >>
> >>
> >
More information about the wine-devel
mailing list