[FFmpeg-devel] [PATCH V2 3/5] libavutil/hwcontext_vulkan: Allocate vkFrame in one memory
Lynne
dev at lynne.ee
Thu Nov 25 06:14:21 EET 2021
25 Nov 2021, 03:58 by wenbin.chen at intel.com:
>> 24 Nov 2021, 06:28 by wenbin.chen at intel.com:
>>
>> > The vaapi can import external frame, but the planes of the external
>> > frames should be in the same drm object. A new option
>> "contiguous_planes"
>> > is added to device. This flag tells device to allocate places in one
>> > memory. When device is derived from vaapi this flag will be enabled.
>> > A new flag frame_flag is also added to AVVulkanFramesContext. User
>> > can use this flag to force enable or disable this behaviour.
>> > A new variable "offset "is added to AVVKFrame. It describe describe the
>> > offset from the memory currently bound to the VkImage.
>> >
>> > Signed-off-by: Wenbin Chen <wenbin.chen at intel.com>
>> > ---
>> > libavutil/hwcontext_vulkan.c | 62
>> ++++++++++++++++++++++++++++++++++--
>> > libavutil/hwcontext_vulkan.h | 22 +++++++++++++
>> > 2 files changed, 82 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
>> > index f1e750cd3e..4100e8b0a2 100644
>> > --- a/libavutil/hwcontext_vulkan.c
>> > +++ b/libavutil/hwcontext_vulkan.c
>> > @@ -103,6 +103,9 @@ typedef struct VulkanDevicePriv {
>> > /* Settings */
>> > int use_linear_images;
>> >
>> > + /* allocate planes in a contiguous memory */
>> > + int contiguous_planes;
>> > +
>> > /* Nvidia */
>> > int dev_is_nvidia;
>> >
>>
>> Add a new `int dev_is_intel;` field, and set it
>> in `vulkan_device_init()`, where `dev_is_nvidia` is set.
>>
>>
>> > } VulkanDevicePriv;
>> > @@ -1266,6 +1269,11 @@ static int
>> vulkan_device_create_internal(AVHWDeviceContext *ctx,
>> > if (opt_d)
>> > p->use_linear_images = strtol(opt_d->value, NULL, 10);
>> >
>> > + opt_d = av_dict_get(opts, "contiguous_planes", NULL, 0);
>> > + if (opt_d)
>> > + p->contiguous_planes = strtol(opt_d->value, NULL, 10);
>> >
>>
>> Set `p->contiguous_planes` to -1 if not specified.
>>
>>
>> > hwctx->enabled_dev_extensions = dev_info.ppEnabledExtensionNames;
>> > hwctx->nb_enabled_dev_extensions = dev_info.enabledExtensionCount;
>> >
>> > @@ -1410,8 +1418,10 @@ static int
>> vulkan_device_derive(AVHWDeviceContext *ctx,
>> > return AVERROR_EXTERNAL;
>> > }
>> >
>> > - if (strstr(vendor, "Intel"))
>> > + if (strstr(vendor, "Intel")) {
>> > + av_dict_set_int(&opts, "contiguous_planes", 1, 0);
>> >
>>
>> Don't set this here, it's not needed with the changes I mentioned.
>>
>>
>> > dev_select.vendor_id = 0x8086;
>> > + }
>> > if (strstr(vendor, "AMD"))
>> > dev_select.vendor_id = 0x1002;
>> >
>> > @@ -1634,8 +1644,12 @@ static int
>> alloc_bind_mem(AVHWFramesContext *hwfc, AVVkFrame *f,
>> > AVHWDeviceContext *ctx = hwfc->device_ctx;
>> > VulkanDevicePriv *p = ctx->internal->priv;
>> > FFVulkanFunctions *vk = &p->vkfn;
>> > + AVVulkanFramesContext *hwfctx = hwfc->hwctx;
>> > const int planes = av_pix_fmt_count_planes(hwfc->sw_format);
>> > VkBindImageMemoryInfo bind_info[AV_NUM_DATA_POINTERS] = { { 0 } };
>> > + VkMemoryRequirements memory_requirements = { 0 };
>> > + int mem_size = 0;
>> > + int mem_size_list[AV_NUM_DATA_POINTERS] = { 0 };
>> >
>> > AVVulkanDeviceContext *hwctx = ctx->hwctx;
>> >
>> > @@ -1663,6 +1677,19 @@ static int
>> alloc_bind_mem(AVHWFramesContext *hwfc, AVVkFrame *f,
>> > req.memoryRequirements.size = FFALIGN(req.memoryRequirements.size,
>> > p->props.properties.limits.minMemoryMapAlignment);
>> >
>> > + if (hwfctx->contiguous_planes ==
>> AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY) {
>> >
>>
>> Bitwise &. Not equals.
>>
>>
>> > + if (memory_requirements.size == 0) {
>> > + memory_requirements = req.memoryRequirements;
>> > + } else if (memory_requirements.memoryTypeBits !=
>> req.memoryRequirements.memoryTypeBits) {
>> > + av_log(hwfc, AV_LOG_ERROR, "the param for each planes are
>> not the same\n");
>> > + return AVERROR(EINVAL);
>> > + }
>> > +
>> > + mem_size_list[i] = req.memoryRequirements.size;
>> > + mem_size += mem_size_list[i];
>> > + continue;
>> > + }
>> > +
>> > /* In case the implementation prefers/requires dedicated allocation */
>> > use_ded_mem = ded_req.prefersDedicatedAllocation |
>> > ded_req.requiresDedicatedAllocation;
>> > @@ -1684,6 +1711,29 @@ static int
>> alloc_bind_mem(AVHWFramesContext *hwfc, AVVkFrame *f,
>> > bind_info[i].memory = f->mem[i];
>> > }
>> >
>> > + if (hwfctx->contiguous_planes ==
>> AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY) {
>> >
>>
>> Same.
>>
>>
>> > + memory_requirements.size = mem_size;
>> > +
>> > + /* Allocate memory */
>> > + if ((err = alloc_mem(ctx, &memory_requirements,
>> > + f->tiling == VK_IMAGE_TILING_LINEAR ?
>> > + VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT :
>> > + VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT,
>> > + (void *)(((uint8_t *)alloc_pnext)),
>> > + &f->flags, &f->mem[0])))
>> > + return err;
>> > +
>> > + f->size[0] = memory_requirements.size;
>> > +
>> > + for (int i = 0; i < planes; i++) {
>> > + bind_info[i].sType =
>> VK_STRUCTURE_TYPE_BIND_IMAGE_MEMORY_INFO;
>> > + bind_info[i].image = f->img[i];
>> > + bind_info[i].memory = f->mem[0];
>> > + bind_info[i].memoryOffset = i == 0 ? 0 : mem_size_list[i-1];
>> > + f->offset[i] = bind_info[i].memoryOffset;
>> > + }
>> > + }
>> > +
>> > /* Bind the allocated memory to the images */
>> > ret = vk->BindImageMemory2(hwctx->act_dev, planes, bind_info);
>> > if (ret != VK_SUCCESS) {
>> > @@ -2046,6 +2096,12 @@ static int
>> vulkan_frames_init(AVHWFramesContext *hwfc)
>> > if (!hwctx->usage)
>> > hwctx->usage = FF_VK_DEFAULT_USAGE_FLAGS;
>> >
>> > + if (!(hwctx->contiguous_planes & 1ULL)) {
>> > + hwctx->contiguous_planes = p->contiguous_planes ?
>> > + AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY :
>> > + AV_VK_FRAME_FLAG_NONE;
>> > + }
>> >
>>
>> The code should look something like this:
>> ```
>> if (!(hwfc->flags & AV_VK_FRAME_FLAG_NONE) {
>> if (p->contiguous_planes == 1 ||
>> ((p->contiguous_planes == -1) && p->dev_is_intel))
>> hwfc->flags |= AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY;
>> }
>> ```
>>
>> This way, if the `contiguous_planes` option in `VulkanDevicePriv`
>> is not set (-1), then if the vendor is intel, the flag is added, or
>> if the flag in `VulkanDevicePriv` is explicitly set, the flag is added
>> as well, unless `p->contiguous_planes` is set to 0.
>>
>>
>> > err = create_exec_ctx(hwfc, &fp->conv_ctx,
>> > dev_hwctx->queue_family_comp_index,
>> > dev_hwctx->nb_comp_queues);
>> > @@ -2966,6 +3022,7 @@ static int
>> vulkan_map_to_drm(AVHWFramesContext *hwfc, AVFrame *dst,
>> > FFVulkanFunctions *vk = &p->vkfn;
>> > VulkanFramesPriv *fp = hwfc->internal->priv;
>> > AVVulkanDeviceContext *hwctx = hwfc->device_ctx->hwctx;
>> > + AVVulkanFramesContext *hwfctx = hwfc->hwctx;
>> > const int planes = av_pix_fmt_count_planes(hwfc->sw_format);
>> > VkImageDrmFormatModifierPropertiesEXT drm_mod = {
>> > .sType =
>> VK_STRUCTURE_TYPE_IMAGE_DRM_FORMAT_MODIFIER_PROPERTIES_EXT,
>> > @@ -3034,7 +3091,8 @@ static int
>> vulkan_map_to_drm(AVHWFramesContext *hwfc, AVFrame *dst,
>> > continue;
>> >
>> > vk->GetImageSubresourceLayout(hwctx->act_dev, f->img[i], &sub,
>> &layout);
>> > - drm_desc->layers[i].planes[0].offset = layout.offset;
>> > + drm_desc->layers[i].planes[0].offset = hwfctx->contiguous_planes
>> == AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY ?
>> > + f->offset[i] : layout.offset;
>> > drm_desc->layers[i].planes[0].pitch = layout.rowPitch;
>> > }
>> >
>> > diff --git a/libavutil/hwcontext_vulkan.h b/libavutil/hwcontext_vulkan.h
>> > index fdf2a60156..62ea56ecdd 100644
>> > --- a/libavutil/hwcontext_vulkan.h
>> > +++ b/libavutil/hwcontext_vulkan.h
>> > @@ -35,6 +35,14 @@
>> > * with the data pointer set to an AVVkFrame.
>> > */
>> >
>> > +/**
>> > + * Behaviour of frame allocation
>> > + */
>> > +typedef enum {
>> > + AV_VK_FRAME_FLAG_NONE = (1ULL << 0),
>> > + AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY = (1ULL << 1) | 1ULL
>> > +} AVVkFrameFlags;
>> > +
>> > /**
>> > * Main Vulkan context, allocated as AVHWDeviceContext.hwctx.
>> > * All of these can be set before init to change what the context uses
>> > @@ -157,6 +165,15 @@ typedef struct AVVulkanFramesContext {
>> > */
>> > void *create_pnext;
>> >
>> > + /**
>> > + * Defines the behaviour of frame allocation
>> > + * Default is 0, this flag will be autamatically set.
>> > + * AV_VK_FRAME_FLAG_NONE, planes will be allocated in separte
>> memory
>> > + * AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY, planes will be
>> allocated in a
>> > + * contiguous memory.
>> > + */
>> > + AVVkFrameFlags contiguous_planes;
>> >
>>
>> Rename this to `flags`, and move the description of each
>> option to the enum.
>> Mention here that if no flag is set, then the flags are automatically
>> determined based on the device.
>>
>>
>> > /**
>> > * Extension data for memory allocation. Must have as many entries as
>> > * the number of planes of the sw_format.
>> > @@ -198,6 +215,11 @@ typedef struct AVVkFrame {
>> > VkDeviceMemory mem[AV_NUM_DATA_POINTERS];
>> > size_t size[AV_NUM_DATA_POINTERS];
>> >
>> > + /**
>> > + * Describe the offset from the memory currently bound to the VkImage.
>> > + */
>> > + size_t offset[AV_NUM_DATA_POINTERS];
>> > +
>> > /**
>> > * OR'd flags for all memory allocated
>> > */
>> >
>>
>> With those changes, it should look good, just resubmit and if it does
>> look good and works fine, I'll apply it.
>>
>
> Thanks for your detailed review.
> I have one question here. If I use bitwise to check flag,
> "flags & AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY" will be both true when
> set flag to AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY (0b11) or
> to AV_VK_FRAME_FLAG_NONE (0b01), so I need to write code like this:
> ```
> If ((flags & AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY) == AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY)
> ```
> Or like this
> ```
> If (flags & AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY & (~AV_VK_FRAME_FLAG_NONE))
> ```
> Is this acceptable?
>
Just define
```
#define VKF_FLAG(x, f) (((x) & (~AV_VK_FRAME_FLAG_NONE)) & (f))
```
somewhere at the start of hwcontext_vulkan.c and use it
`if (VKF_FLAG(hwfc->flags, AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY)) {`
More information about the ffmpeg-devel
mailing list