| Message ID | 20201103231734.154984-1-bas@basnieuwenhuizen.nl |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel,1/3] kmsgrab: Use invalid modifier if modifiers weren't used. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
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
On Wed, Nov 4, 2020 at 12:52 AM Mark Thompson <sw@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
On Wed, Nov 4, 2020 at 1:21 AM Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote: > > On Wed, Nov 4, 2020 at 12:52 AM Mark Thompson <sw@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. 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
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: "