[i-g-t] Lib: Move __gem_context_create to common ioctl wrapper library.

Commit Message

Antonio Argenziano Oct. 11, 2017, 11:07 p.m. UTC

This patch adds a context creation ioctl wrapper that returns the error
for the caller to consume. Multiple tests that implemented this already,
have been changed to use the new library function.

Signed-off-by: Antonio Argenziano <[e-mail]>
---
 benchmarks/gem_exec_ctx.c   | 16 ++++------------
 benchmarks/gem_exec_trace.c |  9 +--------
 lib/ioctl_wrappers.c        | 33 +++++++++++++++++++++++----------
 lib/ioctl_wrappers.h        |  1 +
 tests/gem_ctx_create.c      | 13 ++++++-------
 tests/gem_ctx_switch.c      | 13 -------------
 tests/gem_eio.c             | 13 +------------
 tests/gem_exec_await.c      | 14 ++------------
 tests/gem_exec_nop.c        | 13 -------------
 tests/gem_exec_parallel.c   | 15 +++------------
 tests/gem_exec_reuse.c      | 13 -------------
 tests/gem_exec_whisper.c    | 13 -------------
 12 files changed, 41 insertions(+), 125 deletions(-)

Comments

Chris Wilson Oct. 12, 2017, 7:59 a.m. UTC | #1

Quoting Antonio Argenziano (2017-10-12 00:07:34)
> +int __gem_context_create(int fd, uint32_t *ctx_id)
> +{
> +       struct drm_i915_gem_context_create create;
> +       int err = 0;
> +
> +       memset(&create, 0, sizeof(create));
> +       if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create) == 0)
> +               *ctx_id = create.ctx_id;
> +       else
> +               err = -errno;
> +
> +       errno = 0;
> +       return err;
> +}
> +
>  /**
>   * gem_context_create:
>   * @fd: open i915 drm file descriptor
> @@ -906,18 +921,16 @@ int gem_madvise(int fd, uint32_t handle, int state)
>   */
>  uint32_t gem_context_create(int fd)
>  {
> -       struct drm_i915_gem_context_create create;
> +       uint32_t ctx_id;
> +       int ret;
>  
> -       memset(&create, 0, sizeof(create));
> -       if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create)) {
> -               int err = -errno;
> -               igt_skip_on(err == -ENODEV || errno == -EINVAL);
> -               igt_assert_eq(err, 0);
> -       }
> -       igt_assert(create.ctx_id != 0);
> -       errno = 0;
> +       ret = __gem_context_create(fd, &ctx_id);
> +
> +       igt_skip_on(ret == -ENODEV || errno == -EINVAL);

So close and yet so far.

> +       igt_assert_eq(ret, 0);
> +       igt_assert(ctx_id != 0);
>  
> -       return create.ctx_id;
> +       return ctx_id;
>  }

Whilst you are here, can you remove that igt_skip_on() entirely and add
gem_require_contexts() so that we don't have the spam hiding important
details in tests.

Please also think twice before adding it outside of tests/ as the igt_()
infrastructure is snafu there.
-Chris

Patch

diff --git a/benchmarks/gem_exec_ctx.c b/benchmarks/gem_exec_ctx.c
index 0eac04b0..b41eb204 100644
--- a/benchmarks/gem_exec_ctx.c
+++ b/benchmarks/gem_exec_ctx.c
@@ -64,16 +64,6 @@  static uint32_t batch(int fd)
 	return handle;
 }
 
-static uint32_t __gem_context_create(int fd)
-{
-	struct drm_i915_gem_context_create create;
-
-	memset(&create, 0, sizeof(create));
-	drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create);
-
-	return create.ctx_id;
-}
-
 static int loop(unsigned ring,
 		int reps,
 		enum mode mode,
@@ -84,6 +74,7 @@  static int loop(unsigned ring,
 	struct drm_i915_gem_exec_object2 obj;
 	double *shared;
 	int fds[2], fd;
+	uint32_t ctx_id = 0;
 
 	shared = mmap(0, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
 
@@ -101,7 +92,8 @@  static int loop(unsigned ring,
 	execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT;
 	execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC;
 	if (mode != DEFAULT) {
-		execbuf.rsvd1 = __gem_context_create(fd);
+		__gem_context_create(fd, &ctx_id);
+		execbuf.rsvd1 = ctx_id;
 		if (execbuf.rsvd1 == 0)
 			return 77;
 	}
@@ -125,7 +117,7 @@  static int loop(unsigned ring,
 			uint32_t ctx = 0;
 
 			if (mode != DEFAULT && mode != NOP) {
-				execbuf.rsvd1 = __gem_context_create(fd);
+				execbuf.rsvd1 = gem_context_create(fd);
 				ctx = gem_context_create(fd);
 			}
 
diff --git a/benchmarks/gem_exec_trace.c b/benchmarks/gem_exec_trace.c
index 12577649..eebd6872 100644
--- a/benchmarks/gem_exec_trace.c
+++ b/benchmarks/gem_exec_trace.c
@@ -105,13 +105,6 @@  static double elapsed(const struct timespec *start, const struct timespec *end)
 	return 1e3*(end->tv_sec - start->tv_sec) + 1e-6*(end->tv_nsec - start->tv_nsec);
 }
 
-static uint32_t __gem_context_create(int fd)
-{
-	struct drm_i915_gem_context_create arg = {};
-	drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &arg);
-	return arg.ctx_id;
-}
-
 static double replay(const char *filename, long nop, long range)
 {
 	struct timespec t_start, t_end;
@@ -216,7 +209,7 @@  static double replay(const char *filename, long nop, long range)
 				num_ctx = new_ctx;
 			}
 
-			ctx[t->handle] = __gem_context_create(fd);
+			ctx[t->handle] = gem_context_create(fd);
 			break;
 		}
 	case DEL_CTX:
diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 87511fc6..561e2bb7 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -894,6 +894,21 @@  int gem_madvise(int fd, uint32_t handle, int state)
 	return madv.retained;
 }
 
+int __gem_context_create(int fd, uint32_t *ctx_id)
+{
+	struct drm_i915_gem_context_create create;
+	int err = 0;
+
+	memset(&create, 0, sizeof(create));
+	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create) == 0)
+		*ctx_id = create.ctx_id;
+	else
+		err = -errno;
+
+	errno = 0;
+	return err;
+}
+
 /**
  * gem_context_create:
  * @fd: open i915 drm file descriptor
@@ -906,18 +921,16 @@  int gem_madvise(int fd, uint32_t handle, int state)
  */
 uint32_t gem_context_create(int fd)
 {
-	struct drm_i915_gem_context_create create;
+	uint32_t ctx_id;
+	int ret;
 
-	memset(&create, 0, sizeof(create));
-	if (igt_ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create)) {
-		int err = -errno;
-		igt_skip_on(err == -ENODEV || errno == -EINVAL);
-		igt_assert_eq(err, 0);
-	}
-	igt_assert(create.ctx_id != 0);
-	errno = 0;
+	ret = __gem_context_create(fd, &ctx_id);
+
+	igt_skip_on(ret == -ENODEV || errno == -EINVAL);
+	igt_assert_eq(ret, 0);
+	igt_assert(ctx_id != 0);
 
-	return create.ctx_id;
+	return ctx_id;
 }
 
 int __gem_context_destroy(int fd, uint32_t ctx_id)
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index 1663b7f8..05aad196 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -119,6 +119,7 @@  int gem_munmap(void *ptr, uint64_t size);
 
 int gem_madvise(int fd, uint32_t handle, int state);
 
+int __gem_context_create(int fd, uint32_t *ctx_id);
 uint32_t gem_context_create(int fd);
 void gem_context_destroy(int fd, uint32_t ctx_id);
 int __gem_context_destroy(int fd, uint32_t ctx_id);
diff --git a/tests/gem_ctx_create.c b/tests/gem_ctx_create.c
index ae0825a1..65050c09 100644
--- a/tests/gem_ctx_create.c
+++ b/tests/gem_ctx_create.c
@@ -45,7 +45,7 @@  static unsigned all_nengine;
 static unsigned ppgtt_engines[16];
 static unsigned ppgtt_nengine;
 
-static int __gem_context_create(int fd, struct drm_i915_gem_context_create *arg)
+static int __gem_context_create2(int fd, struct drm_i915_gem_context_create *arg)
 {
 	int ret = 0;
 	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, arg))
@@ -233,7 +233,7 @@  static uint64_t total_avail_mem(unsigned mode)
 
 static void maximum(int fd, int ncpus, unsigned mode)
 {
-	struct drm_i915_gem_context_create create;
+	uint32_t ctx_id;
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	struct drm_i915_gem_execbuffer2 execbuf;
 	struct drm_i915_gem_exec_object2 obj[2];
@@ -242,7 +242,6 @@  static void maximum(int fd, int ncpus, unsigned mode)
 	uint32_t *contexts = NULL;
 	unsigned long count = 0;
 
-	memset(&create, 0, sizeof(create));
 	do {
 		int err;
 
@@ -255,14 +254,14 @@  static void maximum(int fd, int ncpus, unsigned mode)
 
 		err = -ENOMEM;
 		if (avail_mem > (count + 1) * ctx_size)
-			err = __gem_context_create(fd, &create);
+			err = __gem_context_create(fd, &ctx_id);
 		if (err) {
 			igt_info("Created %lu contexts, before failing with '%s' [%d]\n",
 				 count, strerror(-err), -err);
 			break;
 		}
 
-		contexts[count++] = create.ctx_id;
+		contexts[count++] = ctx_id;
 	} while (1);
 	igt_require(count);
 
@@ -347,7 +346,7 @@  igt_main
 		memset(&create, 0, sizeof(create));
 		create.ctx_id = rand();
 		create.pad = 0;
-		igt_assert_eq(__gem_context_create(fd, &create), 0);
+		igt_assert_eq(__gem_context_create2(fd, &create), 0);
 		igt_assert(create.ctx_id != 0);
 		gem_context_destroy(fd, create.ctx_id);
 	}
@@ -356,7 +355,7 @@  igt_main
 		memset(&create, 0, sizeof(create));
 		create.ctx_id = rand();
 		create.pad = 1;
-		igt_assert_eq(__gem_context_create(fd, &create), -EINVAL);
+		igt_assert_eq(__gem_context_create2(fd, &create), -EINVAL);
 	}
 
 	igt_subtest("maximum-mem")
diff --git a/tests/gem_ctx_switch.c b/tests/gem_ctx_switch.c
index b6ea71cf..43105fea 100644
--- a/tests/gem_ctx_switch.c
+++ b/tests/gem_ctx_switch.c
@@ -45,19 +45,6 @@ 
 
 #define INTERRUPTIBLE 1
 
-static int __gem_context_create(int fd, uint32_t *ctx_id)
-{
-	struct drm_i915_gem_context_create arg;
-	int ret = 0;
-
-	memset(&arg, 0, sizeof(arg));
-	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &arg))
-		ret = -errno;
-
-	*ctx_id = arg.ctx_id;
-	return ret;
-}
-
 static double elapsed(const struct timespec *start, const struct timespec *end)
 {
 	return ((end->tv_sec - start->tv_sec) +
diff --git a/tests/gem_eio.c b/tests/gem_eio.c
index 608b2dfd..138e8a17 100644
--- a/tests/gem_eio.c
+++ b/tests/gem_eio.c
@@ -207,17 +207,6 @@  static void test_inflight(int fd)
 	}
 }
 
-static uint32_t __gem_context_create(int fd)
-{
-	struct drm_i915_gem_context_create create;
-
-	memset(&create, 0, sizeof(create));
-	if (ioctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &create))
-		return 0;
-
-	return create.ctx_id;
-}
-
 static void test_inflight_contexts(int fd)
 {
 	struct drm_i915_gem_execbuffer2 execbuf;
@@ -229,7 +218,7 @@  static void test_inflight_contexts(int fd)
 
 	igt_require(gem_has_exec_fence(fd));
 
-	ctx[0] = __gem_context_create(fd);
+	ctx[0] = gem_context_create(fd);
 	igt_require(ctx[0]);
 	for (unsigned int n = 1; n < ARRAY_SIZE(ctx); n++)
 		ctx[n] = gem_context_create(fd);
diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
index fb5c0f30..3a14882e 100644
--- a/tests/gem_exec_await.c
+++ b/tests/gem_exec_await.c
@@ -55,15 +55,6 @@  static bool ignore_engine(int fd, unsigned engine)
 	return false;
 }
 
-static uint32_t __gem_context_create(int fd)
-{
-	struct drm_i915_gem_context_create arg;
-
-	memset(&arg, 0, sizeof(arg));
-	drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &arg);
-	return arg.ctx_id;
-}
-
 static void xchg_obj(void *array, unsigned i, unsigned j)
 {
 	struct drm_i915_gem_exec_object2 *obj = array;
@@ -130,8 +121,7 @@  static void wide(int fd, int ring_size, int timeout, unsigned int flags)
 					 LOCAL_I915_EXEC_HANDLE_LUT);
 
 		if (flags & CONTEXTS) {
-			exec[e].execbuf.rsvd1 = __gem_context_create(fd);
-			igt_require(exec[e].execbuf.rsvd1);
+			exec[e].execbuf.rsvd1 = gem_context_create(fd);
 		}
 
 		exec[e].exec[0].handle = gem_create(fd, 4096);
@@ -174,7 +164,7 @@  static void wide(int fd, int ring_size, int timeout, unsigned int flags)
 
 			if (flags & CONTEXTS) {
 				gem_context_destroy(fd, exec[e].execbuf.rsvd1);
-				exec[e].execbuf.rsvd1 = __gem_context_create(fd);
+				exec[e].execbuf.rsvd1 = gem_context_create(fd);
 			}
 
 			exec[e].reloc.presumed_offset = exec[e].exec[1].offset;
diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index 0af64557..151b39db 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -365,19 +365,6 @@  static void xchg(void *array, unsigned i, unsigned j)
 	u[j] = tmp;
 }
 
-static int __gem_context_create(int fd, uint32_t *ctx_id)
-{
-	struct drm_i915_gem_context_create arg;
-	int ret = 0;
-
-	memset(&arg, 0, sizeof(arg));
-	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &arg))
-		ret = -errno;
-
-	*ctx_id = arg.ctx_id;
-	return ret;
-}
-
 static void sequential(int fd, uint32_t handle, unsigned flags, int timeout)
 {
 	const int ncpus = flags & FORKED ? sysconf(_SC_NPROCESSORS_ONLN) : 1;
diff --git a/tests/gem_exec_parallel.c b/tests/gem_exec_parallel.c
index 1c53bd64..5c0b027b 100644
--- a/tests/gem_exec_parallel.c
+++ b/tests/gem_exec_parallel.c
@@ -55,20 +55,11 @@  static void check_bo(int fd, uint32_t handle, int pass)
 	munmap(map, 4096);
 }
 
-static uint32_t __gem_context_create(int fd)
-{
-	struct drm_i915_gem_context_create arg;
-
-	memset(&arg, 0, sizeof(arg));
-	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &arg) == 0)
-		gem_context_destroy(fd, arg.ctx_id);
-
-	return arg.ctx_id;
-}
-
 static void gem_require_context(int fd)
 {
-	igt_require(__gem_context_create(fd));
+	uint32_t ctx_id = 0;
+	__gem_context_create(fd, &ctx_id);
+	igt_require(ctx_id);
 }
 
 static bool ignore_engine(int fd, unsigned engine)
diff --git a/tests/gem_exec_reuse.c b/tests/gem_exec_reuse.c
index 4e3907cf..11185de1 100644
--- a/tests/gem_exec_reuse.c
+++ b/tests/gem_exec_reuse.c
@@ -56,19 +56,6 @@  static void noop(struct noop *n,
 	gem_execbuf(n->fd, &execbuf);
 }
 
-static int __gem_context_create(int fd, uint32_t *ctx_id)
-{
-	struct drm_i915_gem_context_create arg;
-	int ret = 0;
-
-	memset(&arg, 0, sizeof(arg));
-	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &arg))
-		ret = -errno;
-
-	*ctx_id = arg.ctx_id;
-	return ret;
-}
-
 static int fls(uint64_t x)
 {
 	int t;
diff --git a/tests/gem_exec_whisper.c b/tests/gem_exec_whisper.c
index 15989616..00eb4c8c 100644
--- a/tests/gem_exec_whisper.c
+++ b/tests/gem_exec_whisper.c
@@ -79,19 +79,6 @@  static void verify_reloc(int fd, uint32_t handle,
 	}
 }
 
-static int __gem_context_create(int fd, uint32_t *ctx_id)
-{
-	struct drm_i915_gem_context_create arg;
-	int ret = 0;
-
-	memset(&arg, 0, sizeof(arg));
-	if (drmIoctl(fd, DRM_IOCTL_I915_GEM_CONTEXT_CREATE, &arg))
-		ret = -errno;
-
-	*ctx_id = arg.ctx_id;
-	return ret;
-}
-
 static bool ignore_engine(int fd, unsigned engine)
 {
 	if (engine == 0)