On Tue, Sep 26, 2017 at 12:16 PM, Daniel Stone <dani...@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 <dani...@collabora.com>
> ---
> configure.ac | 3 -
> libweston/compositor-drm.c | 181
> +++++++++++++++++++++++++++++++++------------
> 2 files changed, 133 insertions(+), 51 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index f09d0e04..3996c80c 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -203,9 +203,6 @@ 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])])
> fi
>
>
> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
> index 7557ef55..65934a01 100644
> --- a/libweston/compositor-drm.c
> +++ b/libweston/compositor-drm.c
> @@ -273,6 +273,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 */
> @@ -928,6 +929,118 @@ 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)
> +{
> + 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;
> + 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);
> + 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);
> + 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)
> @@ -990,7 +1103,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);
> }
>
> @@ -1015,6 +1128,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;
> @@ -1217,9 +1333,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))
> @@ -1234,58 +1350,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;
> -
> 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)
> - goto err;
> -
> - bo = gbm_bo_import(b->gbm, GBM_BO_IMPORT_FD, &gbm_dmabuf,
> - GBM_BO_USE_SCANOUT);
> -#else
> - return NULL;
> -#endif
> + fb = drm_fb_get_from_dmabuf(dmabuf, b, is_opaque);
> + if (!fb)
> + return NULL;
> } 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);
> - }
There seems to be a small hole in the logic here. Views whose buffers
are from wl_drm (Mesa GLES clients) are falling through all this logic
and failing to import. linux_dmabuf_buffer_get() succeeds on them,
which causes control to go down the successful case in "if (dmabuf) {
... }". But then drm_fb_get_from_dmabuf() itself fails.
It appears that the import would still actually work if control went
down the gbm_bo_import(GBM_BO_IMPORT_WL_BUFFER) path instead, since
the Wayland-EGL builtin for importing bo's from Wayland buffers would
succeed.
I did the testing on v13 of your patch series, which has a slightly
different implementation of than your v12 edition picture heref. But
the problem seems the same.
> -
> - 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);
> --
> 2.14.1
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
