[FFmpeg-devel] [PATCH 4/7] libavutil/hwcontext_vulkan: Allocate vkFrame in one memory

[Lynne]

15 Nov 2021, 08:25 by wenbin.chen at intel.com:

>> 9 Nov 2021, 10:18 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. I add a new function to
>> > allocate vkFrame in one memory and vulkan device will choose a way
>> > to allocate memory according to one_memory flag.
>> > A new variable is added to AVVKFrame to store the offset of each plane.
>> >
>> > Signed-off-by: Wenbin Chen <wenbin.chen at intel.com>
>> > ---
>> >  libavutil/hwcontext_vulkan.c | 46
>> +++++++++++++++++++++++++++++++++++-
>> >  libavutil/hwcontext_vulkan.h |  1 +
>> >  2 files changed, 46 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
>> > index ccf3e58f49..f7878ed9c3 100644
>> > --- a/libavutil/hwcontext_vulkan.c
>> > +++ b/libavutil/hwcontext_vulkan.c
>> > @@ -1600,6 +1600,9 @@ static int alloc_bind_mem(AVHWFramesContext
>> *hwfc, AVVkFrame *f,
>> >  FFVulkanFunctions *vk = &p->vkfn;
>> >  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;
>> >
>> > @@ -1627,6 +1630,23 @@ static int
>> alloc_bind_mem(AVHWFramesContext *hwfc, AVVkFrame *f,
>> >  req.memoryRequirements.size = FFALIGN(req.memoryRequirements.size,
>> >  p->props.properties.limits.minMemoryMapAlignment);
>> >
>> > +        if (p->use_one_memory) {
>> > +            if (ded_req.prefersDedicatedAllocation |
>> ded_req.requiresDedicatedAllocation) {
>> > +                av_log(hwfc, AV_LOG_ERROR, "Cannot use dedicated allocation
>> for intel vaapi\n");
>> > +                return AVERROR(EINVAL);
>> > +            }
>> >
>>
>> We don't set the flag unless the driver tells us to, so if the
>> driver asks us to use dedicated memory when it can't handle such
>> images, shouldn't the driver just not set this flag?
>>
>
> I check the dedicatedAllocation flag because I don't know if vaapi driver 
> support importing dedicated memory.
> Actually I am not sure if I need to check this flag for vaapi. I can remove it.
>
>>
>>
>> > +            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;
>> > @@ -1648,6 +1668,29 @@ static int
>> alloc_bind_mem(AVHWFramesContext *hwfc, AVVkFrame *f,
>> >  bind_info[i].memory = f->mem[i];
>> >  }
>> >
>> > +    if (p->use_one_memory) {
>> > +        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) {
>> > @@ -2924,7 +2967,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       = p->use_one_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 9264f70dbf..efb602ef27 100644
>> > --- a/libavutil/hwcontext_vulkan.h
>> > +++ b/libavutil/hwcontext_vulkan.h
>> > @@ -189,6 +189,7 @@ typedef struct AVVkFrame {
>> >  */
>> >  VkDeviceMemory mem[AV_NUM_DATA_POINTERS];
>> >  size_t size[AV_NUM_DATA_POINTERS];
>> > +    size_t offset[AV_NUM_DATA_POINTERS];
>> >
>>
>> Is this necessary? Can't you use vkGetImageSubresourceLayout
>> to retrieve it?
>>
>
> I think it is needed. The offset we get from vkGetImgeSubresourceLayout
> is relative to the start of the image, but the offset drm_descriptor need 
> is relative to the start to the this whole memory object. I don't know which API can retrieve
> it, so I store it. 
>
> Thanks
>

I thought about it. IMO, you should do the following:
Introduce a new AVVulkanFramesContext entry called
"contiguous_planes", which would enable or disable the behaviour.
Additionally, keep the device option, just rename it to "contiguous_planes",
such that those without the right hardware can still test it. Also keep the
way it's set to 1 by default on intel hardware.
Add an offset field to AVVkFrame, and document that it describes
the offset from the memory currently bound to the VkImage.

As for the modifier field, I'm still unsure. What situation requires that
the modifier is known in advance? Can't the driver simply pick
the best modifier in the exact same way your code does it, by
checking the usage flags?
Images with the drm tiling are limited and have a lot of corner cases
described in the spec, so would be really nice if we could always output
images with the optimal tiling, whilst the driver takes care of the modifier
opaquely from API users.