From 5aea1bc522f0874e6cc07f5120fbcf1736706536 Mon Sep 17 00:00:00 2001 From: Leandro Ribeiro Date: Wed, 21 Apr 2021 10:38:58 -0300 Subject: [PATCH] backend-drm: do not import dmabuf buffers with no modifiers to KMS We can't tell the layout of a buffer that has been allocated with no modifiers. Although usually drivers use linear layouts to allocate in these cases, it is not a rule. It can use a tiling layout, for instance, under the hood. So it is not safe to import a buffer with no modifiers to KMS, as it can't tell the buffer layout and this may result in garbage being displayed. In this patch we start to require explicit modifiers to import buffers to KMS. In most cases things just work as expected, but just because both sides (display and render driver) usually end up using the linear layout when modifiers are not exposed. It also works on systems where the display and render devices are tied (desktops with Intel or AMD, for instance), as there's only one driver and it knows the layout of the buffer (no need to guess). But in SoC's where the display and render device are split, things can go wrong. It is better to lose performance and not break things. To solve the problem, drivers should be updated to expose modifiers (even if only DRM_FORMAT_MOD_LINEAR), as the concept of implicit modifiers is the root of the problem. Signed-off-by: Leandro Ribeiro --- libweston/backend-drm/fb.c | 65 +++++++++++--------------------------- 1 file changed, 19 insertions(+), 46 deletions(-) diff --git a/libweston/backend-drm/fb.c b/libweston/backend-drm/fb.c index 25813c37..986a4aa8 100644 --- a/libweston/backend-drm/fb.c +++ b/libweston/backend-drm/fb.c @@ -222,15 +222,14 @@ static struct drm_fb * drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf, struct drm_backend *backend, bool is_opaque) { +#ifndef HAVE_GBM_FD_IMPORT + /* Importing a buffer to KMS requires explicit modifiers, so + * we can't continue with the legacy GBM_BO_IMPORT_FD instead + * of GBM_BO_IMPORT_FD_MODIFIER. */ + return NULL; +#else struct drm_fb *fb; - struct gbm_import_fd_data import_legacy = { - .width = dmabuf->attributes.width, - .height = dmabuf->attributes.height, - .format = dmabuf->attributes.format, - .stride = dmabuf->attributes.stride[0], - .fd = dmabuf->attributes.fd[0], - }; -#ifdef HAVE_GBM_FD_IMPORT + int i; struct gbm_import_fd_modifier_data import_mod = { .width = dmabuf->attributes.width, .height = dmabuf->attributes.height, @@ -238,9 +237,16 @@ drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf, .num_fds = dmabuf->attributes.n_planes, .modifier = dmabuf->attributes.modifier[0], }; -#endif /* HAVE_GBM_FD_IMPORT */ - int i; + /* We should not import to KMS a buffer that has been allocated using no + * modifiers. Usually drivers use linear layouts to allocate with no + * modifiers, but this is not a rule. The driver could use, for + * instance, a tiling layout under the hood - and both Weston and the + * KMS driver can't know. So giving the buffer to KMS is not safe, as + * not knowing its layout can result in garbage being displayed. In + * short, importing a buffer to KMS requires explicit modifiers. */ + if (dmabuf->attributes.modifier[0] == DRM_FORMAT_MOD_INVALID) + return NULL; /* XXX: TODO: * @@ -261,7 +267,6 @@ drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf, fb->refcnt = 1; fb->type = BUFFER_DMABUF; -#ifdef HAVE_GBM_FD_IMPORT static_assert(ARRAY_LENGTH(import_mod.fds) == ARRAY_LENGTH(dmabuf->attributes.fd), "GBM and linux_dmabuf FD size must match"); @@ -286,27 +291,9 @@ drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf, "GBM and linux_dmabuf offset size must match"); memcpy(import_mod.offsets, dmabuf->attributes.offset, sizeof(import_mod.offsets)); -#endif /* NOT HAVE_GBM_FD_IMPORT */ - - /* The legacy FD-import path does not allow us to supply modifiers, - * multiple planes, or buffer offsets. */ - if (dmabuf->attributes.modifier[0] != DRM_FORMAT_MOD_INVALID || - dmabuf->attributes.n_planes > 1 || - dmabuf->attributes.offset[0] > 0) { -#ifdef HAVE_GBM_FD_IMPORT - fb->bo = gbm_bo_import(backend->gbm, GBM_BO_IMPORT_FD_MODIFIER, - &import_mod, - GBM_BO_USE_SCANOUT); -#else /* NOT HAVE_GBM_FD_IMPORT */ - drm_debug(backend, "\t\t\t[dmabuf] Unsupported use of modifiers.\n"); - goto err_free; -#endif /* NOT HAVE_GBM_FD_IMPORT */ - } else { - fb->bo = gbm_bo_import(backend->gbm, GBM_BO_IMPORT_FD, - &import_legacy, - GBM_BO_USE_SCANOUT); - } + fb->bo = gbm_bo_import(backend->gbm, GBM_BO_IMPORT_FD_MODIFIER, + &import_mod, GBM_BO_USE_SCANOUT); if (!fb->bo) goto err_free; @@ -347,7 +334,6 @@ drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf, goto err_free; } -#ifdef HAVE_GBM_MODIFIERS fb->num_planes = dmabuf->attributes.n_planes; for (i = 0; i < dmabuf->attributes.n_planes; i++) { union gbm_bo_handle handle; @@ -357,20 +343,6 @@ drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf, goto err_free; fb->handles[i] = handle.u32; } -#else /* NOT HAVE_GBM_MODIFIERS */ - { - union gbm_bo_handle handle; - - fb->num_planes = 1; - - handle = gbm_bo_get_handle(fb->bo); - - if (handle.s32 == -1) - goto err_free; - fb->handles[0] = handle.u32; - } -#endif /* NOT HAVE_GBM_MODIFIERS */ - if (drm_fb_addfb(backend, fb) != 0) goto err_free; @@ -380,6 +352,7 @@ drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf, err_free: drm_fb_destroy_dmabuf(fb); return NULL; +#endif } struct drm_fb *