[PATCH v10 [rebased] 2/4] vulkan-1/tests: Add tests for VK_KHR_external_memory_win32.

[Zebediah Figura (she/her)]

On 7/6/21 11:24 AM, Derek Lesho wrote:
> 
> On 7/1/21 3:59 PM, Zebediah Figura (she/her) wrote:
>>> +    argc = winetest_get_mainargs(&argv);
>>> +    if (argc > 3 && !strcmp(argv[2], "resource"))
>>> +    {
>>> +        sscanf(argv[6], "%p", &handle);
>>> +
>>> +        import_handle_info.sType =
>>> VK_STRUCTURE_TYPE_IMPORT_MEMORY_WIN32_HANDLE_INFO_KHR;
>>> +        import_handle_info.pNext = &dedicated_alloc_info;
>>> +        import_handle_info.handleType = strcmp(argv[5], "kmt") ?
>>> +            VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_BIT_KHR :
>>> + VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_KMT_BIT_KHR;
>>> +        import_handle_info.handle = handle;
>>> +        import_handle_info.name = NULL;
>>> +
>>> +        alloc_info.pNext = &import_handle_info;
>>> +
>>> +        vr = vkAllocateMemory(vk_device, &alloc_info, NULL,
>>> &vk_memory);
>>> +        ok(vr == VK_SUCCESS, "vkAllocateMemory failed, VkResult
>>> %d.\n", vr);
>>> +
>>> +        vkFreeMemory(vk_device, vk_memory, NULL);
>>> +        vkDestroyBuffer(vk_device, vk_buffer, NULL);
>>> +        vkDestroyDevice(vk_device, NULL);
>>
>> Any reason not to include the by-name tests here?
>>
>> For that matter, you could add a helper function to test both
>> in-process and cross-process import.
> 
> I don't think a helper function for this would be very helpful, the only
> part you could truly abstract away would be filling import_handle_info
> and import_handle_info, which isn't too much boilerplate to duplicate.

I don't follow. As far as I can see you should be able to perform the 
same tests both in-process and out-of-process—i.e. import by handle and 
by name with vkAllocateMemory(). Hence my comment regarding the by-name 
tests. The calls to vkAllocateMemory look identical to me; am I missing 
something?

>>
>>> +static void test_unique_device(uint8_t driver_uuid[VK_UUID_SIZE],
>>> uint8_t device_uuid[VK_UUID_SIZE],
>>> +        uint32_t extension_count, const char * const
>>> *enabled_extensions,
>>> +        void (*test_func_instance)(VkInstance, VkPhysicalDevice),
>>> void (*test_func)(VkPhysicalDevice))
>>
>> Could we instead use for_each_device_instance(), and match the UUID in
>> the callback?
> 
> I added this in v11, with the one drawback being that we no longer
> ensure that there exists a physical device in the child process that
> matches.

Strikes me as really weird if there isn't; I think it's fair to assume 
that all processes start from the same state ;-)

>>
>>> +            if (test_func_instance)
>>> +                test_func_instance(vk_instance,
>>> vk_physical_devices[i]);
>>> +            else
>>> +                test_func(vk_physical_devices[i]);
>>
>> This isn't new, but it strikes me as really awkward. Can't we just
>> pass a VkInstance to the callback and leave it unused?
> 
> Agreed, but yeah normalizing this will have to come into another patch,
> as I removed test_unique_device in v11.
>>>    # Either internal extensions which aren't present on the win32
>>> platform which
>>>    # winevulkan may nonetheless use, or extensions we want to generate
>>> headers for
>>>    # but not expose to applications (useful for test commits)
>>> -UNEXPOSED_EXTENSIONS = {}
>>> +UNEXPOSED_EXTENSIONS = {
>>> +    "VK_KHR_external_memory_win32",
>>> +}
>>>      # The Vulkan loader provides entry-points for core functionality
>>> and important
>>>    # extensions. Based on vulkan-1.def this amounts to WSI extensions
>>> on 1.0.51.
>>>
>>
>> This doesn't look like it belongs in this patch.
> 
> 
> Ended up keeping this in one patch for v11.
> 
>