| Message ID | M7DjQAy--3-2@lynne.ee |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel,1/2] hwcontext_vulkan: expose the amount of queues for each queue family | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
Hi, I guess you already know this but just as a reminder: On Wed, 13 May 2020 17:53:33 +0200 (CEST), Lynne <dev@lynne.ee> wrote: > + 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 Enabling shaderStorageImageExtendedFormats makes no sense, because it's specified that enabling this feature does nothing. It only exists for informational purposes. I realize you copied this code from libplacebo, but libplacebo had the same issue and I removed it after verifying that it made no difference. It seems to have been a legacy leftover from a bug in the validation layers or something. Rest LGTM, although note that libplacebo has switched to VkPhysicalDeviceFeatures2KHR in the meantime, to avoid API breaks if I decide to use extension features in the future. It's not a huge deal since that struct is almost identical to VkPhysicalDeviceFeatures and therefore it's trivially interoperable with you code whether you decide to use it as well or not.
May 13, 2020, 17:45 by ffmpeg@haasn.xyz: > Hi, I guess you already know this but just as a reminder: > > On Wed, 13 May 2020 17:53:33 +0200 (CEST), Lynne <dev@lynne.ee> wrote: > >> + 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 >> > > Enabling shaderStorageImageExtendedFormats makes no sense, because it's > specified that enabling this feature does nothing. It only exists for > informational purposes. > > I realize you copied this code from libplacebo, but libplacebo had the > same issue and I removed it after verifying that it made no difference. > It seems to have been a legacy leftover from a bug in the validation > layers or something. > Yes, I forgot about that even though I told you about it :) Removed. > Rest LGTM, although note that libplacebo has switched to > VkPhysicalDeviceFeatures2KHR in the meantime, to avoid API breaks if I > decide to use extension features in the future. It's not a huge deal > since that struct is almost identical to VkPhysicalDeviceFeatures and > therefore it's trivially interoperable with you code whether you decide > to use it as well or not. > Switched over here as well. New patch attached. The API bump and APIchanges for the 2 commits will happen in a 3rd commit, which I'll make as I'm pushing these patches.
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@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". > 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. > + > /* 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? > + */ > + VkPhysicalDeviceFeatures device_features; > } AVVulkanDeviceContext; > > /** > -- > 2.26.2 - Mark
May 19, 2020, 20:57 by sw@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@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. >> + >> /* 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.
On 19/05/2020 21:56, Lynne wrote: > May 19, 2020, 20:57 by sw@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@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
May 20, 2020, 22:56 by sw@jkqxz.net: > On 19/05/2020 21:56, Lynne wrote: > > "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? > Yes, I fixed that after haasn pointed it out. Here's the new patch just in case with that fixed and the new description. Removed the "that is, it should be a copy of the pEnabledFeatures field passed in..." since when VkDeviceCreateInfo.pNext is VkPhysicalDeviceFeatures2 the pEnabledFeatures field must not be set. > * This structure should be set to the set of features that present and enabled > * during device creation. hen a device is created by FFmpeg, it will default to > * enabling all that are present of the shaderImageGatherExtended, > * fragmentStoresAndAtomics, shaderInt64 and vertexPipelineStoresAndAtomics features.
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 + /* 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. + */ + VkPhysicalDeviceFeatures device_features; } AVVulkanDeviceContext; /**