[PATCH v2 3/4] winevulkan: Store a mapping from native handles to wrappers.
Liam Middlebrook
lmiddlebrook at nvidia.com
Fri Sep 25 04:33:24 CDT 2020
On 9/22/20 3:26 PM, Dad Schoorse wrote:
> On 22. Sept. 2020, 23:25, Liam Middlebrook wrote:
> > On 9/22/20 7:31 AM, Georg Lehmann wrote:
> >> Signed-off-by: Georg Lehmann <dadschoorse at gmail.com
> <mailto:dadschoorse at gmail.com>>
> >> ---
> >> dlls/winevulkan/vulkan.c | 69 ++++++++++++++++++++++++++++++++
> >> dlls/winevulkan/vulkan_private.h | 26 ++++++++++++
> >> 2 files changed, 95 insertions(+)
> >>
> >> diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
> >> index f730c04923..2747f440ad 100644
> >> --- a/dlls/winevulkan/vulkan.c
> >> +++ b/dlls/winevulkan/vulkan.c
> >> @@ -66,11 +66,57 @@ static VkResult
> (*p_vkEnumerateInstanceVersion)(uint32_t *version);
> >> void WINAPI wine_vkGetPhysicalDeviceProperties(VkPhysicalDevice
> physical_device,
> >> VkPhysicalDeviceProperties *properties);
> >>
> >> +#define WINE_VK_ADD_HANDLE_MAPPING(instance, object, native_handle)\
> >> + wine_vk_add_handle_mapping((instance), (uint64_t) (uintptr_t)
> (object), (uint64_t) (native_handle), &(object)->mapping)
> >
> > nit: please put a space before the final '\' here and for other
> > continuations like this.
> >
> > Rather than explicitly casting to (uintptr_t) at almost every call to
> > VK_ADD_HANDLE_MAPPING() would it make more sense to do the same
> > double-cast that you're doing above for the object parameter?
> >
>
> We can't implicitly cast to uintptr_t because non-dispatchable handles are
> 64bit integers on 32bit systems.
>
> But I think it makes sense to have a macro for dispatchable handles with
> the implicit cast and one without it.
Sounds good to me.
>
> >> +static void wine_vk_add_handle_mapping(struct VkInstance_T
> *instance, uint64_t wrapper,
> >> + uint64_t native_handle, struct wine_vk_mapping *mapping)
> >> +{
> >> + if (instance->enable_wrapper_list)
> >> + {
> >> + mapping->native_handle = native_handle;
> >> + mapping->wine_wrapper = wrapper;
> >> + AcquireSRWLockExclusive(&instance->wrapper_lock);
> >> + list_add_tail(&instance->wrappers, &mapping->link);
> >> + ReleaseSRWLockExclusive(&instance->wrapper_lock);
> >> + }
> >> +}
> >> +
> >> +#define WINE_VK_REMOVE_HANDLE_MAPPING(instance, object)\
> >> + wine_vk_remove_handle_mapping((instance), &(object)->mapping)
> >> +static void wine_vk_remove_handle_mapping(struct VkInstance_T
> *instance, struct wine_vk_mapping *mapping)
> >> +{
> >> + if (instance->enable_wrapper_list)
> >> + {
> >> + AcquireSRWLockExclusive(&instance->wrapper_lock);
> >> + list_remove(&mapping->link);
> >> + ReleaseSRWLockExclusive(&instance->wrapper_lock);
> >> + }
> >> +}
> >> +
> >> +static uint64_t wine_vk_get_wrapper(struct VkInstance_T *instance,
> uint64_t native_handle)
> >> +{
> >> + struct wine_vk_mapping *mapping;
> >> + uint64_t result = 0;
> >> +
> >> + AcquireSRWLockShared(&instance->wrapper_lock);
> >> + LIST_FOR_EACH_ENTRY(mapping, &instance->wrappers, struct
> wine_vk_mapping, link)
> >> + {
> >> + if (mapping->native_handle == native_handle)
> >> + {
> >> + result = mapping->wine_wrapper;
> >> + break;
> >> + }
> >> + }
> >> + ReleaseSRWLockShared(&instance->wrapper_lock);
> >> + return result;
> >> +}
> >> +
> >> static void wine_vk_physical_device_free(struct VkPhysicalDevice_T
> *phys_dev)
> >> {
> >> if (!phys_dev)
> >> return;
> >>
> >> + WINE_VK_REMOVE_HANDLE_MAPPING(phys_dev->instance, phys_dev);
> >> heap_free(phys_dev->extensions);
> >> heap_free(phys_dev);
> >> }
> >> @@ -91,6 +137,8 @@ static struct VkPhysicalDevice_T
> *wine_vk_physical_device_alloc(struct VkInstanc
> >> object->instance = instance;
> >> object->phys_dev = phys_dev;
> >>
> >> + WINE_VK_ADD_HANDLE_MAPPING(instance, object, (uintptr_t) phys_dev);
> >> +
> >> res =
> instance->funcs.p_vkEnumerateDeviceExtensionProperties(phys_dev,
> >> NULL, &num_host_properties, NULL);
> >> if (res != VK_SUCCESS)
> >> @@ -169,6 +217,7 @@ static void wine_vk_free_command_buffers(struct
> VkDevice_T *device,
> >>
> >> device->funcs.p_vkFreeCommandBuffers(device->device,
> pool->command_pool, 1, &buffers[i]->command_buffer);
> >> list_remove(&buffers[i]->pool_link);
> >> + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance,
> buffers[i]);
> >> heap_free(buffers[i]);
> >> }
> >> }
> >> @@ -212,6 +261,8 @@ static struct VkQueue_T
> *wine_vk_device_alloc_queues(struct VkDevice_T *device,
> >> {
> >> device->funcs.p_vkGetDeviceQueue(device->device,
> family_index, i, &queue->queue);
> >> }
> >> +
> >> + WINE_VK_ADD_HANDLE_MAPPING(device->phys_dev->instance,
> queue, (uintptr_t) queue->queue);
> >> }
> >>
> >> return queues;
> >> @@ -294,6 +345,8 @@ static void wine_vk_device_free(struct
> VkDevice_T *device)
> >> unsigned int i;
> >> for (i = 0; i < device->max_queue_families; i++)
> >> {
> >> + if (device->queues[i] && device->queues[i]->queue)
> >> +
> WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance,
> device->queues[i]);
> >> heap_free(device->queues[i]);
> >> }
> >> heap_free(device->queues);
> >> @@ -302,6 +355,7 @@ static void wine_vk_device_free(struct
> VkDevice_T *device)
> >>
> >> if (device->device && device->funcs.p_vkDestroyDevice)
> >> {
> >> + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance,
> device);
> >> device->funcs.p_vkDestroyDevice(device->device, NULL /*
> pAllocator */);
> >> }
> >>
> >> @@ -512,6 +566,7 @@ VkResult WINAPI
> wine_vkAllocateCommandBuffers(VkDevice device,
> >> list_add_tail(&pool->command_buffers, &buffers[i]->pool_link);
> >> res = device->funcs.p_vkAllocateCommandBuffers(device->device,
> >> &allocate_info_host, &buffers[i]->command_buffer);
> >> + WINE_VK_ADD_HANDLE_MAPPING(device->phys_dev->instance,
> buffers[i], (uintptr_t) buffers[i]->command_buffer);
> >> if (res != VK_SUCCESS)
> >> {
> >> ERR("Failed to allocate command buffer, res=%d.\n", res);
> >> @@ -589,6 +644,7 @@ VkResult WINAPI
> wine_vkCreateDevice(VkPhysicalDevice phys_dev,
> >> return VK_ERROR_OUT_OF_HOST_MEMORY;
> >>
> >> object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE;
> >> + object->phys_dev = phys_dev;
> >>
> >> res = wine_vk_device_convert_create_info(create_info,
> &create_info_host);
> >> if (res != VK_SUCCESS)
> >> @@ -597,6 +653,7 @@ VkResult WINAPI
> wine_vkCreateDevice(VkPhysicalDevice phys_dev,
> >> res =
> phys_dev->instance->funcs.p_vkCreateDevice(phys_dev->phys_dev,
> >> &create_info_host, NULL /* allocator */, &object->device);
> >> wine_vk_device_free_create_info(&create_info_host);
> >> + WINE_VK_ADD_HANDLE_MAPPING(phys_dev->instance, object,
> (uintptr_t) object->device);
> >> if (res != VK_SUCCESS)
> >> {
> >> WARN("Failed to create device, res=%d.\n", res);
> >> @@ -678,6 +735,8 @@ VkResult WINAPI wine_vkCreateInstance(const
> VkInstanceCreateInfo *create_info,
> >> return VK_ERROR_OUT_OF_HOST_MEMORY;
> >> }
> >> object->base.loader_magic = VULKAN_ICD_MAGIC_VALUE;
> >> + list_init(&object->wrappers);
> >> + InitializeSRWLock(&object->wrapper_lock);
> >
> > I wasn't able to find cleanup functions for these on instance
> destruction.
> >
>
> There's no cleanup necessary for both of these.
Ah sorry about that. I didn't realize these weren't treated as distinct
heap allocations.
>
> >>
> >> res = wine_vk_instance_convert_create_info(create_info,
> &create_info_host, object);
> >> if (res != VK_SUCCESS)
> >> @@ -695,6 +754,8 @@ VkResult WINAPI wine_vkCreateInstance(const
> VkInstanceCreateInfo *create_info,
> >> return res;
> >> }
> >>
> >> + WINE_VK_ADD_HANDLE_MAPPING(object, object, (uintptr_t)
> object->instance);
> >> +
> >
> > I wasn't able to find a matching WINE_VK_REMOVE_HANDLE_MAPPING call for
> > this.
> >
>
> With the current handle mapping implementation that call is not necessary.
> VkInstance is going to be the last object in the list anyway and freeing
> the instance object will also free the list.
> But it's probably a good idea to call WINE_VK_REMOVE_HANDLE_MAPPING anyway,
> in case something changes in the future?
I don't think it'll necessarily change in the future, but I'd prefer
this to be explicitly cleaned up.
>
> >> /* Load all instance functions we are aware of. Note the
> loader takes care
> >> * of any filtering for extensions which were not requested,
> but which the
> >> * ICD may support.
> >> @@ -1129,9 +1190,14 @@ VkResult WINAPI
> wine_vkCreateCommandPool(VkDevice device, const VkCommandPoolCre
> >> res = device->funcs.p_vkCreateCommandPool(device->device,
> info, NULL, &object->command_pool);
> >>
> >> if (res == VK_SUCCESS)
> >> + {
> >> + WINE_VK_ADD_HANDLE_MAPPING(device->phys_dev->instance,
> object, object->command_pool);
> >> *command_pool = wine_cmd_pool_to_handle(object);
> >> + }
> >> else
> >> + {
> >> heap_free(object);
> >> + }
> >>
> >> return res;
> >> }
> >> @@ -1156,9 +1222,12 @@ void WINAPI
> wine_vkDestroyCommandPool(VkDevice device, VkCommandPool handle,
> >> */
> >> LIST_FOR_EACH_ENTRY_SAFE(buffer, cursor,
> &pool->command_buffers, struct VkCommandBuffer_T, pool_link)
> >> {
> >> + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance,
> buffer);
> >> heap_free(buffer);
> >> }
> >>
> >> + WINE_VK_REMOVE_HANDLE_MAPPING(device->phys_dev->instance, pool);
> >> +
> >> device->funcs.p_vkDestroyCommandPool(device->device,
> pool->command_pool, NULL);
> >> heap_free(pool);
> >> }
> >> diff --git a/dlls/winevulkan/vulkan_private.h
> b/dlls/winevulkan/vulkan_private.h
> >> index 4bcc4de440..146bbd8fa4 100644
> >> --- a/dlls/winevulkan/vulkan_private.h
> >> +++ b/dlls/winevulkan/vulkan_private.h
> >> @@ -60,6 +60,16 @@ struct wine_vk_base
> >> UINT_PTR loader_magic;
> >> };
> >>
> >> +/* Some extensions have callbacks for those we need to be able to
> >> + * get the wine wrapper for a native handle
> >> + */
> >> +struct wine_vk_mapping
> >> +{
> >> + struct list link;
> >> + uint64_t native_handle;
> >> + uint64_t wine_wrapper;
> >
> > It might be good to add a comment here noting the bitsize of Vulkan
> > handles in comparison to native pointers.
> >
>
> I'm not sure what the comment should say, generic handles are always
> passed as uint64_t in Vulkan.
Eh, I guess a comment isn't really needed here. Let's just go with the
suggested renaming below, and that should have a similar effect for clarity.
Thanks,
Liam Middlebrook
>
> > Also I think it would make sense to include 'handle' in the name for the
> > WINE wrapper handle. Perhaps:
> > wine_wrapped_handle
> > wrapped_handle
> > or something else?
> >
>
> I wasn't sure myself, but wine_wrapped_handle is probably the clearer name.
>
> Thanks,
>
> Georg Lehmann
>
> >
> > Thanks,
> >
> > Liam Middlebrook
> >
> >
> >> +};
> >> +
> >> struct VkCommandBuffer_T
> >> {
> >> struct wine_vk_base base;
> >> @@ -67,18 +77,22 @@ struct VkCommandBuffer_T
> >> VkCommandBuffer command_buffer; /* native command buffer */
> >>
> >> struct list pool_link;
> >> + struct wine_vk_mapping mapping;
> >> };
> >>
> >> struct VkDevice_T
> >> {
> >> struct wine_vk_base base;
> >> struct vulkan_device_funcs funcs;
> >> + struct VkPhysicalDevice_T *phys_dev; /* parent */
> >> VkDevice device; /* native device */
> >>
> >> struct VkQueue_T **queues;
> >> uint32_t max_queue_families;
> >>
> >> unsigned int quirks;
> >> +
> >> + struct wine_vk_mapping mapping;
> >> };
> >>
> >> struct VkInstance_T
> >> @@ -93,7 +107,13 @@ struct VkInstance_T
> >> struct VkPhysicalDevice_T **phys_devs;
> >> uint32_t phys_dev_count;
> >>
> >> + VkBool32 enable_wrapper_list;
> >> + struct list wrappers;
> >> + SRWLOCK wrapper_lock;
> >> +
> >> unsigned int quirks;
> >> +
> >> + struct wine_vk_mapping mapping;
> >> };
> >>
> >> struct VkPhysicalDevice_T
> >> @@ -104,6 +124,8 @@ struct VkPhysicalDevice_T
> >>
> >> VkExtensionProperties *extensions;
> >> uint32_t extension_count;
> >> +
> >> + struct wine_vk_mapping mapping;
> >> };
> >>
> >> struct VkQueue_T
> >> @@ -113,6 +135,8 @@ struct VkQueue_T
> >> VkQueue queue; /* native queue */
> >>
> >> VkDeviceQueueCreateFlags flags;
> >> +
> >> + struct wine_vk_mapping mapping;
> >> };
> >>
> >> struct wine_cmd_pool
> >> @@ -120,6 +144,8 @@ struct wine_cmd_pool
> >> VkCommandPool command_pool;
> >>
> >> struct list command_buffers;
> >> +
> >> + struct wine_vk_mapping mapping;
> >> };
> >>
> >> static inline struct wine_cmd_pool
> *wine_cmd_pool_from_handle(VkCommandPool handle)
> >>
More information about the wine-devel
mailing list