| Message ID | 20220113195947.1536897-8-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixes for i915_hangman and gem_exec_capture | expand |
On Thu, Jan 13, 2022 at 11:59:39AM -0800, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > 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 <John.C.Harrison@Intel.com> > --- > 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 > > diff --git a/lib/igt_store.c b/lib/igt_store.c > new file mode 100644 > index 000000000..42c888b55 > --- /dev/null > +++ b/lib/igt_store.c > @@ -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); > + 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? > + 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_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 > + } 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); > +} > diff --git a/lib/igt_store.h b/lib/igt_store.h > new file mode 100644 > index 000000000..5c6c8263c > --- /dev/null > +++ b/lib/igt_store.h > @@ -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); > 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..196236b27 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, 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); > } > } > diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c > index 6656b3fcd..5a0c9497c 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" > -- > 2.25.1 >
On Thu, Jan 13, 2022 at 12:27:00PM -0800, John Harrison wrote: > On 1/13/2022 12:10, Matthew Brost wrote: > > On Thu, Jan 13, 2022 at 11:59:39AM -0800, John.C.Harrison@Intel.com wrote: > > > From: John Harrison <John.C.Harrison@Intel.com> > > > > > > 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 <John.C.Harrison@Intel.com> > > > --- > > > 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 > > > > > > diff --git a/lib/igt_store.c b/lib/igt_store.c > > > new file mode 100644 > > > index 000000000..42c888b55 > > > --- /dev/null > > > +++ b/lib/igt_store.c > > > @@ -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); > > > + 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. > > > > > > + 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 > > > + 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? > > > > Matt > Yeah, some copies of this code were definitely broken for >32bit addresses. > > John. > > > > > > + } 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); > > > +} > > > diff --git a/lib/igt_store.h b/lib/igt_store.h > > > new file mode 100644 > > > index 000000000..5c6c8263c > > > --- /dev/null > > > +++ b/lib/igt_store.h > > > @@ -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); > > > 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..196236b27 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, 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); > > > } > > > } > > > diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c > > > index 6656b3fcd..5a0c9497c 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" > > > -- > > > 2.25.1 > > > >
On 1/13/2022 12:10, Matthew Brost wrote: > On Thu, Jan 13, 2022 at 11:59:39AM -0800, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> >> >> 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 <John.C.Harrison@Intel.com> >> --- >> 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 >> >> diff --git a/lib/igt_store.c b/lib/igt_store.c >> new file mode 100644 >> index 000000000..42c888b55 >> --- /dev/null >> +++ b/lib/igt_store.c >> @@ -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); >> + 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. > >> + 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_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 Yeah, some copies of this code were definitely broken for >32bit addresses. John. > >> + } 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); >> +} >> diff --git a/lib/igt_store.h b/lib/igt_store.h >> new file mode 100644 >> index 000000000..5c6c8263c >> --- /dev/null >> +++ b/lib/igt_store.h >> @@ -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); >> 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..196236b27 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, 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); >> } >> } >> diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c >> index 6656b3fcd..5a0c9497c 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" >> -- >> 2.25.1 >>
On 1/13/2022 12:23, Matthew Brost wrote: > On Thu, Jan 13, 2022 at 12:27:00PM -0800, John Harrison wrote: >> On 1/13/2022 12:10, Matthew Brost wrote: >>> On Thu, Jan 13, 2022 at 11:59:39AM -0800, John.C.Harrison@Intel.com wrote: >>>> From: John Harrison <John.C.Harrison@Intel.com> >>>> >>>> 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 <John.C.Harrison@Intel.com> >>>> --- >>>> 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 >>>> >>>> diff --git a/lib/igt_store.c b/lib/igt_store.c >>>> new file mode 100644 >>>> index 000000000..42c888b55 >>>> --- /dev/null >>>> +++ b/lib/igt_store.c >>>> @@ -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); >>>> + 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. >> >>>> + 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. Indeed. And in the <gen8 cases below. John. > Matt > >>>> + 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? >>> >>> Matt >> Yeah, some copies of this code were definitely broken for >32bit addresses. >> >> John. >> >>>> + } 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); >>>> +} >>>> diff --git a/lib/igt_store.h b/lib/igt_store.h >>>> new file mode 100644 >>>> index 000000000..5c6c8263c >>>> --- /dev/null >>>> +++ b/lib/igt_store.h >>>> @@ -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); >>>> 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..196236b27 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, 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); >>>> } >>>> } >>>> diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c >>>> index 6656b3fcd..5a0c9497c 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" >>>> -- >>>> 2.25.1 >>>>
diff --git a/lib/igt_store.c b/lib/igt_store.c new file mode 100644 index 000000000..42c888b55 --- /dev/null +++ b/lib/igt_store.c @@ -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); + 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 = 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; + } 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); +} diff --git a/lib/igt_store.h b/lib/igt_store.h new file mode 100644 index 000000000..5c6c8263c --- /dev/null +++ b/lib/igt_store.h @@ -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); 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..196236b27 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, 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); } } diff --git a/tests/i915/i915_hangman.c b/tests/i915/i915_hangman.c index 6656b3fcd..5a0c9497c 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"