On Wed, 20 Dec 2017 12:26:46 +0000 Daniel Stone <[email protected]> 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 <[email protected]> > --- > 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
pgpdUNNRsN0ak.pgp
Description: OpenPGP digital signature
_______________________________________________ wayland-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/wayland-devel
