diff mbox series

[FFmpeg-devel,3/3] hwcontext_vaapi: Use PRIME_2 memory type for modifiers.

Message ID 20201103231734.154984-3-bas@basnieuwenhuizen.nl
State Superseded
Headers show
Series [FFmpeg-devel,1/3] kmsgrab: Use invalid modifier if modifiers weren't used. | expand

Checks

Context Check Description
andriy/PPC64_make success Make finished
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Bas Nieuwenhuizen Nov. 3, 2020, 11:17 p.m. UTC 
This way we can pass explicit modifiers in. Sometimes the
modifier matters for the number of memory planes that
libva accepts, in particular when dealing with
driver-compressed textures. Furthermore the driver might
not actually be able to determine the implicit modifier
if all the buffer-passing has used explicit modifier.
All these issues should be resolved by passing in the
modifier, and for that we switch to using the PRIME_2
memory type.

Tested with experimental radeonsi patches for modifiers
and kmsgrab. Also tested with radeonsi without the
patches to double-check it works without PRIME_2 support.
---
 libavutil/hwcontext_vaapi.c | 118 +++++++++++++++++++++++++++---------
 1 file changed, 90 insertions(+), 28 deletions(-)

Comments

Mark Thompson Nov. 6, 2020, 12:14 a.m. UTC | #1 
On 03/11/2020 23:17, Bas Nieuwenhuizen wrote:
> This way we can pass explicit modifiers in. Sometimes the
> modifier matters for the number of memory planes that
> libva accepts, in particular when dealing with
> driver-compressed textures. Furthermore the driver might
> not actually be able to determine the implicit modifier
> if all the buffer-passing has used explicit modifier.
> All these issues should be resolved by passing in the
> modifier, and for that we switch to using the PRIME_2
> memory type.
> 
> Tested with experimental radeonsi patches for modifiers
> and kmsgrab. Also tested with radeonsi without the
> patches to double-check it works without PRIME_2 support.
> ---
>   libavutil/hwcontext_vaapi.c | 118 +++++++++++++++++++++++++++---------
>   1 file changed, 90 insertions(+), 28 deletions(-)
> 
> diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
> index 2227d6ed69..3ef4bf5bed 100644
> --- a/libavutil/hwcontext_vaapi.c
> +++ b/libavutil/hwcontext_vaapi.c
> @@ -1030,22 +1030,21 @@ static int vaapi_map_from_drm(AVHWFramesContext *src_fc, AVFrame *dst,
>       VASurfaceID surface_id;
>       VAStatus vas;
>       uint32_t va_fourcc;
> -    int err, i, j, k;
> +    int err, i, j;
>   
> -    unsigned long buffer_handle;
> -    VASurfaceAttribExternalBuffers buffer_desc;
> -    VASurfaceAttrib attrs[2] = {
> +    VADRMPRIMESurfaceDescriptor prime_desc;
> +    VASurfaceAttrib prime_attrs[2] = {
>           {
>               .type  = VASurfaceAttribMemoryType,
>               .flags = VA_SURFACE_ATTRIB_SETTABLE,
>               .value.type    = VAGenericValueTypeInteger,
> -            .value.value.i = VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME,
> +            .value.value.i = VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME_2,
>           },
>           {
>               .type  = VASurfaceAttribExternalBufferDescriptor,
>               .flags = VA_SURFACE_ATTRIB_SETTABLE,
>               .value.type    = VAGenericValueTypePointer,
> -            .value.value.p = &buffer_desc,
> +            .value.value.p = &prime_desc,
>           }
>       };
>   
> @@ -1083,35 +1082,98 @@ static int vaapi_map_from_drm(AVHWFramesContext *src_fc, AVFrame *dst,
>       format_desc = vaapi_format_from_fourcc(va_fourcc);
>       av_assert0(format_desc);
>   
> -    buffer_handle = desc->objects[0].fd;
> -    buffer_desc.pixel_format = va_fourcc;
> -    buffer_desc.width        = src_fc->width;
> -    buffer_desc.height       = src_fc->height;
> -    buffer_desc.data_size    = desc->objects[0].size;
> -    buffer_desc.buffers      = &buffer_handle;
> -    buffer_desc.num_buffers  = 1;
> -    buffer_desc.flags        = 0;
> -
> -    k = 0;
> -    for (i = 0; i < desc->nb_layers; i++) {
> -        for (j = 0; j < desc->layers[i].nb_planes; j++) {
> -            buffer_desc.pitches[k] = desc->layers[i].planes[j].pitch;
> -            buffer_desc.offsets[k] = desc->layers[i].planes[j].offset;
> -            ++k;
> -        }
> +
> +    prime_desc.fourcc = va_fourcc;
> +    prime_desc.width = src_fc->width;
> +    prime_desc.height = src_fc->height;
> +    prime_desc.num_objects = desc->nb_objects;
> +    for (i = 0; i < desc->nb_objects; ++i) {
> +        prime_desc.objects[i].fd = desc->objects[i].fd;
> +        prime_desc.objects[i].size = desc->objects[i].size;
> +        prime_desc.objects[i].drm_format_modifier =
> +                desc->objects[i].format_modifier;
>       }
> -    buffer_desc.num_planes = k;
>   
> -    if (format_desc->chroma_planes_swapped &&
> -        buffer_desc.num_planes == 3) {
> -        FFSWAP(uint32_t, buffer_desc.pitches[1], buffer_desc.pitches[2]);
> -        FFSWAP(uint32_t, buffer_desc.offsets[1], buffer_desc.offsets[2]);
> +    prime_desc.num_layers = desc->nb_layers;
> +    for (i = 0; i < desc->nb_layers; ++i) {
> +        prime_desc.layers[i].drm_format = desc->layers[i].format;
> +        prime_desc.layers[i].num_planes = desc->layers[i].nb_planes;
> +        for (j = 0; j < desc->layers[i].nb_planes; ++j) {
> +            prime_desc.layers[i].object_index[j] =
> +                    desc->layers[i].planes[j].object_index;
> +            prime_desc.layers[i].offset[j] = desc->layers[i].planes[j].offset;
> +            prime_desc.layers[i].pitch[j] = desc->layers[i].planes[j].pitch;
> +        }
> +
> +        if (format_desc->chroma_planes_swapped &&
> +            desc->layers[i].nb_planes == 3) {
> +            FFSWAP(uint32_t, prime_desc.layers[i].pitch[1],
> +                   prime_desc.layers[i].pitch[2]);
> +            FFSWAP(uint32_t, prime_desc.layers[i].offset[1],
> +                   prime_desc.layers[i].offset[2]);
> +        }
>       }
>   
> +    /*
> +     * We can query for PRIME_2 support with vaQuerySurfaceAttributes, but that
> +     * needs the config_id which we don't have here ... Both Intel and Gallium
> +     * seem to do the correct error checks, so lets just try the PRIME_2 import
> +     * first.
> +     */
>       vas = vaCreateSurfaces(dst_dev->display, format_desc->rt_format,
>                              src->width, src->height,
>                              &surface_id, 1,
> -                           attrs, FF_ARRAY_ELEMS(attrs));
> +                           prime_attrs, FF_ARRAY_ELEMS(prime_attrs));

Agree that querying isn't sane, but I don't think whether this works is ever going to change on a later call on a given setup so can you record whether the PRIME_2 case worked or not (in src_fc->internal->priv, I think) to only do the test once?

Making it fail repeatedly seems a bit of a waste and annoys tracing.

> +
> +    if (vas != VA_STATUS_SUCCESS) {
> +        int k;
> +        unsigned long buffer_handle;
> +        VASurfaceAttribExternalBuffers buffer_desc;
> +        VASurfaceAttrib buffer_attrs[2] = {
> +            {
> +                .type  = VASurfaceAttribMemoryType,
> +                .flags = VA_SURFACE_ATTRIB_SETTABLE,
> +                .value.type    = VAGenericValueTypeInteger,
> +                .value.value.i = VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME,
> +            },
> +            {
> +                .type  = VASurfaceAttribExternalBufferDescriptor,
> +                .flags = VA_SURFACE_ATTRIB_SETTABLE,
> +                .value.type    = VAGenericValueTypePointer,
> +                .value.value.p = &buffer_desc,
> +            }
> +        };
> +
> +        buffer_handle = desc->objects[0].fd;
> +        buffer_desc.pixel_format = va_fourcc;
> +        buffer_desc.width        = src_fc->width;
> +        buffer_desc.height       = src_fc->height;
> +        buffer_desc.data_size    = desc->objects[0].size;
> +        buffer_desc.buffers      = &buffer_handle;
> +        buffer_desc.num_buffers  = 1;
> +        buffer_desc.flags        = 0;
> +
> +        k = 0;
> +        for (i = 0; i < desc->nb_layers; i++) {
> +            for (j = 0; j < desc->layers[i].nb_planes; j++) {
> +                buffer_desc.pitches[k] = desc->layers[i].planes[j].pitch;
> +                buffer_desc.offsets[k] = desc->layers[i].planes[j].offset;
> +                ++k;
> +            }
> +        }
> +        buffer_desc.num_planes = k;
> +
> +        if (format_desc->chroma_planes_swapped &&
> +            buffer_desc.num_planes == 3) {
> +            FFSWAP(uint32_t, buffer_desc.pitches[1], buffer_desc.pitches[2]);
> +            FFSWAP(uint32_t, buffer_desc.offsets[1], buffer_desc.offsets[2]);
> +        }
> +
> +        vas = vaCreateSurfaces(dst_dev->display, format_desc->rt_format,
> +                               src->width, src->height,
> +                               &surface_id, 1,
> +                               buffer_attrs, FF_ARRAY_ELEMS(buffer_attrs));
> +    }
>       if (vas != VA_STATUS_SUCCESS) {
>           av_log(dst_fc, AV_LOG_ERROR, "Failed to create surface from DRM "
>                  "object: %d (%s).\n", vas, vaErrorStr(vas));
> 

I did a bit of testing on icelake and it all seems happy (even though the driver doesn't advertise PRIME_2 support anywhere).

Thanks,

- Mark
diff mbox series

Patch

diff --git a/libavutil/hwcontext_vaapi.c b/libavutil/hwcontext_vaapi.c
index 2227d6ed69..3ef4bf5bed 100644
--- a/libavutil/hwcontext_vaapi.c
+++ b/libavutil/hwcontext_vaapi.c
@@ -1030,22 +1030,21 @@  static int vaapi_map_from_drm(AVHWFramesContext *src_fc, AVFrame *dst,
     VASurfaceID surface_id;
     VAStatus vas;
     uint32_t va_fourcc;
-    int err, i, j, k;
+    int err, i, j;
 
-    unsigned long buffer_handle;
-    VASurfaceAttribExternalBuffers buffer_desc;
-    VASurfaceAttrib attrs[2] = {
+    VADRMPRIMESurfaceDescriptor prime_desc;
+    VASurfaceAttrib prime_attrs[2] = {
         {
             .type  = VASurfaceAttribMemoryType,
             .flags = VA_SURFACE_ATTRIB_SETTABLE,
             .value.type    = VAGenericValueTypeInteger,
-            .value.value.i = VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME,
+            .value.value.i = VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME_2,
         },
         {
             .type  = VASurfaceAttribExternalBufferDescriptor,
             .flags = VA_SURFACE_ATTRIB_SETTABLE,
             .value.type    = VAGenericValueTypePointer,
-            .value.value.p = &buffer_desc,
+            .value.value.p = &prime_desc,
         }
     };
 
@@ -1083,35 +1082,98 @@  static int vaapi_map_from_drm(AVHWFramesContext *src_fc, AVFrame *dst,
     format_desc = vaapi_format_from_fourcc(va_fourcc);
     av_assert0(format_desc);
 
-    buffer_handle = desc->objects[0].fd;
-    buffer_desc.pixel_format = va_fourcc;
-    buffer_desc.width        = src_fc->width;
-    buffer_desc.height       = src_fc->height;
-    buffer_desc.data_size    = desc->objects[0].size;
-    buffer_desc.buffers      = &buffer_handle;
-    buffer_desc.num_buffers  = 1;
-    buffer_desc.flags        = 0;
-
-    k = 0;
-    for (i = 0; i < desc->nb_layers; i++) {
-        for (j = 0; j < desc->layers[i].nb_planes; j++) {
-            buffer_desc.pitches[k] = desc->layers[i].planes[j].pitch;
-            buffer_desc.offsets[k] = desc->layers[i].planes[j].offset;
-            ++k;
-        }
+
+    prime_desc.fourcc = va_fourcc;
+    prime_desc.width = src_fc->width;
+    prime_desc.height = src_fc->height;
+    prime_desc.num_objects = desc->nb_objects;
+    for (i = 0; i < desc->nb_objects; ++i) {
+        prime_desc.objects[i].fd = desc->objects[i].fd;
+        prime_desc.objects[i].size = desc->objects[i].size;
+        prime_desc.objects[i].drm_format_modifier =
+                desc->objects[i].format_modifier;
     }
-    buffer_desc.num_planes = k;
 
-    if (format_desc->chroma_planes_swapped &&
-        buffer_desc.num_planes == 3) {
-        FFSWAP(uint32_t, buffer_desc.pitches[1], buffer_desc.pitches[2]);
-        FFSWAP(uint32_t, buffer_desc.offsets[1], buffer_desc.offsets[2]);
+    prime_desc.num_layers = desc->nb_layers;
+    for (i = 0; i < desc->nb_layers; ++i) {
+        prime_desc.layers[i].drm_format = desc->layers[i].format;
+        prime_desc.layers[i].num_planes = desc->layers[i].nb_planes;
+        for (j = 0; j < desc->layers[i].nb_planes; ++j) {
+            prime_desc.layers[i].object_index[j] =
+                    desc->layers[i].planes[j].object_index;
+            prime_desc.layers[i].offset[j] = desc->layers[i].planes[j].offset;
+            prime_desc.layers[i].pitch[j] = desc->layers[i].planes[j].pitch;
+        }
+
+        if (format_desc->chroma_planes_swapped &&
+            desc->layers[i].nb_planes == 3) {
+            FFSWAP(uint32_t, prime_desc.layers[i].pitch[1],
+                   prime_desc.layers[i].pitch[2]);
+            FFSWAP(uint32_t, prime_desc.layers[i].offset[1],
+                   prime_desc.layers[i].offset[2]);
+        }
     }
 
+    /*
+     * We can query for PRIME_2 support with vaQuerySurfaceAttributes, but that
+     * needs the config_id which we don't have here ... Both Intel and Gallium
+     * seem to do the correct error checks, so lets just try the PRIME_2 import
+     * first.
+     */
     vas = vaCreateSurfaces(dst_dev->display, format_desc->rt_format,
                            src->width, src->height,
                            &surface_id, 1,
-                           attrs, FF_ARRAY_ELEMS(attrs));
+                           prime_attrs, FF_ARRAY_ELEMS(prime_attrs));
+
+    if (vas != VA_STATUS_SUCCESS) {
+        int k;
+        unsigned long buffer_handle;
+        VASurfaceAttribExternalBuffers buffer_desc;
+        VASurfaceAttrib buffer_attrs[2] = {
+            {
+                .type  = VASurfaceAttribMemoryType,
+                .flags = VA_SURFACE_ATTRIB_SETTABLE,
+                .value.type    = VAGenericValueTypeInteger,
+                .value.value.i = VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME,
+            },
+            {
+                .type  = VASurfaceAttribExternalBufferDescriptor,
+                .flags = VA_SURFACE_ATTRIB_SETTABLE,
+                .value.type    = VAGenericValueTypePointer,
+                .value.value.p = &buffer_desc,
+            }
+        };
+
+        buffer_handle = desc->objects[0].fd;
+        buffer_desc.pixel_format = va_fourcc;
+        buffer_desc.width        = src_fc->width;
+        buffer_desc.height       = src_fc->height;
+        buffer_desc.data_size    = desc->objects[0].size;
+        buffer_desc.buffers      = &buffer_handle;
+        buffer_desc.num_buffers  = 1;
+        buffer_desc.flags        = 0;
+
+        k = 0;
+        for (i = 0; i < desc->nb_layers; i++) {
+            for (j = 0; j < desc->layers[i].nb_planes; j++) {
+                buffer_desc.pitches[k] = desc->layers[i].planes[j].pitch;
+                buffer_desc.offsets[k] = desc->layers[i].planes[j].offset;
+                ++k;
+            }
+        }
+        buffer_desc.num_planes = k;
+
+        if (format_desc->chroma_planes_swapped &&
+            buffer_desc.num_planes == 3) {
+            FFSWAP(uint32_t, buffer_desc.pitches[1], buffer_desc.pitches[2]);
+            FFSWAP(uint32_t, buffer_desc.offsets[1], buffer_desc.offsets[2]);
+        }
+
+        vas = vaCreateSurfaces(dst_dev->display, format_desc->rt_format,
+                               src->width, src->height,
+                               &surface_id, 1,
+                               buffer_attrs, FF_ARRAY_ELEMS(buffer_attrs));
+    }
     if (vas != VA_STATUS_SUCCESS) {
         av_log(dst_fc, AV_LOG_ERROR, "Failed to create surface from DRM "
                "object: %d (%s).\n", vas, vaErrorStr(vas));