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

[Zebediah Figura (she/her)]

On 7/12/21 9:27 AM, Derek Lesho wrote:
> 
> On 7/9/21 8:50 PM, Zebediah Figura (she/her) wrote:
>> 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?
>>
>>
> Yeah, I think we are in agreement here on what the helper function would
> look like, I'm just not sure it the code savings are worth it (the
> current boilerplate is quite short and readable IMO).  As it stands, the
> redundant duplicated code is executing the name test, ok-ing the
> VkResults, freeing the allocated memory, and setting
> import_handle_info.sType.  The disadvantages would be having to pass in
> tangentially related parameters: dedicated_alloc_info and, if we wish to
> retain the vk_memory_import != vk_memory test, the initially exported
> memory.  I have to say that after trying it out, this was more code we
> could de-duplicate than I expected, but if you don't mind I'll leave the
> final judgement call up to you, here's a pastebin sample w/ both forms
> (current and helper/deduplicated):
> 
> https://paste.ubuntu.com/p/nGBZ83C3XV/
> 
> 

Personally, I think the helper is fine. I'm willing to leave the issue 
alone, though, if you strongly feel otherwise.

In answer to my other question, I see that by-name imports are only 
actually allowed with NT handles (1.2 specification § 11.2.4). That 
might be helpful to point out in the tests.