Re: [FFmpeg-devel] [PATCH] hwcontext_vulkan: fix exporting multi-plane DRM modifiers

[Lynne <dev@lynne.ee>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 6270 bytes --]

On 01/05/2025 07:05, Russell Greene wrote:
> From: Russell Greene <russell@shotover.com>
> 
> Previously, it was assumed that `drmFormatModifierPlaneCount` was one
> for all modifiers when exporting, which is not always the case, in
> particular for AMD GPUs and maybe others.
> 
> Fetch the number of memory planes and fill the structs appropriately in this situation.
> 
> The encoded stream is still bad in the case whre modifers are involved,
> but I think this patch still stands on its own and I suspect that may be a driver bug.
> 
> A potential improvement that could be make is to cache the format
> information, so we can avoid the two GetPhysicalDeviceFormatProperties2
> calls for each export, as well as the allocation. I doubt this is very
> expensive, but seemed worth noting.
> 
> Signed-off-by: Russell Greene <russellgreene8@gmail.com>
> ---
>   libavutil/hwcontext_vulkan.c | 76 +++++++++++++++++++++++++++++++-----
>   1 file changed, 67 insertions(+), 9 deletions(-)
> 
> diff --git a/libavutil/hwcontext_vulkan.c b/libavutil/hwcontext_vulkan.c
> index ade0235ef1..d14fa4655b 100644
> --- a/libavutil/hwcontext_vulkan.c
> +++ b/libavutil/hwcontext_vulkan.c
> @@ -3787,6 +3787,17 @@ static inline uint32_t vulkan_fmt_to_drm(VkFormat vkfmt)
>       return DRM_FORMAT_INVALID;
>   }
>   
> +#define MAX_MEMORY_PLANES 4
> +static VkImageAspectFlags plane_index_to_aspect(int plane) {
> +    if (plane == 0) return VK_IMAGE_ASPECT_MEMORY_PLANE_0_BIT_EXT;
> +    if (plane == 1) return VK_IMAGE_ASPECT_MEMORY_PLANE_1_BIT_EXT;
> +    if (plane == 2) return VK_IMAGE_ASPECT_MEMORY_PLANE_2_BIT_EXT;
> +    if (plane == 3) return VK_IMAGE_ASPECT_MEMORY_PLANE_3_BIT_EXT;
> +
> +    av_assert2 (false && "Invalid plane index");
> +    return VK_IMAGE_ASPECT_MEMORY_PLANE_0_BIT_EXT;
> +}
> +
>   static int vulkan_map_to_drm(AVHWFramesContext *hwfc, AVFrame *dst,
>                                const AVFrame *src, int flags)
>   {
> @@ -3855,14 +3866,65 @@ static int vulkan_map_to_drm(AVHWFramesContext *hwfc, AVFrame *dst,
>   
>       drm_desc->nb_layers = planes;
>       for (int i = 0; i < drm_desc->nb_layers; i++) {
> -        VkSubresourceLayout layout;
> -        VkImageSubresource sub = {
> -            .aspectMask = VK_IMAGE_ASPECT_MEMORY_PLANE_0_BIT_EXT,
> +        VkDrmFormatModifierPropertiesListEXT modp = {
> +            .sType = VK_STRUCTURE_TYPE_DRM_FORMAT_MODIFIER_PROPERTIES_LIST_EXT,
> +        };
> +        VkFormatProperties2 fmtp = {
> +            .sType = VK_STRUCTURE_TYPE_FORMAT_PROPERTIES_2,
> +            .pNext = &modp,
>           };
>           VkFormat plane_vkfmt = av_vkfmt_from_pixfmt(hwfc->sw_format)[i];
>   
> -        drm_desc->layers[i].format    = vulkan_fmt_to_drm(plane_vkfmt);
> -        drm_desc->layers[i].nb_planes = 1;
> +        drm_desc->layers[i].format = vulkan_fmt_to_drm(plane_vkfmt);
> +
> +        /* query drmFormatModifierCount by keeping pDrmFormatModifierProperties NULL */
> +        vk->GetPhysicalDeviceFormatProperties2(hwctx->phys_dev, plane_vkfmt, &fmtp);
> +
> +        modp.pDrmFormatModifierProperties =
> +            av_calloc(modp.drmFormatModifierCount, sizeof(*modp.pDrmFormatModifierProperties));
> +        if (!modp.pDrmFormatModifierProperties) {
> +            err = AVERROR(ENOMEM);
> +            goto end;
> +        }
> +        vk->GetPhysicalDeviceFormatProperties2(hwctx->phys_dev, plane_vkfmt, &fmtp);
> +
> +        VkDrmFormatModifierPropertiesEXT *mod_props = NULL;
> +        for (uint32_t i = 0; i < modp.drmFormatModifierCount; ++i) {
> +            VkDrmFormatModifierPropertiesEXT *m = &modp.pDrmFormatModifierProperties[i];
> +            if (m->drmFormatModifier == drm_mod.drmFormatModifier) {
> +                mod_props = m;
> +                break;
> +            }
> +        }
> +
> +        if (!mod_props) {
> +            av_free(modp.pDrmFormatModifierProperties);
> +            av_log(hwfc, AV_LOG_ERROR, "Cannot fetch modifier properties for modifier "PRIu64"!\n",
> +                   drm_mod.drmFormatModifier);
> +            err = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +        drm_desc->layers[i].nb_planes = mod_props->drmFormatModifierPlaneCount;
> +        av_free(modp.pDrmFormatModifierProperties);
> +
> +        if (drm_desc->layers[i].nb_planes > MAX_MEMORY_PLANES) {
> +            av_log(hwfc, AV_LOG_ERROR, "Too many memory planes for DRM format!\n");
> +            err = AVERROR_EXTERNAL;
> +            goto end;
> +        }
> +
> +        for (int j = 0; j < drm_desc->layers[i].nb_planes; j++) {
> +            VkSubresourceLayout layout;
> +            VkImageSubresource sub = {
> +                .aspectMask = plane_index_to_aspect(j),
> +            };
> +
> +            drm_desc->layers[i].planes[j].object_index = FFMIN(i, drm_desc->nb_objects - 1);
> +
> +            vk->GetImageSubresourceLayout(hwctx->act_dev, f->img[i], &sub, &layout);
> +            drm_desc->layers[i].planes[j].offset = layout.offset;
> +            drm_desc->layers[i].planes[j].pitch  = layout.rowPitch;
> +        }
>   
>           if (drm_desc->layers[i].format == DRM_FORMAT_INVALID) {
>               av_log(hwfc, AV_LOG_ERROR, "Cannot map to DRM layer, unsupported!\n");
> @@ -3870,14 +3932,10 @@ static int vulkan_map_to_drm(AVHWFramesContext *hwfc, AVFrame *dst,
>               goto end;
>           }
>   
> -        drm_desc->layers[i].planes[0].object_index = FFMIN(i, drm_desc->nb_objects - 1);
>   
>           if (f->tiling == VK_IMAGE_TILING_OPTIMAL)
>               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].pitch  = layout.rowPitch;
>       }
>   
>       dst->width   = src->width;

You don't need allocation for this, you can just use a fixed number 
large enough for a few coefficients. From memory, most software uses 16 
modifiers.

The dmabuf export code is still very much in a bad shape. I wrote code 
which made the decoder output dmabuf-backed VkImages, and ran into this 
issue.

[-- Attachment #1.1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 637 bytes --]

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[-- Attachment #2: Type: text/plain, Size: 251 bytes --]

_______________________________________________
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".