[PATCH v2 3/4] winevulkan: Store a mapping from native handles to wrappers.
Dad Schoorse
dadschoorse at gmail.com
Tue Sep 22 17:26:20 CDT 2020
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>
>> ---
>> 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.
>> +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.
>>
>> 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?
>> /* 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.
> 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)
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200923/e339cafa/attachment-0001.htm>
More information about the wine-devel
mailing list