[PATCH v16 13/23] compositor-drm: Add modifiers to GBM dmabuf import
Pekka Paalanen
ppaalanen at gmail.com
Mon Jul 9 09:39:30 UTC 2018
On Thu, 5 Jul 2018 18:16:40 +0100
Daniel Stone <daniels at collabora.com> wrote:
> Add support for the GBM_BO_IMPORT_FD_MODIFIER path, which allows us to
> import multi-plane dmabufs, as well as format modifiers.
>
> Signed-off-by: Daniel Stone <daniels at collabora.com>
> Tested-by: Emre Ucan <eucan at de.adit-jv.com>
> ---
> configure.ac | 6 +-
> libweston/compositor-drm.c | 181 +++++++++++++++++++++++++++----------
> 2 files changed, 137 insertions(+), 50 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index adb998dda..ef55ace36 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -212,9 +212,9 @@ if test x$enable_drm_compositor = xyes; then
> PKG_CHECK_MODULES(DRM_COMPOSITOR_ATOMIC, [libdrm >= 2.4.78],
> [AC_DEFINE([HAVE_DRM_ATOMIC], 1, [libdrm supports atomic API])],
> [AC_MSG_WARN([libdrm does not support atomic modesetting, will omit that capability])])
> - PKG_CHECK_MODULES(DRM_COMPOSITOR_GBM, [gbm >= 10.2],
> - [AC_DEFINE([HAVE_GBM_FD_IMPORT], 1, [gbm supports dmabuf import])],
> - [AC_MSG_WARN([gbm does not support dmabuf import, will omit that capability])])
> + PKG_CHECK_MODULES(DRM_COMPOSITOR_GBM, [gbm >= 17.2],
> + [AC_DEFINE([HAVE_GBM_FD_IMPORT], 1, [gbm supports import with modifiers])],
> + [AC_MSG_WARN([GBM does not support dmabuf import, will omit that capability])])
> fi
>
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 0aaaf79e8..98c8ed584 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -284,6 +284,7 @@ struct drm_mode {
> enum drm_fb_type {
> BUFFER_INVALID = 0, /**< never used */
> BUFFER_CLIENT, /**< directly sourced from client */
> + BUFFER_DMABUF, /**< imported from linux_dmabuf client */
> BUFFER_PIXMAN_DUMB, /**< internal Pixman rendering */
> BUFFER_GBM_SURFACE, /**< internal EGL rendering */
> BUFFER_CURSOR, /**< internal cursor buffer */
> @@ -988,6 +989,120 @@ drm_fb_ref(struct drm_fb *fb)
> return fb;
> }
>
> +static void
> +drm_fb_destroy_dmabuf(struct drm_fb *fb)
> +{
> + /* We deliberately do not close the GEM handles here; GBM manages
> + * their lifetime through the BO. */
> + gbm_bo_destroy(fb->bo);
Hi,
gbm_bo_destroy() may be called with NULL. Is that ok?
> + drm_fb_destroy(fb);
> +}
> +
> +static struct drm_fb *
> +drm_fb_get_from_dmabuf(struct linux_dmabuf_buffer *dmabuf,
> + struct drm_backend *backend, bool is_opaque)
> +{
> +#ifdef HAVE_GBM_FD_IMPORT
> + 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],
> + };
> + struct gbm_import_fd_modifier_data import_mod = {
> + .width = dmabuf->attributes.width,
> + .height = dmabuf->attributes.height,
> + .format = dmabuf->attributes.format,
> + .num_fds = dmabuf->attributes.n_planes,
> + .modifier = dmabuf->attributes.modifier[0],
> + };
> + int i;
> +
> + /* XXX: TODO:
> + *
> + * Currently the buffer is rejected if any dmabuf attribute
> + * flag is set. This keeps us from passing an inverted /
> + * interlaced / bottom-first buffer (or any other type that may
> + * be added in the future) through to an overlay. Ultimately,
> + * these types of buffers should be handled through buffer
> + * transforms and not as spot-checks requiring specific
> + * knowledge. */
Bad whitespace in the above indentation.
I'm also not sure I agree with how things should be done instead. It's
possible the dmabuf wl_buffer is created by a library (libEGL, media,
etc.) which may not communicate these details outside, may not give a
possibility to add state to the wl_surface.commit, or may not actually
drive wl_surface itself. There could be many client-side designs where
this detail is not combinable with wl_surface state in the client.
Also, only y-inverted would be possible to deal with buffer transform.
The other flags probably do require specific knowledge if they were to
be implemented properly.
Hmm, it's an interesting question how buffer_damage should be
interpreted in case of e.g. y-inverted dmabuf.
> + if (dmabuf->attributes.flags)
> + return NULL;
> +
> + fb = zalloc(sizeof *fb);
> + if (fb == NULL)
> + return NULL;
> +
> + fb->refcnt = 1;
> + fb->type = BUFFER_DMABUF;
> +
> + memcpy(import_mod.fds, dmabuf->attributes.fd, sizeof(import_mod.fds));
> + memcpy(import_mod.strides, dmabuf->attributes.stride,
> + sizeof(import_mod.fds));
> + memcpy(import_mod.offsets, dmabuf->attributes.offset,
> + sizeof(import_mod.fds));
Do we have (build-time) asserts somewhere to ensure
sizeof(import_mod.xxx) == sizeof(dmabuf->attributes.xxx)?
> +
> + if (dmabuf->attributes.modifier[0] != DRM_FORMAT_MOD_INVALID) {
> + fb->bo = gbm_bo_import(backend->gbm, GBM_BO_IMPORT_FD_MODIFIER,
> + &import_mod,
> + GBM_BO_USE_SCANOUT);
> + } else {
> + fb->bo = gbm_bo_import(backend->gbm, GBM_BO_IMPORT_FD,
> + &import_legacy,
> + GBM_BO_USE_SCANOUT);
Previously this path refused, if n_planes != 1 or offset[0] != 0.
Shouldn't those continue to be refused?
> + }
> +
> + if (!fb->bo)
> + goto err_free;
> +
> + fb->width = dmabuf->attributes.width;
> + fb->height = dmabuf->attributes.height;
> + memcpy(fb->strides, dmabuf->attributes.stride, sizeof(fb->strides));
> + memcpy(fb->offsets, dmabuf->attributes.offset, sizeof(fb->offsets));
Assert sizeof(fb->xxx) == sizeof(dmabuf->attributes.xxx)?
> + fb->format = pixel_format_get_info(dmabuf->attributes.format);
> + fb->modifier = dmabuf->attributes.modifier[0];
> + fb->size = 0;
> + fb->fd = backend->drm.fd;
> +
> + if (!fb->format) {
> + weston_log("couldn't look up format info for 0x%lx\n",
> + (unsigned long) fb->format);
That should probably be dmabuf->attributes.format, right?
Otherwise it's always 0.
> + goto err_free;
> + }
> +
> + if (is_opaque)
> + fb->format = pixel_format_get_opaque_substitute(fb->format);
> +
> + if (backend->min_width > fb->width ||
> + fb->width > backend->max_width ||
> + backend->min_height > fb->height ||
> + fb->height > backend->max_height) {
> + weston_log("bo geometry out of bounds\n");
> + goto err_free;
> + }
> +
> + for (i = 0; i < dmabuf->attributes.n_planes; i++) {
> + fb->handles[i] = gbm_bo_get_handle_for_plane(fb->bo, i).u32;
> + if (!fb->handles[i])
> + goto err_free;
> + }
> +
> + if (drm_fb_addfb(fb) != 0) {
> + weston_log("failed to create kms fb: %m\n");
> + goto err_free;
> + }
> +
> + return fb;
> +
> +err_free:
> + drm_fb_destroy_dmabuf(fb);
> +#endif
> + return NULL;
> +}
> +
> static struct drm_fb *
> drm_fb_get_from_bo(struct gbm_bo *bo, struct drm_backend *backend,
> bool is_opaque, enum drm_fb_type type)
> @@ -1051,7 +1166,7 @@ static void
> drm_fb_set_buffer(struct drm_fb *fb, struct weston_buffer *buffer)
> {
> assert(fb->buffer_ref.buffer == NULL);
> - assert(fb->type == BUFFER_CLIENT);
> + assert(fb->type == BUFFER_CLIENT || fb->type == BUFFER_DMABUF);
> weston_buffer_reference(&fb->buffer_ref, buffer);
> }
>
> @@ -1076,6 +1191,9 @@ drm_fb_unref(struct drm_fb *fb)
> case BUFFER_GBM_SURFACE:
> gbm_surface_release_buffer(fb->gbm_surface, fb->bo);
> break;
> + case BUFFER_DMABUF:
> + drm_fb_destroy_dmabuf(fb);
> + break;
> default:
> assert(NULL);
> break;
> @@ -1322,9 +1440,9 @@ drm_fb_get_from_view(struct drm_output_state *state, struct weston_view *ev)
> struct drm_output *output = state->output;
> struct drm_backend *b = to_drm_backend(output->base.compositor);
> struct weston_buffer *buffer = ev->surface->buffer_ref.buffer;
> + bool is_opaque = drm_view_is_opaque(ev);
> struct linux_dmabuf_buffer *dmabuf;
> struct drm_fb *fb;
> - struct gbm_bo *bo;
>
> /* Don't import buffers which span multiple outputs. */
> if (ev->output_mask != (1u << output->base.id))
> @@ -1342,58 +1460,27 @@ drm_fb_get_from_view(struct drm_output_state *state, struct weston_view *ev)
> if (wl_shm_buffer_get(buffer->resource))
> return NULL;
>
> - if (!b->gbm)
> - return NULL;
> -
Why is this check removed from the dmabuf path?
drm_fb_get_from_dmabuf() is still using GBM but not checking gbm is
available at runtime.
> dmabuf = linux_dmabuf_buffer_get(buffer->resource);
> if (dmabuf) {
> -#ifdef HAVE_GBM_FD_IMPORT
> - /* XXX: TODO:
> - *
> - * Use AddFB2 directly, do not go via GBM.
> - * Add support for multiplanar formats.
> - * Both require refactoring in the DRM-backend to
> - * support a mix of gbm_bos and drmfbs.
> - */
> - struct gbm_import_fd_data gbm_dmabuf = {
> - .fd = dmabuf->attributes.fd[0],
> - .width = dmabuf->attributes.width,
> - .height = dmabuf->attributes.height,
> - .stride = dmabuf->attributes.stride[0],
> - .format = dmabuf->attributes.format
> - };
> -
> - /* XXX: TODO:
> - *
> - * Currently the buffer is rejected if any dmabuf attribute
> - * flag is set. This keeps us from passing an inverted /
> - * interlaced / bottom-first buffer (or any other type that may
> - * be added in the future) through to an overlay. Ultimately,
> - * these types of buffers should be handled through buffer
> - * transforms and not as spot-checks requiring specific
> - * knowledge. */
> - if (dmabuf->attributes.n_planes != 1 ||
> - dmabuf->attributes.offset[0] != 0 ||
> - dmabuf->attributes.flags)
> + fb = drm_fb_get_from_dmabuf(dmabuf, b, is_opaque);
> + if (!fb)
> return NULL;
> -
> - bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_FD, &gbm_dmabuf,
> - GBM_BO_USE_SCANOUT);
> -#else
> - return NULL;
> -#endif
> } else {
> + struct gbm_bo *bo;
> +
> + if (!b->gbm)
> + return NULL;
> +
> bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_WL_BUFFER,
> buffer->resource, GBM_BO_USE_SCANOUT);
> - }
> -
> - if (!bo)
> - return NULL;
> + if (!bo)
> + return NULL;
>
> - fb = drm_fb_get_from_bo(bo, b, drm_view_is_opaque(ev), BUFFER_CLIENT);
> - if (!fb) {
> - gbm_bo_destroy(bo);
> - return NULL;
> + fb = drm_fb_get_from_bo(bo, b, is_opaque, BUFFER_CLIENT);
> + if (!fb) {
> + gbm_bo_destroy(bo);
> + return NULL;
> + }
> }
>
> drm_fb_set_buffer(fb, buffer);
Otherwise looks ok.
Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20180709/32b6282e/attachment.sig>
More information about the wayland-devel
mailing list