[PATCH v2 0/1] MR5658: winevulkan: Make device memory wrapper available in callbacks.
With [`VK_EXT_device_address_binding_report`](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_dev...) we can get [debug_util](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_EXT_deb...) callbacks used to track memory bindings. Since it's the host's implementation that starts the callback we have to be sure that we have a way of converting it to the client side's variant before it's added to the handle map - i.e. we don't know the host handle at that time yet. This is [used by vkd3d-proton](https://github.com/HansKristian-Work/vkd3d-proton/pull/1962). Requires Mesa with https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/28649. before (note the missing BIND for VkDevice which actually means `VkDeviceMemory`): ``` vkd3d-proton % VKD3D_TEST_FILTER=create_placed_resource_size VKD3D_CONFIG=fault VKD3D_DEBUG=trace ~/src/wine/build/wine ./tests/d3d12.exe 2>&1 | grep vkd3d_address_binding_callback trace:vkd3d-proton:vkd3d_address_binding_callback: BIND || VkBuffer || VA ffff800100200000 || size 0000000001000000. trace:vkd3d-proton:vkd3d_address_binding_callback: BIND || VkImage || VA ffff800100200000 || size 000000000019a000. trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkImage || VA ffff800100200000 || size 000000000019a000. trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkBuffer || VA ffff800100200000 || size 0000000001000000. :trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkDevice || VA ffff800100200000 || size 0000000001000000. 232285.553:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: BIND || VkBuffer || VA ffff800100200000 || size 0000000001000000. 232285.553:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkBuffer || VA ffff800100200000 || size 0000000001000000. 232285.553:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkDevice || VA ffff800100200000 || size 0000000001000000. ``` after: ``` % VKD3D_TEST_FILTER=create_placed_resource_size VKD3D_CONFIG=fault VKD3D_DEBUG=trace ~/src/wine/build/wine ./tests/d3d12.exe 2>&1 | grep vkd3d_address_binding_callback 232338.036:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: BIND || VkDevice || VA ffff800100200000 || size 0000000001000000. 232338.036:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: BIND || VkBuffer || VA ffff800100200000 || size 0000000001000000. 232338.037:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: BIND || VkImage || VA ffff800100200000 || size 000000000019a000. 232338.037:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkImage || VA ffff800100200000 || size 000000000019a000. 232338.037:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkBuffer || VA ffff800100200000 || size 0000000001000000. 232338.037:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkDevice || VA ffff800100200000 || size 0000000001000000. 232338.037:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: BIND || VkDevice || VA ffff800100200000 || size 0000000001000000. 232338.037:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: BIND || VkBuffer || VA ffff800100200000 || size 0000000001000000. 232338.037:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkBuffer || VA ffff800100200000 || size 0000000001000000. 232338.037:0020:0024:trace:vkd3d-proton:vkd3d_address_binding_callback: UNBIND || VkDevice || VA ffff800100200000 || size 0000000001000000. ``` [The spec guarantees](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#de...) the following:
An application can receive multiple callbacks if multiple VkDebugUtilsMessengerEXT objects are created. A callback will always be executed in the same thread as the originating Vulkan call.
As of TLS I went with a suggestion from IRC: ``` <ivyl_> There's an issue with how we handle callbacks related to VK_EXT_debug_utils and VK_EXT_debug_report. In wine_vkAllocateMemory() we call device->funcs.p_vkAllocateMemory() which may execute callback. <ivyl_> At this point we still don't have handle mapping added because we don't know what host_memory handle will be. <ivyl_> AFAIU there's a spec guarantee that callback will be executed in the same thread as the call causing it, so I was thinking about maybe keeping `memory` in a tls (if enable_wrapper_list) and using that? <ivyl_> but that's I don't see any precedent of using thread local storage on the unix side <ivyl_> tls value would be short-lived in such case - only for the duration of the call to `device->funcs.p_vkAllocateMemory()` <jacekc_> we usually use teb on unix side, see ntuser_thread_info ``` -- v2: winevulkan: Make device memory wrapper available in callbacks. https://gitlab.winehq.org/wine/wine/-/merge_requests/5658
From: Arkadiusz Hiler <ahiler(a)codeweavers.com> With VK_EXT_device_address_binding_report we can get debug_util callbacks used to track memory bindings. Since it's the host's implementation that starts the callback we have to be sure that we have a way of converting it to the client side's variant before it's added to the handle map - i.e. we don't know the host handle at that time yet. --- dlls/winevulkan/vulkan.c | 33 ++++++++++++++++++++++++++++++--- include/ntuser.h | 1 + 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c index e7d00813a42..4e1086b7cd4 100644 --- a/dlls/winevulkan/vulkan.c +++ b/dlls/winevulkan/vulkan.c @@ -159,6 +159,28 @@ static uint64_t client_handle_from_host(struct wine_instance *instance, uint64_t return result; } +static void set_transient_client_handle(struct wine_instance *instance, struct wine_device_memory *memory) +{ + struct ntuser_thread_info *thread_info; + if (!instance->enable_wrapper_list) return; + thread_info = NtUserGetThreadInfo(); + thread_info->vulkan_data = (uintptr_t)memory; +} + +static uint64_t get_transient_handle(uint64_t host_handle) +{ + struct ntuser_thread_info *thread_info; + struct wine_device_memory *memory; + + thread_info = NtUserGetThreadInfo(); + if (!thread_info->vulkan_data) return 0; + + memory = (struct wine_device_memory *)(uintptr_t)thread_info->vulkan_data; + memory->host_memory = host_handle; + + return (uintptr_t)memory; +} + static VkBool32 debug_utils_callback_conversion(VkDebugUtilsMessageSeverityFlagBitsEXT severity, VkDebugUtilsMessageTypeFlagsEXT message_types, const VkDebugUtilsMessengerCallbackDataEXT *callback_data, @@ -200,6 +222,8 @@ static VkBool32 debug_utils_callback_conversion(VkDebugUtilsMessageSeverityFlagB if (wine_vk_is_type_wrapped(callback_data->pObjects[i].objectType)) { object_name_infos[i].objectHandle = client_handle_from_host(object->instance, callback_data->pObjects[i].objectHandle); + if (!object_name_infos[i].objectHandle && object_name_infos[i].objectType == VK_OBJECT_TYPE_DEVICE_MEMORY) + object_name_infos[i].objectHandle = get_transient_handle(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)); @@ -1901,16 +1925,19 @@ VkResult wine_vkAllocateMemory(VkDevice handle, const VkMemoryAllocateInfo *allo if (!(memory = malloc(sizeof(*memory)))) return VK_ERROR_OUT_OF_HOST_MEMORY; + memory->size = info.allocationSize; + memory->vm_map = mapping; + + set_transient_client_handle(device->phys_dev->instance, memory); result = device->funcs.p_vkAllocateMemory(device->host_device, &info, NULL, &memory->host_memory); + set_transient_client_handle(device->phys_dev->instance, NULL); + if (result != VK_SUCCESS) { free(memory); return result; } - memory->size = info.allocationSize; - memory->vm_map = mapping; - *ret = (VkDeviceMemory)(uintptr_t)memory; add_handle_mapping(device->phys_dev->instance, *ret, memory->host_memory, &memory->wrapper_entry); return VK_SUCCESS; diff --git a/include/ntuser.h b/include/ntuser.h index f947fec7fea..14fc51756f6 100644 --- a/include/ntuser.h +++ b/include/ntuser.h @@ -87,6 +87,7 @@ struct ntuser_thread_info UINT default_imc; /* default input context */ UINT64 client_imm; /* client IMM thread info */ UINT64 wmchar_data; /* client data for WM_CHAR mappings */ + UINT64 vulkan_data; /* used by winevulkan for tls */ }; static inline struct ntuser_thread_info *NtUserGetThreadInfo(void) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5658
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=145856 Your paranoid android. === debian11b (64 bit WoW report) === winmm: mci: Timeout
Rémi Bernon (@rbernon) commented about dlls/winevulkan/vulkan.c:
+ thread_info = NtUserGetThreadInfo(); + thread_info->vulkan_data = (uintptr_t)memory; +} + +static uint64_t get_transient_handle(uint64_t host_handle) +{ + struct ntuser_thread_info *thread_info; + struct wine_device_memory *memory; + + thread_info = NtUserGetThreadInfo(); + if (!thread_info->vulkan_data) return 0; + + memory = (struct wine_device_memory *)(uintptr_t)thread_info->vulkan_data; + memory->host_memory = host_handle; + + return (uintptr_t)memory; I don't think we should update the client object here, and this should IMO just return the client handle for the debug callback.
Only once the object is created the client object will be updated, and a proper handle mapping added. (This was what I though it was doing, and the reason why I said you didn't need the host handle). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5658#note_71334
On Sat May 25 13:22:59 2024 +0000, Rémi Bernon wrote:
I don't think we should update the client object here, and this should IMO just return the client handle for the debug callback. Only once the object is created the client object will be updated, and a proper handle mapping added. (This was what I though it was doing, and the reason why I said you didn't need the host handle). I had a look at what vkd3d-proton does and indeed it just uses the value of the handle.
Reading through the spec again I've [found](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/PFN_vkDebu...):
The callback must not make calls to any Vulkan commands
I thought that the object may be used from the callback but that's not true. I'll fix this and push a new version. Thanks! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5658#note_73899
participants (4)
-
Arek Hiler (＠ivyl) -
Arkadiusz Hiler
-
Marvin -
Rémi Bernon