[PATCH 2/3] winevulkan: Implement vkEnumeratePhysicalDevices.
Roderick Colenbrander
thunderbird2k at gmail.com
Fri Mar 2 09:44:37 CST 2018
On Fri, Mar 2, 2018 at 3:57 AM, Józef Kucia <joseph.kucia at gmail.com> wrote:
> On Fri, Mar 2, 2018 at 7:13 AM, Roderick Colenbrander
> <thunderbird2k at gmail.com> wrote:
>> Signed-off-by: Roderick Colenbrander <thunderbird2k at gmail.com>
>> ---
>> dlls/winevulkan/make_vulkan | 1 +
>> dlls/winevulkan/vulkan.c | 140 +++++++++++++++++++++++++++++++++++++++
>> dlls/winevulkan/vulkan_private.h | 14 ++++
>> dlls/winevulkan/vulkan_thunks.c | 6 --
>> dlls/winevulkan/vulkan_thunks.h | 1 +
>> 5 files changed, 156 insertions(+), 6 deletions(-)
>>
>> diff --git a/dlls/winevulkan/make_vulkan b/dlls/winevulkan/make_vulkan
>> index 5657b70c4c..3b045db8a9 100755
>> --- a/dlls/winevulkan/make_vulkan
>> +++ b/dlls/winevulkan/make_vulkan
>> @@ -90,6 +90,7 @@ FUNCTION_OVERRIDES = {
>>
>> # Instance functions
>> "vkDestroyInstance" : {"dispatch" : True, "driver" : True, "thunk" : False },
>> + "vkEnumeratePhysicalDevices" : {"dispatch" : True, "driver" : False, "thunk" : False},
>> }
>>
>>
>> diff --git a/dlls/winevulkan/vulkan.c b/dlls/winevulkan/vulkan.c
>> index 0f48d971e2..234be589ae 100644
>> --- a/dlls/winevulkan/vulkan.c
>> +++ b/dlls/winevulkan/vulkan.c
>> @@ -58,6 +58,85 @@ static BOOL wine_vk_init(void)
>> return TRUE;
>> }
>>
>> +/* Helper function which stores wrapped physical devices in the instance object. */
>> +static VkResult wine_vk_instance_load_physical_devices(struct VkInstance_T *instance)
>> +{
>> + VkResult res;
>> + struct VkPhysicalDevice_T **tmp_phys_devs = NULL;
>> + uint32_t num_phys_devs = 0;
>> + unsigned int i;
>> +
>> + res = instance->funcs.p_vkEnumeratePhysicalDevices(instance->instance, &num_phys_devs, NULL);
>> + if (res != VK_SUCCESS)
>> + {
>> + ERR("Failed to enumerate physical devices, res=%d\n", res);
>> + return res;
>> + }
>> +
>> + /* Don't bother with any of the rest if the system just lacks devices. */
>> + if (num_phys_devs == 0)
>> + {
>> + instance->num_phys_devs = 0;
>
> instance->phys_devs isn't initialized.
>
>> + return VK_SUCCESS;
>> + }
>> +
>> + tmp_phys_devs = heap_alloc(num_phys_devs * sizeof(*tmp_phys_devs));
>> + if (!tmp_phys_devs)
>> + return VK_ERROR_OUT_OF_HOST_MEMORY;
>
> This leaves a lot of fields uninitialized in "instance". We don't want
> to crash in wine_vk_instance_free().
>
>> +
>> + res = instance->funcs.p_vkEnumeratePhysicalDevices(instance->instance, &num_phys_devs, tmp_phys_devs);
>> + if (res != VK_SUCCESS)
>> + goto err;
>> +
>> + instance->phys_devs = heap_alloc(num_phys_devs * sizeof(*instance->phys_devs));
>
> heap_calloc() may be more suitable.
>
>> + if (!instance->phys_devs)
>> + {
>> + res = VK_ERROR_OUT_OF_HOST_MEMORY;
>> + goto err;
>> + }
>> +
>> + /* Wrap each native physical device handle into a dispatchable object for the ICD loader. */
>> + for (i = 0; i < num_phys_devs; i++)
>> + {
>> + VkPhysicalDevice phys_dev = heap_alloc(sizeof(*phys_dev));
>
> struct VkPhysicalDevice_T *
>
>> + if (!phys_dev)
>> + {
>> + ERR("Unable to allocate memory for physical device!\n");
>> + res = VK_ERROR_OUT_OF_HOST_MEMORY;
>> + goto err;
>> + }
>> +
>> + phys_dev->base.loader_magic = VULKAN_ICD_MAGIC_VALUE;
>> + phys_dev->instance = instance;
>> + phys_dev->phys_dev = tmp_phys_devs[i];
>> +
>> + instance->phys_devs[i] = phys_dev;
>> + instance->num_phys_devs = i;
>> + }
>> + instance->num_phys_devs = num_phys_devs;
>> +
>> + heap_free(tmp_phys_devs);
>> + return VK_SUCCESS;
>> +
>> +err:
>> + if (tmp_phys_devs)
>> + heap_free(tmp_phys_devs);
>
> It's fine to call heap_free() with NULL.
I believe it results in undefined behaviour, that's what MSDN told me
and our heap_alloc doesn't have guards for NULL.
>From MSDN:
"A pointer to the memory block to be freed. This pointer is returned
by the HeapAlloc or HeapReAlloc function. If this pointer is NULL, the
behavior is undefined."
>> +
>> + if (instance->phys_devs)
>> + {
>> + for (i = 0; i < instance->num_phys_devs; i++)
>> + {
>> + heap_free(instance->phys_devs[i]);
>> + instance->phys_devs[i] = NULL;
>> + }
>> + heap_free(instance->phys_devs);
>> + instance->num_phys_devs = 0;
>> + instance->phys_devs = NULL;
>> + }
>> +
>> + return res;
>> +}
>> +
>> /* Helper function used for freeing an instance structure. This function supports full
>> * and partial object cleanups and can thus be used for vkCreateInstance failures.
>> */
>> @@ -66,6 +145,17 @@ static void wine_vk_instance_free(struct VkInstance_T *instance)
>> if (!instance)
>> return;
>>
>> + if (instance->phys_devs)
>> + {
>> + unsigned int i;
>> +
>> + for (i = 0; i < instance->num_phys_devs; i++)
>> + {
>> + heap_free(&instance->phys_devs[i]);
>> + }
>> + heap_free(instance->phys_devs);
>> + }
>
> It might crash because "phys_devs" is uninitialized. We probably want
> to use heap_alloc_zero() to allocate "instance".
You are right. I used to have a wine_vk_alloc or some call before
heap_alloc got introduced, which did the zero'ing. Forgot to change
this spot to heap_alloc_zero.
>
>> +
>> if (instance->instance)
>> vk_funcs->p_vkDestroyInstance(instance->instance, NULL /* allocator */);
>>
>> @@ -108,6 +198,18 @@ static VkResult WINAPI wine_vkCreateInstance(const VkInstanceCreateInfo *create_
>> ALL_VK_INSTANCE_FUNCS()
>> #undef USE_VK_FUNC
>>
>> + /* Cache physical devices for vkEnumeratePhysicalDevices within the instance as
>> + * each vkPhysicalDevice is a dispatchable object, which means we need to wrap
>> + * the native physical device and present those the application.
>> + * Cleanup happens as part of wine_vkDestroyInstance.
>> + */
>> + res = wine_vk_instance_load_physical_devices(object);
>> + if (res != VK_SUCCESS)
>> + {
>> + ERR("Failed to cache physical devices, res=%d\n", res);
>> + goto err;
>> + }
>> +
>> *instance = object;
>> TRACE("Done, instance=%p native_instance=%p\n", object, object->instance);
>> return VK_SUCCESS;
>> @@ -134,6 +236,44 @@ static VkResult WINAPI wine_vkEnumerateInstanceExtensionProperties(const char *l
>> return vk_funcs->p_vkEnumerateInstanceExtensionProperties(layer_name, count, properties);
>> }
>>
>> +VkResult WINAPI wine_vkEnumeratePhysicalDevices(VkInstance instance, uint32_t *device_count,
>> + VkPhysicalDevice *devices)
>> +{
>> + VkResult res;
>> + unsigned int i, num_copies;
>> +
>> + TRACE("%p %p %p\n", instance, device_count, devices);
>> +
>> + if (!devices)
>> + {
>> + *device_count = instance->num_phys_devs;
>> + return VK_SUCCESS;
>> + }
>> +
>> + if (*device_count < instance->num_phys_devs)
>> + {
>> + /* Incomplete is a type of success used to signal the application
>> + * that not all devices got copied.
>> + */
>> + num_copies = *device_count;
>> + res = VK_INCOMPLETE;
>> + }
>> + else
>> + {
>> + num_copies = instance->num_phys_devs;
>> + res = VK_SUCCESS;
>> + }
>> +
>> + for (i = 0; i < num_copies; i++)
>> + {
>> + devices[i] = instance->phys_devs[i];
>> + }
>> + *device_count = num_copies;
>> +
>> + TRACE("Returning %d devices\n", *device_count);
>> + return res;
>> +}
>> +
>> static PFN_vkVoidFunction WINAPI wine_vkGetInstanceProcAddr(VkInstance instance, const char *name)
>> {
>> void *func;
>> diff --git a/dlls/winevulkan/vulkan_private.h b/dlls/winevulkan/vulkan_private.h
>> index 16aa0ce55c..8ef4b924a2 100644
>> --- a/dlls/winevulkan/vulkan_private.h
>> +++ b/dlls/winevulkan/vulkan_private.h
>> @@ -52,7 +52,21 @@ struct VkInstance_T
>> {
>> struct wine_vk_base base;
>> struct vulkan_instance_funcs funcs;
>> +
>> + /* We cache devices as we need to wrap them as they are
>> + * dispatchable objects.
>> + */
>> + uint32_t num_phys_devs;
>> + struct VkPhysicalDevice_T **phys_devs;
>> +
>> VkInstance instance; /* native instance */
>> };
>>
>> +struct VkPhysicalDevice_T
>> +{
>> + struct wine_vk_base base;
>> + struct VkInstance_T *instance; /* parent */
>> + VkPhysicalDevice phys_dev; /* native physical device */
>> +};
>> +
>> #endif /* __WINE_VULKAN_PRIVATE_H */
>> diff --git a/dlls/winevulkan/vulkan_thunks.c b/dlls/winevulkan/vulkan_thunks.c
>> index c1c0b079a5..ae4f773ef0 100644
>> --- a/dlls/winevulkan/vulkan_thunks.c
>> +++ b/dlls/winevulkan/vulkan_thunks.c
>> @@ -28,12 +28,6 @@ static VkResult WINAPI wine_vkEnumerateDeviceLayerProperties(VkPhysicalDevice ph
>> return VK_ERROR_OUT_OF_HOST_MEMORY;
>> }
>>
>> -static VkResult WINAPI wine_vkEnumeratePhysicalDevices(VkInstance instance, uint32_t *pPhysicalDeviceCount, VkPhysicalDevice *pPhysicalDevices)
>> -{
>> - FIXME("stub: %p, %p, %p\n", instance, pPhysicalDeviceCount, pPhysicalDevices);
>> - return VK_ERROR_OUT_OF_HOST_MEMORY;
>> -}
>> -
>> static void WINAPI wine_vkGetPhysicalDeviceFeatures(VkPhysicalDevice physicalDevice, VkPhysicalDeviceFeatures *pFeatures)
>> {
>> FIXME("stub: %p, %p\n", physicalDevice, pFeatures);
>> diff --git a/dlls/winevulkan/vulkan_thunks.h b/dlls/winevulkan/vulkan_thunks.h
>> index 087fa17d0e..26e114ab74 100644
>> --- a/dlls/winevulkan/vulkan_thunks.h
>> +++ b/dlls/winevulkan/vulkan_thunks.h
>> @@ -8,6 +8,7 @@ void *wine_vk_get_instance_proc_addr(const char *name) DECLSPEC_HIDDEN;
>>
>> /* Functions for which we have custom implementations outside of the thunks. */
>> void WINAPI wine_vkDestroyInstance(VkInstance instance, const VkAllocationCallbacks *pAllocator) DECLSPEC_HIDDEN;
>> +VkResult WINAPI wine_vkEnumeratePhysicalDevices(VkInstance instance, uint32_t *pPhysicalDeviceCount, VkPhysicalDevice *pPhysicalDevices) DECLSPEC_HIDDEN;
>>
>> /* For use by vkInstance and children */
>> struct vulkan_instance_funcs
>> --
>> 2.14.3
>>
>>
>>
More information about the wine-devel
mailing list