[PATCH weston] simple-dmabuf-drm: use GBM generic calls

Pekka Paalanen ppaalanen at gmail.com
Wed Jul 11 08:16:45 UTC 2018


Adding more people to CC.

On Tue, 10 Jul 2018 17:53:15 +0200
Emilio Pozuelo Monfort <pochu27 at gmail.com> 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>
> ---
> 
> Hi,
> 
> This simplifies the code a lot, using gbm_bo as Emil suggested. Some problems
> I still see:
> 
> - NV12 doesn't work, it seems that backends/dri/gbm_dri.c:gbm_format_to_dri_format()
>   doesn't support it.

I think NV12 and other less common formats, should someone add them in
this program, should not be lost. That may be part of the reason GBM
wasn't used: the need to test YUV formats, and non-GPU-renderable
formats in general.

> 
> - We are still linking to libdrm, that's just to get the DRM_FORMAT_* definitions.
>   I suppose we could switch those to GBM_FORMAT_* instead.

We have precedent in the build system of using libdrm only for headers
but not link to it, specifically to get the pixel format codes. Just
leave out the $(LIBDRM_LIBS) or such.

> 
> - Not sure if I need to pass any flags to gbm_bo_create or gbm_bo_map.
> 
> This works fine for me with XRGB8888.
> 
> Thoughts? Comments?
> 
> Thanks,
> Emilio


Thanks,
pq

> 
>  clients/simple-dmabuf-drm.c | 320 +++---------------------------------
>  configure.ac                |   2 +-
>  2 files changed, 27 insertions(+), 295 deletions(-)
> 
> diff --git a/clients/simple-dmabuf-drm.c b/clients/simple-dmabuf-drm.c
> index fcab30e5..cdee27b3 100644
> --- a/clients/simple-dmabuf-drm.c
> +++ b/clients/simple-dmabuf-drm.c
> @@ -41,18 +41,7 @@
>  #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>
> @@ -66,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;
> @@ -88,46 +73,22 @@ struct display {
>  	int req_dmabuf_modifiers;
>  };
>  
> -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;
> -
> -#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 */
> +	struct gbm_device *dev;
> +	struct gbm_bo *bo;
>  
>  	uint32_t gem_handle;
>  	int dmabuf_fd;
> -	uint8_t *mmap;
> +	void *mmap;
>  
>  	int width;
>  	int height;
>  	int bpp;
> -	unsigned long stride;
> +	uint32_t stride;
>  	int format;
>  };
>  
> @@ -163,170 +124,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)
>  {
> @@ -373,83 +170,22 @@ fill_content(struct buffer *my_buf, uint64_t modifier)
>  }
>  
>  static void
> -drm_device_destroy(struct buffer *buf)
> +gbm_shutdown(struct buffer *buf)
>  {
> -	buf->dev->device_destroy(buf);
> +	gbm_device_destroy(buf->dev);
>  	close(buf->drm_fd);
>  }
>  
> -static int
> -drm_device_init(struct buffer *buf)
> -{
> -	struct drm_device *dev = calloc(1, sizeof(struct drm_device));
> -
> -	drmVersionPtr version = drmGetVersion(buf->drm_fd);
> -
> -	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;
> -}
> -
> -static int
> -drm_connect(struct buffer *my_buf)
> +static struct gbm_device *
> +gbm_connect(struct buffer *buffer)
>  {
>  	/* This won't work with card0 as we need to be authenticated; instead,
>  	 * boot with drm.rnodes=1 and use that. */
> -	my_buf->drm_fd = open("/dev/dri/renderD128", O_RDWR);
> -	if (my_buf->drm_fd < 0)
> +	buffer->drm_fd = open("/dev/dri/renderD128", O_RDWR);
> +	if (buffer->drm_fd < 0)
>  		return 0;
>  
> -	return drm_device_init(my_buf);
> -}
> -
> -static void
> -drm_shutdown(struct buffer *my_buf)
> -{
> -	drm_device_destroy(my_buf);
> +	return gbm_create_device(buffer->drm_fd);
>  }
>  
>  
> @@ -491,44 +227,42 @@ create_dmabuf_buffer(struct display *display, struct buffer *buffer,
>  	struct zwp_linux_buffer_params_v1 *params;
>  	uint64_t modifier = 0;
>  	uint32_t flags = 0;
> -	struct drm_device *drm_dev;
>  
> -	if (!drm_connect(buffer)) {
> -		fprintf(stderr, "drm_connect failed\n");
> +	if ((buffer->dev = gbm_connect(buffer)) == NULL) {
> +		fprintf(stderr, "gbm_device_connect failed\n");
>  		goto error;
>  	}
> -	drm_dev = buffer->dev;
>  
>  	buffer->width = width;
>  	switch (format) {
>  	case DRM_FORMAT_NV12:
> +		format = GBM_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:
> +		format = GBM_FORMAT_XRGB8888;
>  		buffer->height = height;
>  		buffer->bpp = 32;
>  	}
>  	buffer->format = format;
>  
> -	if (!drm_dev->alloc_bo(buffer)) {
> -		fprintf(stderr, "alloc_bo failed\n");
> +	if ((buffer->bo = gbm_bo_create(buffer->dev, width, height, format, 0 /* usage */)) == NULL) {
> +		fprintf(stderr, "bo_create failed\n");
>  		goto error1;
>  	}
>  
> -	if (!drm_dev->map_bo(buffer)) {
> +	if ((buffer->mmap = gbm_bo_map(buffer->bo, 0 /* x */, 0 /* y */, width, height,
> +				       0 /* flags */, &buffer->stride, &buffer->mmap)) == NULL) {
>  		fprintf(stderr, "map_bo failed\n");
>  		goto error2;
>  	}
>  	fill_content(buffer, modifier);
> -	drm_dev->unmap_bo(buffer);
> +	gbm_bo_unmap(buffer->bo, buffer->mmap);
>  
> -	if (drm_dev->export_bo_to_prime(buffer) != 0) {
> -		fprintf(stderr, "gem_export_to_prime failed\n");
> -		goto error2;
> -	}
> +	buffer->dmabuf_fd = gbm_bo_get_fd(buffer->bo);
>  	if (buffer->dmabuf_fd < 0) {
>  		fprintf(stderr, "error: dmabuf_fd < 0\n");
>  		goto error2;
> @@ -582,9 +316,9 @@ create_dmabuf_buffer(struct display *display, struct buffer *buffer,
>  	return 0;
>  
>  error2:
> -	drm_dev->free_bo(buffer);
> +	gbm_bo_destroy(buffer->bo);
>  error1:
> -	drm_shutdown(buffer);
> +	gbm_shutdown(buffer);
>  error:
>  	return -1;
>  }
> @@ -687,7 +421,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)
> @@ -698,10 +431,9 @@ destroy_window(struct window *window)
>  			continue;
>  
>  		wl_buffer_destroy(window->buffers[i].buffer);
> -		dev = window->buffers[i].dev;
> -		dev->free_bo(&window->buffers[i]);
> +		gbm_bo_destroy(window->buffers[i].bo);
>  		close(window->buffers[i].dmabuf_fd);
> -		drm_shutdown(&window->buffers[i]);
> +		gbm_shutdown(&window->buffers[i]);
>  	}
>  
>  	if (window->xdg_toplevel)
> diff --git a/configure.ac b/configure.ac
> index 39168e20..2169a8d3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -391,7 +391,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 egl], [have_simple_dmabuf_libs=yes],
> +  PKG_CHECK_MODULES(SIMPLE_DMABUF_DRM_CLIENT, [wayland-client libdrm egl gbm], [have_simple_dmabuf_libs=yes],
>  		    [have_simple_dmabuf_libs=no])
>  
>    PKG_CHECK_MODULES(LIBDRM_PLATFORM_FREEDRENO, [libdrm_freedreno],

-------------- 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/20180711/14caaf35/attachment-0001.sig>


More information about the wayland-devel mailing list