[PATCH v17 04/14] compositor-drm: Add modifiers to GBM dmabuf import

[Pekka Paalanen]

On Mon,  9 Jul 2018 14:23:10 +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 | 201 +++++++++++++++++++++++++++++--------
>  2 files changed, 160 insertions(+), 47 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 29fa98da6..ae4a4a542 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -326,6 +326,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 */
> @@ -1038,6 +1039,145 @@ 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. */
> +	if (fb->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;
> +
> +	BUILD_BUG_ON(ARRAY_LENGTH(import_mod.fds) !=
> +		     ARRAY_LENGTH(dmabuf->attributes.fd));
> +	BUILD_BUG_ON(sizeof(import_mod.fds) != sizeof(dmabuf->attributes.fd));
> +	memcpy(import_mod.fds, dmabuf->attributes.fd, sizeof(import_mod.fds));

Do we need the ARRAY_LENGTH check as well? I'd thought sizeof check
would suffice to avoid any damage.

> +
> +	BUILD_BUG_ON(ARRAY_LENGTH(import_mod.strides) !=
> +		     ARRAY_LENGTH(dmabuf->attributes.stride));
> +	BUILD_BUG_ON(sizeof(import_mod.strides) !=
> +		     sizeof(dmabuf->attributes.stride));
> +	memcpy(import_mod.strides, dmabuf->attributes.stride,
> +	       sizeof(import_mod.strides));
> +
> +	BUILD_BUG_ON(ARRAY_LENGTH(import_mod.offsets) !=
> +		     ARRAY_LENGTH(dmabuf->attributes.offset));
> +	BUILD_BUG_ON(sizeof(import_mod.offsets) !=
> +		     sizeof(dmabuf->attributes.offset));
> +	memcpy(import_mod.offsets, dmabuf->attributes.offset,
> +	       sizeof(import_mod.offsets));

Nice catch, I didn't even see the wrong names before.

All my comments are addressed.

Reviewed-by: Pekka Paalanen <pekka.paalanen at collabora.co.uk>


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/ffdf45ae/attachment.sig>