[FFmpeg-devel] [PATCH 2/2] hwcontext_vulkan: expose the enabled device features

[Mark Thompson]

On 19/05/2020 21:56, Lynne wrote:
> May 19, 2020, 20:57 by sw at jkqxz.net:
> 
>> On 13/05/2020 16:53, Lynne wrote:
>>
>>> With this, the puzze of making libplacebo, ffmpeg and any other Vulkan 
>>> API users interoperable is complete.
>>> Users of both libraries can initialize one another's contexts without having
>>> to create a new one.
>>>> From 28264793295b0d7861527f40fa7c7041a3b34907 Mon Sep 17 00:00:00 2001
>>> From: Lynne <dev at lynne.ee>
>>> Date: Wed, 13 May 2020 16:39:00 +0100
>>> Subject: [PATCH 2/2] hwcontext_vulkan: expose the enabled device features
>>>
>>> With this, the puzze of making libplacebo, ffmpeg and any other Vulkan
>>>
>>
>> The "puzze".
>>
> 
> Fixed.
> 
> 
> 
>>> API users interoperable is complete.
>>> Users of both libraries can initialize one another's contexts without having
>>> to create a new one.
>>> ---
>>>  libavutil/hwcontext_vulkan.c | 11 +++++++++++
>>>  libavutil/hwcontext_vulkan.h |  7 +++++++
>>>  2 files changed, 18 insertions(+)
>>>
>>> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
>>> index 82ceb7013a..d05dd2cf5d 100644
>>> --- a/libavutil/hwcontext_vulkan.c
>>> +++ b/libavutil/hwcontext_vulkan.c
>>> @@ -807,6 +807,7 @@ static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
>>>  AVDictionaryEntry *opt_d;
>>>  VulkanDevicePriv *p = ctx->internal->priv;
>>>  AVVulkanDeviceContext *hwctx = ctx->hwctx;
>>> +    VkPhysicalDeviceFeatures dev_features = { 0 };
>>>  VkDeviceQueueCreateInfo queue_create_info[3] = {
>>>  { .sType = VK_STRUCTURE_TYPE_DEVICE_QUEUE_CREATE_INFO, },
>>>  { .sType = VK_STRUCTURE_TYPE_DEVICE_QUEUE_CREATE_INFO, },
>>> @@ -815,6 +816,7 @@ static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
>>>  
>>>  VkDeviceCreateInfo dev_info = {
>>>  .sType                = VK_STRUCTURE_TYPE_DEVICE_CREATE_INFO,
>>> +        .pEnabledFeatures     = &hwctx->device_features,
>>>  .pQueueCreateInfos    = queue_create_info,
>>>  .queueCreateInfoCount = 0,
>>>  };
>>> @@ -839,6 +841,15 @@ static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
>>>  av_log(ctx, AV_LOG_VERBOSE, "    minMemoryMapAlignment:              %li\n",
>>>  p->props.limits.minMemoryMapAlignment);
>>>  
>>> +    vkGetPhysicalDeviceFeatures(hwctx->phys_dev, &dev_features);
>>> +#define COPY_FEATURE(DST, NAME) (DST).NAME = dev_features.NAME;
>>> +    COPY_FEATURE(hwctx->device_features, shaderImageGatherExtended)
>>> +    COPY_FEATURE(hwctx->device_features, shaderStorageImageExtendedFormats)
>>> +    COPY_FEATURE(hwctx->device_features, fragmentStoresAndAtomics)
>>> +    COPY_FEATURE(hwctx->device_features, vertexPipelineStoresAndAtomics)
>>> +    COPY_FEATURE(hwctx->device_features, shaderInt64)
>>> +#undef COPY_FEATURE
>>>
>>
>> Forgive me if I'm being stupid here, but why can't the user run exactly the same code to find the features themselves?  They do have hwctx->phys_dev.
>>
> 
> vkGetPhysicalDeviceFeatures gets the list of features the device supports.
> You have to manually put the ones you enable in the VkDeviceCreateInfo struct.
> Even after you do that, vkGetPhysicalDeviceFeatures still gives you the features the device
> supports, not what you've enabled. There's no way to get any enabled features or extensions
> for a device or an instance after its created, but only what's supported.

Ok.  (Sorry, I misread the order of these - I thought you were copying out after vkCreateDevice() rather than setting before.)

>>> +
>>>  /* Search queue family */
>>>  if ((err = search_queue_families(ctx, &dev_info)))
>>>  goto end;
>>> diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
>>> index 9fbe8b9dcb..b73097d514 100644
>>> --- a/libavutil/hwcontext_vulkan.h
>>> +++ b/libavutil/hwcontext_vulkan.h
>>> @@ -94,6 +94,13 @@ typedef struct AVVulkanDeviceContext {
>>>  */
>>>  const char * const *enabled_dev_extensions;
>>>  int nb_enabled_dev_extensions;
>>> +    /**
>>> +     * This structure lists all device features that are present and enabled
>>> +     * during device creation. By default, if present, shaderImageGatherExtended,
>>> +     * shaderStorageImageExtendedFormats, fragmentStoresAndAtomics, shaderInt64,
>>> +     * and vertexPipelineStoresAndAtomics are enabled.
>>>
>>
>> And what should an API user set it to?  Do they need to enable that same set of features?
>>
> 
> The API user can set it to whatever they have enabled in their VkDeviceCreateInfo struct.
> They don't need to have the same extensions enabled. We don't (and won't) depend on any
> extensions, but optionally use them if they're enabled.
> Not sure how to make this clearer.

Something like:

This structure should be set to the set of features that present and enabled during
device creation - that is, it should be a copy of the pEnabledFeatures field passed in
VkDeviceCreateInfo to vkCreateDevice().  When a device is created by FFmpeg, it will
default to enabling all that are present of the shaderImageGatherExtended,
fragmentStoresAndAtomics, shaderInt64 and vertexPipelineStoresAndAtomics features.

> @@ -807,6 +807,7 @@ static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
>      AVDictionaryEntry *opt_d;
>      VulkanDevicePriv *p = ctx->internal->priv;
>      AVVulkanDeviceContext *hwctx = ctx->hwctx;
> +    VkPhysicalDeviceFeatures dev_features = { 0 };
>      VkDeviceQueueCreateInfo queue_create_info[3] = {
>          { .sType = VK_STRUCTURE_TYPE_DEVICE_QUEUE_CREATE_INFO, },
>          { .sType = VK_STRUCTURE_TYPE_DEVICE_QUEUE_CREATE_INFO, },
> @@ -815,6 +816,7 @@ static int vulkan_device_create_internal(AVHWDeviceContext *ctx,
>  
>      VkDeviceCreateInfo dev_info = {
>          .sType                = VK_STRUCTURE_TYPE_DEVICE_CREATE_INFO,
> +        .pNext                = &hwctx->device_features,
>          .pQueueCreateInfos    = queue_create_info,
>          .queueCreateInfoCount = 0,
>      };

"Each pNext member of any structure (including this one) in the pNext chain must be either NULL or a pointer to a valid instance of ... [big list which does not include VkPhysicalDeviceFeatures]."  Was that meant to be VkPhysicalDeviceFeatures2?

- Mark