diff mbox series

[FFmpeg-devel,v2,1/3] kmsgrab: Use invalid modifier if modifiers weren't used.

Message ID 20201113231548.869559-1-bas@basnieuwenhuizen.nl
State Accepted
Commit 03f4b203ba0ec58fc5c1ef8ee1fe740b8fcab9ab
Headers show
Series [FFmpeg-devel,v2,1/3] kmsgrab: Use invalid modifier if modifiers weren't used.
Related show

Checks

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

Commit Message

Bas Nieuwenhuizen Nov. 13, 2020, 11:15 p.m. UTC
The kernel defaults to initializing the field to 0 when modifiers
are not used and this happens to be linear. If we end up actually
passing the modifier to a driver, tiling issues happen.

So if the kernel doesn't return a modifier set it explicitly to
INVALID. That way later processing knows there is no explicit
modifier.

v2:
  Fix support for modifier overrides in case the getfb2 call does
  not return a modifier.
---
 libavdevice/kmsgrab.c | 27 +++++++++++++++++----------
 1 file changed, 17 insertions(+), 10 deletions(-)

Comments

Bas Nieuwenhuizen Jan. 13, 2021, 12:19 a.m. UTC | #1
A friendly ping on reviewing this series. Thanks!

On Sat, Nov 14, 2020 at 12:15 AM Bas Nieuwenhuizen
<bas@basnieuwenhuizen.nl> wrote:
>
> The kernel defaults to initializing the field to 0 when modifiers
> are not used and this happens to be linear. If we end up actually
> passing the modifier to a driver, tiling issues happen.
>
> So if the kernel doesn't return a modifier set it explicitly to
> INVALID. That way later processing knows there is no explicit
> modifier.
>
> v2:
>   Fix support for modifier overrides in case the getfb2 call does
>   not return a modifier.
> ---
>  libavdevice/kmsgrab.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/libavdevice/kmsgrab.c b/libavdevice/kmsgrab.c
> index a0aa9dc22f..b740a32171 100644
> --- a/libavdevice/kmsgrab.c
> +++ b/libavdevice/kmsgrab.c
> @@ -160,6 +160,7 @@ static int kmsgrab_get_fb2(AVFormatContext *avctx,
>      KMSGrabContext *ctx = avctx->priv_data;
>      drmModeFB2 *fb;
>      int err, i, nb_objects;
> +    uint64_t modifier = ctx->drm_format_modifier;
>
>      fb = drmModeGetFB2(ctx->hwctx->fd, plane->fb_id);
>      if (!fb) {
> @@ -195,6 +196,9 @@ static int kmsgrab_get_fb2(AVFormatContext *avctx,
>          goto fail;
>      }
>
> +    if (fb->flags & DRM_MODE_FB_MODIFIERS)
> +        modifier = fb->modifier;
> +
>      *desc = (AVDRMFrameDescriptor) {
>          .nb_layers = 1,
>          .layers[0] = {
> @@ -243,7 +247,7 @@ static int kmsgrab_get_fb2(AVFormatContext *avctx,
>              desc->objects[obj] = (AVDRMObjectDescriptor) {
>                  .fd              = fd,
>                  .size            = size,
> -                .format_modifier = fb->modifier,
> +                .format_modifier = modifier,
>              };
>              desc->layers[0].planes[i] = (AVDRMPlaneDescriptor) {
>                  .object_index = obj,
> @@ -557,15 +561,18 @@ static av_cold int kmsgrab_read_header(AVFormatContext *avctx)
>              err = AVERROR(EINVAL);
>              goto fail;
>          }
> -        if (ctx->drm_format_modifier != DRM_FORMAT_MOD_INVALID &&
> -            ctx->drm_format_modifier != fb2->modifier) {
> -            av_log(avctx, AV_LOG_ERROR, "Framebuffer format modifier "
> -                   "%"PRIx64" does not match expected modifier.\n",
> -                   fb2->modifier);
> -            err = AVERROR(EINVAL);
> -            goto fail;
> -        } else {
> -            ctx->drm_format_modifier = fb2->modifier;
> +
> +        if (fb2->flags & DRM_MODE_FB_MODIFIERS) {
> +            if (ctx->drm_format_modifier != DRM_FORMAT_MOD_INVALID &&
> +                ctx->drm_format_modifier != fb2->modifier) {
> +                av_log(avctx, AV_LOG_ERROR, "Framebuffer format modifier "
> +                       "%"PRIx64" does not match expected modifier.\n",
> +                       fb2->modifier);
> +                err = AVERROR(EINVAL);
> +                goto fail;
> +            } else {
> +                ctx->drm_format_modifier = fb2->modifier;
> +            }
>          }
>          av_log(avctx, AV_LOG_VERBOSE, "Format is %s, from "
>                 "DRM format %"PRIx32" modifier %"PRIx64".\n",
> --
> 2.29.2
>
Mark Thompson Jan. 13, 2021, 11:13 p.m. UTC | #2
On 13/01/2021 00:19, Bas Nieuwenhuizen wrote:
> A friendly ping on reviewing this series. Thanks!

Apologies; last time I looked at this I got distracted by Intel having broken import of X-tiled surfaces from kmsgrab, which was a bit of a problem to my testing.  (<https://github.com/intel/media-driver/issues/1096>, since fixed.)

Patches 1 and 2 look good, applied.

Thanks,

- Mark
diff mbox series

Patch

diff --git a/libavdevice/kmsgrab.c b/libavdevice/kmsgrab.c
index a0aa9dc22f..b740a32171 100644
--- a/libavdevice/kmsgrab.c
+++ b/libavdevice/kmsgrab.c
@@ -160,6 +160,7 @@  static int kmsgrab_get_fb2(AVFormatContext *avctx,
     KMSGrabContext *ctx = avctx->priv_data;
     drmModeFB2 *fb;
     int err, i, nb_objects;
+    uint64_t modifier = ctx->drm_format_modifier;
 
     fb = drmModeGetFB2(ctx->hwctx->fd, plane->fb_id);
     if (!fb) {
@@ -195,6 +196,9 @@  static int kmsgrab_get_fb2(AVFormatContext *avctx,
         goto fail;
     }
 
+    if (fb->flags & DRM_MODE_FB_MODIFIERS)
+        modifier = fb->modifier;
+
     *desc = (AVDRMFrameDescriptor) {
         .nb_layers = 1,
         .layers[0] = {
@@ -243,7 +247,7 @@  static int kmsgrab_get_fb2(AVFormatContext *avctx,
             desc->objects[obj] = (AVDRMObjectDescriptor) {
                 .fd              = fd,
                 .size            = size,
-                .format_modifier = fb->modifier,
+                .format_modifier = modifier,
             };
             desc->layers[0].planes[i] = (AVDRMPlaneDescriptor) {
                 .object_index = obj,
@@ -557,15 +561,18 @@  static av_cold int kmsgrab_read_header(AVFormatContext *avctx)
             err = AVERROR(EINVAL);
             goto fail;
         }
-        if (ctx->drm_format_modifier != DRM_FORMAT_MOD_INVALID &&
-            ctx->drm_format_modifier != fb2->modifier) {
-            av_log(avctx, AV_LOG_ERROR, "Framebuffer format modifier "
-                   "%"PRIx64" does not match expected modifier.\n",
-                   fb2->modifier);
-            err = AVERROR(EINVAL);
-            goto fail;
-        } else {
-            ctx->drm_format_modifier = fb2->modifier;
+
+        if (fb2->flags & DRM_MODE_FB_MODIFIERS) {
+            if (ctx->drm_format_modifier != DRM_FORMAT_MOD_INVALID &&
+                ctx->drm_format_modifier != fb2->modifier) {
+                av_log(avctx, AV_LOG_ERROR, "Framebuffer format modifier "
+                       "%"PRIx64" does not match expected modifier.\n",
+                       fb2->modifier);
+                err = AVERROR(EINVAL);
+                goto fail;
+            } else {
+                ctx->drm_format_modifier = fb2->modifier;
+            }
         }
         av_log(avctx, AV_LOG_VERBOSE, "Format is %s, from "
                "DRM format %"PRIx32" modifier %"PRIx64".\n",