[PATCH weston 2/2] simple-dmabuf-drm: use GBM generic calls
Guido Günther
agx at sigxcpu.org
Fri Jul 20 19:21:59 UTC 2018
Hi,
On Thu, Jul 19, 2018 at 04:45:02PM +0200, Emilio Pozuelo Monfort wrote:
> No need to write libdrm driver specific code for each supported
> driver, we can just let GBM call the right one for us now.
>
> Signed-off-by: Emilio Pozuelo Monfort <emilio.pozuelo at collabora.co.uk>
> ---
> Some improvements from Daniel (thanks!). I also added missing error messages,
> formatting fixes and a logic error in your improvements that caused NV12 to
> abort if create_bo was working fine.
>
> No changes to XRGB, so that's likely to still fail for you. I suspect
> a bug in mesa's i965 code, I can look at that separately.
>
> Cheers,
> Emilio
>
> clients/simple-dmabuf-drm.c | 444 ++++++++++--------------------------
> configure.ac | 2 +-
> 2 files changed, 122 insertions(+), 324 deletions(-)
>
> diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c
> index 5bc5323e..5cbd2bb1 100644
> --- a/clients/simple-dmabuf-drm.c
> +++ b/clients/simple-dmabuf-drm.c
> @@ -41,21 +41,11 @@
> #include <getopt.h>
> #include <errno.h>
>
> -#include <xf86drm.h>
> -
> -#ifdef HAVE_LIBDRM_INTEL
> -#include <i915_drm.h>
> -#include <intel_bufmgr.h>
> -#endif
> -#ifdef HAVE_LIBDRM_FREEDRENO
> -#include <freedreno/freedreno_drmif.h>
> -#endif
> -#ifdef HAVE_LIBDRM_ETNAVIV
> -#include <etnaviv_drmif.h>
> -#endif
> +#include <gbm.h>
> #include <drm_fourcc.h>
>
> #include <wayland-client.h>
> +#include "shared/helpers.h"
> #include "shared/zalloc.h"
> #include "xdg-shell-unstable-v6-client-protocol.h"
> #include "fullscreen-shell-unstable-v1-client-protocol.h"
> @@ -65,14 +55,10 @@
> #define DRM_FORMAT_MOD_LINEAR 0
> #endif
>
> -struct buffer;
> -
> /* Possible options that affect the displayed image */
> #define OPT_Y_INVERTED 1 /* contents has y axis inverted */
> #define OPT_IMMEDIATE 2 /* create wl_buffer immediately */
>
> -#define ALIGN(v, a) ((v + a - 1) & ~(a - 1))
> -
> struct display {
> struct wl_display *display;
> struct wl_registry *registry;
> @@ -86,46 +72,24 @@ struct display {
> int req_dmabuf_immediate;
> };
>
> -struct drm_device {
> - int fd;
> - char *name;
> -
> - int (*alloc_bo)(struct buffer *buf);
> - void (*free_bo)(struct buffer *buf);
> - int (*export_bo_to_prime)(struct buffer *buf);
> - int (*map_bo)(struct buffer *buf);
> - void (*unmap_bo)(struct buffer *buf);
> - void (*device_destroy)(struct buffer *buf);
> -};
> -
> struct buffer {
> struct wl_buffer *buffer;
> int busy;
>
> - struct drm_device *dev;
> int drm_fd;
> + struct gbm_device *dev;
> + struct gbm_bo *bo;
> + struct gbm_bo *bo2;
>
> -#ifdef HAVE_LIBDRM_INTEL
> - drm_intel_bufmgr *bufmgr;
> - drm_intel_bo *intel_bo;
> -#endif /* HAVE_LIBDRM_INTEL */
> -#if HAVE_LIBDRM_FREEDRENO
> - struct fd_device *fd_dev;
> - struct fd_bo *fd_bo;
> -#endif /* HAVE_LIBDRM_FREEDRENO */
> -#if HAVE_LIBDRM_ETNAVIV
> - struct etna_device *etna_dev;
> - struct etna_bo *etna_bo;
> -#endif /* HAVE_LIBDRM_ETNAVIV */
> -
> - uint32_t gem_handle;
> int dmabuf_fd;
> - uint8_t *mmap;
> + int dmabuf_fd2;
> + void *mmap;
> + void *mmap2;
>
> int width;
> int height;
> - int bpp;
> - unsigned long stride;
> + uint32_t stride;
> + uint32_t stride2;
> int format;
> };
>
> @@ -161,170 +125,6 @@ static const struct wl_buffer_listener buffer_listener = {
> buffer_release
> };
>
> -
> -#ifdef HAVE_LIBDRM_INTEL
> -static int
> -intel_alloc_bo(struct buffer *my_buf)
> -{
> - /* XXX: try different tiling modes for testing FB modifiers. */
> - uint32_t tiling = I915_TILING_NONE;
> -
> - assert(my_buf->bufmgr);
> -
> - my_buf->intel_bo = drm_intel_bo_alloc_tiled(my_buf->bufmgr, "test",
> - my_buf->width, my_buf->height,
> - (my_buf->bpp / 8), &tiling,
> - &my_buf->stride, 0);
> -
> - printf("buffer allocated w %d, h %d, stride %lu, size %lu\n",
> - my_buf->width, my_buf->height, my_buf->stride, my_buf->intel_bo->size);
> -
> - if (!my_buf->intel_bo)
> - return 0;
> -
> - if (tiling != I915_TILING_NONE)
> - return 0;
> -
> - return 1;
> -}
> -
> -static void
> -intel_free_bo(struct buffer *my_buf)
> -{
> - drm_intel_bo_unreference(my_buf->intel_bo);
> -}
> -
> -static int
> -intel_map_bo(struct buffer *my_buf)
> -{
> - if (drm_intel_gem_bo_map_gtt(my_buf->intel_bo) != 0)
> - return 0;
> -
> - my_buf->mmap = my_buf->intel_bo->virtual;
> -
> - return 1;
> -}
> -
> -static int
> -intel_bo_export_to_prime(struct buffer *buffer)
> -{
> - return drm_intel_bo_gem_export_to_prime(buffer->intel_bo, &buffer->dmabuf_fd);
> -}
> -
> -static void
> -intel_unmap_bo(struct buffer *my_buf)
> -{
> - drm_intel_gem_bo_unmap_gtt(my_buf->intel_bo);
> -}
> -
> -static void
> -intel_device_destroy(struct buffer *my_buf)
> -{
> - drm_intel_bufmgr_destroy(my_buf->bufmgr);
> -}
> -
> -#endif /* HAVE_LIBDRM_INTEL */
> -#ifdef HAVE_LIBDRM_FREEDRENO
> -
> -static int
> -fd_alloc_bo(struct buffer *buf)
> -{
> - int flags = DRM_FREEDRENO_GEM_CACHE_WCOMBINE;
> - int size;
> -
> - buf->fd_dev = fd_device_new(buf->drm_fd);
> - buf->stride = ALIGN(buf->width, 32) * buf->bpp / 8;
> - size = buf->stride * buf->height;
> - buf->fd_dev = fd_device_new(buf->drm_fd);
> - buf->fd_bo = fd_bo_new(buf->fd_dev, size, flags);
> -
> - if (!buf->fd_bo)
> - return 0;
> - return 1;
> -}
> -
> -static void
> -fd_free_bo(struct buffer *buf)
> -{
> - fd_bo_del(buf->fd_bo);
> -}
> -
> -static int
> -fd_bo_export_to_prime(struct buffer *buf)
> -{
> - buf->dmabuf_fd = fd_bo_dmabuf(buf->fd_bo);
> - return buf->dmabuf_fd < 0;
> -}
> -
> -static int
> -fd_map_bo(struct buffer *buf)
> -{
> - buf->mmap = fd_bo_map(buf->fd_bo);
> - return buf->mmap != NULL;
> -}
> -
> -static void
> -fd_unmap_bo(struct buffer *buf)
> -{
> -}
> -
> -static void
> -fd_device_destroy(struct buffer *buf)
> -{
> - fd_device_del(buf->fd_dev);
> -}
> -#endif /* HAVE_LIBDRM_FREEDRENO */
> -#ifdef HAVE_LIBDRM_ETNAVIV
> -
> -static int
> -etna_alloc_bo(struct buffer *buf)
> -{
> - int flags = DRM_ETNA_GEM_CACHE_WC;
> - int size;
> -
> - buf->stride = ALIGN(buf->width, 32) * buf->bpp / 8;
> - size = buf->stride * buf->height;
> - buf->etna_dev = etna_device_new(buf->drm_fd);
> - buf->etna_bo = etna_bo_new(buf->etna_dev, size, flags);
> -
> - return buf->etna_bo != NULL;
> -}
> -
> -static void
> -etna_free_bo(struct buffer *buf)
> -{
> - etna_bo_del(buf->etna_bo);
> -}
> -
> -static int
> -etna_bo_export_to_prime(struct buffer *buf)
> -{
> - buf->dmabuf_fd = etna_bo_dmabuf(buf->etna_bo);
> - return buf->dmabuf_fd < 0;
> -}
> -
> -static int
> -etna_map_bo(struct buffer *buf)
> -{
> - buf->mmap = etna_bo_map(buf->etna_bo);
> - return buf->mmap != NULL;
> -}
> -
> -static void
> -etna_unmap_bo(struct buffer *buf)
> -{
> - if (munmap(buf->mmap, buf->stride * buf->height) < 0)
> - fprintf(stderr, "Failed to unmap buffer: %s", strerror(errno));
> - buf->mmap = NULL;
> -}
> -
> -static void
> -etna_device_destroy(struct buffer *buf)
> -{
> - etna_device_del(buf->etna_dev);
> -}
> -#endif /* HAVE_LIBDRM_ENTAVIV */
> -
> static void
> fill_content(struct buffer *my_buf, uint64_t modifier)
> {
> @@ -343,19 +143,18 @@ fill_content(struct buffer *my_buf, uint64_t modifier)
> }
> else if (modifier == DRM_FORMAT_MOD_LINEAR) {
> uint8_t *pix8;
> - /* first plane: Y (2/3 of the buffer) */
> - for (y = 0; y < my_buf->height * 2/3; y++) {
> + /* first plane: Y */
> + for (y = 0; y < my_buf->height; y++) {
> pix8 = my_buf->mmap + y * my_buf->stride;
> for (x = 0; x < my_buf->width; x++)
> *pix8++ = x % 256;
> }
> - /* second plane (CbCr) is half the size of Y
> - plane (last 1/3 of the buffer) */
> - for (y = my_buf->height * 2/3; y < my_buf->height; y++) {
> - pix8 = my_buf->mmap + y * my_buf->stride;
> - for (x = 0; x < my_buf->width; x+=2) {
> - *pix8++ = x % 256;
> - *pix8++ = y % 256;
> + /* second plane (CbCr) is half the size of Y */
> + for (y = 0; y < my_buf->height / 2; y++) {
> + pix8 = my_buf->mmap2 + y * my_buf->stride2;
> + for (x = 0; x < my_buf->width / 2; x++) {
> + *pix8++ = (x * 2) % 256;
> + *pix8++ = (y * 2) % 256;
> }
> }
> }
> @@ -371,69 +170,26 @@ fill_content(struct buffer *my_buf, uint64_t modifier)
> }
>
> static void
> -drm_device_destroy(struct buffer *buf)
> +buffer_free(struct buffer *buf)
> {
> - buf->dev->device_destroy(buf);
> - close(buf->drm_fd);
> -}
> + if (buf->buffer)
> + wl_buffer_destroy(buf->buffer);
>
> -static int
> -drm_device_init(struct buffer *buf)
> -{
> - struct drm_device *dev = calloc(1, sizeof(struct drm_device));
> + if (buf->bo)
> + gbm_bo_destroy(buf->bo);
> + if (buf->bo2)
> + gbm_bo_destroy(buf->bo2);
>
> - drmVersionPtr version = drmGetVersion(buf->drm_fd);
> + close(buf->dmabuf_fd);
> + close(buf->dmabuf_fd2);
>
> - dev->fd = buf->drm_fd;
> - dev->name = strdup(version->name);
> - if (0) {
> - /* nothing */
> - }
> -#ifdef HAVE_LIBDRM_INTEL
> - else if (!strcmp(dev->name, "i915")) {
> - buf->bufmgr = drm_intel_bufmgr_gem_init(buf->drm_fd, 32);
> - if (!buf->bufmgr)
> - return 0;
> - dev->alloc_bo = intel_alloc_bo;
> - dev->free_bo = intel_free_bo;
> - dev->export_bo_to_prime = intel_bo_export_to_prime;
> - dev->map_bo = intel_map_bo;
> - dev->unmap_bo = intel_unmap_bo;
> - dev->device_destroy = intel_device_destroy;
> - }
> -#endif
> -#ifdef HAVE_LIBDRM_FREEDRENO
> - else if (!strcmp(dev->name, "msm")) {
> - dev->alloc_bo = fd_alloc_bo;
> - dev->free_bo = fd_free_bo;
> - dev->export_bo_to_prime = fd_bo_export_to_prime;
> - dev->map_bo = fd_map_bo;
> - dev->unmap_bo = fd_unmap_bo;
> - dev->device_destroy = fd_device_destroy;
> - }
> -#endif
> -#ifdef HAVE_LIBDRM_ETNAVIV
> - else if (!strcmp(dev->name, "etnaviv")) {
> - dev->alloc_bo = etna_alloc_bo;
> - dev->free_bo = etna_free_bo;
> - dev->export_bo_to_prime = etna_bo_export_to_prime;
> - dev->map_bo = etna_map_bo;
> - dev->unmap_bo = etna_unmap_bo;
> - dev->device_destroy = etna_device_destroy;
> - }
> -#endif
> - else {
> - fprintf(stderr, "Error: drm device %s unsupported.\n",
> - dev->name);
> - free(dev);
> - return 0;
> - }
> - buf->dev = dev;
> - return 1;
> + if (buf->dev)
> + gbm_device_destroy(buf->dev);
> + close(buf->drm_fd);
> }
>
> -static int
> -drm_connect(struct buffer *my_buf)
> +static struct gbm_device *
> +gbm_connect(struct buffer *my_buf)
> {
> /* This won't work with card0 as we need to be authenticated; instead,
> * boot with drm.rnodes=1 and use that. */
> @@ -441,16 +197,9 @@ drm_connect(struct buffer *my_buf)
> if (my_buf->drm_fd < 0)
> return 0;
>
> - return drm_device_init(my_buf);
> + return my_buf->dev = gbm_create_device(my_buf->drm_fd);
> }
>
> -static void
> -drm_shutdown(struct buffer *my_buf)
> -{
> - drm_device_destroy(my_buf);
> -}
> -
> -
> static void
> create_succeeded(void *data,
> struct zwp_linux_buffer_params_v1 *params,
> @@ -482,54 +231,111 @@ static const struct zwp_linux_buffer_params_v1_listener params_listener = {
> create_failed
> };
>
> +static struct gbm_bo *
> +create_bo(struct buffer *buffer, int format, int subsamp,
> + const uint64_t *modifiers, const unsigned int count)
> +{
> + struct gbm_bo *bo;
> +
> + if (!modifiers) {
> + bo = gbm_bo_create(buffer->dev,
> + buffer->width / subsamp, buffer->height / subsamp,
> + format,
> + 0 /* usage */);
> + } else {
> + bo = gbm_bo_create_with_modifiers(buffer->dev,
> + buffer->width / subsamp,
> + buffer->height / subsamp,
> + format,
> + modifiers,
> + count);
> + }
> +
> + if (!bo)
> + fprintf(stderr, "bo_create failed\n");
> +
> + return bo;
> +}
> +
> static int
> create_dmabuf_buffer(struct display *display, struct buffer *buffer,
> int width, int height, int format, uint32_t opts)
> {
> struct zwp_linux_buffer_params_v1 *params;
> - uint64_t modifier = 0;
> + uint64_t modifier = 0, modifier2 = 0;
> uint32_t flags = 0;
> - struct drm_device *drm_dev;
>
> - if (!drm_connect(buffer)) {
> - fprintf(stderr, "drm_connect failed\n");
> + if (!gbm_connect(buffer)) {
> + fprintf(stderr, "gbm_device_connect failed\n");
> goto error;
> }
> - drm_dev = buffer->dev;
>
> buffer->width = width;
> + buffer->height = height;
> + buffer->format = format;
> switch (format) {
> case DRM_FORMAT_NV12:
> - /* adjust height for allocation of NV12 Y and UV planes */
> - buffer->height = height * 3 / 2;
> - buffer->bpp = 8;
> modifier = display->nv12_modifier;
> break;
> - default:
> - buffer->height = height;
> - buffer->bpp = 32;
> }
> - buffer->format = format;
>
> - if (!drm_dev->alloc_bo(buffer)) {
> - fprintf(stderr, "alloc_bo failed\n");
> - goto error1;
> + if (format == DRM_FORMAT_XRGB8888) {
> + uint64_t modifiers[] = { DRM_FORMAT_MOD_LINEAR };
> +
> + buffer->bo = create_bo(buffer, format, 1,
> + modifiers, ARRAY_LENGTH(modifiers));
> + if (!buffer->bo) {
> + fprintf(stderr, "create_bo for XRGB8888 failed\n");
> + goto error;
> + }
> + } else {
> + uint64_t modifiers[] = { modifier };
> +
> + buffer->bo = create_bo(buffer, GBM_FORMAT_R8, 1,
> + modifiers, ARRAY_LENGTH(modifiers));
> + buffer->bo2 = create_bo(buffer, GBM_FORMAT_GR88, 2,
> + modifiers, ARRAY_LENGTH(modifiers));
> + if (!buffer->bo || !buffer->bo2) {
> + fprintf(stderr, "create_bo for NV12 failed\n");
> + goto error;
> + }
> }
>
> - if (!drm_dev->map_bo(buffer)) {
> + buffer->mmap = gbm_bo_map(buffer->bo, 0 /* x */, 0 /* y */,
> + width, height,
> + GBM_BO_TRANSFER_WRITE,
> + &buffer->stride, &buffer->mmap);
> + if (!buffer->mmap) {
> fprintf(stderr, "map_bo failed\n");
> - goto error2;
> + goto error;
> }
> - fill_content(buffer, modifier);
> - drm_dev->unmap_bo(buffer);
>
> - if (drm_dev->export_bo_to_prime(buffer) != 0) {
> - fprintf(stderr, "gem_export_to_prime failed\n");
> - goto error2;
> + if (format == DRM_FORMAT_NV12) {
> + buffer->mmap2 = gbm_bo_map(buffer->bo2, 0 /* x */, 0 /* y */,
> + width / 2, height / 2,
I might be wrong here but wouldn't you want half the pixels of the Y
plane here, not a quarter (2x2 subsampling for both U and V)?
> + GBM_BO_TRANSFER_WRITE,
> + &buffer->stride2, &buffer->mmap2);
> + if (!buffer->mmap2) {
> + fprintf(stderr, "map_bo failed\n");
> + goto error;
> + }
> }
> - if (buffer->dmabuf_fd < 0) {
> +
> + modifier = gbm_bo_get_modifier(buffer->bo);
> + fill_content(buffer, modifier);
> + gbm_bo_unmap(buffer->bo, buffer->mmap);
> + if (buffer->bo2)
> + gbm_bo_unmap(buffer->bo2, buffer->mmap2);
> +
> + buffer->dmabuf_fd = gbm_bo_get_fd(buffer->bo);
> + if (buffer->bo2)
> + buffer->dmabuf_fd2 = gbm_bo_get_fd(buffer->bo2);
> + else
> + buffer->dmabuf_fd2 = -1;
> +
> + if (buffer->dmabuf_fd < 0 || (format == DRM_FORMAT_NV12 && buffer->dmabuf_fd2 < 0)) {
> fprintf(stderr, "error: dmabuf_fd < 0\n");
> - goto error2;
> + goto error;
> }
>
> /* We now have a dmabuf! For format XRGB8888, it should contain
> 2x2
This code here looks like:
/* We now have a dmabuf! For format XRGB8888, it should contain 2x2
* tiles (i.e. each tile is 256x256) of misc colours, and be mappable,
* either as ARGB8888, or XRGB8888. For format NV12, it should contain
* the Y and UV components, and needs to be re-adjusted for passing the
* correct height to the compositor.
*/
buffer->height = height;
And I think the comment as well as the assignment are outdated now since
the readjustment is not needed anymore (already done at the beginning of
the function).
Cheers,
-- Guido
> @@ -554,12 +360,12 @@ create_dmabuf_buffer(struct display *display, struct buffer *buffer,
> if (format == DRM_FORMAT_NV12) {
> /* add the second plane params */
> zwp_linux_buffer_params_v1_add(params,
> - buffer->dmabuf_fd,
> + buffer->dmabuf_fd2,
> 1,
> - buffer->width * buffer->height,
> - buffer->stride,
> - modifier >> 32,
> - modifier & 0xffffffff);
> + 0,
> + buffer->stride2,
> + modifier2 >> 32,
> + modifier2 & 0xffffffff);
> }
> zwp_linux_buffer_params_v1_add_listener(params, ¶ms_listener, buffer);
> if (display->req_dmabuf_immediate) {
> @@ -579,11 +385,8 @@ create_dmabuf_buffer(struct display *display, struct buffer *buffer,
>
> return 0;
>
> -error2:
> - drm_dev->free_bo(buffer);
> -error1:
> - drm_shutdown(buffer);
> error:
> + buffer_free(buffer);
> return -1;
> }
>
> @@ -685,7 +488,6 @@ create_window(struct display *display, int width, int height, int format,
> static void
> destroy_window(struct window *window)
> {
> - struct drm_device* dev;
> int i;
>
> if (window->callback)
> @@ -695,11 +497,7 @@ destroy_window(struct window *window)
> if (!window->buffers[i].buffer)
> continue;
>
> - wl_buffer_destroy(window->buffers[i].buffer);
> - dev = window->buffers[i].dev;
> - dev->free_bo(&window->buffers[i]);
> - close(window->buffers[i].dmabuf_fd);
> - drm_shutdown(&window->buffers[i]);
> + buffer_free(&window->buffers[i]);
> }
>
> if (window->xdg_toplevel)
> diff --git a/configure.ac b/configure.ac
> index 093d6b54..8ff7d5bb 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -400,7 +400,7 @@ AC_ARG_ENABLE(simple-dmabuf-drm-client,
> [do not build the simple dmabuf drm client]),,
> enable_simple_dmabuf_drm_client="auto")
> if ! test "x$enable_simple_dmabuf_drm_client" = "xno"; then
> - PKG_CHECK_MODULES(SIMPLE_DMABUF_DRM_CLIENT, [wayland-client libdrm], [have_simple_dmabuf_libs=yes],
> + PKG_CHECK_MODULES(SIMPLE_DMABUF_DRM_CLIENT, [wayland-client libdrm gbm >= 17.1], [have_simple_dmabuf_libs=yes],
> [have_simple_dmabuf_libs=no])
>
> PKG_CHECK_MODULES(LIBDRM_PLATFORM_FREEDRENO, [libdrm_freedreno],
> --
> 2.18.0
>
More information about the wayland-devel
mailing list