Re: [PATCH v14 29/41] compositor-drm: Add modifiers to GBM dmabuf import
On Wed, 20 Dec 2017 12:26:46 +
Daniel Stone 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
> ---
> 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_
[PATCH v14 29/41] compositor-drm: Add modifiers to GBM dmabuf import
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
---
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));
+ memcpy(import_mod.strides, dmabuf->attributes.stride,
+ sizeof(import_mod.fds));
+ memcpy(import_mod.offsets, dmabuf->attributes.offset,
+ sizeof(import_mod.fds));
+
+ 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;
+
+ 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));
+ fb->format = pixel_format_get_info(dmabuf->attributes.format);
+ fb->modifier = dmabuf->attributes.modifier[0];
+ fb->size = 0;
+ fb->fd = backend->drm.f
