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

[Bas Nieuwenhuizen]

On Wed, Nov 4, 2020 at 12:52 AM Mark Thompson <sw at jkqxz.net> wrote:
>
> 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.)

Can't 100% guarantee that in general as the display and encoder device
might be from different vendors. However, if the flag is not set then
it was not set on modeset either. So the KMS driver has to depend on
implicit modifiers (i.e. a default for all shared images or some out
of band info). With suitably matched devices this should always work.

In particular the case where a user-specified modifier could
conceivably help we have a new encoder (supports modifiers) but old
KMS (doesn't support modifiers). That means the compositor+driver
userspace had to set the correct implicit modifier for KMS to work,
and the encoder should be able to pick that up as long as you don't
feed it a real modifier (so you really want DRM_FORMAT_MOD_INVALID in
this case). So it doesn't really help ... In fact it can confuse
things as with an user-specified (non-INVALID) modifier the encoder
doesn't know to look at the implicit modifier.

As an aside, your mention of the user-supplied-modifier made me notice
I probably broke that by always setting it to DRM_FORMAT_MOD_INVALID
in the GetFB path ... Will delete that in the V2.

>
> > 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