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

[Derek Lesho]

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/