[PATCH i-g-t 0/8] extract cork and ring measure code

[PATCH i-g-t 0/8] extract cork and ring measure code

[Daniele Ceraolo Spurio]

The "cork" bo used to stall execution and the interruptible execbuf
used to measure the ring size are repeated almost identically in
several files. Extracting those to common files helps simplifying
the code and avoiding repetition.

Cc: Chris Wilson <chris@chris-wilson.co.uk>

Daniele Ceraolo Spurio (8):
  lib/igt_dummyload: add igt_cork
  lib/igt_gt: add intel_measure_ring_size
  tests/gem_exec_schedule: use new common functions
  tests/gem_exec_fence: use new common functions
  tests/gem_exec_latency: use new common functions
  tests/gem_wait: use igt_cork
  tests/gem_exec_await: use intel_measure_ring_size
  tests/gem_ringfill: use intel_measure_ring_size

 lib/igt_dummyload.c       |  75 ++++++++++++++++++++++
 lib/igt_dummyload.h       |  11 ++++
 lib/igt_gt.c              |  83 ++++++++++++++++++++++++
 lib/igt_gt.h              |   2 +
 tests/gem_exec_await.c    |  90 +-------------------------
 tests/gem_exec_fence.c    | 115 +++------------------------------
 tests/gem_exec_latency.c  |  95 +++------------------------
 tests/gem_exec_schedule.c | 159 ++++++++++------------------------------------
 tests/gem_ringfill.c      |  96 +---------------------------
 tests/gem_wait.c          |  52 +++------------
 10 files changed, 235 insertions(+), 543 deletions(-)

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Daniele Ceraolo Spurio]

The "cork" bo (imported bo with attached fence) is used in several
tests to stall execution. Moving it to a common place makes the codebase
cleaner.

Note that the actual test updates is done in follow up patches as it is
simpler to do in one go after one more common function is added in the
next patch.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/igt_dummyload.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_dummyload.h | 11 ++++++++
 2 files changed, 86 insertions(+)

diff --git a/lib/igt_dummyload.c b/lib/igt_dummyload.c
index bb2be55..913cc93 100644
--- a/lib/igt_dummyload.c
+++ b/lib/igt_dummyload.c
@@ -29,6 +29,7 @@
 #include <i915_drm.h>
 
 #include "igt_core.h"
+#include "drmtest.h"
 #include "igt_dummyload.h"
 #include "igt_gt.h"
 #include "intel_chipset.h"
@@ -300,3 +301,77 @@ void igt_terminate_spin_batches(void)
 	igt_list_for_each(iter, &spin_list, link)
 		igt_spin_batch_end(iter);
 }
+
+/**
+ * igt_cork_new
+ * @fd: open drm file descriptor
+ *
+ * Imports a vgem bo with a fence attached to it. This bo can be used as a
+ * dependency during submission to stall execution until the fence is signaled.
+ *
+ * Returns:
+ * Structure with the handle of the imported bo and helper internal state
+ * for igt_cork_signal() and igt_cork_free().
+ */
+igt_cork_t *igt_cork_new(int fd)
+{
+	igt_cork_t *cork;
+	struct vgem_bo bo;
+	int dmabuf;
+
+	cork = calloc(1, sizeof(igt_cork_t));
+	igt_assert(cork);
+
+	cork->device = drm_open_driver(DRIVER_VGEM);
+
+	igt_require(vgem_has_fences(cork->device));
+
+	bo.width = bo.height = 1;
+	bo.bpp = 4;
+	vgem_create(cork->device, &bo);
+	cork->fence = vgem_fence_attach(cork->device, &bo, VGEM_FENCE_WRITE);
+
+	dmabuf = prime_handle_to_fd(cork->device, bo.handle);
+	cork->handle = prime_fd_to_handle(fd, dmabuf);
+	close(dmabuf);
+
+	return cork;
+}
+
+/**
+ * igt_cork_signal:
+ * @cork: cork state from igt_cork_new()
+ *
+ * This function signals the fence attached to the imported object, thus
+ * unblocking any stalled execution.
+ */
+void igt_cork_signal(igt_cork_t *cork)
+{
+	if (!cork)
+		return;
+
+	vgem_fence_signal(cork->device, cork->fence);
+	close(cork->device);
+	cork->device = 0;
+}
+
+/**
+ * igt_cork_free:
+ * @cork: cork state from igt_cork_new()
+ * @fd: open drm file descriptor
+ *
+ * This function signals the fence attached to the imported object (if it
+ * hasn't already been signaled by igt_cork_signal) and does the necessary
+ * post-processing.
+ */
+void igt_cork_free(int fd, igt_cork_t *cork)
+{
+	if (!cork)
+		return;
+
+	if (cork->device)
+		igt_cork_signal(cork);
+
+	gem_close(fd, cork->handle);
+	free(cork);
+}
diff --git a/lib/igt_dummyload.h b/lib/igt_dummyload.h
index 215425f..d20a867 100644
--- a/lib/igt_dummyload.h
+++ b/lib/igt_dummyload.h
@@ -29,6 +29,7 @@
 #include <time.h>
 
 #include "igt_aux.h"
+#include "igt_vgem.h"
 
 typedef struct igt_spin {
 	unsigned int handle;
@@ -51,4 +52,14 @@ void igt_spin_batch_free(int fd, igt_spin_t *spin);
 
 void igt_terminate_spin_batches(void);
 
+typedef struct igt_cork {
+	int device;
+	uint32_t handle;
+	uint32_t fence;
+} igt_cork_t;
+
+igt_cork_t *igt_cork_new(int fd);
+void igt_cork_signal(igt_cork_t *cork);
+void igt_cork_free(int fd, igt_cork_t *cork);
+
 #endif /* __IGT_DUMMYLOAD_H__ */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 2/8] lib/igt_gt: add intel_measure_ring_size

[Daniele Ceraolo Spurio]

The logic to measure the ring size is replicated almost identically in
several tests. Adding it as a common function will make the code
cleaner.

The tests are updated in follow up patches.

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 lib/igt_gt.c | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_gt.h |  2 ++
 2 files changed, 85 insertions(+)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index b3f3b38..3b70941 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -40,6 +40,7 @@
 #include "ioctl_wrappers.h"
 #include "intel_reg.h"
 #include "intel_chipset.h"
+#include "igt_dummyload.h"
 
 /**
  * SECTION:igt_gt
@@ -537,6 +538,88 @@ unsigned intel_detect_and_clear_missed_interrupts(int fd)
 	return missed;
 }
 
+static void alarm_handler(int sig)
+{
+}
+
+/**
+ * intel_measure_ring_size:
+ * @fd: open i915 drm file descriptor
+ * @engine: execbuf engine flag
+ * @new_ctx: use a new context to account for the space used by the lrc init
+ *
+ * This function calculates the maximum number of batches that can be inserted
+ * at the same time in the ring on the selected engine.
+ *
+ * Returns:
+ * Number of batches that fit in the ring
+ */
+unsigned int intel_measure_ring_size(int fd, unsigned int engine, bool new_ctx)
+{
+	struct sigaction old_sa, sa = { .sa_handler = alarm_handler };
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_execbuffer2 execbuf;
+	const uint32_t bbe = MI_BATCH_BUFFER_END;
+	unsigned int count, last;
+	struct itimerval itv;
+	igt_cork_t *c;
+
+	igt_require_intel(fd);
+
+	memset(obj, 0, sizeof(obj));
+	obj[1].handle = gem_create(fd, 4096);
+	gem_write(fd, obj[1].handle, 0, &bbe, sizeof(bbe));
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(&obj[1]);
+	execbuf.buffer_count = 1;
+	execbuf.flags = engine;
+	gem_execbuf(fd, &execbuf);
+	gem_sync(fd, obj[1].handle);
+
+	c = igt_cork_new(fd);
+	obj[0].handle = c->handle;
+
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = 2;
+
+	if (new_ctx)
+		execbuf.rsvd1 = gem_context_create(fd);
+
+	sigaction(SIGALRM, &sa, &old_sa);
+	itv.it_interval.tv_sec = 0;
+	itv.it_interval.tv_usec = 100;
+	itv.it_value.tv_sec = 0;
+	itv.it_value.tv_usec = 1000;
+	setitimer(ITIMER_REAL, &itv, NULL);
+
+	last = -1;
+	count = 0;
+	do {
+		if (ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf) == 0) {
+			count++;
+			continue;
+		}
+
+		if (last == count)
+			break;
+
+		last = count;
+	} while (1);
+
+	memset(&itv, 0, sizeof(itv));
+	setitimer(ITIMER_REAL, &itv, NULL);
+	sigaction(SIGALRM, &old_sa, NULL);
+
+	igt_cork_free(fd, c);
+	gem_close(fd, obj[1].handle);
+
+	if (new_ctx)
+		gem_context_destroy(fd, execbuf.rsvd1);
+
+	return count;
+}
+
 const struct intel_execution_engine intel_execution_engines[] = {
 	{ "default", NULL, 0, 0 },
 	{ "render", "rcs0", I915_EXEC_RENDER, 0 },
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index 2579cbd..4e4fc01 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -63,6 +63,8 @@ void igt_clflush_range(void *addr, int size);
 
 unsigned intel_detect_and_clear_missed_interrupts(int fd);
 
+unsigned int intel_measure_ring_size(int fd, unsigned int engine, bool new_ctx);
+
 extern const struct intel_execution_engine {
 	const char *name;
 	const char *full_name;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 3/8] tests/gem_exec_schedule: use new common functions

[Daniele Ceraolo Spurio]

With intel_measure_ring_size and igt_cork added as common utilities we
can use them instead of the local copy of those utilities

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_schedule.c | 159 ++++++++++------------------------------------
 1 file changed, 35 insertions(+), 124 deletions(-)

diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index ad3688c..0e022cb 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -129,35 +129,6 @@ static void store_dword(int fd, uint32_t ctx, unsigned ring,
 	gem_close(fd, obj[2].handle);
 }
 
-struct cork {
-	int device;
-	uint32_t handle;
-	uint32_t fence;
-};
-
-static void plug(int fd, struct cork *c)
-{
-	struct vgem_bo bo;
-	int dmabuf;
-
-	c->device = drm_open_driver(DRIVER_VGEM);
-
-	bo.width = bo.height = 1;
-	bo.bpp = 4;
-	vgem_create(c->device, &bo);
-	c->fence = vgem_fence_attach(c->device, &bo, VGEM_FENCE_WRITE);
-
-	dmabuf = prime_handle_to_fd(c->device, bo.handle);
-	c->handle = prime_fd_to_handle(fd, dmabuf);
-	close(dmabuf);
-}
-
-static void unplug(struct cork *c)
-{
-	vgem_fence_signal(c->device, c->fence);
-	close(c->device);
-}
-
 static uint32_t create_highest_priority(int fd)
 {
 	uint32_t ctx = gem_context_create(fd);
@@ -172,7 +143,7 @@ static uint32_t create_highest_priority(int fd)
 	return ctx;
 }
 
-static void unplug_show_queue(int fd, struct cork *c, unsigned int engine)
+static void unplug_show_queue(int fd, igt_cork_t *c, unsigned int engine)
 {
 	igt_spin_t *spin[BUSY_QLEN];
 
@@ -182,7 +153,7 @@ static void unplug_show_queue(int fd, struct cork *c, unsigned int engine)
 		gem_context_destroy(fd, ctx);
 	}
 
-	unplug(c); /* batches will now be queued on the engine */
+	igt_cork_free(fd, c); /* batches will now be queued on the engine */
 	igt_debugfs_dump(fd, "i915_engine_info");
 
 	for (int n = 0; n < ARRAY_SIZE(spin); n++)
@@ -192,19 +163,19 @@ static void unplug_show_queue(int fd, struct cork *c, unsigned int engine)
 
 static void fifo(int fd, unsigned ring)
 {
-	struct cork cork;
+	igt_cork_t *cork;
 	uint32_t scratch;
 	uint32_t *ptr;
 
 	scratch = gem_create(fd, 4096);
 
-	plug(fd, &cork);
+	cork = igt_cork_new(fd);
 
 	/* Same priority, same timeline, final result will be the second eb */
-	store_dword(fd, 0, ring, scratch, 0, 1, cork.handle, 0);
-	store_dword(fd, 0, ring, scratch, 0, 2, cork.handle, 0);
+	store_dword(fd, 0, ring, scratch, 0, 1, cork->handle, 0);
+	store_dword(fd, 0, ring, scratch, 0, 2, cork->handle, 0);
 
-	unplug_show_queue(fd, &cork, ring);
+	unplug_show_queue(fd, cork, ring);
 
 	ptr = gem_mmap__gtt(fd, scratch, 4096, PROT_READ);
 	gem_set_domain(fd, scratch, /* no write hazard lies! */
@@ -292,7 +263,7 @@ static void smoketest(int fd, unsigned ring, unsigned timeout)
 static void reorder(int fd, unsigned ring, unsigned flags)
 #define EQUAL 1
 {
-	struct cork cork;
+	igt_cork_t *cork;
 	uint32_t scratch;
 	uint32_t *ptr;
 	uint32_t ctx[2];
@@ -304,15 +275,15 @@ static void reorder(int fd, unsigned ring, unsigned flags)
 	ctx_set_priority(fd, ctx[HI], flags & EQUAL ? MIN_PRIO : 0);
 
 	scratch = gem_create(fd, 4096);
-	plug(fd, &cork);
+	cork = igt_cork_new(fd);
 
 	/* We expect the high priority context to be executed first, and
 	 * so the final result will be value from the low priority context.
 	 */
-	store_dword(fd, ctx[LO], ring, scratch, 0, ctx[LO], cork.handle, 0);
-	store_dword(fd, ctx[HI], ring, scratch, 0, ctx[HI], cork.handle, 0);
+	store_dword(fd, ctx[LO], ring, scratch, 0, ctx[LO], cork->handle, 0);
+	store_dword(fd, ctx[HI], ring, scratch, 0, ctx[HI], cork->handle, 0);
 
-	unplug_show_queue(fd, &cork, ring);
+	unplug_show_queue(fd, cork, ring);
 
 	gem_context_destroy(fd, ctx[LO]);
 	gem_context_destroy(fd, ctx[HI]);
@@ -331,7 +302,7 @@ static void reorder(int fd, unsigned ring, unsigned flags)
 
 static void promotion(int fd, unsigned ring)
 {
-	struct cork cork;
+	igt_cork_t *cork;
 	uint32_t result, dep;
 	uint32_t *ptr;
 	uint32_t ctx[3];
@@ -348,15 +319,15 @@ static void promotion(int fd, unsigned ring)
 	result = gem_create(fd, 4096);
 	dep = gem_create(fd, 4096);
 
-	plug(fd, &cork);
+	cork = igt_cork_new(fd);
 
 	/* Expect that HI promotes LO, so the order will be LO, HI, NOISE.
 	 *
 	 * fifo would be NOISE, LO, HI.
 	 * strict priority would be  HI, NOISE, LO
 	 */
-	store_dword(fd, ctx[NOISE], ring, result, 0, ctx[NOISE], cork.handle, 0);
-	store_dword(fd, ctx[LO], ring, result, 0, ctx[LO], cork.handle, 0);
+	store_dword(fd, ctx[NOISE], ring, result, 0, ctx[NOISE], cork->handle, 0);
+	store_dword(fd, ctx[LO], ring, result, 0, ctx[LO], cork->handle, 0);
 
 	/* link LO <-> HI via a dependency on another buffer */
 	store_dword(fd, ctx[LO], ring, dep, 0, ctx[LO], 0, I915_GEM_DOMAIN_INSTRUCTION);
@@ -364,7 +335,7 @@ static void promotion(int fd, unsigned ring)
 
 	store_dword(fd, ctx[HI], ring, result, 0, ctx[HI], 0, 0);
 
-	unplug_show_queue(fd, &cork, ring);
+	unplug_show_queue(fd, cork, ring);
 
 	gem_context_destroy(fd, ctx[NOISE]);
 	gem_context_destroy(fd, ctx[LO]);
@@ -544,7 +515,7 @@ static void deep(int fd, unsigned ring)
 	const unsigned int nctx = MAX_PRIO - MIN_PRIO;
 	const unsigned size = ALIGN(4*nctx, 4096);
 	struct timespec tv = {};
-	struct cork cork;
+	igt_cork_t *cork;
 	uint32_t result, dep[XS];
 	uint32_t expected = 0;
 	uint32_t *ptr;
@@ -585,12 +556,12 @@ static void deep(int fd, unsigned ring)
 		gem_sync(fd, result);
 	}
 
-	plug(fd, &cork);
+	cork = igt_cork_new(fd);
 
 	/* Create a deep dependency chain, with a few branches */
 	for (int n = 0; n < nctx && igt_seconds_elapsed(&tv) < 8; n++)
 		for (int m = 0; m < XS; m++)
-			store_dword(fd, ctx[n], ring, dep[m], 4*n, ctx[n], cork.handle, I915_GEM_DOMAIN_INSTRUCTION);
+			store_dword(fd, ctx[n], ring, dep[m], 4*n, ctx[n], cork->handle, I915_GEM_DOMAIN_INSTRUCTION);
 
 	for (int n = 0; n < nctx && igt_seconds_elapsed(&tv) < 6; n++) {
 		for (int m = 0; m < XS; m++) {
@@ -600,7 +571,7 @@ static void deep(int fd, unsigned ring)
 		expected = ctx[n];
 	}
 
-	unplug_show_queue(fd, &cork, ring);
+	unplug_show_queue(fd, cork, ring);
 	igt_require(expected); /* too slow */
 
 	for (int n = 0; n < nctx; n++)
@@ -643,72 +614,13 @@ static int __execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf)
 	return err;
 }
 
-static unsigned int measure_ring_size(int fd, unsigned int ring)
-{
-	struct sigaction sa = { .sa_handler = alarm_handler };
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_execbuffer2 execbuf;
-	const uint32_t bbe = MI_BATCH_BUFFER_END;
-	unsigned int count, last;
-	struct itimerval itv;
-	struct cork c;
-
-	memset(obj, 0, sizeof(obj));
-	obj[1].handle = gem_create(fd, 4096);
-	gem_write(fd, obj[1].handle, 0, &bbe, sizeof(bbe));
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj + 1);
-	execbuf.buffer_count = 1;
-	execbuf.flags = ring;
-	gem_execbuf(fd, &execbuf);
-	gem_sync(fd, obj[1].handle);
-
-	plug(fd, &c);
-	obj[0].handle = c.handle;
-
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-	execbuf.rsvd1 = gem_context_create(fd);
-
-	sigaction(SIGALRM, &sa, NULL);
-	itv.it_interval.tv_sec = 0;
-	itv.it_interval.tv_usec = 100;
-	itv.it_value.tv_sec = 0;
-	itv.it_value.tv_usec = 1000;
-	setitimer(ITIMER_REAL, &itv, NULL);
-
-	last = -1;
-	count = 0;
-	do {
-		if (__execbuf(fd, &execbuf) == 0) {
-			count++;
-			continue;
-		}
-
-		if (last == count)
-			break;
-
-		last = count;
-	} while (1);
-
-	memset(&itv, 0, sizeof(itv));
-	setitimer(ITIMER_REAL, &itv, NULL);
-
-	unplug(&c);
-	gem_close(fd, obj[1].handle);
-	gem_context_destroy(fd, execbuf.rsvd1);
-
-	return count;
-}
-
 static void wide(int fd, unsigned ring)
 {
 #define NCTX 4096
 	struct timespec tv = {};
-	unsigned int ring_size = measure_ring_size(fd, ring);
+	unsigned int ring_size = intel_measure_ring_size(fd, ring, true);
 
-	struct cork cork;
+	igt_cork_t *cork;
 	uint32_t result;
 	uint32_t *ptr;
 	uint32_t *ctx;
@@ -720,20 +632,20 @@ static void wide(int fd, unsigned ring)
 
 	result = gem_create(fd, 4*NCTX);
 
-	plug(fd, &cork);
+	cork = igt_cork_new(fd);
 
 	/* Lots of in-order requests, plugged and submitted simultaneously */
 	for (count = 0;
 	     igt_seconds_elapsed(&tv) < 5 && count < ring_size;
 	     count++) {
 		for (int n = 0; n < NCTX; n++) {
-			store_dword(fd, ctx[n], ring, result, 4*n, ctx[n], cork.handle, I915_GEM_DOMAIN_INSTRUCTION);
+			store_dword(fd, ctx[n], ring, result, 4*n, ctx[n], cork->handle, I915_GEM_DOMAIN_INSTRUCTION);
 		}
 	}
 	igt_info("Submitted %d requests over %d contexts in %.1fms\n",
 		 count, NCTX, igt_nsec_elapsed(&tv) * 1e-6);
 
-	unplug_show_queue(fd, &cork, ring);
+	unplug_show_queue(fd, cork, ring);
 
 	for (int n = 0; n < NCTX; n++)
 		gem_context_destroy(fd, ctx[n]);
@@ -757,21 +669,21 @@ static void reorder_wide(int fd, unsigned ring)
 	struct drm_i915_gem_exec_object2 obj[3];
 	struct drm_i915_gem_execbuffer2 execbuf;
 	struct timespec tv = {};
-	unsigned int ring_size = measure_ring_size(fd, ring);
-	struct cork cork;
+	unsigned int ring_size = intel_measure_ring_size(fd, ring, true);
+	igt_cork_t *cork;
 	uint32_t result, target;
 	uint32_t *found, *expected;
 
 	result = gem_create(fd, 4096);
 	target = gem_create(fd, 4096);
 
-	plug(fd, &cork);
+	cork = igt_cork_new(fd);
 
 	expected = gem_mmap__cpu(fd, target, 0, 4096, PROT_WRITE);
 	gem_set_domain(fd, target, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
 
 	memset(obj, 0, sizeof(obj));
-	obj[0].handle = cork.handle;
+	obj[0].handle = cork->handle;
 	obj[1].handle = result;
 	obj[2].relocs_ptr = to_user_pointer(&reloc);
 	obj[2].relocation_count = 1;
@@ -838,7 +750,7 @@ static void reorder_wide(int fd, unsigned ring)
 		gem_context_destroy(fd, execbuf.rsvd1);
 	}
 
-	unplug_show_queue(fd, &cork, ring);
+	unplug_show_queue(fd, cork, ring);
 
 	found = gem_mmap__gtt(fd, result, 4096, PROT_READ);
 	gem_set_domain(fd, result, /* no write hazard lies! */
@@ -873,7 +785,7 @@ static void test_pi_ringfull(int fd, unsigned int engine)
 	struct drm_i915_gem_exec_object2 obj[2];
 	unsigned int last, count;
 	struct itimerval itv;
-	struct cork c;
+	igt_cork_t *c;
 	bool *result;
 
 	result = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
@@ -895,8 +807,8 @@ static void test_pi_ringfull(int fd, unsigned int engine)
 	gem_sync(fd, obj[1].handle);
 
 	/* Fill the low-priority ring */
-	plug(fd, &c);
-	obj[0].handle = c.handle;
+	c = igt_cork_new(fd);
+	obj[0].handle = c->handle;
 
 	execbuf.buffers_ptr = to_user_pointer(obj);
 	execbuf.buffer_count = 2;
@@ -970,12 +882,11 @@ static void test_pi_ringfull(int fd, unsigned int engine)
 	igt_assert_f(result[2],
 		     "High priority child unable to submit within 10ms\n");
 
-	unplug(&c);
+	igt_cork_free(fd, c);
 	igt_waitchildren();
 
 	gem_context_destroy(fd, execbuf.rsvd1);
 	gem_close(fd, obj[1].handle);
-	gem_close(fd, obj[0].handle);
 	munmap(result, 4096);
 }
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 4/8] tests/gem_exec_fence: use new common functions

[Daniele Ceraolo Spurio]

With intel_measure_ring_size and igt_cork added as common utilities we
can use them instead of the local copy of those utilities

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_fence.c | 115 +++++--------------------------------------------
 1 file changed, 10 insertions(+), 105 deletions(-)

diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index 477386b..c6b747d 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -321,101 +321,6 @@ static void resubmit(int fd, uint32_t handle, unsigned int ring, int count)
 		gem_execbuf(fd, &execbuf);
 }
 
-struct cork {
-	int device;
-	uint32_t handle;
-	uint32_t fence;
-};
-
-static void plug(int fd, struct cork *c)
-{
-	struct vgem_bo bo;
-	int dmabuf;
-
-	c->device = drm_open_driver(DRIVER_VGEM);
-
-	bo.width = bo.height = 1;
-	bo.bpp = 4;
-	vgem_create(c->device, &bo);
-	c->fence = vgem_fence_attach(c->device, &bo, VGEM_FENCE_WRITE);
-
-	dmabuf = prime_handle_to_fd(c->device, bo.handle);
-	c->handle = prime_fd_to_handle(fd, dmabuf);
-	close(dmabuf);
-}
-
-static void unplug(int fd, struct cork *c)
-{
-	vgem_fence_signal(c->device, c->fence);
-	gem_close(fd, c->handle);
-	close(c->device);
-}
-
-static void alarm_handler(int sig)
-{
-}
-
-static int __execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf)
-{
-	return ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, execbuf);
-}
-
-static unsigned int measure_ring_size(int fd)
-{
-	struct sigaction sa = { .sa_handler = alarm_handler };
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_execbuffer2 execbuf;
-	const uint32_t bbe = MI_BATCH_BUFFER_END;
-	unsigned int count, last;
-	struct itimerval itv;
-	struct cork c;
-
-	memset(obj, 0, sizeof(obj));
-	obj[1].handle = gem_create(fd, 4096);
-	gem_write(fd, obj[1].handle, 0, &bbe, sizeof(bbe));
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(&obj[1]);
-	execbuf.buffer_count = 1;
-	gem_execbuf(fd, &execbuf);
-	gem_sync(fd, obj[1].handle);
-
-	plug(fd, &c);
-	obj[0].handle = c.handle;
-
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-
-	sigaction(SIGALRM, &sa, NULL);
-	itv.it_interval.tv_sec = 0;
-	itv.it_interval.tv_usec = 100;
-	itv.it_value.tv_sec = 0;
-	itv.it_value.tv_usec = 1000;
-	setitimer(ITIMER_REAL, &itv, NULL);
-
-	last = -1;
-	count = 0;
-	do {
-		if (__execbuf(fd, &execbuf) == 0) {
-			count++;
-			continue;
-		}
-
-		if (last == count)
-			break;
-
-		last = count;
-	} while (1);
-
-	memset(&itv, 0, sizeof(itv));
-	setitimer(ITIMER_REAL, &itv, NULL);
-
-	unplug(fd, &c);
-	gem_close(fd, obj[1].handle);
-
-	return count;
-}
-
 static void test_parallel(int fd, unsigned int master)
 {
 	const int SCRATCH = 0;
@@ -430,15 +335,15 @@ static void test_parallel(int fd, unsigned int master)
 	uint32_t batch[16];
 	igt_spin_t *spin;
 	unsigned engine;
-	struct cork c;
+	igt_cork_t *c;
 	int i, x = 0;
 
-	plug(fd, &c);
+	c = igt_cork_new(fd);
 
 	/* Fill the queue with many requests so that the next one has to
 	 * wait before it can be executed by the hardware.
 	 */
-	spin = igt_spin_batch_new(fd, 0, master, c.handle);
+	spin = igt_spin_batch_new(fd, 0, master, c->handle);
 	resubmit(fd, spin->handle, master, 16);
 
 	/* Now queue the master request and its secondaries */
@@ -559,7 +464,7 @@ static void test_parallel(int fd, unsigned int master)
 	}
 
 	/* Unblock the master */
-	unplug(fd, &c);
+	igt_cork_free(fd, c);
 	igt_spin_batch_end(spin);
 
 	/* Wait for all secondaries to complete. If we used a regular fence
@@ -597,7 +502,7 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
 	unsigned int nengine, n, s;
 	unsigned long limit;
 	int all_fences;
-	struct cork c;
+	igt_cork_t *c;
 
 	limit = -1;
 	if (!gem_uses_full_ppgtt(fd))
@@ -632,8 +537,8 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
 	execbuf.buffers_ptr = to_user_pointer(obj);
 	execbuf.buffer_count = 2;
 
-	plug(fd, &c);
-	obj[0].handle = c.handle;
+	c = igt_cork_new(fd);
+	obj[0].handle = c->handle;
 
 	igt_until_timeout(5) {
 		execbuf.rsvd1 = gem_context_create(fd);
@@ -661,7 +566,7 @@ static void test_long_history(int fd, long ring_size, unsigned flags)
 		if (!--limit)
 			break;
 	}
-	unplug(fd, &c);
+	igt_cork_free(fd, c);
 
 	igt_info("History depth = %d\n", sync_fence_count(all_fences));
 
@@ -1536,7 +1441,7 @@ igt_main
 	}
 
 	igt_subtest("long-history") {
-		long ring_size = measure_ring_size(i915) - 1;
+		long ring_size = intel_measure_ring_size(i915, 0, false) - 1;
 
 		igt_info("Ring size: %ld batches\n", ring_size);
 		igt_require(ring_size);
@@ -1545,7 +1450,7 @@ igt_main
 	}
 
 	igt_subtest("expired-history") {
-		long ring_size = measure_ring_size(i915) - 1;
+		long ring_size = intel_measure_ring_size(i915, 0, false) - 1;
 
 		igt_info("Ring size: %ld batches\n", ring_size);
 		igt_require(ring_size);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 5/8] tests/gem_exec_latency: use new common functions

[Daniele Ceraolo Spurio]

With intel_measure_ring_size and igt_cork added as common utilities we
can use them instead of the local copy of those utilities

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_latency.c | 99 +++++++-----------------------------------------
 1 file changed, 13 insertions(+), 86 deletions(-)

diff --git a/tests/gem_exec_latency.c b/tests/gem_exec_latency.c
index f86dfcb..02dbb00 100644
--- a/tests/gem_exec_latency.c
+++ b/tests/gem_exec_latency.c
@@ -54,83 +54,6 @@
 
 static unsigned int ring_size;
 
-struct cork {
-	int device;
-	uint32_t handle;
-	uint32_t fence;
-};
-
-static void plug(int fd, struct cork *c)
-{
-	struct vgem_bo bo;
-	int dmabuf;
-
-	c->device = drm_open_driver(DRIVER_VGEM);
-
-	bo.width = bo.height = 1;
-	bo.bpp = 4;
-	vgem_create(c->device, &bo);
-	c->fence = vgem_fence_attach(c->device, &bo, VGEM_FENCE_WRITE);
-
-	dmabuf = prime_handle_to_fd(c->device, bo.handle);
-	c->handle = prime_fd_to_handle(fd, dmabuf);
-	close(dmabuf);
-}
-
-static void unplug(struct cork *c)
-{
-	vgem_fence_signal(c->device, c->fence);
-	close(c->device);
-}
-
-static void alarm_handler(int sig)
-{
-}
-
-static void set_timeout(int seconds)
-{
-	struct sigaction sa = { .sa_handler = alarm_handler };
-
-	sigaction(SIGALRM, seconds ? &sa : NULL, NULL);
-	alarm(seconds);
-}
-
-static int __execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf)
-{
-	return ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, execbuf);
-}
-
-static unsigned int measure_ring_size(int fd)
-{
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_execbuffer2 execbuf;
-	const uint32_t bbe = MI_BATCH_BUFFER_END;
-	unsigned int count;
-	struct cork c;
-
-	memset(obj, 0, sizeof(obj));
-	obj[1].handle = gem_create(fd, 4096);
-	gem_write(fd, obj[1].handle, 0, &bbe, sizeof(bbe));
-
-	plug(fd, &c);
-	obj[0].handle = c.handle;
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-
-	count = 0;
-	set_timeout(1);
-	while (__execbuf(fd, &execbuf) == 0)
-		count++;
-	set_timeout(0);
-
-	unplug(&c);
-	gem_close(fd, obj[1].handle);
-
-	return count;
-}
-
 #define RCS_TIMESTAMP (0x2000 + 0x358)
 static void latency_on_ring(int fd,
 			    unsigned ring, const char *name,
@@ -141,7 +64,7 @@ static void latency_on_ring(int fd,
 	struct drm_i915_gem_exec_object2 obj[3];
 	struct drm_i915_gem_relocation_entry reloc;
 	struct drm_i915_gem_execbuffer2 execbuf;
-	struct cork c;
+	igt_cork_t *c;
 	volatile uint32_t *reg;
 	unsigned repeats = ring_size;
 	uint32_t start, end, *map, *results;
@@ -205,8 +128,8 @@ static void latency_on_ring(int fd,
 	}
 
 	if (flags & CORK) {
-		plug(fd, &c);
-		obj[0].handle = c.handle;
+		c = igt_cork_new(fd);
+		obj[0].handle = c->handle;
 		execbuf.buffers_ptr = to_user_pointer(&obj[0]);
 		execbuf.buffer_count = 3;
 	}
@@ -227,7 +150,7 @@ static void latency_on_ring(int fd,
 	igt_assert(reloc.presumed_offset == obj[1].offset);
 
 	if (flags & CORK)
-		unplug(&c);
+		igt_cork_signal(c);
 
 	gem_set_domain(fd, obj[1].handle, I915_GEM_DOMAIN_GTT, 0);
 	gpu_latency = (results[repeats-1] - results[0]) / (double)(repeats-1);
@@ -268,6 +191,10 @@ static void latency_on_ring(int fd,
 
 	munmap(map, 64*1024);
 	munmap(results, 4096);
+
+	if (flags & CORK)
+		igt_cork_free(fd, c);
+
 	gem_close(fd, obj[1].handle);
 	gem_close(fd, obj[2].handle);
 }
@@ -319,7 +246,7 @@ static void latency_from_ring(int fd,
 	reloc.target_handle = flags & CORK ? 1 : 0;
 
 	for (e = intel_execution_engines; e->name; e++) {
-		struct cork c;
+		igt_cork_t *c;
 
 		if (e->exec_id == 0)
 			continue;
@@ -332,8 +259,8 @@ static void latency_from_ring(int fd,
 			       I915_GEM_DOMAIN_GTT);
 
 		if (flags & CORK) {
-			plug(fd, &c);
-			obj[0].handle = c.handle;
+			c = igt_cork_new(fd);
+			obj[0].handle = c->handle;
 			execbuf.buffers_ptr = to_user_pointer(&obj[0]);
 			execbuf.buffer_count = 3;
 		}
@@ -391,7 +318,7 @@ static void latency_from_ring(int fd,
 		}
 
 		if (flags & CORK)
-			unplug(&c);
+			igt_cork_free(fd, c);
 
 		gem_set_domain(fd, obj[1].handle,
 			       I915_GEM_DOMAIN_GTT,
@@ -448,7 +375,7 @@ igt_main
 
 		print_welcome(device);
 
-		ring_size = measure_ring_size(device);
+		ring_size = intel_measure_ring_size(device, 0, false);
 		igt_info("Ring size: %d batches\n", ring_size);
 		igt_require(ring_size > 8);
 		ring_size -= 8; /* leave some spare */
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 6/8] tests/gem_wait: use igt_cork

[Daniele Ceraolo Spurio]

With igt_cork added as common utility we can use it instead of the
local copy

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_wait.c | 52 ++++++++--------------------------------------------
 1 file changed, 8 insertions(+), 44 deletions(-)

diff --git a/tests/gem_wait.c b/tests/gem_wait.c
index cf8c815..5fba233 100644
--- a/tests/gem_wait.c
+++ b/tests/gem_wait.c
@@ -69,51 +69,14 @@ static void invalid_buf(int fd)
 #define AWAIT 4
 #define WRITE 8
 
-struct cork {
-	int device;
-	uint32_t handle;
-	uint32_t fence;
-};
-
-static struct cork plug(int fd, unsigned flags)
-{
-	struct cork c;
-	struct vgem_bo bo;
-	int dmabuf;
-
-	if ((flags & (WRITE | AWAIT)) == 0)
-		return (struct cork){0};
-
-	c.device = drm_open_driver(DRIVER_VGEM);
-
-	bo.width = bo.height = 1;
-	bo.bpp = 4;
-	vgem_create(c.device, &bo);
-	c.fence = vgem_fence_attach(c.device, &bo, VGEM_FENCE_WRITE);
-
-	dmabuf = prime_handle_to_fd(c.device, bo.handle);
-	c.handle = prime_fd_to_handle(fd, dmabuf);
-	close(dmabuf);
-
-	return c;
-}
-
-static void unplug(struct cork *c)
-{
-	if (!c->device)
-		return;
-
-	vgem_fence_signal(c->device, c->fence);
-	close(c->device);
-}
-
 static void basic(int fd, unsigned engine, unsigned flags)
 {
-	struct cork cork = plug(fd, flags);
-	igt_spin_t *spin = igt_spin_batch_new(fd, 0, engine, cork.handle);
+	igt_cork_t *cork = flags & (WRITE | AWAIT) ? igt_cork_new(fd) : NULL;
+	igt_spin_t *spin = igt_spin_batch_new(fd, 0, engine,
+					      cork ? cork->handle : 0);
 	struct drm_i915_gem_wait wait = {
-	       	flags & WRITE ? cork.handle : spin->handle
-       	};
+		flags & WRITE ? cork->handle : spin->handle
+	};
 
 	igt_assert_eq(__gem_wait(fd, &wait), -ETIME);
 
@@ -127,7 +90,7 @@ static void basic(int fd, unsigned engine, unsigned flags)
 			timeout = 1;
 		}
 
-		unplug(&cork);
+		igt_cork_signal(cork);
 		igt_assert_eq(__gem_wait(fd, &wait), -ETIME);
 
 		while (__gem_wait(fd, &wait) == -ETIME)
@@ -137,7 +100,7 @@ static void basic(int fd, unsigned engine, unsigned flags)
 		igt_assert_eq(__gem_wait(fd, &wait), -ETIME);
 		igt_assert_eq_s64(wait.timeout_ns, 0);
 
-		unplug(&cork);
+		igt_cork_signal(cork);
 		wait.timeout_ns = 0;
 		igt_assert_eq(__gem_wait(fd, &wait), -ETIME);
 
@@ -157,6 +120,7 @@ static void basic(int fd, unsigned engine, unsigned flags)
 		igt_assert(wait.timeout_ns == 0);
 	}
 
+	igt_cork_free(fd, cork);
 	igt_spin_batch_free(fd, spin);
 }
 
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 7/8] tests/gem_exec_await: use intel_measure_ring_size

[Daniele Ceraolo Spurio]

With intel_measure_ring_size added as common function we can use it
instead of the local copy

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_await.c | 90 +-------------------------------------------------
 1 file changed, 1 insertion(+), 89 deletions(-)

diff --git a/tests/gem_exec_await.c b/tests/gem_exec_await.c
index fb5c0f3..b896901 100644
--- a/tests/gem_exec_await.c
+++ b/tests/gem_exec_await.c
@@ -280,94 +280,6 @@ out:
 	return result;
 }
 
-struct cork {
-	int device;
-	uint32_t handle;
-	uint32_t fence;
-};
-
-static void plug(int fd, struct cork *c)
-{
-	struct vgem_bo bo;
-	int dmabuf;
-
-	c->device = drm_open_driver(DRIVER_VGEM);
-
-	bo.width = bo.height = 1;
-	bo.bpp = 4;
-	vgem_create(c->device, &bo);
-	c->fence = vgem_fence_attach(c->device, &bo, VGEM_FENCE_WRITE);
-
-	dmabuf = prime_handle_to_fd(c->device, bo.handle);
-	c->handle = prime_fd_to_handle(fd, dmabuf);
-	close(dmabuf);
-}
-
-static void unplug(struct cork *c)
-{
-	vgem_fence_signal(c->device, c->fence);
-	close(c->device);
-}
-
-static void alarm_handler(int sig)
-{
-}
-
-static int __execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf)
-{
-	return ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, execbuf);
-}
-
-static unsigned int measure_ring_size(int fd)
-{
-	struct sigaction sa = { .sa_handler = alarm_handler };
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_execbuffer2 execbuf;
-	const uint32_t bbe = MI_BATCH_BUFFER_END;
-	unsigned int count, last;
-	struct itimerval itv;
-	struct cork c;
-
-	memset(obj, 0, sizeof(obj));
-	obj[1].handle = gem_create(fd, 4096);
-	gem_write(fd, obj[1].handle, 0, &bbe, sizeof(bbe));
-
-	plug(fd, &c);
-	obj[0].handle = c.handle;
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-
-	sigaction(SIGALRM, &sa, NULL);
-	itv.it_interval.tv_sec = 0;
-	itv.it_interval.tv_usec = 100;
-	itv.it_value.tv_sec = 0;
-	itv.it_value.tv_usec = 1000;
-	setitimer(ITIMER_REAL, &itv, NULL);
-
-	last = count = 0;
-	do {
-		if (__execbuf(fd, &execbuf) == 0) {
-			count++;
-			continue;
-		}
-
-		if (last == count)
-			break;
-
-		last = count;
-	} while (1);
-
-	memset(&itv, 0, sizeof(itv));
-	setitimer(ITIMER_REAL, &itv, NULL);
-
-	unplug(&c);
-	gem_close(fd, obj[1].handle);
-
-	return count;
-}
-
 igt_main
 {
 	int ring_size = 0;
@@ -380,7 +292,7 @@ igt_main
 		igt_require_gem(device);
 		caps = print_welcome(device);
 
-		ring_size = measure_ring_size(device) - 10;
+		ring_size = intel_measure_ring_size(device, 0, false) - 10;
 		if (!(caps & HAVE_EXECLISTS))
 			ring_size /= 2;
 		igt_info("Ring size: %d batches\n", ring_size);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t 8/8] tests/gem_ringfill: use intel_measure_ring_size

[Daniele Ceraolo Spurio]

With intel_measure_ring_size added as common function we can use it
instead of the local copy

Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_ringfill.c | 96 +---------------------------------------------------
 1 file changed, 1 insertion(+), 95 deletions(-)

diff --git a/tests/gem_ringfill.c b/tests/gem_ringfill.c
index 01cbd0a..8247e89 100644
--- a/tests/gem_ringfill.c
+++ b/tests/gem_ringfill.c
@@ -236,100 +236,6 @@ static void run_test(int fd, unsigned ring, unsigned flags, unsigned timeout)
 		run_test(fd, ring, 0, 0);
 }
 
-struct cork {
-	int device;
-	uint32_t handle;
-	uint32_t fence;
-};
-
-static void plug(int fd, struct cork *c)
-{
-	struct vgem_bo bo;
-	int dmabuf;
-
-	c->device = drm_open_driver(DRIVER_VGEM);
-
-	bo.width = bo.height = 1;
-	bo.bpp = 4;
-	vgem_create(c->device, &bo);
-	c->fence = vgem_fence_attach(c->device, &bo, VGEM_FENCE_WRITE);
-
-	dmabuf = prime_handle_to_fd(c->device, bo.handle);
-	c->handle = prime_fd_to_handle(fd, dmabuf);
-	close(dmabuf);
-}
-
-static void unplug(struct cork *c)
-{
-	vgem_fence_signal(c->device, c->fence);
-	close(c->device);
-}
-
-static void alarm_handler(int sig)
-{
-}
-
-static int __execbuf(int fd, struct drm_i915_gem_execbuffer2 *execbuf)
-{
-	return ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, execbuf);
-}
-
-static unsigned int measure_ring_size(int fd)
-{
-	struct sigaction sa = { .sa_handler = alarm_handler };
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_execbuffer2 execbuf;
-	const uint32_t bbe = MI_BATCH_BUFFER_END;
-	unsigned int count, last;
-	struct itimerval itv;
-	struct cork c;
-
-	memset(obj, 0, sizeof(obj));
-	obj[1].handle = gem_create(fd, 4096);
-	gem_write(fd, obj[1].handle, 0, &bbe, sizeof(bbe));
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj + 1);
-	execbuf.buffer_count = 1;
-	gem_execbuf(fd, &execbuf);
-	gem_sync(fd, obj[1].handle);
-
-	plug(fd, &c);
-	obj[0].handle = c.handle;
-
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-
-	sigaction(SIGALRM, &sa, NULL);
-	itv.it_interval.tv_sec = 0;
-	itv.it_interval.tv_usec = 100;
-	itv.it_value.tv_sec = 0;
-	itv.it_value.tv_usec = 1000;
-	setitimer(ITIMER_REAL, &itv, NULL);
-
-	last = -1;
-	count = 0;
-	do {
-		if (__execbuf(fd, &execbuf) == 0) {
-			count++;
-			continue;
-		}
-
-		if (last == count)
-			break;
-
-		last = count;
-	} while (1);
-
-	memset(&itv, 0, sizeof(itv));
-	setitimer(ITIMER_REAL, &itv, NULL);
-
-	unplug(&c);
-	gem_close(fd, obj[1].handle);
-
-	return count;
-}
-
 igt_main
 {
 	const struct {
@@ -364,7 +270,7 @@ igt_main
 			master = true;
 		}
 
-		ring_size = measure_ring_size(fd);
+		ring_size = intel_measure_ring_size(fd, 0, false);
 		igt_info("Ring size: %d batches\n", ring_size);
 		igt_require(ring_size);
 	}
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for extract cork and ring measure code

[Patchwork]

== Series Details ==

Series: extract cork and ring measure code
URL   : https://patchwork.freedesktop.org/series/31858/
State : success

== Summary ==

IGT patchset tested on top of latest successful build
58616272b23efce1e62a3ee0d37e13de6ffc012f igt/gem_eio: Check hang/eio recovery during suspend

with latest DRM-Tip kernel build CI_DRM_3226
490bd134d635 drm-tip: 2017y-10m-12d-20h-08m-10s UTC integration manifest

No testlist changes.

Test chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#102514

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:459s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:471s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:392s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:576s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:288s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:524s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:522s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:543s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:525s
fi-cfl-s         total:289  pass:253  dwarn:4   dfail:0   fail:0   skip:32  time:561s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:439s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:277s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:609s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:441s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:462s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:509s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:478s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:504s
fi-kbl-7567u     total:289  pass:265  dwarn:4   dfail:0   fail:0   skip:20  time:494s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:596s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:672s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:473s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:665s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:540s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:522s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:477s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:587s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:430s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_344/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
> diff --git a/lib/igt_dummyload.h b/lib/igt_dummyload.h
> index 215425f..d20a867 100644
> --- a/lib/igt_dummyload.h
> +++ b/lib/igt_dummyload.h

<insert ambition for killing off using igt for i915 specific constructs>

> @@ -29,6 +29,7 @@
>  #include <time.h>
>  
>  #include "igt_aux.h"
> +#include "igt_vgem.h"
>  
>  typedef struct igt_spin {
>         unsigned int handle;
> @@ -51,4 +52,14 @@ void igt_spin_batch_free(int fd, igt_spin_t *spin);
>  
>  void igt_terminate_spin_batches(void);
>  
> +typedef struct igt_cork {
> +       int device;
> +       uint32_t handle;
> +       uint32_t fence;
> +} igt_cork_t;

Nothing here depends on igt_vgem.h, that should just be for
igt_dummyload.c

> +
> +igt_cork_t *igt_cork_new(int fd);
> +void igt_cork_signal(igt_cork_t *cork);
> +void igt_cork_free(int fd, igt_cork_t *cork);
> +
>  #endif /* __IGT_DUMMYLOAD_H__ */
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
> +typedef struct igt_cork {
> +       int device;
> +       uint32_t handle;
> +       uint32_t fence;
> +} igt_cork_t;

Oh, just struct no need for typedef when you are transparent.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
> +igt_cork_t *igt_cork_new(int fd);

_new does not imply plugged.

> +void igt_cork_signal(igt_cork_t *cork);

When have you signaled a cork?

> +void igt_cork_free(int fd, igt_cork_t *cork);

_free does not imply unplug.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 0/8] extract cork and ring measure code

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:26)
> The "cork" bo used to stall execution and the interruptible execbuf
> used to measure the ring size are repeated almost identically in
> several files. Extracting those to common files helps simplifying
> the code and avoiding repetition.

If you are looking at this, there are two different types of cork
around. The vgem style where we use the implicit fence, and the sw-sync
style where we use an explicit fence. Both have slightly different
use-cases, one may be more convenient that the other depending on the
rest of the test setup.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for extract cork and ring measure code

[Patchwork]

== Series Details ==

Series: extract cork and ring measure code
URL   : https://patchwork.freedesktop.org/series/31858/
State : success

== Summary ==

Test gem_flink_race:
        Subgroup flink_close:
                pass       -> FAIL       (shard-hsw) fdo#102655

fdo#102655 https://bugs.freedesktop.org/show_bug.cgi?id=102655

shard-hsw        total:2553 pass:1439 dwarn:1   dfail:0   fail:10  skip:1103 time:9677s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_344/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Chris Wilson]

Quoting Chris Wilson (2017-10-12 23:57:38)
> Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
> > +igt_cork_t *igt_cork_new(int fd);
> 
> _new does not imply plugged.
> 
> > +void igt_cork_signal(igt_cork_t *cork);
> 
> When have you signaled a cork?
> 
> > +void igt_cork_free(int fd, igt_cork_t *cork);
> 
> _free does not imply unplug.

To be clear the verbs are to plug and unplug a queue/schedule. Cork is a
reference to TCP_CORK which does the same thing, but plug/unplug are
more commonplace (at least in kernel code).

I don't see any reason why we need a malloc here.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Daniele Ceraolo Spurio]

On 13/10/17 01:31, Chris Wilson wrote:
> Quoting Chris Wilson (2017-10-12 23:57:38)
>> Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
>>> +igt_cork_t *igt_cork_new(int fd);
>>
>> _new does not imply plugged.
>>
>>> +void igt_cork_signal(igt_cork_t *cork);
>>
>> When have you signaled a cork?
>>
>>> +void igt_cork_free(int fd, igt_cork_t *cork);
>>
>> _free does not imply unplug.
> 
> To be clear the verbs are to plug and unplug a queue/schedule. Cork is a
> reference to TCP_CORK which does the same thing, but plug/unplug are
> more commonplace (at least in kernel code).
> 
> I don't see any reason why we need a malloc here.
> -Chris
> 

I added the malloc just to use the same approach as the spin_batch, I'll 
get rid of it.
My concern with the existing plug/unplug scheme was that the plug() 
function in the various tests didn't really plug anything but just 
created the bo and that was slightly confusing.
What do you think of going with:

	struct igt_cork {
		int device;
		uint32_t handle;
		uint32_t fence;
	};

	struct igt_cork igt_cork_create(int fd);
	void igt_cork_unplug(struct igt_cork *cork);
	void igt_cork_close(int fd, struct igt_cork *cork);
	void igt_cork_unplug_and_close(int fd, struct igt_cork *cork);

The plug() function is still missing, as we do the actual plugging by 
adding the object to the execbuf and I don't think that would be clean 
to wrap in the library. I thought of adding something like 
"get_plugging_handle()" to return cork->handle and make it more explicit 
that that handle was responsible for the plugging but it seemed a bit 
overkill.

Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Daniele Ceraolo Spurio]

On 13/10/17 09:37, Daniele Ceraolo Spurio wrote:
> 
> 
> On 13/10/17 01:31, Chris Wilson wrote:
>> Quoting Chris Wilson (2017-10-12 23:57:38)
>>> Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
>>>> +igt_cork_t *igt_cork_new(int fd);
>>>
>>> _new does not imply plugged.
>>>
>>>> +void igt_cork_signal(igt_cork_t *cork);
>>>
>>> When have you signaled a cork?
>>>
>>>> +void igt_cork_free(int fd, igt_cork_t *cork);
>>>
>>> _free does not imply unplug.
>>
>> To be clear the verbs are to plug and unplug a queue/schedule. Cork is a
>> reference to TCP_CORK which does the same thing, but plug/unplug are
>> more commonplace (at least in kernel code).
>>
>> I don't see any reason why we need a malloc here.
>> -Chris
>>
> 
> I added the malloc just to use the same approach as the spin_batch, I'll 
> get rid of it.
> My concern with the existing plug/unplug scheme was that the plug() 
> function in the various tests didn't really plug anything but just 
> created the bo and that was slightly confusing.
> What do you think of going with:
> 
>      struct igt_cork {
>          int device;
>          uint32_t handle;
>          uint32_t fence;
>      };
> 
>      struct igt_cork igt_cork_create(int fd);
>      void igt_cork_unplug(struct igt_cork *cork);
>      void igt_cork_close(int fd, struct igt_cork *cork);
>      void igt_cork_unplug_and_close(int fd, struct igt_cork *cork);
> 
> The plug() function is still missing, as we do the actual plugging by 
> adding the object to the execbuf and I don't think that would be clean 
> to wrap in the library. I thought of adding something like 
> "get_plugging_handle()" to return cork->handle and make it more explicit 
> that that handle was responsible for the plugging but it seemed a bit 
> overkill.
> 
> Daniele
>

Hi Chris,

any feedback on this?

Thanks,
Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-18 16:49:24)
> 
> 
> On 13/10/17 09:37, Daniele Ceraolo Spurio wrote:
> > 
> > 
> > On 13/10/17 01:31, Chris Wilson wrote:
> >> Quoting Chris Wilson (2017-10-12 23:57:38)
> >>> Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
> >>>> +igt_cork_t *igt_cork_new(int fd);
> >>>
> >>> _new does not imply plugged.
> >>>
> >>>> +void igt_cork_signal(igt_cork_t *cork);
> >>>
> >>> When have you signaled a cork?
> >>>
> >>>> +void igt_cork_free(int fd, igt_cork_t *cork);
> >>>
> >>> _free does not imply unplug.
> >>
> >> To be clear the verbs are to plug and unplug a queue/schedule. Cork is a
> >> reference to TCP_CORK which does the same thing, but plug/unplug are
> >> more commonplace (at least in kernel code).
> >>
> >> I don't see any reason why we need a malloc here.
> >> -Chris
> >>
> > 
> > I added the malloc just to use the same approach as the spin_batch, I'll 
> > get rid of it.
> > My concern with the existing plug/unplug scheme was that the plug() 
> > function in the various tests didn't really plug anything but just 
> > created the bo and that was slightly confusing.

It created a bo with an unsignaled fence, that's enough to plug anything
attached to it. Since we can't just say plug(device) we have to say
execbuf(device, plug()).

> > What do you think of going with:
> > 
> >      struct igt_cork {
> >          int device;
> >          uint32_t handle;
> >          uint32_t fence;
> >      };
> > 
> >      struct igt_cork igt_cork_create(int fd);
> >      void igt_cork_unplug(struct igt_cork *cork);
> >      void igt_cork_close(int fd, struct igt_cork *cork);
> >      void igt_cork_unplug_and_close(int fd, struct igt_cork *cork);

close will always be unplug; there's no differentiation, in both APIs we
ensure that any fence associated with the device or timeline fd is
signaled upon release. We could lose the fence and still work, but for
us it gives us the means by which we can do a test-and-set and report an
issue where the fence was signaled too early (due to slow test setup).
Similarly once unplugged, there is no use for the struct anymore, you
could release the device/timeline, but we've embedded it because in
terms of overhead, so far it has been insignificant.

Leaving a fence dangling by separating unplug/close is a good way to
leave lots of timeouts and GPU resets behind.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Daniele Ceraolo Spurio]

On 18/10/17 09:04, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2017-10-18 16:49:24)
>>
>>
>> On 13/10/17 09:37, Daniele Ceraolo Spurio wrote:
>>>
>>>
>>> On 13/10/17 01:31, Chris Wilson wrote:
>>>> Quoting Chris Wilson (2017-10-12 23:57:38)
>>>>> Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
>>>>>> +igt_cork_t *igt_cork_new(int fd);
>>>>>
>>>>> _new does not imply plugged.
>>>>>
>>>>>> +void igt_cork_signal(igt_cork_t *cork);
>>>>>
>>>>> When have you signaled a cork?
>>>>>
>>>>>> +void igt_cork_free(int fd, igt_cork_t *cork);
>>>>>
>>>>> _free does not imply unplug.
>>>>
>>>> To be clear the verbs are to plug and unplug a queue/schedule. Cork is a
>>>> reference to TCP_CORK which does the same thing, but plug/unplug are
>>>> more commonplace (at least in kernel code).
>>>>
>>>> I don't see any reason why we need a malloc here.
>>>> -Chris
>>>>
>>>
>>> I added the malloc just to use the same approach as the spin_batch, I'll
>>> get rid of it.
>>> My concern with the existing plug/unplug scheme was that the plug()
>>> function in the various tests didn't really plug anything but just
>>> created the bo and that was slightly confusing.
> 
> It created a bo with an unsignaled fence, that's enough to plug anything
> attached to it. Since we can't just say plug(device) we have to say
> execbuf(device, plug()).
> 
>>> What do you think of going with:
>>>
>>>       struct igt_cork {
>>>           int device;
>>>           uint32_t handle;
>>>           uint32_t fence;
>>>       };
>>>
>>>       struct igt_cork igt_cork_create(int fd);
>>>       void igt_cork_unplug(struct igt_cork *cork);
>>>       void igt_cork_close(int fd, struct igt_cork *cork);
>>>       void igt_cork_unplug_and_close(int fd, struct igt_cork *cork);
> 
> close will always be unplug; there's no differentiation, in both APIs we
> ensure that any fence associated with the device or timeline fd is
> signaled upon release. We could lose the fence and still work, but for
> us it gives us the means by which we can do a test-and-set and report an
> issue where the fence was signaled too early (due to slow test setup).
> Similarly once unplugged, there is no use for the struct anymore, you
> could release the device/timeline, but we've embedded it because in
> terms of overhead, so far it has been insignificant.
> 
> Leaving a fence dangling by separating unplug/close is a good way to
> leave lots of timeouts and GPU resets behind.
> -Chris
> 

What I wanted to separate is the unplugging from the closing of the BO 
handle, because in some case we keep the BO around for a while after 
unblocking the execution. In most of those cases the BO handle is not 
currently closed, but it seemed dirty to me to keep that behavior and 
have library functions that didn't clean up after themselves. Would it 
be ok for you to keep both unplug and close, with the first doing just 
the signaling of the fence and the second doing both the signaling (if 
not already done) and the bo handle closure? this would basically be 
what is in the current version of the patches but with a different name. 
Alternatively, we could explicitly return the handle from the plug() 
call to make it clearer that it needs closing:

uint32_t igt_plug(int fd, struct igt_cork *cork);
void igt_unplug(struct igt_cork *cork);

Could also keep the typedef in this case since the handle is returned 
and we don't need to peek inside the cork struct.

Thanks,
Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Chris Wilson]

Quoting Daniele Ceraolo Spurio (2017-10-18 17:50:33)
> 
> 
> On 18/10/17 09:04, Chris Wilson wrote:
> > Quoting Daniele Ceraolo Spurio (2017-10-18 16:49:24)
> >>
> >>
> >> On 13/10/17 09:37, Daniele Ceraolo Spurio wrote:
> >>>
> >>>
> >>> On 13/10/17 01:31, Chris Wilson wrote:
> >>>> Quoting Chris Wilson (2017-10-12 23:57:38)
> >>>>> Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
> >>>>>> +igt_cork_t *igt_cork_new(int fd);
> >>>>>
> >>>>> _new does not imply plugged.
> >>>>>
> >>>>>> +void igt_cork_signal(igt_cork_t *cork);
> >>>>>
> >>>>> When have you signaled a cork?
> >>>>>
> >>>>>> +void igt_cork_free(int fd, igt_cork_t *cork);
> >>>>>
> >>>>> _free does not imply unplug.
> >>>>
> >>>> To be clear the verbs are to plug and unplug a queue/schedule. Cork is a
> >>>> reference to TCP_CORK which does the same thing, but plug/unplug are
> >>>> more commonplace (at least in kernel code).
> >>>>
> >>>> I don't see any reason why we need a malloc here.
> >>>> -Chris
> >>>>
> >>>
> >>> I added the malloc just to use the same approach as the spin_batch, I'll
> >>> get rid of it.
> >>> My concern with the existing plug/unplug scheme was that the plug()
> >>> function in the various tests didn't really plug anything but just
> >>> created the bo and that was slightly confusing.
> > 
> > It created a bo with an unsignaled fence, that's enough to plug anything
> > attached to it. Since we can't just say plug(device) we have to say
> > execbuf(device, plug()).
> > 
> >>> What do you think of going with:
> >>>
> >>>       struct igt_cork {
> >>>           int device;
> >>>           uint32_t handle;
> >>>           uint32_t fence;
> >>>       };
> >>>
> >>>       struct igt_cork igt_cork_create(int fd);
> >>>       void igt_cork_unplug(struct igt_cork *cork);
> >>>       void igt_cork_close(int fd, struct igt_cork *cork);
> >>>       void igt_cork_unplug_and_close(int fd, struct igt_cork *cork);
> > 
> > close will always be unplug; there's no differentiation, in both APIs we
> > ensure that any fence associated with the device or timeline fd is
> > signaled upon release. We could lose the fence and still work, but for
> > us it gives us the means by which we can do a test-and-set and report an
> > issue where the fence was signaled too early (due to slow test setup).
> > Similarly once unplugged, there is no use for the struct anymore, you
> > could release the device/timeline, but we've embedded it because in
> > terms of overhead, so far it has been insignificant.
> > 
> > Leaving a fence dangling by separating unplug/close is a good way to
> > leave lots of timeouts and GPU resets behind.
> > -Chris
> > 
> 
> What I wanted to separate is the unplugging from the closing of the BO 
> handle, because in some case we keep the BO around for a while after 
> unblocking the execution.

Where? What value could it possibly have? You know the state of its
fence, so presumably you want the contents. In such a situation you don't
need a dummy cork to plug the queue, you have a real object with which
you want interact.

> In most of those cases the BO handle is not 
> currently closed,

Every single unplug() function is closing the device; which closes the
handle; gem_close() is superfluous. Early on I kept the vgem fd around
and just needed to close the handle, but it looks like all of those
functions have now been converted to own their device.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t 1/8] lib/igt_dummyload: add igt_cork

[Daniele Ceraolo Spurio]

On 19/10/17 11:12, Chris Wilson wrote:
> Quoting Daniele Ceraolo Spurio (2017-10-18 17:50:33)
>>
>>
>> On 18/10/17 09:04, Chris Wilson wrote:
>>> Quoting Daniele Ceraolo Spurio (2017-10-18 16:49:24)
>>>>
>>>>
>>>> On 13/10/17 09:37, Daniele Ceraolo Spurio wrote:
>>>>>
>>>>>
>>>>> On 13/10/17 01:31, Chris Wilson wrote:
>>>>>> Quoting Chris Wilson (2017-10-12 23:57:38)
>>>>>>> Quoting Daniele Ceraolo Spurio (2017-10-12 23:27:27)
>>>>>>>> +igt_cork_t *igt_cork_new(int fd);
>>>>>>>
>>>>>>> _new does not imply plugged.
>>>>>>>
>>>>>>>> +void igt_cork_signal(igt_cork_t *cork);
>>>>>>>
>>>>>>> When have you signaled a cork?
>>>>>>>
>>>>>>>> +void igt_cork_free(int fd, igt_cork_t *cork);
>>>>>>>
>>>>>>> _free does not imply unplug.
>>>>>>
>>>>>> To be clear the verbs are to plug and unplug a queue/schedule. Cork is a
>>>>>> reference to TCP_CORK which does the same thing, but plug/unplug are
>>>>>> more commonplace (at least in kernel code).
>>>>>>
>>>>>> I don't see any reason why we need a malloc here.
>>>>>> -Chris
>>>>>>
>>>>>
>>>>> I added the malloc just to use the same approach as the spin_batch, I'll
>>>>> get rid of it.
>>>>> My concern with the existing plug/unplug scheme was that the plug()
>>>>> function in the various tests didn't really plug anything but just
>>>>> created the bo and that was slightly confusing.
>>>
>>> It created a bo with an unsignaled fence, that's enough to plug anything
>>> attached to it. Since we can't just say plug(device) we have to say
>>> execbuf(device, plug()).
>>>
>>>>> What do you think of going with:
>>>>>
>>>>>        struct igt_cork {
>>>>>            int device;
>>>>>            uint32_t handle;
>>>>>            uint32_t fence;
>>>>>        };
>>>>>
>>>>>        struct igt_cork igt_cork_create(int fd);
>>>>>        void igt_cork_unplug(struct igt_cork *cork);
>>>>>        void igt_cork_close(int fd, struct igt_cork *cork);
>>>>>        void igt_cork_unplug_and_close(int fd, struct igt_cork *cork);
>>>
>>> close will always be unplug; there's no differentiation, in both APIs we
>>> ensure that any fence associated with the device or timeline fd is
>>> signaled upon release. We could lose the fence and still work, but for
>>> us it gives us the means by which we can do a test-and-set and report an
>>> issue where the fence was signaled too early (due to slow test setup).
>>> Similarly once unplugged, there is no use for the struct anymore, you
>>> could release the device/timeline, but we've embedded it because in
>>> terms of overhead, so far it has been insignificant.
>>>
>>> Leaving a fence dangling by separating unplug/close is a good way to
>>> leave lots of timeouts and GPU resets behind.
>>> -Chris
>>>
>>
>> What I wanted to separate is the unplugging from the closing of the BO
>> handle, because in some case we keep the BO around for a while after
>> unblocking the execution.
> 
> Where? What value could it possibly have? You know the state of its
> fence, so presumably you want the contents. In such a situation you don't
> need a dummy cork to plug the queue, you have a real object with which
> you want interact.
> 
>> In most of those cases the BO handle is not
>> currently closed,
> 
> Every single unplug() function is closing the device; which closes the
> handle; gem_close() is superfluous. Early on I kept the vgem fd around
> and just needed to close the handle, but it looks like all of those
> functions have now been converted to own their device.
> -Chris
> 

Apologies for being unclear, what I was referring to was the imported BO 
handle, not the one from the vgem device. In a couple of tests 
(gem_wait, gem_exec_latency) we still use it after the unplug. My 
understanding (which could be wrong, since I haven't looked at this area 
a lot) is that the imported handle will stay valid even if the original 
device is closed and that's why I wanted to explicitly clean it up. 
It'll still be closed when the fd is closed so we're not doing something 
inherently bad, but I felt it would've been cleaner to have that in the 
common function. I'll add a note about it in the function description 
instead and keep the existing plug/unplug scheme.

Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx