[Intel-gfx] [igt-dev] [PATCH i-g-t 08/11] lib/store: Refactor common store code into helper function
John Harrison
john.c.harrison at intel.com
Wed Dec 22 02:22:22 UTC 2021
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 at Intel.com wrote:
>>>> From: John Harrison <John.C.Harrison at Intel.com>
>>>>
>>>> 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 at Intel.com>
>>>> ---
>>>> 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 at 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
>>>>
More information about the Intel-gfx
mailing list