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.
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
> > >
>
