On Wed, Nov 4, 2020 at 1:21 AM Bas Nieuwenhuizen
<[email protected]> wrote:
>
> On Wed, Nov 4, 2020 at 12:52 AM Mark Thompson <[email protected]> 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.
I realized that with Vulkan we don't have an import path for implicit
modifiers. I'll update the getfb2 path to only override the
user-supplied modifier if the getfb2 returns a modifier.
>
> 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
_______________________________________________
ffmpeg-devel mailing list
[email protected]
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
[email protected] with subject "unsubscribe".
