[PATCH v5 3/4] winevulkan: Implement VK_KHR_external_memory_win32 for buffers.

[Georg Lehmann]

On 20.05.21 22:16, Derek Lesho wrote:
> 
> On 5/20/21 3:41 PM, Georg Lehmann wrote:
>>
>>
>> On 20.05.21 20:31, Derek Lesho wrote:
>>>
>>>   +static inline void 
>>> wine_vk_normalize_handle_types_win(VkExternalMemoryHandleTypeFlags 
>>> *types)
>>> +{
>>> +    *types &=
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_BIT |
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_KMT_BIT |
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_BIT |
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D11_TEXTURE_KMT_BIT |
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D12_HEAP_BIT |
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_D3D12_RESOURCE_BIT |
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_HOST_ALLOCATION_BIT_EXT |
>>> + VK_EXTERNAL_MEMORY_HANDLE_TYPE_HOST_MAPPED_FOREIGN_MEMORY_BIT_EXT;
>>> +}
>>> +
>>> +static inline void 
>>> wine_vk_normalize_handle_types_host(VkExternalMemoryHandleTypeFlags 
>>> *types)
>>> +{
>>> +    *types &=
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD_BIT |
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_HOST_ALLOCATION_BIT_EXT |
>>> +/*      predicated on VK_KHR_external_memory_dma_buf
>>> +        VK_EXTERNAL_MEMORY_HANDLE_TYPE_DMA_BUF_BIT_EXT | */
>>
>> Why is this here?
> Because VK_KHR_external_memory_dma_buf is blacklisted, and the enum 
> value is not defined.  I put the comment here to note that, if we ever 
> need to back another handle type w/ the DMA BUF handle type, and thereby 
> un-blacklist the extension, we should add the bit.

Ah, okay, but I think/hope we will not need dmabufs since there's no 
equivalent on windows and they are linux specific.

>>
>>> + VK_EXTERNAL_MEMORY_HANDLE_TYPE_HOST_MAPPED_FOREIGN_MEMORY_BIT_EXT;
>>> +}
>>> +
>>> +static void 
>>> wine_vk_get_physical_device_external_buffer_properties(VkPhysicalDevice 
>>> phys_dev,
>>> +        void 
>>> (*p_vkGetPhysicalDeviceExternalBufferProperties)(VkPhysicalDevice, 
>>> const VkPhysicalDeviceExternalBufferInfo *, 
>>> VkExternalBufferProperties *),
>>> +        const VkPhysicalDeviceExternalBufferInfo *buffer_info, 
>>> VkExternalBufferProperties *properties)
>>> +{
>>> +    VkPhysicalDeviceExternalBufferInfo buffer_info_dup = *buffer_info;
>>> +
>>> +    if (buffer_info_dup.handleType == 
>>> VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_WIN32_BIT)
>>> +        buffer_info_dup.handleType = 
>>> VK_EXTERNAL_MEMORY_HANDLE_TYPE_OPAQUE_FD_BIT;
>>> + wine_vk_normalize_handle_types_host(&buffer_info_dup.handleType);
>>
>> This allows the application to query with FD_BIT. Before you modify 
>> any bits you should check if handleType &~ (all the types that we 
>> support) and set all the results to 0 if true.
> 
> True, it looks like the spec doesn't forbid applications from calling 
> this function with non-native bits.  I suppose I could run a 
> wine_vk_normalize_handle_types_win on the input parameter then error out 
> if that cases the resulting handle type to be 0.  I'm guessing you 
> didn't comment on the lack of such a validation in vkAllocateMemory's 
> handling of VkExportMemoryAllocateInfo::handleTypes because there it 
> would be undefined behavior.  (Since the invalid types wouldn't be 
> exposed here).

Yes, it's not UB in this function. (And in 
vkGetPhysicalDeviceImageFormatProperties2)