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

[Lynne]

19 Nov 2021, 19:13 by dev at lynne.ee:

> 19 Nov 2021, 18:59 by dev at lynne.ee:
>
>> 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.
>>
>
> Actually, add a new `typedef enum AVVkFrameFlags`, and a
> new entry `AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY = 1ULL << 1`
> instead of a boolean flag.
> AVVulkanFramesContext is always initialized as 0, and we really
> want to let users supply their own context settings, and with a single
> boolean flag, we wouldn't know if they set it or left it as default.
>

Hmm, that still wouldn't work, we want to still be able to let users
disable the option, even on intel hardware. So, do this instead:
`AV_VK_FRAME_FLAG_NONE = (1ULL << 0)`
`AV_VK_FRAME_FLAG_CONTIGUOUS_MEMORY = (1ULL << 1) | 1ULL`
This way, if a user sets any option, the bottom-most bit will be 1,
and autodetection of flags would be disabled during vulkan_frames_init().
If the bottom bit is not set, however, vulkan_frames_init() will auto-decide
on the flags, and set the field, as well as the bottom-most bit.