public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf
@ 2017-07-28 12:07 Chris Wilson
  2017-07-28 12:08 ` [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace() Chris Wilson
                   ` (9 more replies)
  0 siblings, 10 replies; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:07 UTC (permalink / raw)
  To: intel-gfx

The idea is to implement read(dmabuf) and write(dambuf) so provide some
tests.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/Makefile.sources |   1 +
 tests/prime_rw.c       | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 180 insertions(+)
 create mode 100644 tests/prime_rw.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 5b98a5a3..18b99768 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -225,6 +225,7 @@ TESTS_progs = \
 	prime_mmap \
 	prime_mmap_coherency \
 	prime_mmap_kms \
+	prime_rw \
 	prime_self_import \
 	prime_udl \
 	prime_vgem \
diff --git a/tests/prime_rw.c b/tests/prime_rw.c
new file mode 100644
index 00000000..65abec5d
--- /dev/null
+++ b/tests/prime_rw.c
@@ -0,0 +1,179 @@
+/*
+ * Copyright © 2017 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.
+ */
+
+/*
+ * Testcase: Check whether read()/write()ing dma-buf works
+ */
+#define _GNU_SOURCE
+#include <unistd.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "drmtest.h"
+#include "ioctl_wrappers.h"
+
+static int create_dmabuf(int fd, int size)
+{
+	uint32_t handle;
+	int dmabuf;
+
+	handle = gem_create(fd, size);
+	dmabuf = prime_handle_to_fd_for_mmap(fd, handle);
+	gem_close(fd, handle);
+
+	return dmabuf;
+}
+
+static bool has_dma_buf_read(int fd)
+{
+	int dmabuf;
+	int ret;
+
+	dmabuf = create_dmabuf(fd, 4096);
+	ret = read(dmabuf, NULL, 0);
+	close(dmabuf);
+
+	errno = 0;
+	return ret == 0;
+}
+
+static bool has_dma_buf_write(int fd)
+{
+	int dmabuf;
+	int ret;
+
+	dmabuf = create_dmabuf(fd, 4096);
+	ret = write(dmabuf, NULL, 0);
+	close(dmabuf);
+
+	errno = 0;
+	return ret == 0;
+}
+
+static void test_read(int fd)
+{
+	uint32_t buf[4096];
+	uint32_t *ptr;
+	int i, len, offset;
+	int dmabuf;
+
+	dmabuf = create_dmabuf(fd, sizeof(buf));
+	igt_assert(dmabuf != -1);
+
+	ptr = mmap(NULL, sizeof(buf), PROT_WRITE, MAP_SHARED, dmabuf, 0);
+	igt_assert(ptr != MAP_FAILED);
+
+	prime_sync_start(dmabuf, true);
+	for (i = 0; i < ARRAY_SIZE(buf); i++)
+		ptr[i] = i;
+	prime_sync_end(dmabuf, true);
+
+	munmap(ptr, sizeof(buf));
+
+	prime_sync_start(dmabuf, false);
+	for (len = 1; len <= 4096; len <<= 1) {
+		const int sz = len * sizeof(buf[0]);
+
+		offset = len/2;
+		lseek(dmabuf, offset * sizeof(buf[0]), SEEK_SET);
+		for (; offset + len <= 4096; offset += len) {
+			igt_assert_eq(read(dmabuf, buf, sz), sz);
+			for (i = 0; i < len; i++)
+				igt_assert_eq(buf[i], offset + i);
+		}
+		igt_assert_eq(read(dmabuf, buf, sz),
+			      (4096 - offset) * sizeof(buf[0]));
+		igt_assert_eq(read(dmabuf, buf, sz), 0);
+	}
+	prime_sync_end(dmabuf, false);
+
+	close(dmabuf);
+}
+
+static void test_write(int fd)
+{
+	uint32_t buf[4096];
+	uint32_t *ptr;
+	int i, len, offset;
+	int dmabuf;
+
+	dmabuf = create_dmabuf(fd, sizeof(buf));
+	igt_assert(dmabuf != -1);
+
+	for (i = 0; i < ARRAY_SIZE(buf); i++)
+		buf[i] = i;
+
+	ptr = mmap(NULL, sizeof(buf), PROT_READ, MAP_SHARED, dmabuf, 0);
+	igt_assert(ptr != MAP_FAILED);
+
+	prime_sync_start(dmabuf, true);
+	for (len = 1; len <= 4096; len <<= 1) {
+		const int sz = len * sizeof(buf[0]);
+
+		offset = len/2;
+		lseek(dmabuf, offset * sizeof(buf[0]), SEEK_SET);
+		for (; offset + len <= 4096; offset += len) {
+			igt_assert_eq(write(dmabuf, buf, sz), sz);
+			for (i = 0; i < len; i++)
+				igt_assert_eq(ptr[i + offset], i);
+		}
+		igt_assert_eq(write(dmabuf, buf, sz),
+			      (4096 - offset) * sizeof(buf[0]));
+		igt_assert_eq(write(dmabuf, buf, sz), 0);
+	}
+	prime_sync_end(dmabuf, true);
+
+	munmap(ptr, sizeof(buf));
+
+	close(dmabuf);
+}
+
+igt_main
+{
+	int fd = -1;
+
+	igt_fixture {
+		fd = drm_open_driver(DRIVER_INTEL);
+	}
+
+	igt_subtest_group {
+		igt_fixture {
+			igt_require((has_dma_buf_read(fd)));
+		}
+
+		igt_subtest("basic-read")
+			test_read(fd);
+	}
+
+	igt_subtest_group {
+		igt_fixture {
+			igt_require((has_dma_buf_write(fd)));
+		}
+
+		igt_subtest("basic-write")
+			test_write(fd);
+	}
+
+	igt_fixture
+		close(fd);
+}
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace()
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-31  9:41   ` Tvrtko Ursulin
  2017-07-28 12:08 ` [PATCH igt 03/10] igt/gem_ctx_switch: Exercise all engines at once Chris Wilson
                   ` (8 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

gdb uses ptrace() to peek and poke bytes of the target's address space.
The kernel must implement an vm_ops->access() handler or else gdb will
be unable to inspect the pointer and report it as out-of-bounds. Worse
than useless as it causes immediate suspicion of the valid GTT pointer.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_mmap_gtt.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c
index 4ff5e7f1..61c08406 100644
--- a/tests/gem_mmap_gtt.c
+++ b/tests/gem_mmap_gtt.c
@@ -36,6 +36,8 @@
 #include <errno.h>
 #include <sys/stat.h>
 #include <sys/ioctl.h>
+#include <sys/ptrace.h>
+#include <sys/wait.h>
 #include "drm.h"
 
 #include "igt.h"
@@ -310,6 +312,81 @@ test_write_gtt(int fd)
 	munmap(src, OBJECT_SIZE);
 }
 
+static void *memchr_inv(const void *s, int c, size_t n)
+{
+	const uint8_t *us = s;
+	const uint8_t uc = c;
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wcast-qual"
+	while (n--) {
+		if (*us != uc)
+			return (void *) us;
+		us++;
+	}
+#pragma GCC diagnostic pop
+
+	return NULL;
+}
+
+static void
+test_ptrace(int fd)
+{
+	long AA, CC;
+	long *gtt, *copy;
+	uint32_t bo;
+	pid_t pid;
+
+	memset(&AA, 0xaa, sizeof(AA));
+	memset(&CC, 0x55, sizeof(CC));
+
+	copy = malloc(OBJECT_SIZE);
+	memset(copy, AA, OBJECT_SIZE);
+
+	bo = gem_create(fd, OBJECT_SIZE);
+	gtt = mmap_bo(fd, bo);
+	memset(gtt, CC, OBJECT_SIZE);
+	gem_close(fd, bo);
+
+	igt_assert(!memchr_inv(gtt, CC, OBJECT_SIZE));
+	igt_assert(!memchr_inv(copy, AA, OBJECT_SIZE));
+
+	switch ((pid = fork())) {
+		case -1:
+			igt_assert(pid != -1);
+			break;
+
+		case 0:
+			ptrace(PTRACE_TRACEME, 0, NULL, NULL);
+			raise(SIGSTOP);
+			raise(SIGKILL);
+			exit(0);
+			break;
+
+		default:
+			/* Wait for the child to ready themselves */
+			wait(NULL);
+
+			ptrace(PTRACE_ATTACH, pid, NULL, NULL);
+			for (int i = 0; i < OBJECT_SIZE/sizeof(long); i++) {
+				copy[i] = ptrace(PTRACE_PEEKDATA, pid, gtt+i, NULL);
+				ptrace(PTRACE_POKEDATA, pid, gtt + i, AA);
+			}
+			ptrace(PTRACE_DETACH, pid, NULL, NULL);
+
+			/* Wakeup the child for it to exit */
+			kill(SIGCONT, pid);
+			break;
+	}
+
+	/* The contents of the two buffers should now be swapped */
+	igt_assert(!memchr_inv(gtt, AA, OBJECT_SIZE));
+	igt_assert(!memchr_inv(copy, CC, OBJECT_SIZE));
+
+	munmap(gtt, OBJECT_SIZE);
+	free(copy);
+}
+
 static void
 test_coherency(int fd)
 {
@@ -809,6 +886,8 @@ igt_main
 		test_write(fd);
 	igt_subtest("basic-write-gtt")
 		test_write_gtt(fd);
+	igt_subtest("ptrace")
+		test_ptrace(fd);
 	igt_subtest("coherency")
 		test_coherency(fd);
 	igt_subtest("clflush")
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 03/10] igt/gem_ctx_switch: Exercise all engines at once
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
  2017-07-28 12:08 ` [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace() Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-31  9:57   ` Tvrtko Ursulin
  2017-07-28 12:08 ` [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels Chris Wilson
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

Just a small variant to apply a continuous context-switch load to all
engines.
---
 tests/gem_ctx_switch.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/tests/gem_ctx_switch.c b/tests/gem_ctx_switch.c
index b6ea71cf..df22efec 100644
--- a/tests/gem_ctx_switch.c
+++ b/tests/gem_ctx_switch.c
@@ -140,6 +140,81 @@ static void single(int fd, uint32_t handle,
 		gem_context_destroy(fd, contexts[n]);
 }
 
+static void all(int fd, uint32_t handle, unsigned flags, int timeout)
+{
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct drm_i915_gem_exec_object2 obj[2];
+	unsigned int engine[16], e;
+	const char *name[16];
+	uint32_t contexts[64];
+	unsigned int nengine;
+	int n;
+
+	nengine = 0;
+	for_each_engine(fd, e) {
+		if (e == 0 || e == I915_EXEC_BSD)
+			continue;
+
+		engine[nengine] = e;
+		name[nengine] = e__->name;
+		nengine++;
+	}
+	igt_require(nengine);
+
+	igt_require(__gem_context_create(fd, &contexts[0]) == 0);
+	for (n = 1; n < 64; n++)
+		contexts[n] = gem_context_create(fd);
+
+	memset(obj, 0, sizeof(obj));
+	obj[1].handle = handle;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj + 1);
+	execbuf.buffer_count = 1;
+	execbuf.rsvd1 = contexts[0];
+	execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT;
+	execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC;
+	igt_require(__gem_execbuf(fd, &execbuf) == 0);
+	if (__gem_execbuf(fd, &execbuf)) {
+		execbuf.flags = 0;
+		gem_execbuf(fd, &execbuf);
+	}
+	gem_sync(fd, handle);
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = 2;
+
+	igt_fork(child, nengine) {
+		struct timespec start, now;
+		unsigned int count = 0;
+
+		obj[0].handle = gem_create(fd, 4096);
+		execbuf.flags |= engine[child];
+		gem_execbuf(fd, &execbuf);
+		gem_sync(fd, obj[0].handle);
+
+		clock_gettime(CLOCK_MONOTONIC, &start);
+		do {
+			for (int loop = 0; loop < 64; loop++) {
+				execbuf.rsvd1 = contexts[loop % 64];
+				gem_execbuf(fd, &execbuf);
+			}
+			count += 64;
+			clock_gettime(CLOCK_MONOTONIC, &now);
+		} while (elapsed(&start, &now) < timeout);
+		gem_sync(fd, obj[0].handle);
+		clock_gettime(CLOCK_MONOTONIC, &now);
+		gem_close(fd, obj[0].handle);
+
+		igt_info("[%d] %s: %'u cycles: %.3fus%s\n",
+			 child, name[child], count, elapsed(&start, &now)*1e6 / count,
+			 flags & INTERRUPTIBLE ? " (interruptible)" : "");
+	}
+	igt_waitchildren();
+
+	for (n = 0; n < 64; n++)
+		gem_context_destroy(fd, contexts[n]);
+}
+
 igt_main
 {
 	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
@@ -177,6 +252,11 @@ igt_main
 			single(fd, light, e, INTERRUPTIBLE, ncpus, 150);
 	}
 
+	igt_subtest("basic-all")
+		all(fd, light, 0, 20);
+	igt_subtest("basic-all-heavy")
+		all(fd, heavy, 0, 20);
+
 	igt_fixture {
 		igt_stop_hang_detector();
 		gem_close(fd, heavy);
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
  2017-07-28 12:08 ` [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace() Chris Wilson
  2017-07-28 12:08 ` [PATCH igt 03/10] igt/gem_ctx_switch: Exercise all engines at once Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-31 14:35   ` Tvrtko Ursulin
  2017-07-31 15:27   ` Michał Winiarski
  2017-07-28 12:08 ` [PATCH igt 05/10] igt/drv_hangman: Skip if resets are disallowed Chris Wilson
                   ` (6 subsequent siblings)
  9 siblings, 2 replies; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

Apply a little more stress to the scheduler.
---
 lib/igt_rand.h            |   6 +++
 tests/gem_exec_schedule.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/lib/igt_rand.h b/lib/igt_rand.h
index f664af41..c9cb3243 100644
--- a/lib/igt_rand.h
+++ b/lib/igt_rand.h
@@ -38,4 +38,10 @@ static inline void hars_petruska_f54_1_random_perturb(uint32_t xor)
 	hars_petruska_f54_1_random_seed(hars_petruska_f54_1_random_unsafe());
 }
 
+/* Returns: pseudo-random number in interval [0, ep_ro) */
+static inline uint32_t hars_petruska_f54_1_random_unsafe_max(uint32_t ep_ro)
+{
+	return ((uint64_t)hars_petruska_f54_1_random_unsafe() * ep_ro) >> 32;
+}
+
 #endif /* IGT_RAND_H */
diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
index 545dcc2e..1b6e29be 100644
--- a/tests/gem_exec_schedule.c
+++ b/tests/gem_exec_schedule.c
@@ -25,6 +25,7 @@
 
 #include "igt.h"
 #include "igt_vgem.h"
+#include "igt_rand.h"
 
 #define LOCAL_PARAM_HAS_SCHEDULER 41
 #define LOCAL_CONTEXT_PARAM_PRIORITY 6
@@ -491,7 +492,7 @@ static void wide(int fd, unsigned ring)
 			I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
 	for (int n = 0; n < NCTX; n++)
 		igt_assert_eq_u32(ptr[n], ctx[n]);
-	munmap(ptr, 4096);
+	munmap(ptr, 4*NCTX);
 
 	gem_close(fd, obj.handle);
 	gem_close(fd, result);
@@ -500,6 +501,108 @@ static void wide(int fd, unsigned ring)
 #undef XS
 }
 
+static void reorder_wide(int fd, unsigned ring)
+{
+	const int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_exec_object2 obj[3];
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct cork cork;
+	uint32_t result, target;
+	uint32_t *busy;
+	uint32_t *r, *t;
+
+	result = gem_create(fd, 4096);
+	target = gem_create(fd, 4096);
+
+	busy = make_busy(fd, result, ring);
+	plug(fd, &cork);
+
+	t = 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[1].handle = result;
+	obj[2].relocs_ptr = to_user_pointer(&reloc);
+	obj[2].relocation_count = 1;
+
+	memset(&reloc, 0, sizeof(reloc));
+	reloc.target_handle = result;
+	reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+	reloc.write_domain = 0; /* lies */
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = 3;
+	execbuf.flags = ring;
+	if (gen < 6)
+		execbuf.flags |= I915_EXEC_SECURE;
+
+	for (int n = -MAX_PRIO, x = 1; n <= MAX_PRIO; n++, x++) {
+		uint32_t *batch;
+
+		execbuf.rsvd1 = gem_context_create(fd);
+		ctx_set_priority(fd, execbuf.rsvd1, n);
+
+		obj[2].handle = gem_create(fd, 128 * 64);
+		batch = gem_mmap__gtt(fd, obj[2].handle, 128 * 64, PROT_WRITE);
+		gem_set_domain(fd, obj[2].handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+
+		for (int m = 0; m < 128; m++) {
+			uint64_t addr;
+			int idx = hars_petruska_f54_1_random_unsafe_max( 1024);
+			int i;
+
+			execbuf.batch_start_offset = m * 64;
+			reloc.offset = execbuf.batch_start_offset + sizeof(uint32_t);
+			reloc.delta = idx * sizeof(uint32_t);
+			addr = reloc.presumed_offset + reloc.delta;
+
+			i = execbuf.batch_start_offset / sizeof(uint32_t);
+			batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+			if (gen >= 8) {
+				batch[++i] = addr;
+				batch[++i] = addr >> 32;
+			} else if (gen >= 4) {
+				batch[++i] = 0;
+				batch[++i] = addr;
+				reloc.offset += sizeof(uint32_t);
+			} else {
+				batch[i]--;
+				batch[++i] = addr;
+			}
+			batch[++i] = x;
+			batch[++i] = MI_BATCH_BUFFER_END;
+
+			if (!t[idx])
+				t[idx] =  x;
+
+			gem_execbuf(fd, &execbuf);
+		}
+
+		munmap(batch, 128 * 64);
+		gem_close(fd, obj[2].handle);
+		gem_context_destroy(fd, execbuf.rsvd1);
+	}
+
+	igt_assert(gem_bo_busy(fd, result));
+	unplug(&cork); /* only now submit our batches */
+	igt_debugfs_dump(fd, "i915_engine_info");
+	finish_busy(busy);
+
+	r = gem_mmap__gtt(fd, result, 4096, PROT_READ);
+	gem_set_domain(fd, result, /* no write hazard lies! */
+			I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+	for (int n = 0; n < 1024; n++)
+		igt_assert_eq_u32(r[n], t[n]);
+	munmap(r, 4096);
+	munmap(t, 4096);
+
+	gem_close(fd, result);
+	gem_close(fd, target);
+}
+
 static bool has_scheduler(int fd)
 {
 	drm_i915_getparam_t gp;
@@ -571,6 +674,9 @@ igt_main
 
 				igt_subtest_f("wide-%s", e->name)
 					wide(fd, e->exec_id | e->flags);
+
+				igt_subtest_f("reorder-wide-%s", e->name)
+					reorder_wide(fd, e->exec_id | e->flags);
 			}
 		}
 	}
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 05/10] igt/drv_hangman: Skip if resets are disallowed
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
                   ` (2 preceding siblings ...)
  2017-07-28 12:08 ` [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-28 13:53   ` Michał Winiarski
  2017-07-28 12:08 ` [PATCH igt 06/10] igt/gem_ringfill: Prime execbuf before measuring ring size Chris Wilson
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

If we tell the machine to reset but they are disallowed, we will leave
the system in a wedged state, preventing the majority of subsequent
tests.
---
 tests/drv_hangman.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/drv_hangman.c b/tests/drv_hangman.c
index 0551ec16..de57e128 100644
--- a/tests/drv_hangman.c
+++ b/tests/drv_hangman.c
@@ -30,6 +30,7 @@
 #include <limits.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+#include <sys/ioctl.h>
 #include <fcntl.h>
 
 #include "igt_sysfs.h"
@@ -239,7 +240,9 @@ igt_main
 		int idx;
 
 		device = drm_open_driver(DRIVER_INTEL);
+		igt_allow_hang(device, 0, 0);
 		igt_require_gem(device);
+
 		sysfs = igt_sysfs_open(device, &idx);
 		igt_assert(sysfs != -1);
 	}
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 06/10] igt/gem_ringfill: Prime execbuf before measuring ring size
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
                   ` (3 preceding siblings ...)
  2017-07-28 12:08 ` [PATCH igt 05/10] igt/drv_hangman: Skip if resets are disallowed Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-31  9:09   ` Michał Winiarski
  2017-07-28 12:08 ` [PATCH igt 07/10] igt/gem_exec_suspend: Try to suspend with a pending GPU hang Chris Wilson
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

Make sure that our buffers are ready and loaded to reduce the initial
stall.
---
 tests/gem_ringfill.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/gem_ringfill.c b/tests/gem_ringfill.c
index b52996a4..01cbd0a9 100644
--- a/tests/gem_ringfill.c
+++ b/tests/gem_ringfill.c
@@ -288,10 +288,15 @@ static unsigned int measure_ring_size(int fd)
 	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;
 
-	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = to_user_pointer(obj);
 	execbuf.buffer_count = 2;
 
@@ -302,7 +307,8 @@ static unsigned int measure_ring_size(int fd)
 	itv.it_value.tv_usec = 1000;
 	setitimer(ITIMER_REAL, &itv, NULL);
 
-	last = count = 0;
+	last = -1;
+	count = 0;
 	do {
 		if (__execbuf(fd, &execbuf) == 0) {
 			count++;
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 07/10] igt/gem_exec_suspend: Try to suspend with a pending GPU hang
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
                   ` (4 preceding siblings ...)
  2017-07-28 12:08 ` [PATCH igt 06/10] igt/gem_ringfill: Prime execbuf before measuring ring size Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-31  8:58   ` Michał Winiarski
  2017-07-28 12:08 ` [PATCH igt 08/10] igt/gem_exec_fence: Exercise merging fences Chris Wilson
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

Ensure that we can suspend the GPU even if it is currently busy in an
indefinite loop, requiring us to declare the task hung.
---
 tests/gem_exec_suspend.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
index 763cb108..bcb0e089 100644
--- a/tests/gem_exec_suspend.c
+++ b/tests/gem_exec_suspend.c
@@ -29,6 +29,7 @@
 
 #include "igt.h"
 #include "igt_gt.h"
+#include "igt_dummyload.h"
 
 #define NOSLEEP 0
 #define SUSPEND_DEVICES 1
@@ -44,6 +45,7 @@
 
 #define UNCACHED (0<<8)
 #define CACHED (1<<8)
+#define HANG (2<<8)
 
 static void run_test(int fd, unsigned ring, unsigned flags);
 
@@ -105,6 +107,7 @@ static void run_test(int fd, unsigned engine, unsigned flags)
 	struct drm_i915_gem_execbuffer2 execbuf;
 	unsigned engines[16];
 	unsigned nengine;
+	igt_spin_t *spin = NULL;
 
 	nengine = 0;
 	if (engine == -1) {
@@ -197,6 +200,9 @@ static void run_test(int fd, unsigned engine, unsigned flags)
 		gem_close(fd, obj[1].handle);
 	}
 
+	if (flags & HANG)
+		spin = igt_spin_batch_new(fd, 0, engine, 0);
+
 	switch (mode(flags)) {
 	case NOSLEEP:
 		break;
@@ -222,6 +228,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
 		break;
 	}
 
+	igt_spin_batch_free(fd, spin);
+
 	check_bo(fd, obj[0].handle);
 	gem_close(fd, obj[0].handle);
 
@@ -244,6 +252,7 @@ igt_main
 		{ NULL, 0 }
 	}, *m;
 	const struct intel_execution_engine *e;
+	igt_hang_t hang;
 	int fd;
 
 	igt_fixture {
@@ -278,6 +287,16 @@ igt_main
 
 	igt_fixture {
 		igt_stop_hang_detector();
+		hang = igt_allow_hang(fd, 0, 0);
+	}
+
+	igt_subtest("hang-S3")
+		run_test(fd, 0, SUSPEND | HANG);
+	igt_subtest("hang-S4")
+		run_test(fd, 0, HIBERNATE | HANG);
+
+	igt_fixture {
+		igt_disallow_hang(fd, hang);
 		close(fd);
 	}
 }
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 08/10] igt/gem_exec_fence: Exercise merging fences
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
                   ` (5 preceding siblings ...)
  2017-07-28 12:08 ` [PATCH igt 07/10] igt/gem_exec_suspend: Try to suspend with a pending GPU hang Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-31  8:26   ` Michał Winiarski
  2017-07-28 12:08 ` [PATCH igt 09/10] igt/gem_userptr_blits: Errors from gup are permanent Chris Wilson
                   ` (2 subsequent siblings)
  9 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

Execute the same batch on each engine and check that the composite fence
across all engines completes only after the batch is completed on every
engine.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_exec_fence.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 120 insertions(+)

diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
index b094f585..c7fa249b 100644
--- a/tests/gem_exec_fence.c
+++ b/tests/gem_exec_fence.c
@@ -199,6 +199,111 @@ static void test_fence_busy(int fd, unsigned ring, unsigned flags)
 	gem_quiescent_gpu(fd);
 }
 
+static void test_fence_busy_all(int fd, unsigned flags)
+{
+	const int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_exec_object2 obj;
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	struct timespec tv;
+	uint32_t *batch;
+	unsigned int engine;
+	int all, i, timeout;
+
+	gem_quiescent_gpu(fd);
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(&obj);
+	execbuf.buffer_count = 1;
+
+	memset(&obj, 0, sizeof(obj));
+	obj.handle = gem_create(fd, 4096);
+
+	obj.relocs_ptr = to_user_pointer(&reloc);
+	obj.relocation_count = 1;
+	memset(&reloc, 0, sizeof(reloc));
+
+	batch = gem_mmap__wc(fd, obj.handle, 0, 4096, PROT_WRITE);
+	gem_set_domain(fd, obj.handle,
+		       I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+
+	reloc.target_handle = obj.handle; /* recurse */
+	reloc.presumed_offset = 0;
+	reloc.offset = sizeof(uint32_t);
+	reloc.delta = 0;
+	reloc.read_domains = I915_GEM_DOMAIN_COMMAND;
+	reloc.write_domain = 0;
+
+	i = 0;
+	batch[i] = MI_BATCH_BUFFER_START;
+	if (gen >= 8) {
+		batch[i] |= 1 << 8 | 1;
+		batch[++i] = 0;
+		batch[++i] = 0;
+	} else if (gen >= 6) {
+		batch[i] |= 1 << 8;
+		batch[++i] = 0;
+	} else {
+		batch[i] |= 2 << 6;
+		batch[++i] = 0;
+		if (gen < 4) {
+			batch[i] |= 1;
+			reloc.delta = 1;
+		}
+	}
+	i++;
+
+	for_each_engine(fd, engine) {
+		int fence, new;
+
+		execbuf.flags = engine | LOCAL_EXEC_FENCE_OUT;
+		execbuf.rsvd2 = -1;
+		gem_execbuf_wr(fd, &execbuf);
+		fence = execbuf.rsvd2 >> 32;
+		igt_assert(fence != -1);
+
+		if (all < 0) {
+			all = fence;
+			break;
+		}
+
+		new = sync_fence_merge(all, fence);
+		close(all);
+		close(fence);
+
+		all = new;
+	}
+
+	igt_assert(gem_bo_busy(fd, obj.handle));
+	igt_assert(fence_busy(all));
+
+	timeout = 120;
+	if ((flags & HANG) == 0) {
+		*batch = MI_BATCH_BUFFER_END;
+		__sync_synchronize();
+		timeout = 1;
+	}
+	munmap(batch, 4096);
+
+	if (flags & WAIT) {
+		struct pollfd pfd = { .fd = all, .events = POLLIN };
+		igt_assert(poll(&pfd, 1, timeout*1000) == 1);
+	} else {
+		memset(&tv, 0, sizeof(tv));
+		while (fence_busy(all))
+			igt_assert(igt_seconds_elapsed(&tv) < timeout);
+	}
+
+	igt_assert(!gem_bo_busy(fd, obj.handle));
+	igt_assert_eq(sync_fence_status(all),
+		      flags & HANG ? -EIO : SYNC_FENCE_OK);
+
+	close(all);
+	gem_close(fd, obj.handle);
+
+	gem_quiescent_gpu(fd);
+}
+
 static void test_fence_await(int fd, unsigned ring, unsigned flags)
 {
 	const int gen = intel_gen(intel_get_drm_devid(fd));
@@ -744,6 +849,21 @@ igt_main
 		caps = print_welcome(i915);
 	}
 
+	igt_subtest_group {
+		igt_fixture {
+			igt_fork_hang_detector(i915);
+		}
+
+		igt_subtest("basic-busy-all")
+			test_fence_busy_all(i915, 0);
+		igt_subtest("basic-wait-all")
+			test_fence_busy_all(i915, WAIT);
+
+		igt_fixture {
+			igt_stop_hang_detector();
+		}
+	}
+
 	for (e = intel_execution_engines; e->name; e++) {
 		igt_subtest_group {
 			igt_fixture {
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 09/10] igt/gem_userptr_blits: Errors from gup are permanent
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
                   ` (6 preceding siblings ...)
  2017-07-28 12:08 ` [PATCH igt 08/10] igt/gem_exec_fence: Exercise merging fences Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-28 12:33   ` Michał Winiarski
  2017-07-28 12:08 ` [PATCH igt 10/10] igt/gem_eio: i915.reset is no longer a boolean Chris Wilson
  2017-07-31  9:26 ` [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Tvrtko Ursulin
  9 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

After triggering an error (such as trying to use userptr on a GTT
mmaping), we store the EFAULT on the object permanently. So to test the
error, we must sacrifice the object and recreate the userptr handle.
We restrict the error to just one of the overlapping userptr handles to
check the object independence.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100596
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_userptr_blits.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
index 7767c0ca..849054ad 100644
--- a/tests/gem_userptr_blits.c
+++ b/tests/gem_userptr_blits.c
@@ -681,12 +681,17 @@ static int test_map_fixed_invalidate(int fd, uint32_t flags)
 		gem_set_tiling(fd, mmap_gtt.handle, I915_TILING_NONE, 0);
 		*map = 0xdead;
 
-		if (flags & MAP_FIXED_INVALIDATE_GET_PAGES)
+		if (flags & MAP_FIXED_INVALIDATE_GET_PAGES) {
 			igt_assert_eq(__gem_set_domain(fd, handle[0],
 						       I915_GEM_DOMAIN_GTT,
 						       I915_GEM_DOMAIN_GTT),
 				      -EFAULT);
 
+			/* Errors are permanent, so we have to recreate */
+			gem_close(fd, handle[0]);
+			handle[0] = create_userptr(fd, 0, ptr + PAGE_SIZE/sizeof(*ptr));
+		}
+
 		gem_set_tiling(fd, mmap_gtt.handle, I915_TILING_Y, 512 * 4);
 		*(uint32_t*)map = 0xbeef;
 
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH igt 10/10] igt/gem_eio: i915.reset is no longer a boolean
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
                   ` (7 preceding siblings ...)
  2017-07-28 12:08 ` [PATCH igt 09/10] igt/gem_userptr_blits: Errors from gup are permanent Chris Wilson
@ 2017-07-28 12:08 ` Chris Wilson
  2017-07-28 13:36   ` Michał Winiarski
  2017-07-31  9:26 ` [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Tvrtko Ursulin
  9 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 12:08 UTC (permalink / raw)
  To: intel-gfx

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/gem_eio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/gem_eio.c b/tests/gem_eio.c
index 3c826626..15120842 100644
--- a/tests/gem_eio.c
+++ b/tests/gem_eio.c
@@ -53,7 +53,7 @@ static bool i915_reset_control(bool enable)
 	fd = open(path, O_RDWR);
 	igt_require(fd >= 0);
 
-	ret = write(fd, &"NY"[enable], 1) == 1;
+	ret = write(fd, &"01"[enable], 1) == 1;
 	close(fd);
 
 	return ret;
-- 
2.13.3

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

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 09/10] igt/gem_userptr_blits: Errors from gup are permanent
  2017-07-28 12:08 ` [PATCH igt 09/10] igt/gem_userptr_blits: Errors from gup are permanent Chris Wilson
@ 2017-07-28 12:33   ` Michał Winiarski
  0 siblings, 0 replies; 30+ messages in thread
From: Michał Winiarski @ 2017-07-28 12:33 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jul 28, 2017 at 01:08:07PM +0100, Chris Wilson wrote:
> After triggering an error (such as trying to use userptr on a GTT
> mmaping), we store the EFAULT on the object permanently. So to test the
> error, we must sacrifice the object and recreate the userptr handle.
> We restrict the error to just one of the overlapping userptr handles to
> check the object independence.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=100596
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> ---
>  tests/gem_userptr_blits.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/gem_userptr_blits.c b/tests/gem_userptr_blits.c
> index 7767c0ca..849054ad 100644
> --- a/tests/gem_userptr_blits.c
> +++ b/tests/gem_userptr_blits.c
> @@ -681,12 +681,17 @@ static int test_map_fixed_invalidate(int fd, uint32_t flags)
>  		gem_set_tiling(fd, mmap_gtt.handle, I915_TILING_NONE, 0);
>  		*map = 0xdead;
>  
> -		if (flags & MAP_FIXED_INVALIDATE_GET_PAGES)
> +		if (flags & MAP_FIXED_INVALIDATE_GET_PAGES) {
>  			igt_assert_eq(__gem_set_domain(fd, handle[0],
>  						       I915_GEM_DOMAIN_GTT,
>  						       I915_GEM_DOMAIN_GTT),
>  				      -EFAULT);
>  
> +			/* Errors are permanent, so we have to recreate */
> +			gem_close(fd, handle[0]);
> +			handle[0] = create_userptr(fd, 0, ptr + PAGE_SIZE/sizeof(*ptr));
> +		}
> +
>  		gem_set_tiling(fd, mmap_gtt.handle, I915_TILING_Y, 512 * 4);
>  		*(uint32_t*)map = 0xbeef;
>  
> -- 
> 2.13.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 10/10] igt/gem_eio: i915.reset is no longer a boolean
  2017-07-28 12:08 ` [PATCH igt 10/10] igt/gem_eio: i915.reset is no longer a boolean Chris Wilson
@ 2017-07-28 13:36   ` Michał Winiarski
  2017-07-28 13:50     ` Chris Wilson
  0 siblings, 1 reply; 30+ messages in thread
From: Michał Winiarski @ 2017-07-28 13:36 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jul 28, 2017 at 01:08:08PM +0100, Chris Wilson wrote:
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/gem_eio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/gem_eio.c b/tests/gem_eio.c
> index 3c826626..15120842 100644
> --- a/tests/gem_eio.c
> +++ b/tests/gem_eio.c
> @@ -53,7 +53,7 @@ static bool i915_reset_control(bool enable)
>  	fd = open(path, O_RDWR);
>  	igt_require(fd >= 0);
>  
> -	ret = write(fd, &"NY"[enable], 1) == 1;
> +	ret = write(fd, &"01"[enable], 1) == 1;

That's a fun way of doing "itoa()" :) 
How about using helpers? We have igt_sysfs_set_parameter.

Other thing is the fact that we're leaving the machine in a non-default state
(global reset rather than per-engine reset).
We should actually restore the old value after the test.
I can also see that's "by design" (hang detector), but if we change the
behaviour of hang detector in the future, we may forget to modify gem_eio.

-Michał

>  	close(fd);
>  
>  	return ret;
> -- 
> 2.13.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 10/10] igt/gem_eio: i915.reset is no longer a boolean
  2017-07-28 13:36   ` Michał Winiarski
@ 2017-07-28 13:50     ` Chris Wilson
  2017-07-28 15:56       ` Michał Winiarski
  0 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 13:50 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx

Quoting Michał Winiarski (2017-07-28 14:36:46)
> On Fri, Jul 28, 2017 at 01:08:08PM +0100, Chris Wilson wrote:
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/gem_eio.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tests/gem_eio.c b/tests/gem_eio.c
> > index 3c826626..15120842 100644
> > --- a/tests/gem_eio.c
> > +++ b/tests/gem_eio.c
> > @@ -53,7 +53,7 @@ static bool i915_reset_control(bool enable)
> >       fd = open(path, O_RDWR);
> >       igt_require(fd >= 0);
> >  
> > -     ret = write(fd, &"NY"[enable], 1) == 1;
> > +     ret = write(fd, &"01"[enable], 1) == 1;
> 
> That's a fun way of doing "itoa()" :) 
> How about using helpers? We have igt_sysfs_set_parameter.
> 
> Other thing is the fact that we're leaving the machine in a non-default state
> (global reset rather than per-engine reset).

If you haven't noticed, igt already does exist in a reset=[01] universe.
We are aaiting on the per-engine reset tests to actually enable
i915.reset=2. For coverage we need both, even i915.reset=2 may fallback
to i915.reset=1 in some unlikely situations.

> We should actually restore the old value after the test.
> I can also see that's "by design" (hang detector), but if we change the
> behaviour of hang detector in the future, we may forget to modify gem_eio.

That was the flavour of irc conversation yesterday. Instead of using
module parameters, we create a debugfs interface. The parameter will
only be set/adjusted while the debugfs fd was held open, so that we
would get automatic cleanup on process exit. It also means we should be
able to kill a few more module options that shouldn't exist to tempt
users.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 05/10] igt/drv_hangman: Skip if resets are disallowed
  2017-07-28 12:08 ` [PATCH igt 05/10] igt/drv_hangman: Skip if resets are disallowed Chris Wilson
@ 2017-07-28 13:53   ` Michał Winiarski
  2017-07-28 13:59     ` Chris Wilson
  2017-07-28 14:01     ` Chris Wilson
  0 siblings, 2 replies; 30+ messages in thread
From: Michał Winiarski @ 2017-07-28 13:53 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jul 28, 2017 at 01:08:03PM +0100, Chris Wilson wrote:
> If we tell the machine to reset but they are disallowed, we will leave
> the system in a wedged state, preventing the majority of subsequent
> tests.
> ---
>  tests/drv_hangman.c | 3 +++
>  1 file changed, 3 insertions(+)

Missed sign-off.

> 
> diff --git a/tests/drv_hangman.c b/tests/drv_hangman.c
> index 0551ec16..de57e128 100644
> --- a/tests/drv_hangman.c
> +++ b/tests/drv_hangman.c
> @@ -30,6 +30,7 @@
>  #include <limits.h>
>  #include <sys/types.h>
>  #include <sys/stat.h>
> +#include <sys/ioctl.h>
>  #include <fcntl.h>
>  
>  #include "igt_sysfs.h"
> @@ -239,7 +240,9 @@ igt_main
>  		int idx;
>  
>  		device = drm_open_driver(DRIVER_INTEL);
> +		igt_allow_hang(device, 0, 0);

Umm... That's not skipping.
That's enabling reset, right?

-Michał

>  		igt_require_gem(device);
> +
>  		sysfs = igt_sysfs_open(device, &idx);
>  		igt_assert(sysfs != -1);
>  	}
> -- 
> 2.13.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 05/10] igt/drv_hangman: Skip if resets are disallowed
  2017-07-28 13:53   ` Michał Winiarski
@ 2017-07-28 13:59     ` Chris Wilson
  2017-07-28 14:01     ` Chris Wilson
  1 sibling, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 13:59 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx

Quoting Michał Winiarski (2017-07-28 14:53:40)
> On Fri, Jul 28, 2017 at 01:08:03PM +0100, Chris Wilson wrote:
> > If we tell the machine to reset but they are disallowed, we will leave
> > the system in a wedged state, preventing the majority of subsequent
> > tests.
> > ---
> >  tests/drv_hangman.c | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> Missed sign-off.
> 
> > 
> > diff --git a/tests/drv_hangman.c b/tests/drv_hangman.c
> > index 0551ec16..de57e128 100644
> > --- a/tests/drv_hangman.c
> > +++ b/tests/drv_hangman.c
> > @@ -30,6 +30,7 @@
> >  #include <limits.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> > +#include <sys/ioctl.h>
> >  #include <fcntl.h>
> >  
> >  #include "igt_sysfs.h"
> > @@ -239,7 +240,9 @@ igt_main
> >               int idx;
> >  
> >               device = drm_open_driver(DRIVER_INTEL);
> > +             igt_allow_hang(device, 0, 0);
> 
> Umm... That's not skipping.
> That's enabling reset, right?

It's a skip if we cannot reset either due to env flags or lack of kernel
support for reset. We are simply asking if this test group is allowed to
reset.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 05/10] igt/drv_hangman: Skip if resets are disallowed
  2017-07-28 13:53   ` Michał Winiarski
  2017-07-28 13:59     ` Chris Wilson
@ 2017-07-28 14:01     ` Chris Wilson
  1 sibling, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2017-07-28 14:01 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx

Quoting Michał Winiarski (2017-07-28 14:53:40)
> On Fri, Jul 28, 2017 at 01:08:03PM +0100, Chris Wilson wrote:
> > If we tell the machine to reset but they are disallowed, we will leave
> > the system in a wedged state, preventing the majority of subsequent
> > tests.
> > ---
> >  tests/drv_hangman.c | 3 +++
> >  1 file changed, 3 insertions(+)
> 
> Missed sign-off.
> 
> > 
> > diff --git a/tests/drv_hangman.c b/tests/drv_hangman.c
> > index 0551ec16..de57e128 100644
> > --- a/tests/drv_hangman.c
> > +++ b/tests/drv_hangman.c
> > @@ -30,6 +30,7 @@
> >  #include <limits.h>
> >  #include <sys/types.h>
> >  #include <sys/stat.h>
> > +#include <sys/ioctl.h>
> >  #include <fcntl.h>
> >  
> >  #include "igt_sysfs.h"
> > @@ -239,7 +240,9 @@ igt_main
> >               int idx;
> >  
> >               device = drm_open_driver(DRIVER_INTEL);
> > +             igt_allow_hang(device, 0, 0);
> 
> Umm... That's not skipping.
> That's enabling reset, right?

What it does need though is HANG_ALLOW_CAPTURE, since some of the tests
are about checking the error-state afterwards.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 10/10] igt/gem_eio: i915.reset is no longer a boolean
  2017-07-28 13:50     ` Chris Wilson
@ 2017-07-28 15:56       ` Michał Winiarski
  0 siblings, 0 replies; 30+ messages in thread
From: Michał Winiarski @ 2017-07-28 15:56 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jul 28, 2017 at 02:50:30PM +0100, Chris Wilson wrote:
> Quoting Michał Winiarski (2017-07-28 14:36:46)
> > On Fri, Jul 28, 2017 at 01:08:08PM +0100, Chris Wilson wrote:
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  tests/gem_eio.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/tests/gem_eio.c b/tests/gem_eio.c
> > > index 3c826626..15120842 100644
> > > --- a/tests/gem_eio.c
> > > +++ b/tests/gem_eio.c
> > > @@ -53,7 +53,7 @@ static bool i915_reset_control(bool enable)
> > >       fd = open(path, O_RDWR);
> > >       igt_require(fd >= 0);
> > >  
> > > -     ret = write(fd, &"NY"[enable], 1) == 1;
> > > +     ret = write(fd, &"01"[enable], 1) == 1;
> > 
> > That's a fun way of doing "itoa()" :) 
> > How about using helpers? We have igt_sysfs_set_parameter.
> > 
> > Other thing is the fact that we're leaving the machine in a non-default state
> > (global reset rather than per-engine reset).
> 
> If you haven't noticed, igt already does exist in a reset=[01] universe.
> We are aaiting on the per-engine reset tests to actually enable
> i915.reset=2. For coverage we need both, even i915.reset=2 may fallback
> to i915.reset=1 in some unlikely situations.
> 
> > We should actually restore the old value after the test.
> > I can also see that's "by design" (hang detector), but if we change the
> > behaviour of hang detector in the future, we may forget to modify gem_eio.
> 
> That was the flavour of irc conversation yesterday. Instead of using
> module parameters, we create a debugfs interface. The parameter will
> only be set/adjusted while the debugfs fd was held open, so that we
> would get automatic cleanup on process exit. It also means we should be
> able to kill a few more module options that shouldn't exist to tempt
> users.
> -Chris

I was just worried about potentially adding noise to other tests. If we plan to
remove the module parameter, it won't happen.

With or without the helper:

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 08/10] igt/gem_exec_fence: Exercise merging fences
  2017-07-28 12:08 ` [PATCH igt 08/10] igt/gem_exec_fence: Exercise merging fences Chris Wilson
@ 2017-07-31  8:26   ` Michał Winiarski
  2017-07-31  9:19     ` Chris Wilson
  0 siblings, 1 reply; 30+ messages in thread
From: Michał Winiarski @ 2017-07-31  8:26 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jul 28, 2017 at 01:08:06PM +0100, Chris Wilson wrote:
> Execute the same batch on each engine and check that the composite fence
> across all engines completes only after the batch is completed on every
> engine.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/gem_exec_fence.c | 120 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 120 insertions(+)
> 
> diff --git a/tests/gem_exec_fence.c b/tests/gem_exec_fence.c
> index b094f585..c7fa249b 100644
> --- a/tests/gem_exec_fence.c
> +++ b/tests/gem_exec_fence.c
> @@ -199,6 +199,111 @@ static void test_fence_busy(int fd, unsigned ring, unsigned flags)
>  	gem_quiescent_gpu(fd);
>  }
>  
> +static void test_fence_busy_all(int fd, unsigned flags)
> +{
> +	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	struct drm_i915_gem_exec_object2 obj;
> +	struct drm_i915_gem_relocation_entry reloc;
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct timespec tv;
> +	uint32_t *batch;
> +	unsigned int engine;
> +	int all, i, timeout;

[SNIP]

> +	for_each_engine(fd, engine) {
> +		int fence, new;
> +
> +		execbuf.flags = engine | LOCAL_EXEC_FENCE_OUT;
> +		execbuf.rsvd2 = -1;
> +		gem_execbuf_wr(fd, &execbuf);
> +		fence = execbuf.rsvd2 >> 32;
> +		igt_assert(fence != -1);
> +
> +		if (all < 0) {
> +			all = fence;
> +			break;
> +		}

So... In case we're getting an error from previous sync_file_merge, we'll just
silently continue rather than fail? Or am I missing something here?

> +
> +		new = sync_fence_merge(all, fence);
> +		close(all);
> +		close(fence);
> +
> +		all = new;
> +	}
> +
> +	igt_assert(gem_bo_busy(fd, obj.handle));
> +	igt_assert(fence_busy(all));
> +
> +	timeout = 120;
> +	if ((flags & HANG) == 0) {

We're never passing HANG flag to this test.

-Michał

> +		*batch = MI_BATCH_BUFFER_END;
> +		__sync_synchronize();
> +		timeout = 1;
> +	}
> +	munmap(batch, 4096);
> +
> +	if (flags & WAIT) {
> +		struct pollfd pfd = { .fd = all, .events = POLLIN };
> +		igt_assert(poll(&pfd, 1, timeout*1000) == 1);
> +	} else {
> +		memset(&tv, 0, sizeof(tv));
> +		while (fence_busy(all))
> +			igt_assert(igt_seconds_elapsed(&tv) < timeout);
> +	}
> +
> +	igt_assert(!gem_bo_busy(fd, obj.handle));
> +	igt_assert_eq(sync_fence_status(all),
> +		      flags & HANG ? -EIO : SYNC_FENCE_OK);
> +
> +	close(all);
> +	gem_close(fd, obj.handle);
> +
> +	gem_quiescent_gpu(fd);
> +}
> +
>  static void test_fence_await(int fd, unsigned ring, unsigned flags)
>  {
>  	const int gen = intel_gen(intel_get_drm_devid(fd));
> @@ -744,6 +849,21 @@ igt_main
>  		caps = print_welcome(i915);
>  	}
>  
> +	igt_subtest_group {
> +		igt_fixture {
> +			igt_fork_hang_detector(i915);
> +		}
> +
> +		igt_subtest("basic-busy-all")
> +			test_fence_busy_all(i915, 0);
> +		igt_subtest("basic-wait-all")
> +			test_fence_busy_all(i915, WAIT);
> +
> +		igt_fixture {
> +			igt_stop_hang_detector();
> +		}
> +	}
> +
>  	for (e = intel_execution_engines; e->name; e++) {
>  		igt_subtest_group {
>  			igt_fixture {
> -- 
> 2.13.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 07/10] igt/gem_exec_suspend: Try to suspend with a pending GPU hang
  2017-07-28 12:08 ` [PATCH igt 07/10] igt/gem_exec_suspend: Try to suspend with a pending GPU hang Chris Wilson
@ 2017-07-31  8:58   ` Michał Winiarski
  2017-07-31  9:02     ` Chris Wilson
  0 siblings, 1 reply; 30+ messages in thread
From: Michał Winiarski @ 2017-07-31  8:58 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jul 28, 2017 at 01:08:05PM +0100, Chris Wilson wrote:
> Ensure that we can suspend the GPU even if it is currently busy in an
> indefinite loop, requiring us to declare the task hung.

Missing sign-off.

> ---
>  tests/gem_exec_suspend.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/tests/gem_exec_suspend.c b/tests/gem_exec_suspend.c
> index 763cb108..bcb0e089 100644
> --- a/tests/gem_exec_suspend.c
> +++ b/tests/gem_exec_suspend.c
> @@ -29,6 +29,7 @@
>  
>  #include "igt.h"
>  #include "igt_gt.h"
> +#include "igt_dummyload.h"
>  
>  #define NOSLEEP 0
>  #define SUSPEND_DEVICES 1
> @@ -44,6 +45,7 @@
>  
>  #define UNCACHED (0<<8)
>  #define CACHED (1<<8)
> +#define HANG (2<<8)
>  
>  static void run_test(int fd, unsigned ring, unsigned flags);
>  
> @@ -105,6 +107,7 @@ static void run_test(int fd, unsigned engine, unsigned flags)
>  	struct drm_i915_gem_execbuffer2 execbuf;
>  	unsigned engines[16];
>  	unsigned nengine;
> +	igt_spin_t *spin = NULL;
>  
>  	nengine = 0;
>  	if (engine == -1) {
> @@ -197,6 +200,9 @@ static void run_test(int fd, unsigned engine, unsigned flags)
>  		gem_close(fd, obj[1].handle);
>  	}
>  
> +	if (flags & HANG)
> +		spin = igt_spin_batch_new(fd, 0, engine, 0);

Typo? Using different tree? (3 args here)

Other than that, lgtm:

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> +
>  	switch (mode(flags)) {
>  	case NOSLEEP:
>  		break;
> @@ -222,6 +228,8 @@ static void run_test(int fd, unsigned engine, unsigned flags)
>  		break;
>  	}
>  
> +	igt_spin_batch_free(fd, spin);
> +
>  	check_bo(fd, obj[0].handle);
>  	gem_close(fd, obj[0].handle);
>  
> @@ -244,6 +252,7 @@ igt_main
>  		{ NULL, 0 }
>  	}, *m;
>  	const struct intel_execution_engine *e;
> +	igt_hang_t hang;
>  	int fd;
>  
>  	igt_fixture {
> @@ -278,6 +287,16 @@ igt_main
>  
>  	igt_fixture {
>  		igt_stop_hang_detector();
> +		hang = igt_allow_hang(fd, 0, 0);
> +	}
> +
> +	igt_subtest("hang-S3")
> +		run_test(fd, 0, SUSPEND | HANG);
> +	igt_subtest("hang-S4")
> +		run_test(fd, 0, HIBERNATE | HANG);
> +
> +	igt_fixture {
> +		igt_disallow_hang(fd, hang);
>  		close(fd);
>  	}
>  }
> -- 
> 2.13.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 07/10] igt/gem_exec_suspend: Try to suspend with a pending GPU hang
  2017-07-31  8:58   ` Michał Winiarski
@ 2017-07-31  9:02     ` Chris Wilson
  0 siblings, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2017-07-31  9:02 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx

Quoting Michał Winiarski (2017-07-31 09:58:43)
> > @@ -197,6 +200,9 @@ static void run_test(int fd, unsigned engine, unsigned flags)
> >               gem_close(fd, obj[1].handle);
> >       }
> >  
> > +     if (flags & HANG)
> > +             spin = igt_spin_batch_new(fd, 0, engine, 0);
> 
> Typo? Using different tree? (3 args here)

Don't you have per-context dummy loads yet for basic testing of
preemption?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 06/10] igt/gem_ringfill: Prime execbuf before measuring ring size
  2017-07-28 12:08 ` [PATCH igt 06/10] igt/gem_ringfill: Prime execbuf before measuring ring size Chris Wilson
@ 2017-07-31  9:09   ` Michał Winiarski
  0 siblings, 0 replies; 30+ messages in thread
From: Michał Winiarski @ 2017-07-31  9:09 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jul 28, 2017 at 01:08:04PM +0100, Chris Wilson wrote:
> Make sure that our buffers are ready and loaded to reduce the initial
> stall.

Missing sign-off.

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> ---
>  tests/gem_ringfill.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/gem_ringfill.c b/tests/gem_ringfill.c
> index b52996a4..01cbd0a9 100644
> --- a/tests/gem_ringfill.c
> +++ b/tests/gem_ringfill.c
> @@ -288,10 +288,15 @@ static unsigned int measure_ring_size(int fd)
>  	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;
>  
> -	memset(&execbuf, 0, sizeof(execbuf));
>  	execbuf.buffers_ptr = to_user_pointer(obj);
>  	execbuf.buffer_count = 2;
>  
> @@ -302,7 +307,8 @@ static unsigned int measure_ring_size(int fd)
>  	itv.it_value.tv_usec = 1000;
>  	setitimer(ITIMER_REAL, &itv, NULL);
>  
> -	last = count = 0;
> +	last = -1;
> +	count = 0;
>  	do {
>  		if (__execbuf(fd, &execbuf) == 0) {
>  			count++;
> -- 
> 2.13.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 08/10] igt/gem_exec_fence: Exercise merging fences
  2017-07-31  8:26   ` Michał Winiarski
@ 2017-07-31  9:19     ` Chris Wilson
  0 siblings, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2017-07-31  9:19 UTC (permalink / raw)
  To: Michał Winiarski; +Cc: intel-gfx

Quoting Michał Winiarski (2017-07-31 09:26:44)
> On Fri, Jul 28, 2017 at 01:08:06PM +0100, Chris Wilson wrote:
> > +     for_each_engine(fd, engine) {
> > +             int fence, new;
> > +
> > +             execbuf.flags = engine | LOCAL_EXEC_FENCE_OUT;
> > +             execbuf.rsvd2 = -1;
> > +             gem_execbuf_wr(fd, &execbuf);
> > +             fence = execbuf.rsvd2 >> 32;
> > +             igt_assert(fence != -1);
> > +
> > +             if (all < 0) {
> > +                     all = fence;
> > +                     break;
> > +             }
> 
> So... In case we're getting an error from previous sync_file_merge, we'll just
> silently continue rather than fail? Or am I missing something here?

No, just used to the idiom that the basic igt libraries do the error
checking and you have to use the __func to obtain the error for oneself.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf
  2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
                   ` (8 preceding siblings ...)
  2017-07-28 12:08 ` [PATCH igt 10/10] igt/gem_eio: i915.reset is no longer a boolean Chris Wilson
@ 2017-07-31  9:26 ` Tvrtko Ursulin
  9 siblings, 0 replies; 30+ messages in thread
From: Tvrtko Ursulin @ 2017-07-31  9:26 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 28/07/2017 13:07, Chris Wilson wrote:
> The idea is to implement read(dmabuf) and write(dambuf) so provide some
> tests.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   tests/Makefile.sources |   1 +
>   tests/prime_rw.c       | 179 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 180 insertions(+)
>   create mode 100644 tests/prime_rw.c
> 
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 5b98a5a3..18b99768 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -225,6 +225,7 @@ TESTS_progs = \
>   	prime_mmap \
>   	prime_mmap_coherency \
>   	prime_mmap_kms \
> +	prime_rw \
>   	prime_self_import \
>   	prime_udl \
>   	prime_vgem \
> diff --git a/tests/prime_rw.c b/tests/prime_rw.c
> new file mode 100644
> index 00000000..65abec5d
> --- /dev/null
> +++ b/tests/prime_rw.c
> @@ -0,0 +1,179 @@
> +/*
> + * Copyright © 2017 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.
> + */
> +
> +/*
> + * Testcase: Check whether read()/write()ing dma-buf works
> + */
> +#define _GNU_SOURCE
> +#include <unistd.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +
> +#include "drmtest.h"
> +#include "ioctl_wrappers.h"
> +
> +static int create_dmabuf(int fd, int size)
> +{
> +	uint32_t handle;
> +	int dmabuf;
> +
> +	handle = gem_create(fd, size);
> +	dmabuf = prime_handle_to_fd_for_mmap(fd, handle);
> +	gem_close(fd, handle);
> +
> +	return dmabuf;
> +}
> +
> +static bool has_dma_buf_read(int fd)
> +{
> +	int dmabuf;
> +	int ret;
> +
> +	dmabuf = create_dmabuf(fd, 4096);
> +	ret = read(dmabuf, NULL, 0);
> +	close(dmabuf);
> +
> +	errno = 0;
> +	return ret == 0;
> +}
> +
> +static bool has_dma_buf_write(int fd)
> +{
> +	int dmabuf;
> +	int ret;
> +
> +	dmabuf = create_dmabuf(fd, 4096);
> +	ret = write(dmabuf, NULL, 0);
> +	close(dmabuf);
> +
> +	errno = 0;
> +	return ret == 0;
> +}
> +
> +static void test_read(int fd)
> +{
> +	uint32_t buf[4096];
> +	uint32_t *ptr;
> +	int i, len, offset;
> +	int dmabuf;
> +
> +	dmabuf = create_dmabuf(fd, sizeof(buf));
> +	igt_assert(dmabuf != -1);
> +
> +	ptr = mmap(NULL, sizeof(buf), PROT_WRITE, MAP_SHARED, dmabuf, 0);
> +	igt_assert(ptr != MAP_FAILED);
> +
> +	prime_sync_start(dmabuf, true);
> +	for (i = 0; i < ARRAY_SIZE(buf); i++)
> +		ptr[i] = i;
> +	prime_sync_end(dmabuf, true);
> +
> +	munmap(ptr, sizeof(buf));
> +
> +	prime_sync_start(dmabuf, false);
> +	for (len = 1; len <= 4096; len <<= 1) {
> +		const int sz = len * sizeof(buf[0]);
> +
> +		offset = len/2;

Spaces for coding style.

> +		lseek(dmabuf, offset * sizeof(buf[0]), SEEK_SET);
> +		for (; offset + len <= 4096; offset += len) {
> +			igt_assert_eq(read(dmabuf, buf, sz), sz);
> +			for (i = 0; i < len; i++)
> +				igt_assert_eq(buf[i], offset + i);
> +		}
> +		igt_assert_eq(read(dmabuf, buf, sz),
> +			      (4096 - offset) * sizeof(buf[0]));
> +		igt_assert_eq(read(dmabuf, buf, sz), 0);
> +	}
> +	prime_sync_end(dmabuf, false);

Why so elaborate reading pattern in a basic test? Could you drop a 
comment before the block describing the pattern?

> +
> +	close(dmabuf);
> +}
> +
> +static void test_write(int fd)
> +{
> +	uint32_t buf[4096];
> +	uint32_t *ptr;
> +	int i, len, offset;
> +	int dmabuf;
> +
> +	dmabuf = create_dmabuf(fd, sizeof(buf));
> +	igt_assert(dmabuf != -1);
> +
> +	for (i = 0; i < ARRAY_SIZE(buf); i++)
> +		buf[i] = i;
> +
> +	ptr = mmap(NULL, sizeof(buf), PROT_READ, MAP_SHARED, dmabuf, 0);
> +	igt_assert(ptr != MAP_FAILED);
> +
> +	prime_sync_start(dmabuf, true);
> +	for (len = 1; len <= 4096; len <<= 1) {
> +		const int sz = len * sizeof(buf[0]);
> +
> +		offset = len/2;
> +		lseek(dmabuf, offset * sizeof(buf[0]), SEEK_SET);
> +		for (; offset + len <= 4096; offset += len) {
> +			igt_assert_eq(write(dmabuf, buf, sz), sz);
> +			for (i = 0; i < len; i++)
> +				igt_assert_eq(ptr[i + offset], i);
> +		}
> +		igt_assert_eq(write(dmabuf, buf, sz),
> +			      (4096 - offset) * sizeof(buf[0]));
> +		igt_assert_eq(write(dmabuf, buf, sz), 0);
> +	}
> +	prime_sync_end(dmabuf, true);

Same comments as above.

Also, I don't know what is the prime API supposed to be when mixing 
read/write and mmap accesses? Is intermixing with no 
prime_sync_start/end every time OK?

> +
> +	munmap(ptr, sizeof(buf));
> +
> +	close(dmabuf);
> +}
> +
> +igt_main
> +{
> +	int fd = -1;
> +
> +	igt_fixture {
> +		fd = drm_open_driver(DRIVER_INTEL);
> +	}
> +
> +	igt_subtest_group {
> +		igt_fixture {
> +			igt_require((has_dma_buf_read(fd)));
> +		}
> +
> +		igt_subtest("basic-read")
> +			test_read(fd);
> +	}
> +
> +	igt_subtest_group {
> +		igt_fixture {
> +			igt_require((has_dma_buf_write(fd)));
> +		}
> +
> +		igt_subtest("basic-write")
> +			test_write(fd);
> +	}
> +
> +	igt_fixture
> +		close(fd);
> +}
> 

Regards,

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace()
  2017-07-28 12:08 ` [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace() Chris Wilson
@ 2017-07-31  9:41   ` Tvrtko Ursulin
  2017-07-31 10:17     ` Chris Wilson
  0 siblings, 1 reply; 30+ messages in thread
From: Tvrtko Ursulin @ 2017-07-31  9:41 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 28/07/2017 13:08, Chris Wilson wrote:
> gdb uses ptrace() to peek and poke bytes of the target's address space.
> The kernel must implement an vm_ops->access() handler or else gdb will
> be unable to inspect the pointer and report it as out-of-bounds. Worse
> than useless as it causes immediate suspicion of the valid GTT pointer.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   tests/gem_mmap_gtt.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 79 insertions(+)
> 
> diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c
> index 4ff5e7f1..61c08406 100644
> --- a/tests/gem_mmap_gtt.c
> +++ b/tests/gem_mmap_gtt.c
> @@ -36,6 +36,8 @@
>   #include <errno.h>
>   #include <sys/stat.h>
>   #include <sys/ioctl.h>
> +#include <sys/ptrace.h>
> +#include <sys/wait.h>
>   #include "drm.h"
>   
>   #include "igt.h"
> @@ -310,6 +312,81 @@ test_write_gtt(int fd)
>   	munmap(src, OBJECT_SIZE);
>   }
>   
> +static void *memchr_inv(const void *s, int c, size_t n)
> +{
> +	const uint8_t *us = s;
> +	const uint8_t uc = c;
> +
> +#pragma GCC diagnostic push
> +#pragma GCC diagnostic ignored "-Wcast-qual"
> +	while (n--) {
> +		if (*us != uc)
> +			return (void *) us;
> +		us++;
> +	}
> +#pragma GCC diagnostic pop

Stripping away constness wouldn't work with a simpler:

uint8_t *us = (uint8_t *)s?

> +
> +	return NULL;
> +}
> +
> +static void
> +test_ptrace(int fd)
> +{
> +	long AA, CC;
> +	long *gtt, *copy;
> +	uint32_t bo;
> +	pid_t pid;
> +
> +	memset(&AA, 0xaa, sizeof(AA));
> +	memset(&CC, 0x55, sizeof(CC));
> +
> +	copy = malloc(OBJECT_SIZE);
> +	memset(copy, AA, OBJECT_SIZE);
> +
> +	bo = gem_create(fd, OBJECT_SIZE);
> +	gtt = mmap_bo(fd, bo);
> +	memset(gtt, CC, OBJECT_SIZE);
> +	gem_close(fd, bo);
> +
> +	igt_assert(!memchr_inv(gtt, CC, OBJECT_SIZE));
> +	igt_assert(!memchr_inv(copy, AA, OBJECT_SIZE));
> +
> +	switch ((pid = fork())) {
> +		case -1:
> +			igt_assert(pid != -1);
> +			break;
> +
> +		case 0:
> +			ptrace(PTRACE_TRACEME, 0, NULL, NULL);
> +			raise(SIGSTOP);
> +			raise(SIGKILL);
> +			exit(0);
> +			break;
> +
> +		default:
> +			/* Wait for the child to ready themselves */
> +			wait(NULL);
> +
> +			ptrace(PTRACE_ATTACH, pid, NULL, NULL);
> +			for (int i = 0; i < OBJECT_SIZE/sizeof(long); i++) {
> +				copy[i] = ptrace(PTRACE_PEEKDATA, pid, gtt+i, NULL);
> +				ptrace(PTRACE_POKEDATA, pid, gtt + i, AA);

Inconsistent whitespace in the above three lines. First and second need 
a tidy.

> +			}
> +			ptrace(PTRACE_DETACH, pid, NULL, NULL);
> +
> +			/* Wakeup the child for it to exit */
> +			kill(SIGCONT, pid);
> +			break;
> +	}
> +
> +	/* The contents of the two buffers should now be swapped */
> +	igt_assert(!memchr_inv(gtt, AA, OBJECT_SIZE));
> +	igt_assert(!memchr_inv(copy, CC, OBJECT_SIZE));
> +
> +	munmap(gtt, OBJECT_SIZE);
> +	free(copy);
> +}
> +
>   static void
>   test_coherency(int fd)
>   {
> @@ -809,6 +886,8 @@ igt_main
>   		test_write(fd);
>   	igt_subtest("basic-write-gtt")
>   		test_write_gtt(fd);
> +	igt_subtest("ptrace")
> +		test_ptrace(fd);
>   	igt_subtest("coherency")
>   		test_coherency(fd);
>   	igt_subtest("clflush")
> 

Apart from the little details, and ptrace API which I haven't looked in 
detail (if it works it works), looks simple enough.

Regards,

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 03/10] igt/gem_ctx_switch: Exercise all engines at once
  2017-07-28 12:08 ` [PATCH igt 03/10] igt/gem_ctx_switch: Exercise all engines at once Chris Wilson
@ 2017-07-31  9:57   ` Tvrtko Ursulin
  0 siblings, 0 replies; 30+ messages in thread
From: Tvrtko Ursulin @ 2017-07-31  9:57 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 28/07/2017 13:08, Chris Wilson wrote:
> Just a small variant to apply a continuous context-switch load to all
> engines.
> ---
>   tests/gem_ctx_switch.c | 80 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 80 insertions(+)
> 
> diff --git a/tests/gem_ctx_switch.c b/tests/gem_ctx_switch.c
> index b6ea71cf..df22efec 100644
> --- a/tests/gem_ctx_switch.c
> +++ b/tests/gem_ctx_switch.c
> @@ -140,6 +140,81 @@ static void single(int fd, uint32_t handle,
>   		gem_context_destroy(fd, contexts[n]);
>   }
>   
> +static void all(int fd, uint32_t handle, unsigned flags, int timeout)
> +{
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct drm_i915_gem_exec_object2 obj[2];
> +	unsigned int engine[16], e;

Need to add MAX_ENGINES somewhere in lib/ for maintainability.

> +	const char *name[16];
> +	uint32_t contexts[64];
> +	unsigned int nengine;
> +	int n;
> +
> +	nengine = 0;
> +	for_each_engine(fd, e) {
> +		if (e == 0 || e == I915_EXEC_BSD)
> +			continue;

Engine discrimination! :) Why? BSD1 won't work except on large GTs.

Also, if we cannot agree to drop I915_EXEC_DEFAULT from the 
for_each_engine list, I think we at least need to stop the "e == 0" 
magic which is sprinkled around the tests now. Even though this very 
fact demonstrates having it in a list is most often not what is wanted.

Not sure exactly what to suggest as an alternative. for_each_hw_engine? 
real engine? gt engine?

> +
> +		engine[nengine] = e;
> +		name[nengine] = e__->name;
> +		nengine++;
> +	}
> +	igt_require(nengine);
> +
> +	igt_require(__gem_context_create(fd, &contexts[0]) == 0);

Feels to opaque - should we pencil in adding of 
igt_require_context_support or something?

> +	for (n = 1; n < 64; n++)
> +		contexts[n] = gem_context_create(fd);
> +
> +	memset(obj, 0, sizeof(obj));
> +	obj[1].handle = handle;
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj + 1);
> +	execbuf.buffer_count = 1;
> +	execbuf.rsvd1 = contexts[0];
> +	execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT;
> +	execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC;
> +	igt_require(__gem_execbuf(fd, &execbuf) == 0);
> +	if (__gem_execbuf(fd, &execbuf)) {

Why the same execbuf two times in a row?

> +		execbuf.flags = 0;
> +		gem_execbuf(fd, &execbuf);
> +	}
> +	gem_sync(fd, handle);

What is the purpose of exercising context[0] before the main loop below?

> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = 2;
> +
> +	igt_fork(child, nengine) {
> +		struct timespec start, now;
> +		unsigned int count = 0;
> +
> +		obj[0].handle = gem_create(fd, 4096);
> +		execbuf.flags |= engine[child];
> +		gem_execbuf(fd, &execbuf);
> +		gem_sync(fd, obj[0].handle);
> +
> +		clock_gettime(CLOCK_MONOTONIC, &start);
> +		do {
> +			for (int loop = 0; loop < 64; loop++) {
> +				execbuf.rsvd1 = contexts[loop % 64];
> +				gem_execbuf(fd, &execbuf);
> +			}
> +			count += 64;
> +			clock_gettime(CLOCK_MONOTONIC, &now);
> +		} while (elapsed(&start, &now) < timeout);
> +		gem_sync(fd, obj[0].handle);
> +		clock_gettime(CLOCK_MONOTONIC, &now);
> +		gem_close(fd, obj[0].handle);
> +
> +		igt_info("[%d] %s: %'u cycles: %.3fus%s\n",
> +			 child, name[child], count, elapsed(&start, &now)*1e6 / count,
> +			 flags & INTERRUPTIBLE ? " (interruptible)" : "");
> +	}
> +	igt_waitchildren();
> +
> +	for (n = 0; n < 64; n++)
> +		gem_context_destroy(fd, contexts[n]);
> +}
> +
>   igt_main
>   {
>   	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
> @@ -177,6 +252,11 @@ igt_main
>   			single(fd, light, e, INTERRUPTIBLE, ncpus, 150);
>   	}
>   
> +	igt_subtest("basic-all")
> +		all(fd, light, 0, 20);
> +	igt_subtest("basic-all-heavy")
> +		all(fd, heavy, 0, 20);
> +
>   	igt_fixture {
>   		igt_stop_hang_detector();
>   		gem_close(fd, heavy);
> 

Regards,

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace()
  2017-07-31  9:41   ` Tvrtko Ursulin
@ 2017-07-31 10:17     ` Chris Wilson
  2017-07-31 10:39       ` Tvrtko Ursulin
  0 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2017-07-31 10:17 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2017-07-31 10:41:06)
> 
> On 28/07/2017 13:08, Chris Wilson wrote:
> > gdb uses ptrace() to peek and poke bytes of the target's address space.
> > The kernel must implement an vm_ops->access() handler or else gdb will
> > be unable to inspect the pointer and report it as out-of-bounds. Worse
> > than useless as it causes immediate suspicion of the valid GTT pointer.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   tests/gem_mmap_gtt.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 79 insertions(+)
> > 
> > diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c
> > index 4ff5e7f1..61c08406 100644
> > --- a/tests/gem_mmap_gtt.c
> > +++ b/tests/gem_mmap_gtt.c
> > @@ -36,6 +36,8 @@
> >   #include <errno.h>
> >   #include <sys/stat.h>
> >   #include <sys/ioctl.h>
> > +#include <sys/ptrace.h>
> > +#include <sys/wait.h>
> >   #include "drm.h"
> >   
> >   #include "igt.h"
> > @@ -310,6 +312,81 @@ test_write_gtt(int fd)
> >       munmap(src, OBJECT_SIZE);
> >   }
> >   
> > +static void *memchr_inv(const void *s, int c, size_t n)
> > +{
> > +     const uint8_t *us = s;
> > +     const uint8_t uc = c;
> > +
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wcast-qual"
> > +     while (n--) {
> > +             if (*us != uc)
> > +                     return (void *) us;
> > +             us++;
> > +     }
> > +#pragma GCC diagnostic pop
> 
> Stripping away constness wouldn't work with a simpler:

Or just saying const void *memchr_inv().

In this case we don't need memchr_inv, as just a bool will do,
so memchr_eq().

> 
> uint8_t *us = (uint8_t *)s?
> 
> > +
> > +     return NULL;
> > +}
> > +
> > +static void
> > +test_ptrace(int fd)
> > +{
> > +     long AA, CC;
> > +     long *gtt, *copy;
> > +     uint32_t bo;
> > +     pid_t pid;
> > +
> > +     memset(&AA, 0xaa, sizeof(AA));
> > +     memset(&CC, 0x55, sizeof(CC));
> > +
> > +     copy = malloc(OBJECT_SIZE);
> > +     memset(copy, AA, OBJECT_SIZE);
> > +
> > +     bo = gem_create(fd, OBJECT_SIZE);
> > +     gtt = mmap_bo(fd, bo);
> > +     memset(gtt, CC, OBJECT_SIZE);
> > +     gem_close(fd, bo);
> > +
> > +     igt_assert(!memchr_inv(gtt, CC, OBJECT_SIZE));
> > +     igt_assert(!memchr_inv(copy, AA, OBJECT_SIZE));
> > +
> > +     switch ((pid = fork())) {
> > +             case -1:
> > +                     igt_assert(pid != -1);
> > +                     break;
> > +
> > +             case 0:
> > +                     ptrace(PTRACE_TRACEME, 0, NULL, NULL);
> > +                     raise(SIGSTOP);
> > +                     raise(SIGKILL);
> > +                     exit(0);
> > +                     break;
> > +
> > +             default:
> > +                     /* Wait for the child to ready themselves */
> > +                     wait(NULL);
> > +
> > +                     ptrace(PTRACE_ATTACH, pid, NULL, NULL);
> > +                     for (int i = 0; i < OBJECT_SIZE/sizeof(long); i++) {
> > +                             copy[i] = ptrace(PTRACE_PEEKDATA, pid, gtt+i, NULL);
> > +                             ptrace(PTRACE_POKEDATA, pid, gtt + i, AA);
> 
> Inconsistent whitespace in the above three lines. First and second need 
> a tidy.

? The whitespace carries meaning for me.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace()
  2017-07-31 10:17     ` Chris Wilson
@ 2017-07-31 10:39       ` Tvrtko Ursulin
  0 siblings, 0 replies; 30+ messages in thread
From: Tvrtko Ursulin @ 2017-07-31 10:39 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 31/07/2017 11:17, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-07-31 10:41:06)
>>
>> On 28/07/2017 13:08, Chris Wilson wrote:
>>> gdb uses ptrace() to peek and poke bytes of the target's address space.
>>> The kernel must implement an vm_ops->access() handler or else gdb will
>>> be unable to inspect the pointer and report it as out-of-bounds. Worse
>>> than useless as it causes immediate suspicion of the valid GTT pointer.
>>>
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    tests/gem_mmap_gtt.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 79 insertions(+)
>>>
>>> diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c
>>> index 4ff5e7f1..61c08406 100644
>>> --- a/tests/gem_mmap_gtt.c
>>> +++ b/tests/gem_mmap_gtt.c
>>> @@ -36,6 +36,8 @@
>>>    #include <errno.h>
>>>    #include <sys/stat.h>
>>>    #include <sys/ioctl.h>
>>> +#include <sys/ptrace.h>
>>> +#include <sys/wait.h>
>>>    #include "drm.h"
>>>    
>>>    #include "igt.h"
>>> @@ -310,6 +312,81 @@ test_write_gtt(int fd)
>>>        munmap(src, OBJECT_SIZE);
>>>    }
>>>    
>>> +static void *memchr_inv(const void *s, int c, size_t n)
>>> +{
>>> +     const uint8_t *us = s;
>>> +     const uint8_t uc = c;
>>> +
>>> +#pragma GCC diagnostic push
>>> +#pragma GCC diagnostic ignored "-Wcast-qual"
>>> +     while (n--) {
>>> +             if (*us != uc)
>>> +                     return (void *) us;
>>> +             us++;
>>> +     }
>>> +#pragma GCC diagnostic pop
>>
>> Stripping away constness wouldn't work with a simpler:
> 
> Or just saying const void *memchr_inv().
> 
> In this case we don't need memchr_inv, as just a bool will do,
> so memchr_eq().
> 
>>
>> uint8_t *us = (uint8_t *)s?
>>
>>> +
>>> +     return NULL;
>>> +}
>>> +
>>> +static void
>>> +test_ptrace(int fd)
>>> +{
>>> +     long AA, CC;
>>> +     long *gtt, *copy;
>>> +     uint32_t bo;
>>> +     pid_t pid;
>>> +
>>> +     memset(&AA, 0xaa, sizeof(AA));
>>> +     memset(&CC, 0x55, sizeof(CC));
>>> +
>>> +     copy = malloc(OBJECT_SIZE);
>>> +     memset(copy, AA, OBJECT_SIZE);
>>> +
>>> +     bo = gem_create(fd, OBJECT_SIZE);
>>> +     gtt = mmap_bo(fd, bo);
>>> +     memset(gtt, CC, OBJECT_SIZE);
>>> +     gem_close(fd, bo);
>>> +
>>> +     igt_assert(!memchr_inv(gtt, CC, OBJECT_SIZE));
>>> +     igt_assert(!memchr_inv(copy, AA, OBJECT_SIZE));
>>> +
>>> +     switch ((pid = fork())) {
>>> +             case -1:
>>> +                     igt_assert(pid != -1);
>>> +                     break;
>>> +
>>> +             case 0:
>>> +                     ptrace(PTRACE_TRACEME, 0, NULL, NULL);
>>> +                     raise(SIGSTOP);
>>> +                     raise(SIGKILL);
>>> +                     exit(0);
>>> +                     break;
>>> +
>>> +             default:
>>> +                     /* Wait for the child to ready themselves */
>>> +                     wait(NULL);
>>> +
>>> +                     ptrace(PTRACE_ATTACH, pid, NULL, NULL);
>>> +                     for (int i = 0; i < OBJECT_SIZE/sizeof(long); i++) {
>>> +                             copy[i] = ptrace(PTRACE_PEEKDATA, pid, gtt+i, NULL);
>>> +                             ptrace(PTRACE_POKEDATA, pid, gtt + i, AA);
>>
>> Inconsistent whitespace in the above three lines. First and second need
>> a tidy.
> 
> ? The whitespace carries meaning for me.

"gtt+i" / "gtt + i" ? And "OBJECT_SIZE/sizeof". I know it's only IGT but 
lets be consistent and follow our guidelines.

Regards,

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels
  2017-07-28 12:08 ` [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels Chris Wilson
@ 2017-07-31 14:35   ` Tvrtko Ursulin
  2017-07-31 15:21     ` Chris Wilson
  2017-07-31 15:27   ` Michał Winiarski
  1 sibling, 1 reply; 30+ messages in thread
From: Tvrtko Ursulin @ 2017-07-31 14:35 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx


On 28/07/2017 13:08, Chris Wilson wrote:
> Apply a little more stress to the scheduler.
> ---
>   lib/igt_rand.h            |   6 +++
>   tests/gem_exec_schedule.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-
>   2 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_rand.h b/lib/igt_rand.h
> index f664af41..c9cb3243 100644
> --- a/lib/igt_rand.h
> +++ b/lib/igt_rand.h
> @@ -38,4 +38,10 @@ static inline void hars_petruska_f54_1_random_perturb(uint32_t xor)
>   	hars_petruska_f54_1_random_seed(hars_petruska_f54_1_random_unsafe());
>   }
>   
> +/* Returns: pseudo-random number in interval [0, ep_ro) */
> +static inline uint32_t hars_petruska_f54_1_random_unsafe_max(uint32_t ep_ro)
> +{
> +	return ((uint64_t)hars_petruska_f54_1_random_unsafe() * ep_ro) >> 32;
> +}
> +
>   #endif /* IGT_RAND_H */
> diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
> index 545dcc2e..1b6e29be 100644
> --- a/tests/gem_exec_schedule.c
> +++ b/tests/gem_exec_schedule.c
> @@ -25,6 +25,7 @@
>   
>   #include "igt.h"
>   #include "igt_vgem.h"
> +#include "igt_rand.h"
>   
>   #define LOCAL_PARAM_HAS_SCHEDULER 41
>   #define LOCAL_CONTEXT_PARAM_PRIORITY 6
> @@ -491,7 +492,7 @@ static void wide(int fd, unsigned ring)
>   			I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
>   	for (int n = 0; n < NCTX; n++)
>   		igt_assert_eq_u32(ptr[n], ctx[n]);
> -	munmap(ptr, 4096);
> +	munmap(ptr, 4*NCTX);

If this patch gets stuck a bit it is worth splitting this out.

>   
>   	gem_close(fd, obj.handle);
>   	gem_close(fd, result);
> @@ -500,6 +501,108 @@ static void wide(int fd, unsigned ring)
>   #undef XS
>   }
>   
> +static void reorder_wide(int fd, unsigned ring)
> +{
> +	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	struct drm_i915_gem_relocation_entry reloc;
> +	struct drm_i915_gem_exec_object2 obj[3];
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct cork cork;
> +	uint32_t result, target;
> +	uint32_t *busy;
> +	uint32_t *r, *t;
> +
> +	result = gem_create(fd, 4096);
> +	target = gem_create(fd, 4096);
> +
> +	busy = make_busy(fd, result, ring);

What does make_busy do? It submits eight magic batches which I guess 
will not finish until finish_busy? But why eight of them? And..

> +	plug(fd, &cork);

... why do we need that since we also control when the below will be 
runnable via this?

I think it is time to put some more comments in IGTs to help other 
people looking at the code. High level description of a subtest at 
least, plus a few notes on the implementation approach.

> +
> +	t = 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[1].handle = result;
> +	obj[2].relocs_ptr = to_user_pointer(&reloc);
> +	obj[2].relocation_count = 1;
> +
> +	memset(&reloc, 0, sizeof(reloc));
> +	reloc.target_handle = result;
> +	reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> +	reloc.write_domain = 0; /* lies */
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = 3;
> +	execbuf.flags = ring;
> +	if (gen < 6)
> +		execbuf.flags |= I915_EXEC_SECURE;
> +
> +	for (int n = -MAX_PRIO, x = 1; n <= MAX_PRIO; n++, x++) {
> +		uint32_t *batch;
> +
> +		execbuf.rsvd1 = gem_context_create(fd);
> +		ctx_set_priority(fd, execbuf.rsvd1, n);
> +
> +		obj[2].handle = gem_create(fd, 128 * 64);

What is the significance od 128 and 64?

> +		batch = gem_mmap__gtt(fd, obj[2].handle, 128 * 64, PROT_WRITE);
> +		gem_set_domain(fd, obj[2].handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +
> +		for (int m = 0; m < 128; m++) {
> +			uint64_t addr;
> +			int idx = hars_petruska_f54_1_random_unsafe_max( 1024);

Stray space.

> +			int i;
> +
> +			execbuf.batch_start_offset = m * 64;
> +			reloc.offset = execbuf.batch_start_offset + sizeof(uint32_t);
> +			reloc.delta = idx * sizeof(uint32_t);
> +			addr = reloc.presumed_offset + reloc.delta;
> +
> +			i = execbuf.batch_start_offset / sizeof(uint32_t);
> +			batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +			if (gen >= 8) {
> +				batch[++i] = addr;
> +				batch[++i] = addr >> 32;
> +			} else if (gen >= 4) {
> +				batch[++i] = 0;
> +				batch[++i] = addr;
> +				reloc.offset += sizeof(uint32_t);
> +			} else {
> +				batch[i]--;
> +				batch[++i] = addr;
> +			}
> +			batch[++i] = x;

I wonder is recording another todo, converting all the store_dword_imm 
instruction building to a generic helper would be feasible.

> +			batch[++i] = MI_BATCH_BUFFER_END;
> +
> +			if (!t[idx])
> +				t[idx] =  x;

bo named target is used only from the cpu side? It could be just some 
normal allocated memory in this case if I haven't missed something.

> +
> +			gem_execbuf(fd, &execbuf);
> +		}
> +
> +		munmap(batch, 128 * 64);
> +		gem_close(fd, obj[2].handle);
> +		gem_context_destroy(fd, execbuf.rsvd1);

Does the ABI guarantee this field will be preserved?

> +	}
> +
> +	igt_assert(gem_bo_busy(fd, result));
> +	unplug(&cork); /* only now submit our batches */
> +	igt_debugfs_dump(fd, "i915_engine_info");
> +	finish_busy(busy);
> +
> +	r = gem_mmap__gtt(fd, result, 4096, PROT_READ);
> +	gem_set_domain(fd, result, /* no write hazard lies! */
> +			I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +	for (int n = 0; n < 1024; n++)
> +		igt_assert_eq_u32(r[n], t[n]);
> +	munmap(r, 4096);
> +	munmap(t, 4096);
> +
> +	gem_close(fd, result);
> +	gem_close(fd, target);
> +}
> +
>   static bool has_scheduler(int fd)
>   {
>   	drm_i915_getparam_t gp;
> @@ -571,6 +674,9 @@ igt_main
>   
>   				igt_subtest_f("wide-%s", e->name)
>   					wide(fd, e->exec_id | e->flags);
> +
> +				igt_subtest_f("reorder-wide-%s", e->name)
> +					reorder_wide(fd, e->exec_id | e->flags);
>   			}
>   		}
>   	}
> 

Regards,

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels
  2017-07-31 14:35   ` Tvrtko Ursulin
@ 2017-07-31 15:21     ` Chris Wilson
  0 siblings, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2017-07-31 15:21 UTC (permalink / raw)
  To: Tvrtko Ursulin, intel-gfx

Quoting Tvrtko Ursulin (2017-07-31 15:35:45)
> 
> On 28/07/2017 13:08, Chris Wilson wrote:
> > +static void reorder_wide(int fd, unsigned ring)
> > +{
> > +     const int gen = intel_gen(intel_get_drm_devid(fd));
> > +     struct drm_i915_gem_relocation_entry reloc;
> > +     struct drm_i915_gem_exec_object2 obj[3];
> > +     struct drm_i915_gem_execbuffer2 execbuf;
> > +     struct cork cork;
> > +     uint32_t result, target;
> > +     uint32_t *busy;
> > +     uint32_t *r, *t;
> > +
> > +     result = gem_create(fd, 4096);
> > +     target = gem_create(fd, 4096);
> > +
> > +     busy = make_busy(fd, result, ring);
> 
> What does make_busy do? It submits eight magic batches which I guess 
> will not finish until finish_busy? But why eight of them? And..
> 
> > +     plug(fd, &cork);
> 
> ... why do we need that since we also control when the below will be 
> runnable via this?

Does it? We only need vgem for the test, make_busy was a precursor that
only works for current and previous execlists.
 
> I think it is time to put some more comments in IGTs to help other 
> people looking at the code. High level description of a subtest at 
> least, plus a few notes on the implementation approach.
> 
> > +
> > +     t = 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[1].handle = result;
> > +     obj[2].relocs_ptr = to_user_pointer(&reloc);
> > +     obj[2].relocation_count = 1;
> > +
> > +     memset(&reloc, 0, sizeof(reloc));
> > +     reloc.target_handle = result;
> > +     reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> > +     reloc.write_domain = 0; /* lies */
> > +
> > +     memset(&execbuf, 0, sizeof(execbuf));
> > +     execbuf.buffers_ptr = to_user_pointer(obj);
> > +     execbuf.buffer_count = 3;
> > +     execbuf.flags = ring;
> > +     if (gen < 6)
> > +             execbuf.flags |= I915_EXEC_SECURE;
> > +
> > +     for (int n = -MAX_PRIO, x = 1; n <= MAX_PRIO; n++, x++) {
> > +             uint32_t *batch;
> > +
> > +             execbuf.rsvd1 = gem_context_create(fd);
> > +             ctx_set_priority(fd, execbuf.rsvd1, n);
> > +
> > +             obj[2].handle = gem_create(fd, 128 * 64);
> 
> What is the significance od 128 and 64?

128 is an approximation for ring_size and 64 is the aligned size of each
batch (ilk imposes the harshest alignment restriction of batch offsets).

> > +
> > +                     gem_execbuf(fd, &execbuf);
> > +             }
> > +
> > +             munmap(batch, 128 * 64);
> > +             gem_close(fd, obj[2].handle);
> > +             gem_context_destroy(fd, execbuf.rsvd1);
> 
> Does the ABI guarantee this field will be preserved?

For two reasons:

- we only use gem_execbuf() so the execbuf arg is read-only for the
  kernel

- it's an input parameter that has to maintained so that userspace can
  loop over the ioctl to handle errors.

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels
  2017-07-28 12:08 ` [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels Chris Wilson
  2017-07-31 14:35   ` Tvrtko Ursulin
@ 2017-07-31 15:27   ` Michał Winiarski
  1 sibling, 0 replies; 30+ messages in thread
From: Michał Winiarski @ 2017-07-31 15:27 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, Jul 28, 2017 at 01:08:02PM +0100, Chris Wilson wrote:
> Apply a little more stress to the scheduler.

Missing sign-off.

> ---
>  lib/igt_rand.h            |   6 +++
>  tests/gem_exec_schedule.c | 108 +++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 113 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_rand.h b/lib/igt_rand.h
> index f664af41..c9cb3243 100644
> --- a/lib/igt_rand.h
> +++ b/lib/igt_rand.h
> @@ -38,4 +38,10 @@ static inline void hars_petruska_f54_1_random_perturb(uint32_t xor)
>  	hars_petruska_f54_1_random_seed(hars_petruska_f54_1_random_unsafe());
>  }
>  
> +/* Returns: pseudo-random number in interval [0, ep_ro) */
> +static inline uint32_t hars_petruska_f54_1_random_unsafe_max(uint32_t ep_ro)
> +{
> +	return ((uint64_t)hars_petruska_f54_1_random_unsafe() * ep_ro) >> 32;
> +}
> +

Can we have either this in header or hars_petruska_f54_1_random_unsafe() here?
To have the two next to each other?

>  #endif /* IGT_RAND_H */
> diff --git a/tests/gem_exec_schedule.c b/tests/gem_exec_schedule.c
> index 545dcc2e..1b6e29be 100644
> --- a/tests/gem_exec_schedule.c
> +++ b/tests/gem_exec_schedule.c
> @@ -25,6 +25,7 @@
>  
>  #include "igt.h"
>  #include "igt_vgem.h"
> +#include "igt_rand.h"
>  
>  #define LOCAL_PARAM_HAS_SCHEDULER 41
>  #define LOCAL_CONTEXT_PARAM_PRIORITY 6
> @@ -491,7 +492,7 @@ static void wide(int fd, unsigned ring)
>  			I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
>  	for (int n = 0; n < NCTX; n++)
>  		igt_assert_eq_u32(ptr[n], ctx[n]);
> -	munmap(ptr, 4096);
> +	munmap(ptr, 4*NCTX);

s/CTX/NCTX in "#undef CTX" just outside of context while you're here?

>  
>  	gem_close(fd, obj.handle);
>  	gem_close(fd, result);
> @@ -500,6 +501,108 @@ static void wide(int fd, unsigned ring)
>  #undef XS
>  }
>  
> +static void reorder_wide(int fd, unsigned ring)
> +{
> +	const int gen = intel_gen(intel_get_drm_devid(fd));
> +	struct drm_i915_gem_relocation_entry reloc;
> +	struct drm_i915_gem_exec_object2 obj[3];
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	struct cork cork;
> +	uint32_t result, target;
> +	uint32_t *busy;
> +	uint32_t *r, *t;
> +
> +	result = gem_create(fd, 4096);
> +	target = gem_create(fd, 4096);
> +
> +	busy = make_busy(fd, result, ring);
> +	plug(fd, &cork);
> +
> +	t = 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[1].handle = result;
> +	obj[2].relocs_ptr = to_user_pointer(&reloc);
> +	obj[2].relocation_count = 1;
> +
> +	memset(&reloc, 0, sizeof(reloc));
> +	reloc.target_handle = result;
> +	reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> +	reloc.write_domain = 0; /* lies */
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = 3;
> +	execbuf.flags = ring;
> +	if (gen < 6)
> +		execbuf.flags |= I915_EXEC_SECURE;
> +
> +	for (int n = -MAX_PRIO, x = 1; n <= MAX_PRIO; n++, x++) {
> +		uint32_t *batch;
> +
> +		execbuf.rsvd1 = gem_context_create(fd);
> +		ctx_set_priority(fd, execbuf.rsvd1, n);
> +
> +		obj[2].handle = gem_create(fd, 128 * 64);
> +		batch = gem_mmap__gtt(fd, obj[2].handle, 128 * 64, PROT_WRITE);
> +		gem_set_domain(fd, obj[2].handle, I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +
> +		for (int m = 0; m < 128; m++) {

We can #define that similar to wide()

> +			uint64_t addr;
> +			int idx = hars_petruska_f54_1_random_unsafe_max( 1024);

Whitespace.

> +			int i;
> +
> +			execbuf.batch_start_offset = m * 64;
> +			reloc.offset = execbuf.batch_start_offset + sizeof(uint32_t);
> +			reloc.delta = idx * sizeof(uint32_t);
> +			addr = reloc.presumed_offset + reloc.delta;
> +
> +			i = execbuf.batch_start_offset / sizeof(uint32_t);
> +			batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
> +			if (gen >= 8) {
> +				batch[++i] = addr;
> +				batch[++i] = addr >> 32;
> +			} else if (gen >= 4) {
> +				batch[++i] = 0;
> +				batch[++i] = addr;
> +				reloc.offset += sizeof(uint32_t);
> +			} else {
> +				batch[i]--;
> +				batch[++i] = addr;
> +			}
> +			batch[++i] = x;
> +			batch[++i] = MI_BATCH_BUFFER_END;
> +
> +			if (!t[idx])
> +				t[idx] =  x;

Whitespace.

> +
> +			gem_execbuf(fd, &execbuf);
> +		}
> +
> +		munmap(batch, 128 * 64);
> +		gem_close(fd, obj[2].handle);
> +		gem_context_destroy(fd, execbuf.rsvd1);
> +	}
> +
> +	igt_assert(gem_bo_busy(fd, result));
> +	unplug(&cork); /* only now submit our batches */
> +	igt_debugfs_dump(fd, "i915_engine_info");
> +	finish_busy(busy);
> +
> +	r = gem_mmap__gtt(fd, result, 4096, PROT_READ);
> +	gem_set_domain(fd, result, /* no write hazard lies! */
> +			I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
> +	for (int n = 0; n < 1024; n++)
> +		igt_assert_eq_u32(r[n], t[n]);
> +	munmap(r, 4096);
> +	munmap(t, 4096);
> +
> +	gem_close(fd, result);
> +	gem_close(fd, target);
> +}

Ok, so we're using the fact that higher priority should execute earlier, so the
first request that's changing target should actually execute last.

And there's also a problem that I already mentioned on IRC (vgem/make_busy
timing out on my kernel - ether by hang on make_busy or fence "expiration" on
vgem). But I guess it's something for another patch.

With all of the above.

Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>

-Michał

> +
>  static bool has_scheduler(int fd)
>  {
>  	drm_i915_getparam_t gp;
> @@ -571,6 +674,9 @@ igt_main
>  
>  				igt_subtest_f("wide-%s", e->name)
>  					wide(fd, e->exec_id | e->flags);
> +
> +				igt_subtest_f("reorder-wide-%s", e->name)
> +					reorder_wide(fd, e->exec_id | e->flags);
>  			}
>  		}
>  	}
> -- 
> 2.13.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2017-07-31 15:27 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-07-28 12:07 [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Chris Wilson
2017-07-28 12:08 ` [PATCH igt 02/10] igt/gem_mmap_gtt: Simulate gdb inspecting a GTT mmap using ptrace() Chris Wilson
2017-07-31  9:41   ` Tvrtko Ursulin
2017-07-31 10:17     ` Chris Wilson
2017-07-31 10:39       ` Tvrtko Ursulin
2017-07-28 12:08 ` [PATCH igt 03/10] igt/gem_ctx_switch: Exercise all engines at once Chris Wilson
2017-07-31  9:57   ` Tvrtko Ursulin
2017-07-28 12:08 ` [PATCH igt 04/10] igt/gem_exec_schedule: Exercise reordering with many priority levels Chris Wilson
2017-07-31 14:35   ` Tvrtko Ursulin
2017-07-31 15:21     ` Chris Wilson
2017-07-31 15:27   ` Michał Winiarski
2017-07-28 12:08 ` [PATCH igt 05/10] igt/drv_hangman: Skip if resets are disallowed Chris Wilson
2017-07-28 13:53   ` Michał Winiarski
2017-07-28 13:59     ` Chris Wilson
2017-07-28 14:01     ` Chris Wilson
2017-07-28 12:08 ` [PATCH igt 06/10] igt/gem_ringfill: Prime execbuf before measuring ring size Chris Wilson
2017-07-31  9:09   ` Michał Winiarski
2017-07-28 12:08 ` [PATCH igt 07/10] igt/gem_exec_suspend: Try to suspend with a pending GPU hang Chris Wilson
2017-07-31  8:58   ` Michał Winiarski
2017-07-31  9:02     ` Chris Wilson
2017-07-28 12:08 ` [PATCH igt 08/10] igt/gem_exec_fence: Exercise merging fences Chris Wilson
2017-07-31  8:26   ` Michał Winiarski
2017-07-31  9:19     ` Chris Wilson
2017-07-28 12:08 ` [PATCH igt 09/10] igt/gem_userptr_blits: Errors from gup are permanent Chris Wilson
2017-07-28 12:33   ` Michał Winiarski
2017-07-28 12:08 ` [PATCH igt 10/10] igt/gem_eio: i915.reset is no longer a boolean Chris Wilson
2017-07-28 13:36   ` Michał Winiarski
2017-07-28 13:50     ` Chris Wilson
2017-07-28 15:56       ` Michał Winiarski
2017-07-31  9:26 ` [PATCH igt 01/10] tests/prime_rw: Add basic tests for reading/writing to a dmabuf Tvrtko Ursulin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox