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

[Mark Thompson]

On 03/11/2020 23:17, Bas Nieuwenhuizen 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.
> ---
>   libavdevice/kmsgrab.c | 19 +++++++++++++++----
>   1 file changed, 15 insertions(+), 4 deletions(-)

Huh - I didn't notice that GETFB2 was allowed to not return modifiers.

What does this case actually mean for the returned framebuffers?

For example, if they actually have modifiers applied but the kernel won't tell us then can we guarantee that any following step will also be ok with not having the modifier?  (I'm wondering whether it would be of any value to reuse the GETFB user-supplied-modifer in that case.)

> diff --git a/libavdevice/kmsgrab.c b/libavdevice/kmsgrab.c
> index a0aa9dc22f..2a03ba4122 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 = DRM_FORMAT_MOD_INVALID;
>   
>       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,
> @@ -519,6 +523,8 @@ static av_cold int kmsgrab_read_header(AVFormatContext *avctx)
>           err = AVERROR(err);
>           goto fail;
>       } else {
> +        uint64_t modifier = DRM_FORMAT_MOD_INVALID;
> +
>           av_log(avctx, AV_LOG_INFO, "Template framebuffer is "
>                  "%"PRIu32": %"PRIu32"x%"PRIu32" "
>                  "format %"PRIx32" modifier %"PRIx64" flags %"PRIx32".\n",
> @@ -557,15 +563,19 @@ static av_cold int kmsgrab_read_header(AVFormatContext *avctx)
>               err = AVERROR(EINVAL);
>               goto fail;
>           }
> +
> +        if (fb2->flags & DRM_MODE_FB_MODIFIERS)
> +            modifier = fb2->modifier;
> +
>           if (ctx->drm_format_modifier != DRM_FORMAT_MOD_INVALID &&
> -            ctx->drm_format_modifier != fb2->modifier) {
> +            ctx->drm_format_modifier != modifier) {
>               av_log(avctx, AV_LOG_ERROR, "Framebuffer format modifier "
>                      "%"PRIx64" does not match expected modifier.\n",
> -                   fb2->modifier);
> +                   modifier);
>               err = AVERROR(EINVAL);
>               goto fail;
>           } else {
> -            ctx->drm_format_modifier = fb2->modifier;
> +            ctx->drm_format_modifier = modifier;
>           }
>           av_log(avctx, AV_LOG_VERBOSE, "Format is %s, from "
>                  "DRM format %"PRIx32" modifier %"PRIx64".\n",
> @@ -609,6 +619,7 @@ static av_cold int kmsgrab_read_header(AVFormatContext *avctx)
>   
>           ctx->width  = fb->width;
>           ctx->height = fb->height;
> +        ctx->drm_format_modifier = DRM_FORMAT_MOD_INVALID;
>   
>           if (!fb->handle) {
>               av_log(avctx, AV_LOG_ERROR, "No handle set on framebuffer: "
> 

Thanks,

- Mark