Re: [igt-dev] [PATCH i-g-t 08/11] lib/store: Refactor common store code into helper function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/20/2021 10:13, Zbigniew Kempczyński wrote:
On Thu, Dec 16, 2021 at 02:40:21PM -0800, John Harrison wrote:
On 12/15/2021 23:46, Zbigniew Kempczyński wrote:
On Mon, Dec 13, 2021 at 03:29:11PM -0800, John.C.Harrison@xxxxxxxxx wrote:
From: John Harrison <John.C.Harrison@xxxxxxxxx>

A lot of tests use almost identical code for creating a batch buffer
which does a single write to memory. This patch collects two such
instances 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.

Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
---
   lib/igt_store.c             | 114 ++++++++++++++++++++++++++++++++++++
   lib/igt_store.h             |  30 ++++++++++
   lib/meson.build             |   1 +
   tests/i915/gem_exec_fence.c |  77 ++----------------------
   tests/i915/i915_hangman.c   |  61 +------------------
   5 files changed, 152 insertions(+), 131 deletions(-)
   create mode 100644 lib/igt_store.c
   create mode 100644 lib/igt_store.h

diff --git a/lib/igt_store.c b/lib/igt_store.c
new file mode 100644
index 000000000..6d9869b58
--- /dev/null
+++ b/lib/igt_store.c
@@ -0,0 +1,114 @@
+/*
+ * Copyright © 2020 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.
+ *
+ */
Use SPDX. I like idea of extracting this.

+
+#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_offset, uint32_t target_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);
+	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) * target_value;	/* why value not offset??? */
I guess I know why there's problem here. target_offset is address in vm
passed by the caller. This is regarding to some limitations of allocator
infrastructure - for "reloc" pseudo-allocator you would get new offset
(internally it returns offset and then add size for new "allocation").
With this we don't need to wait for rebind offset for new execbuf.
With "simple" allocator put will release offset so new allocation will
reuse same offset. Ashutosh proposed how to join both functionalities
(stepping as with reloc, stateful like in simple) but I got no time to
code this.

Regarding issue here, target_offset passed from the caller is to avoid
rebind if get_offset() would be called for "reloc" allocator.
So there's not real value offset within bo. I would add separate
value_offset (shift) to allow caller to put place where it wants to
write the value.
If you understand what is going on here and how to improve it then feel free
to either send me an updated version or post an extra patch after this is
merged. I don't get how any of the new reloc (or anti-reloc?) code is meant
to work. These patches were all written as part of the GuC support a long
time ago - before there was any such thing as an AHND (what does that stand
for?) or intel_ctx_t. All I'm doing in this patch is moving the existing
code to a helper and trying to make it generic enough to be used by another
test. And right now, the two tests are passing on both GuC and execlist
platforms for me. So the code can't be all that broken.
I've added some missing parts to your code:

https://patchwork.freedesktop.org/series/98242/

If you think something is missing please add or fix.

Previously value was used both for value and offset within bo
used for example in gem_exec_fence@basic-await. I think adding
separate arg for offset for value within bo makes this code
more universal.
I don't follow. The purpose of the offset is the to say where the value is written within the buffer. As in 'target_offset' is 'offset into target buffer to write value'. You have added a 'value_offset' which surely would be defined as 'the offset at which to write the value in the target buffer'. I.e. exactly the same thing. What is the intention behind having two separate offset variables?

John.



NB: ahnd is short version of allocator_handle

--
Zbigniew



+	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;
Probably I've added this in previous code, for being safe I would
add (target_offset + delta) >> 32 for avoid risk passing invalid higher
offset part on 32b boundary.
Pretty sure we don't have any addresses larger than 32 bits yet, but yes,
delta should be added in.

Thanks,
John.

+	} else if (gen >= 4) {
+		batch[++i] = 0;
+		batch[++i] = delta;
+		reloc.offset += sizeof(uint32_t);
+	} else {
+		batch[i]--;
+		batch[++i] = delta;
+	}
+	batch[++i] = target_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);
+}
diff --git a/lib/igt_store.h b/lib/igt_store.h
new file mode 100644
index 000000000..4d5979e07
--- /dev/null
+++ b/lib/igt_store.h
@@ -0,0 +1,30 @@
+/*
+ * Copyright © 2020 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.
+ *
+ */
Use SPDX and 2021.

--
Zbigniew

+
+#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_offset, uint32_t target_value);
diff --git a/lib/meson.build b/lib/meson.build
index b9568a71b..3e43316d1 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -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',
diff --git a/tests/i915/gem_exec_fence.c b/tests/i915/gem_exec_fence.c
index 9a6336ce9..c4091a454 100644
--- a/tests/i915/gem_exec_fence.c
+++ b/tests/i915/gem_exec_fence.c
@@ -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);
   		} 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);
   				put_ahnd(ahnd);
   			}
   		}
diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c
index 20653b479..4cb9b8b85 100644
--- a/tests/i915/i915_hangman.c
+++ b/tests/i915/i915_hangman.c
@@ -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"
@@ -51,64 +52,6 @@ static int sysfs = -1;
   IGT_TEST_DESCRIPTION("Tests for hang detection and recovery");
-/* Requires master for STORE_DWORD on gen4/5 */
-static void store(int fd, const struct intel_execution_engine2 *e,
-		  int fence, uint32_t target, unsigned offset_value)
-{
-	const int SCRATCH = 0;
-	const int BATCH = 1;
-	const 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];
-	int i;
-
-	memset(&execbuf, 0, sizeof(execbuf));
-	execbuf.buffers_ptr = to_user_pointer(obj);
-	execbuf.buffer_count = ARRAY_SIZE(obj);
-	execbuf.flags = e->flags;
-	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;
-
-	obj[BATCH].handle = gem_create(fd, 4096);
-	obj[BATCH].relocs_ptr = to_user_pointer(&reloc);
-	obj[BATCH].relocation_count = 1;
-	memset(&reloc, 0, sizeof(reloc));
-
-	i = 0;
-	reloc.target_handle = obj[SCRATCH].handle;
-	reloc.presumed_offset = -1;
-	reloc.offset = sizeof(uint32_t) * (i + 1);
-	reloc.delta = sizeof(uint32_t) * offset_value;
-	reloc.read_domains = I915_GEM_DOMAIN_INSTRUCTION;
-	reloc.write_domain = I915_GEM_DOMAIN_INSTRUCTION;
-	batch[i] = MI_STORE_DWORD_IMM | (gen < 6 ? 1 << 22 : 0);
-	if (gen >= 8) {
-		batch[++i] = reloc.delta;
-		batch[++i] = 0;
-	} else if (gen >= 4) {
-		batch[++i] = 0;
-		batch[++i] = reloc.delta;
-		reloc.offset += sizeof(uint32_t);
-	} else {
-		batch[i]--;
-		batch[++i] = reloc.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);
-}
-
   static void check_alive(void)
   {
   	const struct intel_execution_engine2 *engine;
@@ -138,7 +81,7 @@ static void check_alive(void)
   			continue;
   		/* +OFFSET_ALIVE to ensure engine zero doesn't get a false negative */
-		store(fd, engine, -1, scratch, i + OFFSET_ALIVE);
+		igt_store_word(fd, ahnd, ctx, engine, -1, scratch, i + OFFSET_ALIVE, i + OFFSET_ALIVE);
   		i++;
   	}
--
2.25.1





[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux