Quoting Tvrtko Ursulin (2018-05-15 09:20:13)
>
> On 14/05/2018 16:10, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-05-14 11:59:09)
> >>
> >> On 14/05/2018 09:02, Chris Wilson wrote:
> >>> The test wrote to the same dwords from multiple contexts, assuming that
> >>> the writes would be ordered by its submission. However, as it was using
> >>> multiple contexts without a write hazard, those timelines are not
> >>> coupled and the requests may be emitted to hw in any order. So emit a
> >>> write hazard for each individual dword in the scratch (avoiding the
> >>> write hazard for the scratch as a whole) to ensure the writes do occur
> >>> in the expected order.
> >>>
> >>> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >>> ---
> >>> tests/gem_ctx_thrash.c | 92 ++++++++++++++++++++++++------------------
> >>> 1 file changed, 53 insertions(+), 39 deletions(-)
> >>>
> >>> diff --git a/tests/gem_ctx_thrash.c b/tests/gem_ctx_thrash.c
> >>> index 2cd9cfebf..b25f95f13 100644
> >>> --- a/tests/gem_ctx_thrash.c
> >>> +++ b/tests/gem_ctx_thrash.c
> >>> @@ -90,17 +90,13 @@ static void single(const char *name, bool all_engines)
> >>> {
> >>> struct drm_i915_gem_exec_object2 *obj;
> >>> struct drm_i915_gem_relocation_entry *reloc;
> >>> - unsigned engines[16];
> >>> - uint64_t size;
> >>> - uint32_t *ctx, *map, scratch;
> >>> - unsigned num_ctx;
> >>> - int fd, gen, num_engines;
> >>> + unsigned int engines[16], num_engines, num_ctx;
> >>> + uint32_t *ctx, *map, scratch, size;
> >>> + int fd, gen;
> >>> #define MAX_LOOP 16
> >>>
> >>> - fd = drm_open_driver_master(DRIVER_INTEL);
> >>> + fd = drm_open_driver(DRIVER_INTEL);
> >>> igt_require_gem(fd);
> >>> - igt_require(gem_can_store_dword(fd, 0));
> >>> -
> >>> gem_require_contexts(fd);
> >>>
> >>> gen = intel_gen(intel_get_drm_devid(fd));
> >>> @@ -108,54 +104,77 @@ static void single(const char *name, bool all_engines)
> >>> num_engines = 0;
> >>> if (all_engines) {
> >>> unsigned engine;
> >>> +
> >>> for_each_physical_engine(fd, engine) {
> >>> + if (!gem_can_store_dword(fd, engine))
> >>> + continue;
> >>> +
> >>> engines[num_engines++] = engine;
> >>> if (num_engines == ARRAY_SIZE(engines))
> >>> break;
> >>> }
> >>> - } else
> >>> + } else {
> >>> + igt_require(gem_can_store_dword(fd, 0));
> >>> engines[num_engines++] = 0;
> >>> + }
> >>> + igt_require(num_engines);
> >>>
> >>> num_ctx = get_num_contexts(fd, num_engines);
> >>>
> >>> size = ALIGN(num_ctx * sizeof(uint32_t), 4096);
> >>> - scratch = gem_create(fd, ALIGN(num_ctx * sizeof(uint32_t), 4096));
> >>> + scratch = gem_create(fd, size);
> >>> gem_set_caching(fd, scratch, I915_CACHING_CACHED);
> >>> - obj = calloc(num_ctx, 2 * sizeof(*obj));
> >>> - reloc = calloc(num_ctx, sizeof(*reloc));
> >>> + obj = calloc(num_ctx, 3 * sizeof(*obj));
> >>> + reloc = calloc(num_ctx, 2 * sizeof(*reloc));
> >>>
> >>> ctx = malloc(num_ctx * sizeof(uint32_t));
> >>> igt_assert(ctx);
> >>> for (unsigned n = 0; n < num_ctx; n++) {
> >>> ctx[n] = gem_context_create(fd);
> >>> - obj[2*n + 0].handle = scratch;
> >>> -
> >>> - reloc[n].target_handle = scratch;
> >>> - reloc[n].presumed_offset = 0;
> >>> - reloc[n].offset = sizeof(uint32_t);
> >>> - reloc[n].delta = n * sizeof(uint32_t);
> >>> - reloc[n].read_domains = I915_GEM_DOMAIN_INSTRUCTION;
> >>> - reloc[n].write_domain = 0; /* lies! */
> >>> +
> >>> + obj[3*n + 0].handle = gem_create(fd, 4096);
> >>> + reloc[2*n + 0].target_handle = obj[3*n + 0].handle;
> >>> + reloc[2*n + 0].presumed_offset = 0;
> >>> + reloc[2*n + 0].offset = 4000;
> >>> + reloc[2*n + 0].delta = 0;
> >>> + reloc[2*n + 0].read_domains = I915_GEM_DOMAIN_RENDER;
> >>> + reloc[2*n + 0].write_domain = I915_GEM_DOMAIN_RENDER;
> >>> +
> >>> + obj[3*n + 1].handle = scratch;
> >>> + reloc[2*n + 1].target_handle = scratch;
> >>> + reloc[2*n + 1].presumed_offset = 0;
> >>> + reloc[2*n + 1].offset = sizeof(uint32_t);
> >>> + reloc[2*n + 1].delta = n * sizeof(uint32_t);
> >>> + reloc[2*n + 1].read_domains = I915_GEM_DOMAIN_RENDER;
> >>> + reloc[2*n + 1].write_domain = 0; /* lies! */
> >>> if (gen >= 4 && gen < 8)
> >>> - reloc[n].offset += sizeof(uint32_t);
> >>> + reloc[2*n + 1].offset += sizeof(uint32_t);
> >>>
> >>> - obj[2*n + 1].relocs_ptr = to_user_pointer(&reloc[n]);
> >>> - obj[2*n + 1].relocation_count = 1;
> >>> + obj[3*n + 2].relocs_ptr = to_user_pointer(&reloc[2*n]);
> >>> + obj[3*n + 2].relocation_count = 2;
> >>> }
> >>>
> >>> map = gem_mmap__cpu(fd, scratch, 0, size, PROT_WRITE);
> >>> - for (unsigned loop = 1; loop <= MAX_LOOP; loop <<= 1) {
> >>> - unsigned count = loop * num_ctx;
> >>> + for (unsigned int loop = 1; loop <= MAX_LOOP; loop <<= 1) {
> >>> + const unsigned int count = loop * num_ctx;
> >>> uint32_t *all;
> >>>
> >>> all = malloc(count * sizeof(uint32_t));
> >>> - for (unsigned n = 0; n < count; n++)
> >>> + for (unsigned int n = 0; n < count; n++)
> >>> all[n] = ctx[n % num_ctx];
> >>> igt_permute_array(all, count, xchg_int);
> >>> - for (unsigned n = 0; n < count; n++) {
> >>> - struct drm_i915_gem_execbuffer2 execbuf;
> >>> - unsigned r = n % num_ctx;
> >>> - uint64_t offset = reloc[r].presumed_offset + reloc[r].delta;
> >>> +
> >>> + for (unsigned int n = 0; n < count; n++) {
> >>> + const unsigned int r = n % num_ctx;
> >>> + struct drm_i915_gem_execbuffer2 execbuf = {
> >>> + .buffers_ptr = to_user_pointer(&obj[3*r]),
> >>> + .buffer_count = 3,
> >>> + .flags = engines[n % num_engines],
> >>> + .rsvd1 = all[n],
> >>> + };
> >>> + uint64_t offset =
> >>> + reloc[2*r + 1].presumed_offset +
> >>> + reloc[2*r + 1].delta;
> >>> uint32_t handle = gem_create(fd, 4096);
> >>> uint32_t buf[16];
> >>> int i;
> >>> @@ -176,27 +195,22 @@ static void single(const char *name, bool all_engines)
> >>> buf[++i] = all[n];
> >>> buf[++i] = MI_BATCH_BUFFER_END;
> >>> gem_write(fd, handle, 0, buf, sizeof(buf));
> >>> - obj[2*r + 1].handle = handle;
> >>> + obj[3*r + 2].handle = handle;
> >>>
> >>> - memset(&execbuf, 0, sizeof(execbuf));
> >>> - execbuf.buffers_ptr = to_user_pointer(&obj[2*r]);
> >>> - execbuf.buffer_count = 2;
> >>> - execbuf.flags = engines[n % num_engines];
> >>> - execbuf.rsvd1 = all[n];
> >>> gem_execbuf(fd, &execbuf);
> >>> gem_close(fd, handle);
> >>> }
> >>>
> >>> - /* Note we lied about the write-domain when writing from the
> >>> + /*
> >>> + * Note we lied about the write-domain when writing from the
> >>> * GPU (in order to avoid inter-ring synchronisation), so now
> >>> * we have to force the synchronisation here.
> >>> */
> >>> gem_set_domain(fd, scratch,
> >>> I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> >>> - for (unsigned n = count - num_ctx; n < count; n++)
> >>> + for (unsigned int n = count - num_ctx; n < count; n++)
> >>> igt_assert_eq(map[n % num_ctx], all[n]);
> >>> free(all);
> >>> - igt_progress(name, loop, MAX_LOOP);
> >>> }
> >>> munmap(map, size);
> >>>
> >>>
> >>
> >> Just one thing I failed to figure out - what would be the difference
> >> from simply putting a write hazard on the scratch page write? Wouldn't
> >> both guarantee execution in order of execbuf?
> >
> > That would cause every request to be executed in order and prevent
> > concurrent execution across engines. The intent of the test is to check
>
> True, I forgot the context of a patch that each context gets it own
> "ordering" bo, while the execbuf loop is separate.
>
> > what happens when we run out of GTT space for the context/ring objects,
> > and part of that is to load up the GPU as much and as wide as possible.
> >
> > Instead of forcing any ordering, we could just make the scratch big
> > enough for all writes; but it was such a quirky bug that I wanted to
> > share the fix. :)
>
> However, if the "ordering" bo is one for each context, I still don't
> quite understand what is ordered with the patch which wouldn't normally be.
Not one for each context, one for each dword in the scatch.
> And also, the verification loop is after all execbufs have completed -
> so why is even the order of execution important?
Huh? Many contexts write into a single dword, we only check the last
value of the dword and presume which context wrote its id. We never told
the kernel what order the contexts had to be executed in, so the last
value is unspecified.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
