Fixes for i915_hangman and gem_exec_capture

From: John Harrison <[email protected]>

The capture test was still using old style ring_id and ring_name (derived from the engine structure at the higher level). Update it to just take the engine structure directly.

Signed-off-by John Harrison <[email protected]>

---
 tests/i915/i915_hangman.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

tests/i915/i915_hangman.c +5 -5

index f64b8819d..280eac197 100644
@@ -207,8 +207,8 @@ static void check_error_state(const char *expected_ring_name,
 	igt_assert(found);
 }
 
-static void test_error_state_capture(const intel_ctx_t *ctx, unsigned ring_id,
-				     const char *ring_name)
+static void test_error_state_capture(const intel_ctx_t *ctx,
+				     const struct intel_execution_engine2 *e)
 {
 	uint32_t *batch;
 	igt_hang_t hang;

@@ -217,7 +217,7 @@ static void test_error_state_capture(const intel_ctx_t *ctx, unsigned ring_id,
 
 	clear_error_state();
 
-	hang = igt_hang_ctx_with_ahnd(device, ahnd, ctx->id, ring_id,
+	hang = igt_hang_ctx_with_ahnd(device, ahnd, ctx->id, e->flags,
 				      HANG_ALLOW_CAPTURE);
 	offset = hang.spin->obj[IGT_SPIN_BATCH].offset;
 

@@ -226,7 +226,7 @@ static void test_error_state_capture(const intel_ctx_t *ctx, unsigned ring_id,
 
 	igt_post_hang_ring(device, hang);
 
-	check_error_state(ring_name, offset, batch);
+	check_error_state(e->name, offset, batch);
 	munmap(batch, 4096);
 	put_ahnd(ahnd);
 }

@@ -351,7 +351,7 @@ igt_main
 	igt_subtest_with_dynamic("error-state-capture") {
 		for_each_ctx_engine(device, ctx, e) {
 			igt_dynamic_f("%s", e->name)
-				test_error_state_capture(ctx, e->flags, e->name);
+				test_error_state_capture(ctx, e);
 		}
 	}
 

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

The hangman framework sets up a context that is valid for all engines and has things like banning disabled. The 'unterminated' test then ignores it and uses the default context. Fix that.

Signed-off-by John Harrison <[email protected]>

---
 tests/i915/i915_hangman.c | 1 +
 1 file changed, 1 insertion(+)

tests/i915/i915_hangman.c +1 -0

index 354769f39..6656b3fcd 100644
@@ -347,6 +347,7 @@ static void hangcheck_unterminated(const intel_ctx_t *ctx)
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = (uintptr_t)&gem_exec;
 	execbuf.buffer_count = 1;
+	execbuf.rsvd1 = ctx->id;
 
 	gem_execbuf(device, &execbuf);
 	if (gem_wait(device, handle, &timeout_ns) != 0) {

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

A lot of tests use almost identical code for creating a batch buffer which does a single write to memory and another is about to be added. Instead, move the most generic version into a common helper function. Unfortunately, the other instances are all subtly different enough to make it not so trivial to try to use the helper. It could be done but it is unclear if it is worth the effort at this point. This patch proves the concept, if people like it enough then it can be extended.

v2: Fix up object address vs store offset confusion (with help from Zbigniew K).

Signed-off-by John Harrison <[email protected]>

---
 lib/igt_store.c             | 96 +++++++++++++++++++++++++++++++++++++
 lib/igt_store.h             | 12 +++++
 lib/meson.build             |  1 +
 tests/i915/gem_exec_fence.c | 77 ++---------------------------
 tests/i915/i915_hangman.c   |  1 +
 5 files changed, 115 insertions(+), 72 deletions(-)
 create mode 100644 lib/igt_store.c
 create mode 100644 lib/igt_store.h

lib/igt_store.c new file +54 -0

new file mode 100644
index 000000000..42c888b55
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "i915/gem_create.h"
+#include "igt_core.h"
+#include "drmtest.h"
+#include "igt_store.h"
+#include "intel_chipset.h"
+#include "intel_reg.h"
+#include "ioctl_wrappers.h"
+#include "lib/intel_allocator.h"
+
+/**
+ * SECTION:igt_store_word
+ * @short_description: Library for writing a value to memory
+ * @title: StoreWord
+ * @include: igt.h
+ *
+ * A lot of igt testcases need some mechanism for writing a value to memory
+ * as a test that a batch buffer has executed.
+ *
+ * NB: Requires master for STORE_DWORD on gen4/5.
+ */
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value)
+{
+	const int SCRATCH = 0;
+	const int BATCH = 1;
+	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	uint32_t batch[16], delta;
+	uint64_t bb_offset;
+	int i;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	execbuf.flags = e->flags;
+	execbuf.rsvd1 = ctx->id;
+	if (fence != -1) {
+		execbuf.flags |= I915_EXEC_FENCE_IN;
+		execbuf.rsvd2 = fence;
+	}
+	if (gen < 6)
+		execbuf.flags |= I915_EXEC_SECURE;
+
+	memset(obj, 0, sizeof(obj));

lib/igt_store.c new file +7 -0

+	if (!ahnd) {
+		reloc.target_handle = obj[SCRATCH].handle;
+		reloc.presumed_offset = -1;
+		reloc.offset = sizeof(uint32_t) * (i + 1);
+		reloc.delta = delta;
+		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;

lib/igt_store.c new file +15 -0

+	} else if (gen >= 4) {
+		batch[++i] = 0;
+		batch[++i] = delta;
+		reloc.offset += sizeof(uint32_t);
+	} else {
+		batch[i]--;
+		batch[++i] = delta;
+	}
+	batch[++i] = store_value;
+	batch[++i] = MI_BATCH_BUFFER_END;
+	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[BATCH].handle);
+	put_offset(ahnd, obj[BATCH].handle);
+}

lib/igt_store.h new file +12 -0

new file mode 100644
index 000000000..5c6c8263c
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "igt_gt.h"
+
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value);

lib/meson.build +1 -0

index b9568a71b..3e43316d1 100644
@@ -72,6 +72,7 @@ lib_sources = [
 	'igt_map.c',
 	'igt_pm.c',
 	'igt_dummyload.c',
+	'igt_store.c',
 	'uwildmat/uwildmat.c',
 	'igt_kmod.c',
 	'igt_panfrost.c',

tests/i915/gem_exec_fence.c +5 -72

index 9a6336ce9..196236b27 100644
@@ -28,6 +28,7 @@
 #include "i915/gem.h"
 #include "i915/gem_create.h"
 #include "igt.h"
+#include "igt_store.h"
 #include "igt_syncobj.h"
 #include "igt_sysfs.h"
 #include "igt_vgem.h"

@@ -57,74 +58,6 @@ struct sync_merge_data {
 #define   MI_SEMAPHORE_SAD_EQ_SDD       (4 << 12)
 #define   MI_SEMAPHORE_SAD_NEQ_SDD      (5 << 12)
 
-static void store(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
-		  const struct intel_execution_engine2 *e,
-		  int fence, uint32_t target, uint64_t target_offset,
-		  unsigned offset_value)
-{
-	const int SCRATCH = 0;
-	const int BATCH = 1;
-	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_relocation_entry reloc;
-	struct drm_i915_gem_execbuffer2 execbuf;
-	uint32_t batch[16], delta;
-	uint64_t bb_offset;
-	int i;
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-	execbuf.flags = e->flags | I915_EXEC_FENCE_IN;
-	execbuf.rsvd1 = ctx->id;
-	execbuf.rsvd2 = fence;
-	if (gen < 6)
-		execbuf.flags |= I915_EXEC_SECURE;
-
-	memset(obj, 0, sizeof(obj));
-	obj[SCRATCH].handle = target;
-
-	obj[BATCH].handle = gem_create(fd, 4096);
-	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
-	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
-	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
-	memset(&reloc, 0, sizeof(reloc));
-
-	i = 0;
-	delta = sizeof(uint32_t) * offset_value;
-	if (!ahnd) {
-		reloc.target_handle = obj[SCRATCH].handle;
-		reloc.presumed_offset = -1;
-		reloc.offset = sizeof(uint32_t) * (i + 1);
-		reloc.delta = delta;
-		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
-	} else {
-		obj[SCRATCH].offset = target_offset;
-		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
-		obj[BATCH].offset = bb_offset;
-		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
-	}
-	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
-	if (gen >= 8) {
-		batch[++i] = target_offset + delta;
-		batch[++i] = target_offset >> 32;
-	} else if (gen >= 4) {
-		batch[++i] = 0;
-		batch[++i] = delta;
-		reloc.offset += sizeof(uint32_t);
-	} else {
-		batch[i]--;
-		batch[++i] = delta;
-	}
-	batch[++i] = offset_value;
-	batch[++i] = MI_BATCH_BUFFER_END;
-	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
-	gem_execbuf(fd, &execbuf);
-	gem_close(fd, obj[BATCH].handle);
-	put_offset(ahnd, obj[BATCH].handle);
-}
-
 static bool fence_busy(int fence)
 {
 	return poll(&(struct pollfd){fence, POLLIN}, 1, 0) == 0;

@@ -400,13 +333,13 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx,
 			continue;
 
 		if (flags & NONBLOCK) {
-			store(fd, ahnd, ctx, e2, spin->out_fence,
-			      scratch, scratch_offset, i);
+			igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+				       scratch, scratch_offset, i, i);
 		} else {
 			igt_fork(child, 1) {
 				ahnd = get_reloc_ahnd(fd, ctx->id);
-				store(fd, ahnd, ctx, e2, spin->out_fence,
-				      scratch, scratch_offset, i);
+				igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+					       scratch, scratch_offset, i, i);
 				put_ahnd(ahnd);
 			}
 		}

tests/i915/i915_hangman.c +1 -0

index 6656b3fcd..5a0c9497c 100644
@@ -36,6 +36,7 @@
 #include "i915/gem.h"
 #include "i915/gem_create.h"
 #include "igt.h"
+#include "igt_store.h"
 #include "igt_sysfs.h"
 #include "igt_debugfs.h"
 #include "sw_sync.h"

Signature · 1 line

2.25.1

Quoted 87 lines · On Thu, Jan 13, 2022 at 11:59:39AM -0800, [email protected] wrote:

From: John Harrison <[email protected]>

A lot of tests use almost identical code for creating a batch buffer which does a single write to memory and another is about to be added. Instead, move the most generic version into a common helper function. Unfortunately, the other instances are all subtly different enough to make it not so trivial to try to use the helper. It could be done but it is unclear if it is worth the effort at this point. This patch proves the concept, if people like it enough then it can be extended.

v2: Fix up object address vs store offset confusion (with help from Zbigniew K).

Signed-off-by John Harrison <[email protected]>

---
  lib/igt_store.c             | 96 +++++++++++++++++++++++++++++++++++++
  lib/igt_store.h             | 12 +++++
  lib/meson.build             |  1 +
  tests/i915/gem_exec_fence.c | 77 ++---------------------------
  tests/i915/i915_hangman.c   |  1 +
  5 files changed, 115 insertions(+), 72 deletions(-)

create mode 100644 lib/igt_store.c
create mode 100644 lib/igt_store.h

lib/igt_store.c new file +57 -0

new file mode 100644
index 000000000..42c888b55
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "i915/gem_create.h"
+#include "igt_core.h"
+#include "drmtest.h"
+#include "igt_store.h"
+#include "intel_chipset.h"
+#include "intel_reg.h"
+#include "ioctl_wrappers.h"
+#include "lib/intel_allocator.h"
+
+/**
+ * SECTION:igt_store_word
+ * @short_description: Library for writing a value to memory
+ * @title: StoreWord
+ * @include: igt.h
+ *
+ * A lot of igt testcases need some mechanism for writing a value to memory
+ * as a test that a batch buffer has executed.
+ *
+ * NB: Requires master for STORE_DWORD on gen4/5.
+ */
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value)
+{
+	const int SCRATCH = 0;
+	const int BATCH = 1;
+	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	uint32_t batch[16], delta;
+	uint64_t bb_offset;
+	int i;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	execbuf.flags = e->flags;
+	execbuf.rsvd1 = ctx->id;
+	if (fence != -1) {
+		execbuf.flags |= I915_EXEC_FENCE_IN;
+		execbuf.rsvd2 = fence;
+	}
+	if (gen < 6)
+		execbuf.flags |= I915_EXEC_SECURE;
+
+	memset(obj, 0, sizeof(obj));
+	obj[SCRATCH].handle = target_handle;
+
+	obj[BATCH].handle = gem_create(fd, 4096);

lib/igt_store.c line 64

Quoted 7 lines

lib/igt_store.c new file +7 -0

+	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
+	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
+	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
+	memset(&reloc, 0, sizeof(reloc));
+
+	i = 0;
+	delta = sizeof(uint32_t) * store_offset;

Can't this overflow the delta as store_offset is a u64?

Quoted 8 lines

lib/igt_store.c new file +7 -0

+	if (!ahnd) {
+		reloc.target_handle = obj[SCRATCH].handle;
+		reloc.presumed_offset = -1;
+		reloc.offset = sizeof(uint32_t) * (i + 1);
+		reloc.delta = delta;
+		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;

lib/igt_store.c line 81

Quoted 10 lines

lib/igt_store.c new file +10 -0

+	} else {
+		obj[SCRATCH].offset = target_gpu_addr;
+		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
+		obj[BATCH].offset = bb_offset;
+		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
+	}
+	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+	if (gen >= 8) {
+		batch[++i] = target_gpu_addr + delta;
+		batch[++i] = (target_gpu_addr + delta) >> 32;

This is different from the previous code, presumably this is fixing a bug where delta + bits 31:0 of target_gpu_addr overflows into the upper 32 bits?

Matt

Quoted 166 lines

lib/igt_store.c new file +15 -0

+	} else if (gen >= 4) {
+		batch[++i] = 0;
+		batch[++i] = delta;
+		reloc.offset += sizeof(uint32_t);
+	} else {
+		batch[i]--;
+		batch[++i] = delta;
+	}
+	batch[++i] = store_value;
+	batch[++i] = MI_BATCH_BUFFER_END;
+	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[BATCH].handle);
+	put_offset(ahnd, obj[BATCH].handle);
+}

lib/igt_store.h new file +12 -0

new file mode 100644
index 000000000..5c6c8263c
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "igt_gt.h"
+
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value);

lib/meson.build +1 -0

index b9568a71b..3e43316d1 100644
@@ -72,6 +72,7 @@ lib_sources = [
  	'igt_map.c',
  	'igt_pm.c',
  	'igt_dummyload.c',
+	'igt_store.c',
  	'uwildmat/uwildmat.c',
  	'igt_kmod.c',
  	'igt_panfrost.c',

tests/i915/gem_exec_fence.c +5 -72

index 9a6336ce9..196236b27 100644
@@ -28,6 +28,7 @@
  #include "i915/gem.h"
  #include "i915/gem_create.h"
  #include "igt.h"
+#include "igt_store.h"
  #include "igt_syncobj.h"
  #include "igt_sysfs.h"
  #include "igt_vgem.h"

@@ -57,74 +58,6 @@ struct sync_merge_data {
  #define   MI_SEMAPHORE_SAD_EQ_SDD       (4 << 12)
  #define   MI_SEMAPHORE_SAD_NEQ_SDD      (5 << 12)
 
-static void store(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
-		  const struct intel_execution_engine2 *e,
-		  int fence, uint32_t target, uint64_t target_offset,
-		  unsigned offset_value)
-{
-	const int SCRATCH = 0;
-	const int BATCH = 1;
-	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_relocation_entry reloc;
-	struct drm_i915_gem_execbuffer2 execbuf;
-	uint32_t batch[16], delta;
-	uint64_t bb_offset;
-	int i;
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-	execbuf.flags = e->flags | I915_EXEC_FENCE_IN;
-	execbuf.rsvd1 = ctx->id;
-	execbuf.rsvd2 = fence;
-	if (gen < 6)
-		execbuf.flags |= I915_EXEC_SECURE;
-
-	memset(obj, 0, sizeof(obj));
-	obj[SCRATCH].handle = target;
-
-	obj[BATCH].handle = gem_create(fd, 4096);
-	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
-	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
-	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
-	memset(&reloc, 0, sizeof(reloc));
-
-	i = 0;
-	delta = sizeof(uint32_t) * offset_value;
-	if (!ahnd) {
-		reloc.target_handle = obj[SCRATCH].handle;
-		reloc.presumed_offset = -1;
-		reloc.offset = sizeof(uint32_t) * (i + 1);
-		reloc.delta = delta;
-		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
-	} else {
-		obj[SCRATCH].offset = target_offset;
-		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
-		obj[BATCH].offset = bb_offset;
-		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
-	}
-	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
-	if (gen >= 8) {
-		batch[++i] = target_offset + delta;
-		batch[++i] = target_offset >> 32;
-	} else if (gen >= 4) {
-		batch[++i] = 0;
-		batch[++i] = delta;
-		reloc.offset += sizeof(uint32_t);
-	} else {
-		batch[i]--;
-		batch[++i] = delta;
-	}
-	batch[++i] = offset_value;
-	batch[++i] = MI_BATCH_BUFFER_END;
-	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
-	gem_execbuf(fd, &execbuf);
-	gem_close(fd, obj[BATCH].handle);
-	put_offset(ahnd, obj[BATCH].handle);
-}
-
  static bool fence_busy(int fence)
  {
  	return poll(&(struct pollfd){fence, POLLIN}, 1, 0) == 0;

@@ -400,13 +333,13 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx,
  			continue;
 
  		if (flags & NONBLOCK) {
-			store(fd, ahnd, ctx, e2, spin->out_fence,
-			      scratch, scratch_offset, i);
+			igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+				       scratch, scratch_offset, i, i);
  		} else {
  			igt_fork(child, 1) {
  				ahnd = get_reloc_ahnd(fd, ctx->id);
-				store(fd, ahnd, ctx, e2, spin->out_fence,
-				      scratch, scratch_offset, i);
+				igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+					       scratch, scratch_offset, i, i);
  				put_ahnd(ahnd);
  			}
  		}

tests/i915/i915_hangman.c +1 -0

index 6656b3fcd..5a0c9497c 100644
@@ -36,6 +36,7 @@
  #include "i915/gem.h"
  #include "i915/gem_create.h"
  #include "igt.h"
+#include "igt_store.h"
  #include "igt_sysfs.h"
  #include "igt_debugfs.h"
  #include "sw_sync.h"

Signature · 1 line

2.25.1

Quoted 88 lines · Matthew Brost

Quoted 87 lines · On Thu, Jan 13, 2022 at 11:59:39AM -0800, [email protected] wrote:

From: John Harrison <[email protected]>

A lot of tests use almost identical code for creating a batch buffer which does a single write to memory and another is about to be added. Instead, move the most generic version into a common helper function. Unfortunately, the other instances are all subtly different enough to make it not so trivial to try to use the helper. It could be done but it is unclear if it is worth the effort at this point. This patch proves the concept, if people like it enough then it can be extended.

v2: Fix up object address vs store offset confusion (with help from Zbigniew K).

Signed-off-by John Harrison <[email protected]>

---
  lib/igt_store.c             | 96 +++++++++++++++++++++++++++++++++++++
  lib/igt_store.h             | 12 +++++
  lib/meson.build             |  1 +
  tests/i915/gem_exec_fence.c | 77 ++---------------------------
  tests/i915/i915_hangman.c   |  1 +
  5 files changed, 115 insertions(+), 72 deletions(-)

create mode 100644 lib/igt_store.c
create mode 100644 lib/igt_store.h

lib/igt_store.c new file +57 -0

new file mode 100644
index 000000000..42c888b55
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "i915/gem_create.h"
+#include "igt_core.h"
+#include "drmtest.h"
+#include "igt_store.h"
+#include "intel_chipset.h"
+#include "intel_reg.h"
+#include "ioctl_wrappers.h"
+#include "lib/intel_allocator.h"
+
+/**
+ * SECTION:igt_store_word
+ * @short_description: Library for writing a value to memory
+ * @title: StoreWord
+ * @include: igt.h
+ *
+ * A lot of igt testcases need some mechanism for writing a value to memory
+ * as a test that a batch buffer has executed.
+ *
+ * NB: Requires master for STORE_DWORD on gen4/5.
+ */
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value)
+{
+	const int SCRATCH = 0;
+	const int BATCH = 1;
+	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	uint32_t batch[16], delta;
+	uint64_t bb_offset;
+	int i;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	execbuf.flags = e->flags;
+	execbuf.rsvd1 = ctx->id;
+	if (fence != -1) {
+		execbuf.flags |= I915_EXEC_FENCE_IN;
+		execbuf.rsvd2 = fence;
+	}
+	if (gen < 6)
+		execbuf.flags |= I915_EXEC_SECURE;
+
+	memset(obj, 0, sizeof(obj));
+	obj[SCRATCH].handle = target_handle;
+
+	obj[BATCH].handle = gem_create(fd, 4096);

Quoted 8 lines

lib/igt_store.c line 64

Quoted 7 lines

lib/igt_store.c new file +7 -0

+	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
+	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
+	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
+	memset(&reloc, 0, sizeof(reloc));
+
+	i = 0;
+	delta = sizeof(uint32_t) * store_offset;

Can't this overflow the delta as store_offset is a u64?

Oops.

Yeah, this code was a right mess of data words being used as addresses and random copies supporting 64bit or only 32bit offsets. I believe it's currently fine as even platforms which can theoretically support >32bits don't actually use it. But yes, will repost with a 64bit version of delta.

Quoted 1 line

Quoted 4 lines

lib/igt_store.c line 68

Quoted 4 lines

lib/igt_store.c new file +4 -0

+	if (!ahnd) {
+		reloc.target_handle = obj[SCRATCH].handle;
+		reloc.presumed_offset = -1;
+		reloc.offset = sizeof(uint32_t) * (i + 1);

Quoted 17 lines

lib/igt_store.c line 81

Quoted 13 lines

lib/igt_store.c new file +13 -0

+		reloc.delta = delta;
+		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	} else {
+		obj[SCRATCH].offset = target_gpu_addr;
+		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
+		obj[BATCH].offset = bb_offset;
+		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
+	}
+	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+	if (gen >= 8) {
+		batch[++i] = target_gpu_addr + delta;
+		batch[++i] = (target_gpu_addr + delta) >> 32;

This is different from the previous code, presumably this is fixing a bug where delta + bits 31:0 of target_gpu_addr overflows into the upper 32 bits?

Quoted 1 line

Matt

Yeah, some copies of this code were definitely broken for >32bit addresses.

John.

Quoted 164 lines

Quoted 164 lines

lib/igt_store.c new file +15 -0

+	} else if (gen >= 4) {
+		batch[++i] = 0;
+		batch[++i] = delta;
+		reloc.offset += sizeof(uint32_t);
+	} else {
+		batch[i]--;
+		batch[++i] = delta;
+	}
+	batch[++i] = store_value;
+	batch[++i] = MI_BATCH_BUFFER_END;
+	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[BATCH].handle);
+	put_offset(ahnd, obj[BATCH].handle);
+}

lib/igt_store.h new file +12 -0

new file mode 100644
index 000000000..5c6c8263c
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "igt_gt.h"
+
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value);

lib/meson.build +1 -0

index b9568a71b..3e43316d1 100644
@@ -72,6 +72,7 @@ lib_sources = [
  	'igt_map.c',
  	'igt_pm.c',
  	'igt_dummyload.c',
+	'igt_store.c',
  	'uwildmat/uwildmat.c',
  	'igt_kmod.c',
  	'igt_panfrost.c',

tests/i915/gem_exec_fence.c +5 -72

index 9a6336ce9..196236b27 100644
@@ -28,6 +28,7 @@
  #include "i915/gem.h"
  #include "i915/gem_create.h"
  #include "igt.h"
+#include "igt_store.h"
  #include "igt_syncobj.h"
  #include "igt_sysfs.h"
  #include "igt_vgem.h"

@@ -57,74 +58,6 @@ struct sync_merge_data {
  #define   MI_SEMAPHORE_SAD_EQ_SDD       (4 << 12)
  #define   MI_SEMAPHORE_SAD_NEQ_SDD      (5 << 12)
-static void store(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
-		  const struct intel_execution_engine2 *e,
-		  int fence, uint32_t target, uint64_t target_offset,
-		  unsigned offset_value)
-{
-	const int SCRATCH = 0;
-	const int BATCH = 1;
-	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_relocation_entry reloc;
-	struct drm_i915_gem_execbuffer2 execbuf;
-	uint32_t batch[16], delta;
-	uint64_t bb_offset;
-	int i;
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-	execbuf.flags = e->flags | I915_EXEC_FENCE_IN;
-	execbuf.rsvd1 = ctx->id;
-	execbuf.rsvd2 = fence;
-	if (gen < 6)
-		execbuf.flags |= I915_EXEC_SECURE;
-
-	memset(obj, 0, sizeof(obj));
-	obj[SCRATCH].handle = target;
-
-	obj[BATCH].handle = gem_create(fd, 4096);
-	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
-	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
-	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
-	memset(&reloc, 0, sizeof(reloc));
-
-	i = 0;
-	delta = sizeof(uint32_t) * offset_value;
-	if (!ahnd) {
-		reloc.target_handle = obj[SCRATCH].handle;
-		reloc.presumed_offset = -1;
-		reloc.offset = sizeof(uint32_t) * (i + 1);
-		reloc.delta = delta;
-		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
-	} else {
-		obj[SCRATCH].offset = target_offset;
-		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
-		obj[BATCH].offset = bb_offset;
-		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
-	}
-	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
-	if (gen >= 8) {
-		batch[++i] = target_offset + delta;
-		batch[++i] = target_offset >> 32;
-	} else if (gen >= 4) {
-		batch[++i] = 0;
-		batch[++i] = delta;
-		reloc.offset += sizeof(uint32_t);
-	} else {
-		batch[i]--;
-		batch[++i] = delta;
-	}
-	batch[++i] = offset_value;
-	batch[++i] = MI_BATCH_BUFFER_END;
-	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
-	gem_execbuf(fd, &execbuf);
-	gem_close(fd, obj[BATCH].handle);
-	put_offset(ahnd, obj[BATCH].handle);
-}
-
  static bool fence_busy(int fence)
  {
  	return poll(&(struct pollfd){fence, POLLIN}, 1, 0) == 0;

@@ -400,13 +333,13 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx,
  			continue;
  		if (flags & NONBLOCK) {
-			store(fd, ahnd, ctx, e2, spin->out_fence,
-			      scratch, scratch_offset, i);
+			igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+				       scratch, scratch_offset, i, i);
  		} else {
  			igt_fork(child, 1) {
  				ahnd = get_reloc_ahnd(fd, ctx->id);
-				store(fd, ahnd, ctx, e2, spin->out_fence,
-				      scratch, scratch_offset, i);
+				igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+					       scratch, scratch_offset, i, i);
  				put_ahnd(ahnd);
  			}
  		}

tests/i915/i915_hangman.c +1 -0

index 6656b3fcd..5a0c9497c 100644
@@ -36,6 +36,7 @@
  #include "i915/gem.h"
  #include "i915/gem_create.h"
  #include "igt.h"
+#include "igt_store.h"
  #include "igt_sysfs.h"
  #include "igt_debugfs.h"
  #include "sw_sync.h"

Signature · 1 line

2.25.1

Quoted 104 lines · John Harrison

Quoted 88 lines · Matthew Brost

Quoted 87 lines · On Thu, Jan 13, 2022 at 11:59:39AM -0800, [email protected] wrote:

From: John Harrison <[email protected]>

A lot of tests use almost identical code for creating a batch buffer which does a single write to memory and another is about to be added. Instead, move the most generic version into a common helper function. Unfortunately, the other instances are all subtly different enough to make it not so trivial to try to use the helper. It could be done but it is unclear if it is worth the effort at this point. This patch proves the concept, if people like it enough then it can be extended.

v2: Fix up object address vs store offset confusion (with help from Zbigniew K).

Signed-off-by John Harrison <[email protected]>

---
   lib/igt_store.c             | 96 +++++++++++++++++++++++++++++++++++++
   lib/igt_store.h             | 12 +++++
   lib/meson.build             |  1 +
   tests/i915/gem_exec_fence.c | 77 ++---------------------------
   tests/i915/i915_hangman.c   |  1 +
   5 files changed, 115 insertions(+), 72 deletions(-)

create mode 100644 lib/igt_store.c
create mode 100644 lib/igt_store.h

lib/igt_store.c new file +57 -0

new file mode 100644
index 000000000..42c888b55
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "i915/gem_create.h"
+#include "igt_core.h"
+#include "drmtest.h"
+#include "igt_store.h"
+#include "intel_chipset.h"
+#include "intel_reg.h"
+#include "ioctl_wrappers.h"
+#include "lib/intel_allocator.h"
+
+/**
+ * SECTION:igt_store_word
+ * @short_description: Library for writing a value to memory
+ * @title: StoreWord
+ * @include: igt.h
+ *
+ * A lot of igt testcases need some mechanism for writing a value to memory
+ * as a test that a batch buffer has executed.
+ *
+ * NB: Requires master for STORE_DWORD on gen4/5.
+ */
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value)
+{
+	const int SCRATCH = 0;
+	const int BATCH = 1;
+	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	uint32_t batch[16], delta;
+	uint64_t bb_offset;
+	int i;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	execbuf.flags = e->flags;
+	execbuf.rsvd1 = ctx->id;
+	if (fence != -1) {
+		execbuf.flags |= I915_EXEC_FENCE_IN;
+		execbuf.rsvd2 = fence;
+	}
+	if (gen < 6)
+		execbuf.flags |= I915_EXEC_SECURE;
+
+	memset(obj, 0, sizeof(obj));
+	obj[SCRATCH].handle = target_handle;
+
+	obj[BATCH].handle = gem_create(fd, 4096);

Quoted 8 lines

lib/igt_store.c line 64

Quoted 7 lines

lib/igt_store.c new file +7 -0

+	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
+	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
+	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
+	memset(&reloc, 0, sizeof(reloc));
+
+	i = 0;
+	delta = sizeof(uint32_t) * store_offset;

Can't this overflow the delta as store_offset is a u64?

Oops.

Yeah, this code was a right mess of data words being used as addresses and random copies supporting 64bit or only 32bit offsets. I believe it's currently fine as even platforms which can theoretically support >32bits don't actually use it. But yes, will repost with a 64bit version of delta.

Quoted 4 lines

Quoted 4 lines

lib/igt_store.c line 68

Quoted 4 lines

lib/igt_store.c new file +4 -0

+	if (!ahnd) {
+		reloc.target_handle = obj[SCRATCH].handle;
+		reloc.presumed_offset = -1;
+		reloc.offset = sizeof(uint32_t) * (i + 1);

Then just be safe, probably assert the upper 32 bits of delta are clear too.

Matt

Quoted 185 lines

Quoted 17 lines

lib/igt_store.c line 81

Quoted 13 lines

lib/igt_store.c new file +13 -0

+		reloc.delta = delta;
+		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	} else {
+		obj[SCRATCH].offset = target_gpu_addr;
+		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
+		obj[BATCH].offset = bb_offset;
+		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
+	}
+	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+	if (gen >= 8) {
+		batch[++i] = target_gpu_addr + delta;
+		batch[++i] = (target_gpu_addr + delta) >> 32;

This is different from the previous code, presumably this is fixing a bug where delta + bits 31:0 of target_gpu_addr overflows into the upper 32 bits?

Quoted 1 line

Matt

Yeah, some copies of this code were definitely broken for >32bit addresses.

John.

Quoted 163 lines

Quoted 163 lines

lib/igt_store.c new file +15 -0

+	} else if (gen >= 4) {
+		batch[++i] = 0;
+		batch[++i] = delta;
+		reloc.offset += sizeof(uint32_t);
+	} else {
+		batch[i]--;
+		batch[++i] = delta;
+	}
+	batch[++i] = store_value;
+	batch[++i] = MI_BATCH_BUFFER_END;
+	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[BATCH].handle);
+	put_offset(ahnd, obj[BATCH].handle);
+}

lib/igt_store.h new file +12 -0

new file mode 100644
index 000000000..5c6c8263c
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "igt_gt.h"
+
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value);

lib/meson.build +1 -0

index b9568a71b..3e43316d1 100644
@@ -72,6 +72,7 @@ lib_sources = [
   	'igt_map.c',
   	'igt_pm.c',
   	'igt_dummyload.c',
+	'igt_store.c',
   	'uwildmat/uwildmat.c',
   	'igt_kmod.c',
   	'igt_panfrost.c',

tests/i915/gem_exec_fence.c +5 -72

index 9a6336ce9..196236b27 100644
@@ -28,6 +28,7 @@
   #include "i915/gem.h"
   #include "i915/gem_create.h"
   #include "igt.h"
+#include "igt_store.h"
   #include "igt_syncobj.h"
   #include "igt_sysfs.h"
   #include "igt_vgem.h"

@@ -57,74 +58,6 @@ struct sync_merge_data {
   #define   MI_SEMAPHORE_SAD_EQ_SDD       (4 << 12)
   #define   MI_SEMAPHORE_SAD_NEQ_SDD      (5 << 12)
-static void store(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
-		  const struct intel_execution_engine2 *e,
-		  int fence, uint32_t target, uint64_t target_offset,
-		  unsigned offset_value)
-{
-	const int SCRATCH = 0;
-	const int BATCH = 1;
-	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_relocation_entry reloc;
-	struct drm_i915_gem_execbuffer2 execbuf;
-	uint32_t batch[16], delta;
-	uint64_t bb_offset;
-	int i;
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-	execbuf.flags = e->flags | I915_EXEC_FENCE_IN;
-	execbuf.rsvd1 = ctx->id;
-	execbuf.rsvd2 = fence;
-	if (gen < 6)
-		execbuf.flags |= I915_EXEC_SECURE;
-
-	memset(obj, 0, sizeof(obj));
-	obj[SCRATCH].handle = target;
-
-	obj[BATCH].handle = gem_create(fd, 4096);
-	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
-	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
-	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
-	memset(&reloc, 0, sizeof(reloc));
-
-	i = 0;
-	delta = sizeof(uint32_t) * offset_value;
-	if (!ahnd) {
-		reloc.target_handle = obj[SCRATCH].handle;
-		reloc.presumed_offset = -1;
-		reloc.offset = sizeof(uint32_t) * (i + 1);
-		reloc.delta = delta;
-		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
-	} else {
-		obj[SCRATCH].offset = target_offset;
-		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
-		obj[BATCH].offset = bb_offset;
-		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
-	}
-	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
-	if (gen >= 8) {
-		batch[++i] = target_offset + delta;
-		batch[++i] = target_offset >> 32;
-	} else if (gen >= 4) {
-		batch[++i] = 0;
-		batch[++i] = delta;
-		reloc.offset += sizeof(uint32_t);
-	} else {
-		batch[i]--;
-		batch[++i] = delta;
-	}
-	batch[++i] = offset_value;
-	batch[++i] = MI_BATCH_BUFFER_END;
-	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
-	gem_execbuf(fd, &execbuf);
-	gem_close(fd, obj[BATCH].handle);
-	put_offset(ahnd, obj[BATCH].handle);
-}
-
   static bool fence_busy(int fence)
   {
   	return poll(&(struct pollfd){fence, POLLIN}, 1, 0) == 0;

@@ -400,13 +333,13 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx,
   			continue;
   		if (flags & NONBLOCK) {
-			store(fd, ahnd, ctx, e2, spin->out_fence,
-			      scratch, scratch_offset, i);
+			igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+				       scratch, scratch_offset, i, i);
   		} else {
   			igt_fork(child, 1) {
   				ahnd = get_reloc_ahnd(fd, ctx->id);
-				store(fd, ahnd, ctx, e2, spin->out_fence,
-				      scratch, scratch_offset, i);
+				igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+					       scratch, scratch_offset, i, i);
   				put_ahnd(ahnd);
   			}
   		}

tests/i915/i915_hangman.c +1 -0

index 6656b3fcd..5a0c9497c 100644
@@ -36,6 +36,7 @@
   #include "i915/gem.h"
   #include "i915/gem_create.h"
   #include "igt.h"
+#include "igt_store.h"
   #include "igt_sysfs.h"
   #include "igt_debugfs.h"
   #include "sw_sync.h"

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

A lot of tests use almost identical code for creating a batch buffer which does a single write to memory and another is about to be added. Instead, move the most generic version into a common helper function. Unfortunately, the other instances are all subtly different enough to make it not so trivial to try to use the helper. It could be done but it is unclear if it is worth the effort at this point. This patch proves the concept, if people like it enough then it can be extended.

v2: Fix up object address vs store offset confusion (with help from Zbigniew K).
v3: Cope with >32bit store_offset (review feedback from Matthew Brost).

Signed-off-by John Harrison <[email protected]>

---
 lib/igt_store.c             | 100 ++++++++++++++++++++++++++++++++++++
 lib/igt_store.h             |  12 +++++
 lib/meson.build             |   1 +
 tests/i915/gem_exec_fence.c |  77 ++-------------------------
 tests/i915/i915_hangman.c   |   1 +
 5 files changed, 119 insertions(+), 72 deletions(-)
 create mode 100644 lib/igt_store.c
 create mode 100644 lib/igt_store.h

lib/igt_store.c new file +100 -0

new file mode 100644
index 000000000..98c6c4fbd
@@ -0,0 +1,100 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "i915/gem_create.h"
+#include "igt_core.h"
+#include "drmtest.h"
+#include "igt_store.h"
+#include "intel_chipset.h"
+#include "intel_reg.h"
+#include "ioctl_wrappers.h"
+#include "lib/intel_allocator.h"
+
+/**
+ * SECTION:igt_store_word
+ * @short_description: Library for writing a value to memory
+ * @title: StoreWord
+ * @include: igt.h
+ *
+ * A lot of igt testcases need some mechanism for writing a value to memory
+ * as a test that a batch buffer has executed.
+ *
+ * NB: Requires master for STORE_DWORD on gen4/5.
+ */
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value)
+{
+	const int SCRATCH = 0;
+	const int BATCH = 1;
+	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
+	struct drm_i915_gem_exec_object2 obj[2];
+	struct drm_i915_gem_relocation_entry reloc;
+	struct drm_i915_gem_execbuffer2 execbuf;
+	uint32_t batch[16];
+	uint64_t bb_offset, delta;
+	int i;
+
+	memset(&execbuf, 0, sizeof(execbuf));
+	execbuf.buffers_ptr = to_user_pointer(obj);
+	execbuf.buffer_count = ARRAY_SIZE(obj);
+	execbuf.flags = e->flags;
+	execbuf.rsvd1 = ctx->id;
+	if (fence != -1) {
+		execbuf.flags |= I915_EXEC_FENCE_IN;
+		execbuf.rsvd2 = fence;
+	}
+	if (gen < 6)
+		execbuf.flags |= I915_EXEC_SECURE;
+
+	memset(obj, 0, sizeof(obj));
+	obj[SCRATCH].handle = target_handle;
+
+	obj[BATCH].handle = gem_create(fd, 4096);
+	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
+	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
+	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
+	memset(&reloc, 0, sizeof(reloc));
+
+	i = 0;
+	delta = sizeof(uint32_t) * store_offset;
+	if (!ahnd) {
+		reloc.target_handle = obj[SCRATCH].handle;
+		reloc.presumed_offset = -1;
+		reloc.offset = sizeof(uint32_t) * (i + 1);
+		reloc.delta = lower_32_bits(delta);
+		igt_assert_eq(upper_32_bits(delta), 0);
+		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
+		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
+	} else {
+		obj[SCRATCH].offset = target_gpu_addr;
+		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
+		obj[BATCH].offset = bb_offset;
+		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
+	}
+	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
+	if (gen >= 8) {
+		uint64_t addr = target_gpu_addr + delta;
+		batch[++i] = lower_32_bits(addr);
+		batch[++i] = upper_32_bits(addr);
+	} else if (gen >= 4) {
+		batch[++i] = 0;
+		batch[++i] = lower_32_bits(delta);
+		igt_assert_eq(upper_32_bits(delta), 0);
+		reloc.offset += sizeof(uint32_t);
+	} else {
+		batch[i]--;
+		batch[++i] = lower_32_bits(delta);
+		igt_assert_eq(upper_32_bits(delta), 0);
+	}
+	batch[++i] = store_value;
+	batch[++i] = MI_BATCH_BUFFER_END;
+	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
+	gem_execbuf(fd, &execbuf);
+	gem_close(fd, obj[BATCH].handle);
+	put_offset(ahnd, obj[BATCH].handle);
+}

lib/igt_store.h new file +12 -0

new file mode 100644
index 000000000..5c6c8263c
@@ -0,0 +1,12 @@
+/* SPDX-License-Identifier: MIT */
+/*
+ * Copyright © 2021 Intel Corporation
+ */
+
+#include "igt_gt.h"
+
+void igt_store_word(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e,
+		    int fence, uint32_t target_handle,
+		    uint64_t target_gpu_addr,
+		    uint64_t store_offset, uint32_t store_value);

lib/meson.build +1 -0

index b9568a71b..3e43316d1 100644
@@ -72,6 +72,7 @@ lib_sources = [
 	'igt_map.c',
 	'igt_pm.c',
 	'igt_dummyload.c',
+	'igt_store.c',
 	'uwildmat/uwildmat.c',
 	'igt_kmod.c',
 	'igt_panfrost.c',

tests/i915/gem_exec_fence.c +5 -72

index 9a6336ce9..196236b27 100644
@@ -28,6 +28,7 @@
 #include "i915/gem.h"
 #include "i915/gem_create.h"
 #include "igt.h"
+#include "igt_store.h"
 #include "igt_syncobj.h"
 #include "igt_sysfs.h"
 #include "igt_vgem.h"

@@ -57,74 +58,6 @@ struct sync_merge_data {
 #define   MI_SEMAPHORE_SAD_EQ_SDD       (4 << 12)
 #define   MI_SEMAPHORE_SAD_NEQ_SDD      (5 << 12)
 
-static void store(int fd, uint64_t ahnd, const intel_ctx_t *ctx,
-		  const struct intel_execution_engine2 *e,
-		  int fence, uint32_t target, uint64_t target_offset,
-		  unsigned offset_value)
-{
-	const int SCRATCH = 0;
-	const int BATCH = 1;
-	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
-	struct drm_i915_gem_exec_object2 obj[2];
-	struct drm_i915_gem_relocation_entry reloc;
-	struct drm_i915_gem_execbuffer2 execbuf;
-	uint32_t batch[16], delta;
-	uint64_t bb_offset;
-	int i;
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = 2;
-	execbuf.flags = e->flags | I915_EXEC_FENCE_IN;
-	execbuf.rsvd1 = ctx->id;
-	execbuf.rsvd2 = fence;
-	if (gen < 6)
-		execbuf.flags |= I915_EXEC_SECURE;
-
-	memset(obj, 0, sizeof(obj));
-	obj[SCRATCH].handle = target;
-
-	obj[BATCH].handle = gem_create(fd, 4096);
-	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
-	obj[BATCH].relocation_count = !ahnd ? 1 : 0;
-	bb_offset = get_offset(ahnd, obj[BATCH].handle, 4096, 0);
-	memset(&reloc, 0, sizeof(reloc));
-
-	i = 0;
-	delta = sizeof(uint32_t) * offset_value;
-	if (!ahnd) {
-		reloc.target_handle = obj[SCRATCH].handle;
-		reloc.presumed_offset = -1;
-		reloc.offset = sizeof(uint32_t) * (i + 1);
-		reloc.delta = delta;
-		reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-		reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
-	} else {
-		obj[SCRATCH].offset = target_offset;
-		obj[SCRATCH].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE;
-		obj[BATCH].offset = bb_offset;
-		obj[BATCH].flags |= EXEC_OBJECT_PINNED;
-	}
-	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
-	if (gen >= 8) {
-		batch[++i] = target_offset + delta;
-		batch[++i] = target_offset >> 32;
-	} else if (gen >= 4) {
-		batch[++i] = 0;
-		batch[++i] = delta;
-		reloc.offset += sizeof(uint32_t);
-	} else {
-		batch[i]--;
-		batch[++i] = delta;
-	}
-	batch[++i] = offset_value;
-	batch[++i] = MI_BATCH_BUFFER_END;
-	gem_write(fd, obj[BATCH].handle, 0, batch, sizeof(batch));
-	gem_execbuf(fd, &execbuf);
-	gem_close(fd, obj[BATCH].handle);
-	put_offset(ahnd, obj[BATCH].handle);
-}
-
 static bool fence_busy(int fence)
 {
 	return poll(&(struct pollfd){fence, POLLIN}, 1, 0) == 0;

@@ -400,13 +333,13 @@ static void test_fence_await(int fd, const intel_ctx_t *ctx,
 			continue;
 
 		if (flags & NONBLOCK) {
-			store(fd, ahnd, ctx, e2, spin->out_fence,
-			      scratch, scratch_offset, i);
+			igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+				       scratch, scratch_offset, i, i);
 		} else {
 			igt_fork(child, 1) {
 				ahnd = get_reloc_ahnd(fd, ctx->id);
-				store(fd, ahnd, ctx, e2, spin->out_fence,
-				      scratch, scratch_offset, i);
+				igt_store_word(fd, ahnd, ctx, e2, spin->out_fence,
+					       scratch, scratch_offset, i, i);
 				put_ahnd(ahnd);
 			}
 		}

tests/i915/i915_hangman.c +1 -0

index 6656b3fcd..5a0c9497c 100644
@@ -36,6 +36,7 @@
 #include "i915/gem.h"
 #include "i915/gem_create.h"
 #include "igt.h"
+#include "igt_store.h"
 #include "igt_sysfs.h"
 #include "igt_debugfs.h"
 #include "sw_sync.h"

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

Added a an extra step to the i915_hangman tests to check that the system is still alive after the hang and recovery. This submits a simple batch to each engine which does a write to memory and checks that the write occurred.

Signed-off-by John Harrison <[email protected]>

---
 tests/i915/i915_hangman.c | 59 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

tests/i915/i915_hangman.c +59 -0

index 5a0c9497c..918418760 100644
@@ -48,8 +48,57 @@
 static int device = -1;
 static int sysfs = -1;
 
+#define OFFSET_ALIVE	10
+
 IGT_TEST_DESCRIPTION("Tests for hang detection and recovery");
 
+static void check_alive(void)
+{
+	const struct intel_execution_engine2 *engine;
+	const intel_ctx_t *ctx;
+	uint32_t scratch, *out;
+	int fd, i = 0;
+	uint64_t ahnd, scratch_addr;
+
+	fd = drm_open_driver(DRIVER_INTEL);
+	igt_require(gem_class_can_store_dword(fd, 0));
+
+	ctx = intel_ctx_create_all_physical(fd);
+	ahnd = get_reloc_ahnd(fd, ctx->id);
+	scratch = gem_create(fd, 4096);
+	scratch_addr = get_offset(ahnd, scratch, 4096, 0);
+	out = gem_mmap__wc(fd, scratch, 0, 4096, PROT_WRITE);
+	gem_set_domain(fd, scratch,
+			I915_GEM_DOMAIN_GTT, I915_GEM_DOMAIN_GTT);
+
+	for_each_physical_engine(fd, engine) {
+		igt_assert_eq_u32(out[i + OFFSET_ALIVE], 0);
+		i++;
+	}
+
+	i = 0;
+	for_each_ctx_engine(fd, ctx, engine) {
+		if (!gem_class_can_store_dword(fd, engine->class))
+			continue;
+
+		/* +OFFSET_ALIVE to ensure engine zero doesn't get a false negative */
+		igt_store_word(fd, ahnd, ctx, engine, -1, scratch, scratch_addr,
+			       i + OFFSET_ALIVE, i + OFFSET_ALIVE);
+		i++;
+	}
+
+	gem_set_domain(fd, scratch, I915_GEM_DOMAIN_GTT, 0);
+
+	while (i--)
+		igt_assert_eq_u32(out[i + OFFSET_ALIVE], i + OFFSET_ALIVE);
+
+	munmap(out, 4096);
+	gem_close(fd, scratch);
+	put_ahnd(ahnd);
+	intel_ctx_destroy(fd, ctx);
+	close(fd);
+}
+
 static bool has_error_state(int dir)
 {
 	bool result;

@@ -231,6 +280,8 @@ static void test_error_state_capture(const intel_ctx_t *ctx,
 	check_error_state(e->name, offset, batch);
 	munmap(batch, 4096);
 	put_ahnd(ahnd);
+
+	check_alive();
 }
 
 static void

@@ -289,6 +340,8 @@ test_engine_hang(const intel_ctx_t *ctx,
 		put_ahnd(ahndN);
 	}
 	put_ahnd(ahnd);
+
+	check_alive();
 }
 
 static int hang_count;

@@ -321,6 +374,8 @@ static void test_hang_detector(const intel_ctx_t *ctx,
 
 	/* Did it work? */
 	igt_assert(hang_count == 1);
+
+	check_alive();
 }
 
 /* This test covers the case where we end up in an uninitialised area of the

@@ -356,6 +411,8 @@ static void hangcheck_unterminated(const intel_ctx_t *ctx)
 		igt_force_gpu_reset(device);
 		igt_assert_f(0, "unterminated batch did not trigger a hang!\n");
 	}
+
+	check_alive();
 }
 
 static void do_tests(const char *name, const char *prefix,

@@ -433,6 +490,8 @@ igt_main
 		igt_assert(sysfs != -1);
 
 		igt_require(has_error_state(sysfs));
+
+		gem_require_mmap_wc(device);
 	}
 
 	igt_describe("Basic error capture");

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

The hang test was relying on context persitence for no particular reason. That is, it would set a bunch of background spinners running then immediately destroy the active contexts but expect the spinners to keep spinning. With the current implementation of context persistence in i915, that means that super high priority pings are sent to each engine at the start of the test. Depending upon the timing and platform, one of those unexpected pings could cause test failures.

There is no need to require context persitence in this test. So change to managing the contexts cleanly and only destroying them when they are no longer in use.

Signed-off-by John Harrison <[email protected]>

---
 tests/i915/i915_hangman.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

tests/i915/i915_hangman.c +0 -0

index 918418760..6601db5f6 100644

tests/i915/i915_hangman.c +9 -4

 	igt_spin_t *spin, *next;
 	IGT_LIST_HEAD(list);
 	uint64_t ahnd = get_reloc_ahnd(device, ctx->id), ahndN;
+	int num_ctx;
 
 	igt_skip_on(flags & IGT_SPIN_INVALID_CS &&
 		    gem_engine_has_cmdparser(device, &ctx->cfg, e->flags));
 
 	/* Fill all the other engines with background load */
+	num_ctx = 0;
 	for_each_ctx_engine(device, ctx, other) {
 		if (other->flags == e->flags)
 			continue;
 
-		tmp_ctx = intel_ctx_create(device, &ctx->cfg);
-		ahndN = get_reloc_ahnd(device, tmp_ctx->id);
+		local_ctx[num_ctx] = intel_ctx_create(device, &ctx->cfg);
+		ahndN = get_reloc_ahnd(device, local_ctx[num_ctx]->id);
 		spin = __igt_spin_new(device,
 				      .ahnd = ahndN,
-				      .ctx = tmp_ctx,
+				      .ctx = local_ctx[num_ctx],
 				      .engine = other->flags,
 				      .flags = IGT_SPIN_FENCE_OUT);
-		intel_ctx_destroy(device, tmp_ctx);
+		num_ctx++;
 
 		igt_list_move(&spin->link, &list);
 	}

@@ -339,7 +341,10 @@ test_engine_hang(const intel_ctx_t *ctx,
 		igt_spin_free(device, spin);
 		put_ahnd(ahndN);
 	}
+
 	put_ahnd(ahnd);
+	while (num_ctx)
+		intel_ctx_destroy(device, local_ctx[--num_ctx]);
 
 	check_alive();
 }

Signature · 1 line

2.25.1

Quoted 20 lines · On Thu, Jan 13, 2022 at 11:59:41AM -0800, [email protected] wrote:

From: John Harrison <[email protected]>

The hang test was relying on context persitence for no particular reason. That is, it would set a bunch of background spinners running then immediately destroy the active contexts but expect the spinners to keep spinning. With the current implementation of context persistence in i915, that means that super high priority pings are sent to each engine at the start of the test. Depending upon the timing and platform, one of those unexpected pings could cause test failures.

There is no need to require context persitence in this test. So change to managing the contexts cleanly and only destroying them when they are no longer in use.

Signed-off-by John Harrison <[email protected]>

---
  tests/i915/i915_hangman.c | 15 ++++++++++-----
  1 file changed, 10 insertions(+), 5 deletions(-)

tests/i915/i915_hangman.c test_engine_hang(const intel_ctx_t *ctx, · line 292

Quoted 10 lines

tests/i915/i915_hangman.c +1 -1

index 918418760..6601db5f6 100644
@@ -289,27 +289,29 @@ test_engine_hang(const intel_ctx_t *ctx,
  		 const struct intel_execution_engine2 *e, unsigned int flags)
  {
  	const struct intel_execution_engine2 *other;
-	const intel_ctx_t *tmp_ctx;
+	const intel_ctx_t *local_ctx[GEM_MAX_ENGINES];

This is fine for now as GEM_MAX_ENGINES is relatively small but what if we change this to large value, let's say 4k? I think the stack could overflow then. Maybe not a concern, maybe it is? I'll leave this up to if this should be kmalloc'd or not in the next rev.

Everything else looks good.

With that:

Reviewed-by Matthew Brost <[email protected]>

Quoted 43 lines

tests/i915/i915_hangman.c +9 -4

  	igt_spin_t *spin, *next;
  	IGT_LIST_HEAD(list);
  	uint64_t ahnd = get_reloc_ahnd(device, ctx->id), ahndN;
+	int num_ctx;
 
  	igt_skip_on(flags & IGT_SPIN_INVALID_CS &&
  		    gem_engine_has_cmdparser(device, &ctx->cfg, e->flags));
 
  	/* Fill all the other engines with background load */
+	num_ctx = 0;
  	for_each_ctx_engine(device, ctx, other) {
  		if (other->flags == e->flags)
  			continue;
 
-		tmp_ctx = intel_ctx_create(device, &ctx->cfg);
-		ahndN = get_reloc_ahnd(device, tmp_ctx->id);
+		local_ctx[num_ctx] = intel_ctx_create(device, &ctx->cfg);
+		ahndN = get_reloc_ahnd(device, local_ctx[num_ctx]->id);
  		spin = __igt_spin_new(device,
  				      .ahnd = ahndN,
-				      .ctx = tmp_ctx,
+				      .ctx = local_ctx[num_ctx],
  				      .engine = other->flags,
  				      .flags = IGT_SPIN_FENCE_OUT);
-		intel_ctx_destroy(device, tmp_ctx);
+		num_ctx++;
 
  		igt_list_move(&spin->link, &list);
  	}

@@ -339,7 +341,10 @@ test_engine_hang(const intel_ctx_t *ctx,
  		igt_spin_free(device, spin);
  		put_ahnd(ahndN);
  	}
+
  	put_ahnd(ahnd);
+	while (num_ctx)
+		intel_ctx_destroy(device, local_ctx[--num_ctx]);
 
  	check_alive();
  }

Signature · 1 line

2.25.1

Quoted 21 lines · Matthew Brost

Quoted 20 lines · On Thu, Jan 13, 2022 at 11:59:41AM -0800, [email protected] wrote:

From: John Harrison <[email protected]>

The hang test was relying on context persitence for no particular reason. That is, it would set a bunch of background spinners running then immediately destroy the active contexts but expect the spinners to keep spinning. With the current implementation of context persistence in i915, that means that super high priority pings are sent to each engine at the start of the test. Depending upon the timing and platform, one of those unexpected pings could cause test failures.

There is no need to require context persitence in this test. So change to managing the contexts cleanly and only destroying them when they are no longer in use.

Signed-off-by John Harrison <[email protected]>

---
  tests/i915/i915_hangman.c | 15 ++++++++++-----
  1 file changed, 10 insertions(+), 5 deletions(-)

Quoted 14 lines

tests/i915/i915_hangman.c test_engine_hang(const intel_ctx_t *ctx, · line 292

Quoted 10 lines

tests/i915/i915_hangman.c +1 -1

index 918418760..6601db5f6 100644
@@ -289,27 +289,29 @@ test_engine_hang(const intel_ctx_t *ctx,
  		 const struct intel_execution_engine2 *e, unsigned int flags)
  {
  	const struct intel_execution_engine2 *other;
-	const intel_ctx_t *tmp_ctx;
+	const intel_ctx_t *local_ctx[GEM_MAX_ENGINES];

This is fine for now as GEM_MAX_ENGINES is relatively small but what if we change this to large value, let's say 4k? I think the stack could overflow then. Maybe not a concern, maybe it is? I'll leave this up to if this should be kmalloc'd or not in the next rev.

Seems unlikely we are going that big any time soon. And such stack reduction can always be done as part of any huge engine count update. Although, this is userland not kernel - you can slap gigabytes on the stack and it won't blow up ;).

John.

Quoted 43 lines

Everything else looks good.

With that:

Reviewed-by Matthew Brost <[email protected]>

Quoted 38 lines

tests/i915/i915_hangman.c +9 -4

  	igt_spin_t *spin, *next;
  	IGT_LIST_HEAD(list);
  	uint64_t ahnd = get_reloc_ahnd(device, ctx->id), ahndN;
+	int num_ctx;
  	igt_skip_on(flags & IGT_SPIN_INVALID_CS &&
  		    gem_engine_has_cmdparser(device, &ctx->cfg, e->flags));
  	/* Fill all the other engines with background load */
+	num_ctx = 0;
  	for_each_ctx_engine(device, ctx, other) {
  		if (other->flags == e->flags)
  			continue;
-		tmp_ctx = intel_ctx_create(device, &ctx->cfg);
-		ahndN = get_reloc_ahnd(device, tmp_ctx->id);
+		local_ctx[num_ctx] = intel_ctx_create(device, &ctx->cfg);
+		ahndN = get_reloc_ahnd(device, local_ctx[num_ctx]->id);
  		spin = __igt_spin_new(device,
  				      .ahnd = ahndN,
-				      .ctx = tmp_ctx,
+				      .ctx = local_ctx[num_ctx],
  				      .engine = other->flags,
  				      .flags = IGT_SPIN_FENCE_OUT);
-		intel_ctx_destroy(device, tmp_ctx);
+		num_ctx++;
  		igt_list_move(&spin->link, &list);
  	}

@@ -339,7 +341,10 @@ test_engine_hang(const intel_ctx_t *ctx,
  		igt_spin_free(device, spin);
  		put_ahnd(ahndN);
  	}
+
  	put_ahnd(ahnd);
+	while (num_ctx)
+		intel_ctx_destroy(device, local_ctx[--num_ctx]);
  	check_alive();
  }

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

As opposed to only on the non-target engines. This means that there is some other workload present for the scheduler to switch between and so detet the hang immediately.

Signed-off-by John Harrison <[email protected]>

---
 tests/i915/i915_hangman.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

tests/i915/i915_hangman.c +6 -4

index 6601db5f6..9f7f8062c 100644
@@ -298,12 +298,14 @@ test_engine_hang(const intel_ctx_t *ctx,
 	igt_skip_on(flags & IGT_SPIN_INVALID_CS &&
 		    gem_engine_has_cmdparser(device, &ctx->cfg, e->flags));
 
-	/* Fill all the other engines with background load */
+	/*
+	 * Fill all engines with background load.
+	 * This verifies that independent engines are unaffected and gives
+	 * the target engine something to switch between so it notices the
+	 * hang.
+	 */
 	num_ctx = 0;
 	for_each_ctx_engine(device, ctx, other) {
-		if (other->flags == e->flags)
-			continue;
-
 		local_ctx[num_ctx] = intel_ctx_create(device, &ctx->cfg);
 		ahndN = get_reloc_ahnd(device, local_ctx[num_ctx]->id);
 		spin = __igt_spin_new(device,

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

The global context used by all the subtests for causing hangs is marked as unbannable. However, some of the subtests set background spinners running on all engines using a freshly created context. If there is a test failure for any reason, all of those spinners can be killed off as hanging contexts. On systems with lots of engines, that can result in the test being banned from creating any new contexts.

So make the spinner contexts unbannable as well. That way if one subtest fails it won't necessarily bring down all subsequent subtests.

Signed-off-by John Harrison <[email protected]>

---
 tests/i915/i915_hangman.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

tests/i915/i915_hangman.c +0 -0

index 9f7f8062c..567eb71ee 100644

tests/i915/i915_hangman.c +5 -0

+		gem_context_set_param(fd, &param);
+	}
+}
+
 static void
 test_engine_hang(const intel_ctx_t *ctx,
 		 const struct intel_execution_engine2 *e, unsigned int flags)

@@ -307,6 +322,7 @@ test_engine_hang(const intel_ctx_t *ctx,
 	num_ctx = 0;
 	for_each_ctx_engine(device, ctx, other) {
 		local_ctx[num_ctx] = intel_ctx_create(device, &ctx->cfg);
+		context_unban(device, local_ctx[num_ctx]->id);
 		ahndN = get_reloc_ahnd(device, local_ctx[num_ctx]->id);
 		spin = __igt_spin_new(device,
 				      .ahnd = ahndN,

Signature · 1 line

2.25.1

Quoted 22 lines · On Thu, Jan 13, 2022 at 11:59:43AM -0800, [email protected] wrote:

From: John Harrison <[email protected]>

The global context used by all the subtests for causing hangs is marked as unbannable. However, some of the subtests set background spinners running on all engines using a freshly created context. If there is a test failure for any reason, all of those spinners can be killed off as hanging contexts. On systems with lots of engines, that can result in the test being banned from creating any new contexts.

So make the spinner contexts unbannable as well. That way if one subtest fails it won't necessarily bring down all subsequent subtests.

Signed-off-by John Harrison <[email protected]>

---
  tests/i915/i915_hangman.c | 16 ++++++++++++++++
  1 file changed, 16 insertions(+)

tests/i915/i915_hangman.c +0 -0

index 9f7f8062c..567eb71ee 100644

tests/i915/i915_hangman.c static void test_error_state_capture(const intel_ctx_t *ctx, · line 291

Quoted 8 lines

tests/i915/i915_hangman.c +5 -0

@@ -284,6 +284,21 @@ static void test_error_state_capture(const intel_ctx_t *ctx,
  	check_alive();
  }
 
+static void context_unban(int fd, unsigned ctx)
+{
+	struct drm_i915_gem_context_param param = {
+		.ctx_id = ctx,
+		.param = I915_CONTEXT_PARAM_BANNABLE,

Looking at the kernel I don't see how I915_CONTEXT_PARAM_BANNABLE can return -EINVAL unless it is protected context.

tests/i915/i915_hangman.c static void test_error_state_capture(const intel_ctx_t *ctx, · line 297

Quoted 6 lines

tests/i915/i915_hangman.c +6 -0

+		.value = 0,
+	};
+
+	if(__gem_context_set_param(fd, &param) == -EINVAL) {
+		igt_assert_eq(param.value, 0);
+		param.param = I915_CONTEXT_PARAM_BAN_PERIOD;

Also this always returns -EINVAL.

Probably can just go with:

gem_context_set_param on original parameters.

Matt

Quoted 19 lines

tests/i915/i915_hangman.c +5 -0

+		gem_context_set_param(fd, &param);
+	}
+}
+
  static void
  test_engine_hang(const intel_ctx_t *ctx,
  		 const struct intel_execution_engine2 *e, unsigned int flags)

@@ -307,6 +322,7 @@ test_engine_hang(const intel_ctx_t *ctx,
  	num_ctx = 0;
  	for_each_ctx_engine(device, ctx, other) {
  		local_ctx[num_ctx] = intel_ctx_create(device, &ctx->cfg);
+		context_unban(device, local_ctx[num_ctx]->id);
  		ahndN = get_reloc_ahnd(device, local_ctx[num_ctx]->id);
  		spin = __igt_spin_new(device,
  				      .ahnd = ahndN,

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

The global context used by all the subtests for causing hangs is marked as unbannable. However, some of the subtests set background spinners running on all engines using a freshly created context. If there is a test failure for any reason, all of those spinners can be killed off as hanging contexts. On systems with lots of engines, that can result in the test being banned from creating any new contexts.

So make the spinner contexts unbannable as well. That way if one subtest fails it won't necessarily bring down all subsequent subtests.

v2: Simplify anti-banning code (review feedback from Matthew Brost).

Signed-off-by John Harrison <[email protected]>

---
 tests/i915/i915_hangman.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

tests/i915/i915_hangman.c +12 -0

index 9f7f8062c..537ed35a5 100644
@@ -284,6 +284,17 @@ static void test_error_state_capture(const intel_ctx_t *ctx,
 	check_alive();
 }
 
+static void context_unban(int fd, unsigned ctx)
+{
+	struct drm_i915_gem_context_param param = {
+		.ctx_id = ctx,
+		.param = I915_CONTEXT_PARAM_BANNABLE,
+		.value = 0,
+	};
+
+	gem_context_set_param(fd, &param);
+}
+
 static void
 test_engine_hang(const intel_ctx_t *ctx,
 		 const struct intel_execution_engine2 *e, unsigned int flags)

@@ -307,6 +318,7 @@ test_engine_hang(const intel_ctx_t *ctx,
 	num_ctx = 0;
 	for_each_ctx_engine(device, ctx, other) {
 		local_ctx[num_ctx] = intel_ctx_create(device, &ctx->cfg);
+		context_unban(device, local_ctx[num_ctx]->id);
 		ahndN = get_reloc_ahnd(device, local_ctx[num_ctx]->id);
 		spin = __igt_spin_new(device,
 				      .ahnd = ahndN,

Signature · 1 line

2.25.1

From: John Harrison <[email protected]>

The update to use intel_ctx_t missed a line that configures the context to allow hanging. Fix that.

Fixes 09c36188b23f83ef9a7b5414e2a10100adc4291f

Signed-off-by John Harrison <[email protected]>

---
 tests/i915/gem_exec_fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

tests/i915/gem_exec_fence.c +1 -1

index 196236b27..5e45d0518 100644
@@ -3139,7 +3139,7 @@ igt_main
 			igt_hang_t hang;
 
 			igt_fixture {
-				hang = igt_allow_hang(i915, 0, 0);
+				hang = igt_allow_hang(i915, ctx->id, 0);
 				intel_allocator_multiprocess_start();
 			}
 

Signature · 1 line

2.25.1

Quoted 5 lines · On Thu, Jan 13, 2022 at 11:59:44AM -0800, [email protected] wrote:

From: John Harrison <[email protected]>

The update to use intel_ctx_t missed a line that configures the context to allow hanging. Fix that.

Quoted 1 line

Fixes 09c36188b23f83ef9a7b5414e2a10100adc4291f

Typically I thought the Fixes comment was the sha from "git log --oneline" + first line of commit message from that surrounded by ("").

So:
Fixes: <small sha> ("<message>")

With that fixed:

Reviewed-by Matthew Brost <[email protected]>

Quoted 21 lines

Signed-off-by John Harrison <[email protected]>

---
  tests/i915/gem_exec_fence.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

tests/i915/gem_exec_fence.c +1 -1

index 196236b27..5e45d0518 100644
@@ -3139,7 +3139,7 @@ igt_main
  			igt_hang_t hang;
 
  			igt_fixture {
-				hang = igt_allow_hang(i915, 0, 0);
+				hang = igt_allow_hang(i915, ctx->id, 0);
  				intel_allocator_multiprocess_start();
  			}
 

Signature · 1 line

2.25.1