[Piglit] [PATCH 2/3] framework: support for creating dma buffers through libdrm

[Pohjolainen, Topi]

On Wed, Apr 24, 2013 at 03:35:08PM +0200, Chad Versace wrote:
> On 04/16/2013 12:45 PM, Topi Pohjolainen wrote:
> >In order to test EXT_image_dma_buf_import one needs the capability
> >of creating driver specific buffers. By probing the environment for
> >drm libraries one can decide for which drivers the support is to
> >be built.
> >
> >Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> >---
> >  tests/util/CMakeLists.txt                          |  47 +++++
> >  .../util/piglit-framework-gl/piglit_drm_dma_buf.c  | 228 +++++++++++++++++++++
> >  .../util/piglit-framework-gl/piglit_drm_dma_buf.h  |  35 ++++
> >  .../piglit-framework-gl/piglit_x11_framework.c     |   5 +
> >  4 files changed, 315 insertions(+)
> >  create mode 100644 tests/util/piglit-framework-gl/piglit_drm_dma_buf.c
> >  create mode 100644 tests/util/piglit-framework-gl/piglit_drm_dma_buf.h
> >
> >diff --git a/tests/util/CMakeLists.txt b/tests/util/CMakeLists.txt
> >index cbcf050..6a41c24 100644
> >--- a/tests/util/CMakeLists.txt
> >+++ b/tests/util/CMakeLists.txt
> >@@ -51,6 +51,53 @@ set(UTIL_GL_LIBS
> >  	)
> >
> >  if(${CMAKE_SYSTEM_NAME} MATCHES "Linux")
> >+	find_library(DRM_LIBRARY
> >+		NAMES drm
> >+		PATHS /usr/lib
> >+	)
> >+
> >+	find_path(DRM_INCLUDE_BASE_DIR
> >+		NAMES libdrm/drm.h
> >+		PATHS /opt/include
> 
> Why /opt/include? I expected /usr/include.

Good catch, my mistake.

> 
> >+	)
> >+
> >+	find_library(DRM_LIBRARY
> >+		NAMES drm
> >+		PATHS /usr/lib
> >+	)
> >+
> >+	find_library(DRM_INTEL_LIBRARY
> >+		NAMES drm_intel
> >+		PATHS /usr/lib
> >+	)
> 
> The searching above needs to respect pkg-config by using pkg_check_modules().
> Otherwise
> 	1. As written, the above search will fail on 64-bit rpm distros,
> 	   which use /usr/lib64.
> 	2. If someone is doing development on libdrm installed into a
> 	   custom prefix, then the search as-written will not to use the custom libdrm
> 	   as defined by PKG_CONFIG_PATH.
> 
> pkg_check_modules() is documented in cmake's manpage. For example usage, just
> grep piglit.

Many thanks, it's not only cleaner, but it becomes a lot simpler, too.

> 
> >+
> >+	# One needs to have at least one hardware driver present, otherwise
> >+	# there is no point compiling just the dispatcher.
> 
> Yeah, that makes sense.
> 
> >+	if(DRM_LIBRARY AND DRM_INCLUDE_BASE_DIR AND DRM_INTEL_LIBRARY)
> >+		add_definitions(-DHAVE_LIBDRM)
> >+
> >+		list(APPEND UTIL_GL_SOURCES
> >+			piglit-framework-gl/piglit_drm_dma_buf.c
> >+		)
> >+
> >+		list(APPEND UTIL_GL_LIBS
> >+			${DRM_LIBRARY}
> >+		)
> >+
> >+		# This is needed for the direct "drm.h" inclusion by "xh86drm.h"
> >+		list(APPEND UTIL_GL_INCLUDES
> >+			${DRM_INCLUDE_BASE_DIR}/libdrm
> >+		)
> 
> If you use pkg_check_modules(), then the above append is unneeded. libdrm.pc
> lists $PREFIX/include/drm in the include flags.

Indeed.

> 
> >+
> >+		if(DRM_INTEL_LIBRARY)
> >+			add_definitions(-DHAVE_LIBDRM_INTEL)
> >+
> >+			list(APPEND UTIL_GL_LIBS
> >+				${DRM_INTEL_LIBRARY}
> >+			)
> >+		endif()
> >+	endif()
> >+
> >  	set(UTIL_GL_LIBS
> >  		${UTIL_GL_LIBS}
> >  		${X11_X11_LIB}
> >diff --git a/tests/util/piglit-framework-gl/piglit_drm_dma_buf.c b/tests/util/piglit-framework-gl/piglit_drm_dma_buf.c
> >new file mode 100644
> >index 0000000..1bebf14
> >--- /dev/null
> >+++ b/tests/util/piglit-framework-gl/piglit_drm_dma_buf.c
> >@@ -0,0 +1,228 @@
> >+/*
> >+ * Copyright © 2013 Intel Corporation
> >+ *
> >+ * Permission is hereby granted, free of charge, to any person obtaining a
> >+ * copy of this software and associated documentation files (the "Software"),
> >+ * to deal in the Software without restriction, including without limitation
> >+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >+ * and/or sell copies of the Software, and to permit persons to whom the
> >+ * Software is furnished to do so, subject to the following conditions:
> >+ *
> >+ * The above copyright notice and this permission notice (including the next
> >+ * paragraph) shall be included in all copies or substantial portions of the
> >+ * Software.
> >+ *
> >+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> >+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >+ * IN THE SOFTWARE.
> >+ */
> >+
> >+#include "piglit-util-gl-common.h"
> >+#include "piglit_drm_dma_buf.h"
> >+#ifdef HAVE_LIBDRM_INTEL
> >+#include <libdrm/intel_bufmgr.h>
> >+#endif
> >+#include <sys/types.h>
> >+#include <sys/stat.h>
> >+#include <fcntl.h>
> >+#include <string.h>
> >+#include <xf86drm.h>
> >+
> >+static const char *drm_device_filename = "/dev/dri/card0";
> >+#define BATCH_SZ (8192 * sizeof(uint32_t))
> 
> BATCH_SZ is intel-specific and used only in piglit_intel_bufmgr_get(). Let's
> move the variable to be scoped locally to that function.

Makes sense.

> 
> >+
> >+#define ALIGN(value, alignment) (((value) + alignment - 1) & ~(alignment - 1))
> >+
> >+struct piglit_drm_dma_buf {
> >+	unsigned w;
> >+	unsigned h;
> >+	unsigned stride;
> >+	int fd;
> >+	void *priv;
> >+};
> >+
> >+struct piglit_drm_driver {
> >+	const char *name;
> >+
> >+	int
> >+	(*create)(unsigned w, unsigned h, unsigned cpp,
> >+		  const unsigned char *src_data, unsigned src_stride,
> >+		  struct piglit_drm_dma_buf *buf);
> >+
> >+	int
> >+	(*export)(struct piglit_drm_dma_buf *buf);
> >+
> >+	void
> >+	(*destroy)(struct piglit_drm_dma_buf *buf);
> >+};
> >+
> >+static int
> >+piglit_drm_device_get(void)
> >+{
> >+	static int fd = 0;
> >+
> >+	if (fd)
> >+		return fd;
> >+
> >+	fd = open(drm_device_filename, O_RDWR);
> >+
> >+	return fd;
> >+}
> >+
> >+#ifdef HAVE_LIBDRM_INTEL
> >+static drm_intel_bufmgr *
> >+piglit_intel_bufmgr_get(void)
> >+{
> >+	static drm_intel_bufmgr *mgr = NULL;
> >+
> >+	if (mgr)
> >+		return mgr;
> >+
> >+	if (!piglit_drm_device_get())
> >+		return NULL;
> >+
> >+	mgr = intel_bufmgr_gem_init(piglit_drm_device_get(), BATCH_SZ);
> >+
> >+	return mgr;
> >+}
> >+
> >+static int
> >+piglit_intel_buf_create(unsigned w, unsigned h, unsigned cpp,
> >+			const unsigned char *src_data, unsigned src_stride,
> >+			struct piglit_drm_dma_buf *buf)
> >+{
> >+	unsigned i;
> >+	drm_intel_bo *bo;
> >+	unsigned stride = ALIGN(w * cpp, 4);
> >+	drm_intel_bufmgr *mgr = piglit_intel_bufmgr_get();
> >+
> >+	if (!mgr || src_stride > stride || h % 2)
> >+		return -1;
> >+
> >+	bo = drm_intel_bo_alloc(mgr, "piglit_dma_buf", h * stride, 4096);
> >+	if (!bo)
> >+		return -1;
> >+
> >+	if (drm_intel_bo_map(bo, 1)) {
> >+		drm_intel_bo_unreference(bo);
> >+		return -1;
> >+	}
> >+
> >+	for (i = 0; i < h; ++i) {
> >+		memcpy(((unsigned char *)bo->virtual) + i * stride,
> >+			src_data + i * src_stride, src_stride);
> 
> If `memcpy(..., src_stride)` were replaced with `memcpy(..., w)`, could then
> the above check for `src_stride > stride` be removed?

Well, you meant replacing by (w * cpp)? I had it that way originally, but then
I thought I left room for the caller to add sentinel sort of bytes in the end
of the row. I was thinking support for test cases where one specifically wants
to error check against sampling too far in the row. But perhaps that is too far
fetched and could be re-introduced to the interface if its really needed.

> 
> >+	}
> >+
> >+	if (drm_intel_bo_unmap(bo)) {
> >+		drm_intel_bo_unreference(bo);
> >+		return -1;
> >+	}
> >+
> >+	buf->w = w;
> >+	buf->h = h;
> >+	buf->stride = stride;
> >+	buf->priv = bo;
> >+
> >+	return 0;
> >+}
> >+
> >+static int
> >+piglit_intel_buf_export(struct piglit_drm_dma_buf *buf)
> >+{
> >+	if (drm_intel_bo_gem_export_to_prime((drm_intel_bo *)buf->priv,
> >+					&buf->fd)) {
> >+		drm_intel_bo_unreference((drm_intel_bo *)buf->priv);
> >+		return -1;
> >+	}
> >+
> >+	return 0;
> >+}
> >+
> >+static void
> >+piglit_intel_buf_destroy(struct piglit_drm_dma_buf *buf)
> >+{
> >+	drm_intel_bo_unreference((drm_intel_bo *)buf->priv);
> >+}
> >+#endif /* HAVE_LIBDRM_INTEL */
> >+
> >+/**
> >+ * The framework makes sure one does try to compile without any hardware
> >+ * support.
> >+ */
> >+static const struct piglit_drm_driver piglit_drm_drivers[] = {
> >+	{ "i915", piglit_intel_buf_create, piglit_intel_buf_export,
> >+	   piglit_intel_buf_destroy }
> >+};
> >+
> >+static const struct piglit_drm_driver *
> >+piglit_drm_get_driver(void)
> >+{
> >+	unsigned i;
> >+	drmVersionPtr version;
> >+	int fd = piglit_drm_device_get();
> >+
> >+	if (!fd)
> >+		return NULL;
> >+
> >+	version = drmGetVersion(fd);
> >+	if (!version || !version->name)
> >+		return NULL;
> >+
> >+	for (i = 0; i < sizeof(piglit_drm_drivers) /
> >+			sizeof(piglit_drm_drivers[0]); ++i) {
> 
> Use the ARRAY_SIZE defined in piglit-util.h.

Will do.

> 
> >+		if (strcmp(piglit_drm_drivers[i].name, version->name) == 0)
> >+			return &piglit_drm_drivers[i];
> >+	}
> >+
> >+	return NULL;
> >+}
> >+
> >+enum piglit_result
> >+piglit_drm_create_dma_buf(unsigned w, unsigned h, unsigned cpp,
> >+			const void *src_data, unsigned src_stride,
> >+			void **buf, int *fd, unsigned *stride, unsigned *offset)
> >+{
> >+	struct piglit_drm_dma_buf *drm_buf;
> >+	const struct piglit_drm_driver *drv = piglit_drm_get_driver();
> >+
> >+	if (!drv)
> >+		return PIGLIT_SKIP;
> >+
> >+	drm_buf = calloc(sizeof(struct piglit_drm_dma_buf), 1);
> >+	if (!buf)
> >+		return PIGLIT_FAIL;
> 
> Did you mean `if (!drm_buf)`?

Good catch!

> 
> >+
> >+	if (drv->create(w, h, cpp, src_data, src_stride, drm_buf)) {
> >+		free(drm_buf);
> >+		return PIGLIT_FAIL;
> >+	}
> >+
> >+	if (drv->export(drm_buf)) {
> >+		free(drm_buf);
> >+		return PIGLIT_FAIL;
> >+	}
> >+
> >+	*buf = drm_buf;
> >+	*fd = drm_buf->fd;
> >+	*stride = drm_buf->stride;
> >+	*offset = 0;
> >+
> >+	return PIGLIT_PASS;
> >+}
> >+
> >+void
> >+piglit_drm_destroy_dma_buf(void *buf)
> >+{
> >+	const struct piglit_drm_driver *drv = piglit_drm_get_driver();
> >+	struct piglit_drm_dma_buf *drm_buf = (struct piglit_drm_dma_buf *)buf;
> >+
> >+	if (!drv)
> >+		return;
> >+
> >+	drv->destroy(drm_buf);
> >+	free(drm_buf);
> >+}
> >diff --git a/tests/util/piglit-framework-gl/piglit_drm_dma_buf.h b/tests/util/piglit-framework-gl/piglit_drm_dma_buf.h
> >new file mode 100644
> >index 0000000..87adf03
> >--- /dev/null
> >+++ b/tests/util/piglit-framework-gl/piglit_drm_dma_buf.h
> >@@ -0,0 +1,35 @@
> >+/*
> >+ * Copyright © 2013 Intel Corporation
> >+ *
> >+ * Permission is hereby granted, free of charge, to any person obtaining a
> >+ * copy of this software and associated documentation files (the "Software"),
> >+ * to deal in the Software without restriction, including without limitation
> >+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> >+ * and/or sell copies of the Software, and to permit persons to whom the
> >+ * Software is furnished to do so, subject to the following conditions:
> >+ *
> >+ * The above copyright notice and this permission notice (including the next
> >+ * paragraph) shall be included in all copies or substantial portions of the
> >+ * Software.
> >+ *
> >+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> >+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> >+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> >+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> >+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> >+ * IN THE SOFTWARE.
> >+ */
> >+#ifndef PIGLIT_DRM_DMA_BUF_H
> >+#define PIGLIT_DRM_DMA_BUF_H
> >+
> >+enum piglit_result
> >+piglit_drm_create_dma_buf(unsigned w, unsigned h, unsigned cpp,
> >+			  const void *src_data, unsigned src_stride,
> >+			  void **buf, int *fd, unsigned *stride,
> >+			  unsigned *offset);
> >+
> >+void
> >+piglit_drm_destroy_dma_buf(void *buf);
> >+
> >+#endif /* PIGLIT_DRM_DMA_BUF_H */
> >diff --git a/tests/util/piglit-framework-gl/piglit_x11_framework.c b/tests/util/piglit-framework-gl/piglit_x11_framework.c
> >index dafd370..6cbc122 100644
> >--- a/tests/util/piglit-framework-gl/piglit_x11_framework.c
> >+++ b/tests/util/piglit-framework-gl/piglit_x11_framework.c
> >@@ -57,6 +57,7 @@
> >
> >  #include "piglit_gl_framework.h"
> >  #include "piglit_x11_framework.h"
> >+#include "piglit_drm_dma_buf.h"
> >
> >  struct piglit_x11_framework {
> >  	struct piglit_winsys_framework winsys_fw;
> >@@ -215,6 +216,10 @@ piglit_x11_framework_create(const struct piglit_gl_test_config *test_config,
> >  	winsys_fw->show_window = show_window;
> >  	winsys_fw->enter_event_loop = enter_event_loop;
> >  	gl_fw->destroy = destroy;
> >+#ifdef HAVE_LIBDRM
> >+	gl_fw->create_dma_buf = piglit_drm_create_dma_buf;
> >+	gl_fw->destroy_dma_buf = piglit_drm_destroy_dma_buf;
> >+#endif
> >
> >  	return gl_fw;
> >
> >
>