diff mbox series

[FFmpeg-devel,V2,3/5] libavutil/hwcontext_vulkan: Allocate vkFrame in one memory

Message ID 20211124052848.1122682-3-wenbin.chen@intel.com
State New
Headers show
Series [FFmpeg-devel,V2,1/5] hwcontext_vaapi: Use PRIME_2 memory type for modifiers.
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Chen, Wenbin Nov. 24, 2021, 5:28 a.m. UTC
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@intel.com>
---
 libavutil/hwcontext_vulkan.c | 62 ++++++++++++++++++++++++++++++++++--
 libavutil/hwcontext_vulkan.h | 22 +++++++++++++
 2 files changed, 82 insertions(+), 2 deletions(-)

Comments

Lynne Nov. 24, 2021, 11 a.m. UTC | #1
24 Nov 2021, 06:28 by wenbin.chen@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@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.
Chen, Wenbin Nov. 25, 2021, 2:58 a.m. UTC | #2
> 24 Nov 2021, 06:28 by wenbin.chen@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@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?

Thanks
Wenbin 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Lynne Nov. 25, 2021, 4:14 a.m. UTC | #3
25 Nov 2021, 03:58 by wenbin.chen@intel.com:

>> 24 Nov 2021, 06:28 by wenbin.chen@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@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)) {`
diff mbox series

Patch

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;
 } 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);
+
+
     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);
             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) {
+            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) {
+        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;
+    }
+
     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;
+
     /**
      * 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
      */