[PATCH v14 29/41] compositor-drm: Add modifiers to GBM dmabuf import
Pekka Paalanen
ppaalanen at gmail.com
Fri Jan 26 10:48:44 UTC 2018
On Wed, 20 Dec 2017 12:26:46 +0000
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>
> ---
> configure.ac | 6 +-
> libweston/compositor-drm.c | 181 +++++++++++++++++++++++++++++++++------------
> 2 files changed, 137 insertions(+), 50 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 1f3cc28aa..8d0d6582a 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 713bbabdd..b030234e4 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -278,6 +278,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 */
> @@ -971,6 +972,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);
> + 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. */
> + 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));
Ok.
> + memcpy(import_mod.strides, dmabuf->attributes.stride,
> + sizeof(import_mod.fds));
sizeof argument incorrect
type mismatch:
int import_mod.strides
uint32_t dmabuf->attributes.stride
> + memcpy(import_mod.offsets, dmabuf->attributes.offset,
> + sizeof(import_mod.fds));
sizeof argument incorrect
type mismatch:
int import_mod.offsets
uint32_t dmabuf->attributes.offset
Btw. struct gbm_import_fd_data uses uint32_t, unlike struct
gbm_import_fd_modifier_data. Why did the new struct switch to signed
values? Does GBM actually do something with negative offset or stride?
> +
> + 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);
> + }
> +
> + if (!fb->bo)
> + goto err_free;
This error path will call gbm_bo_destroy(NULL) and the function cannot
handle it.
> +
> + 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));
Both memcpys ok.
> + fb->format = pixel_format_get_info(dmabuf->attributes.format);
> + fb->modifier = dmabuf->attributes.modifier[0];
> + fb->size = 0;
I just realized the 'size' member is only ever used for dumb buffers,
maybe move that into the dumb section in the struct definition and
clean up setters to have it non-zero only for dumb buffers? (A separate
patch)
> + fb->fd = backend->drm.fd;
> +
> + if (!fb->format) {
> + weston_log("couldn't look up format info for 0x%lx\n",
> + (unsigned long) fb->format);
> + 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;
> +}
> +
Otherwise all looks good.
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/20180126/91fdcf4b/attachment.sig>
More information about the wayland-devel
mailing list