diff mbox series

[FFmpeg-devel,1/2] hwcontext_vulkan: add a new mechanism to expose used queue families

Message ID 20240709010719.914497-1-dev@lynne.ee
State New
Headers show
Series [FFmpeg-devel,1/2] hwcontext_vulkan: add a new mechanism to expose used queue families | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 fail Make fate failed
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Lynne July 9, 2024, 1:07 a.m. UTC
The issue with the old mechanism is that we had to introduce new
API each time we needed a new queue family, and all the queue families
were functionally fixed to a given purpose.

Nvidia's GPUs are able to handle video encoding and compute on the
same queue, which results in a speedup when pre-processing is required.

Also, this enables us to expose optical flow queues for frame interpolation.
---
APIChanges and lavu version will be bumped when comitting.

 libavutil/hwcontext_vulkan.c | 22 ++++++++++++++++++++++
 libavutil/hwcontext_vulkan.h | 22 ++++++++++++++++++++++
 2 files changed, 44 insertions(+)

Comments

Anton Khirnov July 9, 2024, 6:57 a.m. UTC | #1
Quoting Lynne via ffmpeg-devel (2024-07-09 03:07:12)
> @@ -151,6 +162,17 @@ typedef struct AVVulkanDeviceContext {
>       * Similar to lock_queue(), unlocks a queue. Must only be called after locking.
>       */
>      void (*unlock_queue)(struct AVHWDeviceContext *ctx, uint32_t queue_family, uint32_t index);
> +
> +    /**
> +     * Queue families used. Must be preferentially ordered. List may contain
> +     * duplicates, as long as their capability flags do not match.
> +     *
> +     * For compatibility reasons, all the enabled queue families listed above
> +     * (queue_family_(tx/comp/encode/decode)_index) must also be included in
> +     * this list until they're removed after deprecation.
> +     */
> +    AVVulkanDeviceQueueFamily qf[16];

Why 16? And are we really really sure sizeof(AVVulkanDeviceQueueFamily)
should be a part of the ABI?
Lynne July 9, 2024, 11:56 p.m. UTC | #2
On 09/07/2024 08:57, Anton Khirnov wrote:
> Quoting Lynne via ffmpeg-devel (2024-07-09 03:07:12)
>> @@ -151,6 +162,17 @@ typedef struct AVVulkanDeviceContext {
>>        * Similar to lock_queue(), unlocks a queue. Must only be called after locking.
>>        */
>>       void (*unlock_queue)(struct AVHWDeviceContext *ctx, uint32_t queue_family, uint32_t index);
>> +
>> +    /**
>> +     * Queue families used. Must be preferentially ordered. List may contain
>> +     * duplicates, as long as their capability flags do not match.
>> +     *
>> +     * For compatibility reasons, all the enabled queue families listed above
>> +     * (queue_family_(tx/comp/encode/decode)_index) must also be included in
>> +     * this list until they're removed after deprecation.
>> +     */
>> +    AVVulkanDeviceQueueFamily qf[16];
> 
> Why 16? And are we really really sure sizeof(AVVulkanDeviceQueueFamily)
> should be a part of the ABI?

16 is just an arbitrary limit. I don't expect to need more than this 
ever, but if we do, its not something that we can't wait until a bump 
occurs.
I can increase it to 32 if you're concerned about it.

There are 6 total queue family types, and 6 more currently supported 
encode and decode operations for each queue -> 12.

I'd like to avoid making this not a part of the ABI, particularly as its 
a context that users should be able to easily set themselves.
Anton Khirnov July 10, 2024, 8:18 a.m. UTC | #3
Quoting Lynne via ffmpeg-devel (2024-07-10 01:56:57)
> On 09/07/2024 08:57, Anton Khirnov wrote:
> > Quoting Lynne via ffmpeg-devel (2024-07-09 03:07:12)
> >> @@ -151,6 +162,17 @@ typedef struct AVVulkanDeviceContext {
> >>        * Similar to lock_queue(), unlocks a queue. Must only be called after locking.
> >>        */
> >>       void (*unlock_queue)(struct AVHWDeviceContext *ctx, uint32_t queue_family, uint32_t index);
> >> +
> >> +    /**
> >> +     * Queue families used. Must be preferentially ordered. List may contain
> >> +     * duplicates, as long as their capability flags do not match.
> >> +     *
> >> +     * For compatibility reasons, all the enabled queue families listed above
> >> +     * (queue_family_(tx/comp/encode/decode)_index) must also be included in
> >> +     * this list until they're removed after deprecation.
> >> +     */
> >> +    AVVulkanDeviceQueueFamily qf[16];
> > 
> > Why 16? And are we really really sure sizeof(AVVulkanDeviceQueueFamily)
> > should be a part of the ABI?
> 
> 16 is just an arbitrary limit. I don't expect to need more than this 
> ever, but if we do, its not something that we can't wait until a bump 
> occurs.
> I can increase it to 32 if you're concerned about it.
> 
> There are 6 total queue family types, and 6 more currently supported 
> encode and decode operations for each queue -> 12.
> 
> I'd like to avoid making this not a part of the ABI, particularly as its 
> a context that users should be able to easily set themselves.

I'm more concerned about adding new fields to AVVulkanDeviceQueueFamily.
Can't you just make qf an array of pointers, with a new function that
adds a new queue family to it?
Lynne July 13, 2024, 6:15 a.m. UTC | #4
On 10/07/2024 10:18, Anton Khirnov wrote:
> Quoting Lynne via ffmpeg-devel (2024-07-10 01:56:57)
>> On 09/07/2024 08:57, Anton Khirnov wrote:
>>> Quoting Lynne via ffmpeg-devel (2024-07-09 03:07:12)
>>>> @@ -151,6 +162,17 @@ typedef struct AVVulkanDeviceContext {
>>>>         * Similar to lock_queue(), unlocks a queue. Must only be called after locking.
>>>>         */
>>>>        void (*unlock_queue)(struct AVHWDeviceContext *ctx, uint32_t queue_family, uint32_t index);
>>>> +
>>>> +    /**
>>>> +     * Queue families used. Must be preferentially ordered. List may contain
>>>> +     * duplicates, as long as their capability flags do not match.
>>>> +     *
>>>> +     * For compatibility reasons, all the enabled queue families listed above
>>>> +     * (queue_family_(tx/comp/encode/decode)_index) must also be included in
>>>> +     * this list until they're removed after deprecation.
>>>> +     */
>>>> +    AVVulkanDeviceQueueFamily qf[16];
>>>
>>> Why 16? And are we really really sure sizeof(AVVulkanDeviceQueueFamily)
>>> should be a part of the ABI?
>>
>> 16 is just an arbitrary limit. I don't expect to need more than this
>> ever, but if we do, its not something that we can't wait until a bump
>> occurs.
>> I can increase it to 32 if you're concerned about it.
>>
>> There are 6 total queue family types, and 6 more currently supported
>> encode and decode operations for each queue -> 12.
>>
>> I'd like to avoid making this not a part of the ABI, particularly as its
>> a context that users should be able to easily set themselves.
> 
> I'm more concerned about adding new fields to AVVulkanDeviceQueueFamily.
> Can't you just make qf an array of pointers, with a new function that
> adds a new queue family to it?

I don't foresee needing to add any more fields to the struct, so I'd 
rather not have the complexity.
diff mbox series

Patch

diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
index da377aa1a4..ff5d34d042 100644
--- a/libavutil/hwcontext_vulkan.c
+++ b/libavutil/hwcontext_vulkan.c
@@ -1550,6 +1550,28 @@  static int vulkan_device_init(AVHWDeviceContext *ctx)
 
 #undef CHECK_QUEUE
 
+    /* Update the new queue family fields. If non-zero already,
+     * it means API users have set it. */
+    if (!hwctx->nb_qf) {
+#define ADD_QUEUE(ctx_qf, qc, flag)                                    \
+    do {                                                               \
+        if (ctx_qf != -1) {                                            \
+            hwctx->qf[hwctx->nb_qf++] = (AVVulkanDeviceQueueFamily) {  \
+                .idx = ctx_qf,                                         \
+                .num = qc,                                             \
+                .flags = flag,                                         \
+            };                                                         \
+        }                                                              \
+    } while (0)
+
+        ADD_QUEUE(hwctx->queue_family_index, hwctx->nb_graphics_queues, VK_QUEUE_GRAPHICS_BIT);
+        ADD_QUEUE(hwctx->queue_family_comp_index, hwctx->nb_comp_queues, VK_QUEUE_COMPUTE_BIT);
+        ADD_QUEUE(hwctx->queue_family_tx_index, hwctx->nb_tx_queues, VK_QUEUE_TRANSFER_BIT);
+        ADD_QUEUE(hwctx->queue_family_decode_index, hwctx->nb_decode_queues, VK_QUEUE_VIDEO_DECODE_BIT_KHR);
+        ADD_QUEUE(hwctx->queue_family_encode_index, hwctx->nb_encode_queues, VK_QUEUE_VIDEO_ENCODE_BIT_KHR);
+#undef ADD_QUEUE
+    }
+
     if (!hwctx->lock_queue)
         hwctx->lock_queue = lock_queue;
     if (!hwctx->unlock_queue)
diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
index cbbd2390c1..28fc2c73ff 100644
--- a/libavutil/hwcontext_vulkan.h
+++ b/libavutil/hwcontext_vulkan.h
@@ -30,6 +30,17 @@ 
 
 typedef struct AVVkFrame AVVkFrame;
 
+typedef struct AVVulkanDeviceQueueFamily {
+    /* Queue family index */
+    int idx;
+    /* Number of queues in the queue family in use */
+    int num;
+    /* Queue family capabilities. Must be non-zero.
+     * Flags may be removed to indicate the queue family may not be used
+     * for a given purpose. */
+    VkQueueFlagBits flags;
+} AVVulkanDeviceQueueFamily;
+
 /**
  * @file
  * API-specific header for AV_HWDEVICE_TYPE_VULKAN.
@@ -151,6 +162,17 @@  typedef struct AVVulkanDeviceContext {
      * Similar to lock_queue(), unlocks a queue. Must only be called after locking.
      */
     void (*unlock_queue)(struct AVHWDeviceContext *ctx, uint32_t queue_family, uint32_t index);
+
+    /**
+     * Queue families used. Must be preferentially ordered. List may contain
+     * duplicates, as long as their capability flags do not match.
+     *
+     * For compatibility reasons, all the enabled queue families listed above
+     * (queue_family_(tx/comp/encode/decode)_index) must also be included in
+     * this list until they're removed after deprecation.
+     */
+    AVVulkanDeviceQueueFamily qf[16];
+    int nb_qf;
 } AVVulkanDeviceContext;
 
 /**