Chris,
I applied your patches and tested on DG1 hardware. This new code works well, except there are still two issues to this test (gem_ctx_isloation.c)
1. the test loops over all possible engines (......ccs0, ccs1, ccs2,....). the legacy interface is used to check if an engine is supported:
gem_require_ring(fd, e->flags);
for unsupported engines, the following message is printed by IGT test system:
Test requirement not met in function gem_require_ring, file ../lib/ioctl_wrappers.c:1288:
Test requirement: gem_has_ring(fd, ring)
I think this is a minor issue, enhancement is expected to the IGT test code to handle it.
2. all the xxx-nonpriv-switch subtest fails (where xxx is the supported engine), for rcs0 example:
Starting subtest: rcs0-nonpriv-switch
(gem_ctx_isolation:16463) igt_aux-CRITICAL: Test assertion failure function sig_abort, file ../lib/igt_aux.c:502:
(gem_ctx_isolation:16463) igt_aux-CRITICAL: Failed assertion: !"GPU hung"
Stack trace:
#0 ../lib/igt_core.c:1850 __igt_fail_assert()
#1 ../lib/igt_aux.c:506 igt_fork_hang_detector()
#2 [killpg+0x40]
#3 [ioctl+0xb]
#4 [drmIoctl+0x30]
#5 ../lib/i915/gem_context.c:119 __gem_context_destroy()
#6 ../lib/i915/gem_context.c:137 gem_context_destroy()
#7 ../tests/i915/gem_ctx_isolation.c:666 nonpriv()
#8 ../tests/i915/gem_ctx_isolation.c:947 __real_main893()
#9 ../tests/i915/gem_ctx_isolation.c:893 main()
#10 [__libc_start_main+0xf3]
I spent quite a while to study the source code, but I still don't understand what this subtest is doing. If you know, can you explain some?
--CQ
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of
> Chris Wilson
> Sent: Wednesday, November 13, 2019 4:53 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: igt-dev@xxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH i-g-t 5/9] i915/gem_ctx_isolation: Check engine
> relative registers
>
> Some of the non-privileged registers are at the same offset on each engine.
> We can improve our coverage for unknown HW layout by using the reported
> engine->mmio_base for relative offsets.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
> tests/i915/gem_ctx_isolation.c | 164 ++++++++++++++++++++-------------
> 1 file changed, 100 insertions(+), 64 deletions(-)
>
> diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c
> index 6aa27133c..546ffac3a 100644
> --- a/tests/i915/gem_ctx_isolation.c
> +++ b/tests/i915/gem_ctx_isolation.c
> @@ -70,6 +70,7 @@ static const struct named_register {
> uint32_t ignore_bits;
> uint32_t write_mask; /* some registers bits do not exist */
> bool masked;
> + bool relative;
> } nonpriv_registers[] = {
> { "NOPID", NOCTX, RCS0, 0x2094 },
> { "MI_PREDICATE_RESULT_2", NOCTX, RCS0, 0x23bc }, @@ -109,7
> +110,6 @@ static const struct named_register {
> { "PS_DEPTH_COUNT_1", GEN8, RCS0, 0x22f8, 2 },
> { "BB_OFFSET", GEN8, RCS0, 0x2158, .ignore_bits = 0x7 },
> { "MI_PREDICATE_RESULT_1", GEN8, RCS0, 0x241c },
> - { "CS_GPR", GEN8, RCS0, 0x2600, 32 },
> { "OA_CTX_CONTROL", GEN8, RCS0, 0x2360 },
> { "OACTXID", GEN8, RCS0, 0x2364 },
> { "PS_INVOCATION_COUNT_2", GEN8, RCS0, 0x2448, 2, .write_mask
> = ~0x3 }, @@ -138,79 +138,56 @@ static const struct named_register {
>
> { "CTX_PREEMPT", NOCTX /* GEN10 */, RCS0, 0x2248 },
> { "CS_CHICKEN1", GEN11, RCS0, 0x2580, .masked = true },
> - { "HDC_CHICKEN1", GEN_RANGE(10, 10), RCS0, 0x7304, .masked =
> true },
>
> /* Privileged (enabled by w/a + FORCE_TO_NONPRIV) */
> { "CTX_PREEMPT", NOCTX /* GEN9 */, RCS0, 0x2248 },
> { "CS_CHICKEN1", GEN_RANGE(9, 10), RCS0, 0x2580, .masked = true },
> { "COMMON_SLICE_CHICKEN2", GEN_RANGE(9, 9), RCS0,
> 0x7014, .masked = true },
> - { "HDC_CHICKEN1", GEN_RANGE(9, 9), RCS0, 0x7304, .masked =
> true },
> + { "HDC_CHICKEN1", GEN_RANGE(9, 10), RCS0, 0x7304, .masked =
> true },
> { "SLICE_COMMON_ECO_CHICKEN1", GEN_RANGE(11, 11) /* + glk */,
> RCS0, 0x731c, .masked = true },
> { "L3SQREG4", NOCTX /* GEN9:skl,kbl */, RCS0, 0xb118, .write_mask
> = ~0x1ffff0 },
> { "HALF_SLICE_CHICKEN7", GEN_RANGE(11, 11), RCS0,
> 0xe194, .masked = true },
> { "SAMPLER_MODE", GEN_RANGE(11, 11), RCS0, 0xe18c, .masked =
> true },
>
> - { "BCS_GPR", GEN9, BCS0, 0x22600, 32 },
> { "BCS_SWCTRL", GEN8, BCS0, 0x22200, .write_mask = 0x3, .masked =
> true },
>
> { "MFC_VDBOX1", NOCTX, VCS0, 0x12800, 64 },
> { "MFC_VDBOX2", NOCTX, VCS1, 0x1c800, 64 },
>
> - { "VCS0_GPR", GEN_RANGE(9, 10), VCS0, 0x12600, 32 },
> - { "VCS1_GPR", GEN_RANGE(9, 10), VCS1, 0x1c600, 32 },
> - { "VECS_GPR", GEN_RANGE(9, 10), VECS0, 0x1a600, 32 },
> -
> - { "VCS0_GPR", GEN11, VCS0, 0x1c0600, 32 },
> - { "VCS1_GPR", GEN11, VCS1, 0x1c4600, 32 },
> - { "VCS2_GPR", GEN11, VCS2, 0x1d0600, 32 },
> - { "VCS3_GPR", GEN11, VCS3, 0x1d4600, 32 },
> - { "VECS_GPR", GEN11, VECS0, 0x1c8600, 32 },
> + { "xCS_GPR", GEN9, ALL, 0x600, 32, .relative = true },
>
> {}
> }, ignore_registers[] = {
> { "RCS timestamp", GEN6, ~0u, 0x2358 },
> { "BCS timestamp", GEN7, ~0u, 0x22358 },
>
> - { "VCS0 timestamp", GEN_RANGE(7, 10), ~0u, 0x12358 },
> - { "VCS1 timestamp", GEN_RANGE(7, 10), ~0u, 0x1c358 },
> - { "VECS timestamp", GEN_RANGE(8, 10), ~0u, 0x1a358 },
> -
> - { "VCS0 timestamp", GEN11, ~0u, 0x1c0358 },
> - { "VCS1 timestamp", GEN11, ~0u, 0x1c4358 },
> - { "VCS2 timestamp", GEN11, ~0u, 0x1d0358 },
> - { "VCS3 timestamp", GEN11, ~0u, 0x1d4358 },
> - { "VECS timestamp", GEN11, ~0u, 0x1c8358 },
> + { "xCS timestamp", GEN8, ALL, 0x358, .relative = true },
>
> /* huc read only */
> - { "BSD0 0x2000", GEN11, ~0u, 0x1c0000 + 0x2000 },
> - { "BSD0 0x2000", GEN11, ~0u, 0x1c0000 + 0x2014 },
> - { "BSD0 0x2000", GEN11, ~0u, 0x1c0000 + 0x23b0 },
> -
> - { "BSD1 0x2000", GEN11, ~0u, 0x1c4000 + 0x2000 },
> - { "BSD1 0x2000", GEN11, ~0u, 0x1c4000 + 0x2014 },
> - { "BSD1 0x2000", GEN11, ~0u, 0x1c4000 + 0x23b0 },
> -
> - { "BSD2 0x2000", GEN11, ~0u, 0x1d0000 + 0x2000 },
> - { "BSD2 0x2000", GEN11, ~0u, 0x1d0000 + 0x2014 },
> - { "BSD2 0x2000", GEN11, ~0u, 0x1d0000 + 0x23b0 },
> -
> - { "BSD3 0x2000", GEN11, ~0u, 0x1d4000 + 0x2000 },
> - { "BSD3 0x2000", GEN11, ~0u, 0x1d4000 + 0x2014 },
> - { "BSD3 0x2000", GEN11, ~0u, 0x1d4000 + 0x23b0 },
> + { "BSD 0x2000", GEN11, ALL, 0x2000, .relative = true },
> + { "BSD 0x2014", GEN11, ALL, 0x2014, .relative = true },
> + { "BSD 0x23b0", GEN11, ALL, 0x23b0, .relative = true },
>
> {}
> };
>
> -static const char *register_name(uint32_t offset, char *buf, size_t len)
> +static const char *
> +register_name(uint32_t offset, uint32_t mmio_base, char *buf, size_t
> +len)
> {
> for (const struct named_register *r = nonpriv_registers; r->name;
> r++) {
> unsigned int width = r->count ? 4*r->count : 4;
> - if (offset >= r->offset && offset < r->offset + width) {
> + uint32_t base;
> +
> + base = r->offset;
> + if (r->relative)
> + base += mmio_base;
> +
> + if (offset >= base && offset < base + width) {
> if (r->count <= 1)
> return r->name;
>
> snprintf(buf, len, "%s[%d]",
> - r->name, (offset - r->offset)/4);
> + r->name, (offset - base) / 4);
> return buf;
> }
> }
> @@ -218,22 +195,35 @@ static const char *register_name(uint32_t offset,
> char *buf, size_t len)
> return "unknown";
> }
>
> -static const struct named_register *lookup_register(uint32_t offset)
> +static const struct named_register *
> +lookup_register(uint32_t offset, uint32_t mmio_base)
> {
> for (const struct named_register *r = nonpriv_registers; r->name;
> r++) {
> unsigned int width = r->count ? 4*r->count : 4;
> - if (offset >= r->offset && offset < r->offset + width)
> + uint32_t base;
> +
> + base = r->offset;
> + if (r->relative)
> + base += mmio_base;
> +
> + if (offset >= base && offset < base + width)
> return r;
> }
>
> return NULL;
> }
>
> -static bool ignore_register(uint32_t offset)
> +static bool ignore_register(uint32_t offset, uint32_t mmio_base)
> {
> for (const struct named_register *r = ignore_registers; r->name; r++)
> {
> unsigned int width = r->count ? 4*r->count : 4;
> - if (offset >= r->offset && offset < r->offset + width)
> + uint32_t base;
> +
> + base = r->offset;
> + if (r->relative)
> + base += mmio_base;
> +
> + if (offset >= base && offset < base + width)
> return true;
> }
>
> @@ -248,6 +238,7 @@ static void tmpl_regs(int fd, {
> const unsigned int gen_bit = 1 <<
> intel_gen(intel_get_drm_devid(fd));
> const unsigned int engine_bit = ENGINE(e->class, e->instance);
> + const uint32_t mmio_base = gem_engine_mmio_base(fd, e->name);
> unsigned int regs_size;
> uint32_t *regs;
>
> @@ -259,12 +250,20 @@ static void tmpl_regs(int fd,
> I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
>
> for (const struct named_register *r = nonpriv_registers; r->name;
> r++) {
> + uint32_t offset;
> +
> if (!(r->engine_mask & engine_bit))
> continue;
> if (!(r->gen_mask & gen_bit))
> continue;
> - for (unsigned count = r->count ?: 1, offset = r->offset;
> - count--; offset += 4) {
> + if (r->relative && !mmio_base)
> + continue;
> +
> + offset = r->offset;
> + if (r->relative)
> + offset += mmio_base;
> +
> + for (unsigned count = r->count ?: 1; count--; offset += 4) {
> uint32_t x = value;
> if (r->write_mask)
> x &= r->write_mask;
> @@ -284,6 +283,7 @@ static uint32_t read_regs(int fd,
> const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
> const unsigned int gen_bit = 1 << gen;
> const unsigned int engine_bit = ENGINE(e->class, e->instance);
> + const uint32_t mmio_base = gem_engine_mmio_base(fd, e->name);
> const bool r64b = gen >= 8;
> struct drm_i915_gem_exec_object2 obj[2];
> struct drm_i915_gem_relocation_entry *reloc; @@ -311,13 +311,20
> @@ static uint32_t read_regs(int fd,
>
> n = 0;
> for (const struct named_register *r = nonpriv_registers; r->name;
> r++) {
> + uint32_t offset;
> +
> if (!(r->engine_mask & engine_bit))
> continue;
> if (!(r->gen_mask & gen_bit))
> continue;
> + if (r->relative && !mmio_base)
> + continue;
> +
> + offset = r->offset;
> + if (r->relative)
> + offset += mmio_base;
>
> - for (unsigned count = r->count ?: 1, offset = r->offset;
> - count--; offset += 4) {
> + for (unsigned count = r->count ?: 1; count--; offset += 4) {
> *b++ = 0x24 << 23 | (1 + r64b); /* SRM */
> *b++ = offset;
> reloc[n].target_handle = obj[0].handle; @@ -357,6
> +364,7 @@ static void write_regs(int fd, {
> const unsigned int gen_bit = 1 <<
> intel_gen(intel_get_drm_devid(fd));
> const unsigned int engine_bit = ENGINE(e->class, e->instance);
> + const uint32_t mmio_base = gem_engine_mmio_base(fd, e->name);
> struct drm_i915_gem_exec_object2 obj;
> struct drm_i915_gem_execbuffer2 execbuf;
> unsigned int batch_size;
> @@ -372,12 +380,20 @@ static void write_regs(int fd,
> gem_set_domain(fd, obj.handle,
> I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU);
> for (const struct named_register *r = nonpriv_registers; r->name;
> r++) {
> + uint32_t offset;
> +
> if (!(r->engine_mask & engine_bit))
> continue;
> if (!(r->gen_mask & gen_bit))
> continue;
> - for (unsigned count = r->count ?: 1, offset = r->offset;
> - count--; offset += 4) {
> + if (r->relative && !mmio_base)
> + continue;
> +
> + offset = r->offset;
> + if (r->relative)
> + offset += mmio_base;
> +
> + for (unsigned count = r->count ?: 1; count--; offset += 4) {
> uint32_t x = value;
> if (r->write_mask)
> x &= r->write_mask;
> @@ -410,6 +426,7 @@ static void restore_regs(int fd,
> const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
> const unsigned int gen_bit = 1 << gen;
> const unsigned int engine_bit = ENGINE(e->class, e->instance);
> + const uint32_t mmio_base = gem_engine_mmio_base(fd, e->name);
> const bool r64b = gen >= 8;
> struct drm_i915_gem_exec_object2 obj[2];
> struct drm_i915_gem_execbuffer2 execbuf; @@ -437,13 +454,20
> @@ static void restore_regs(int fd,
>
> n = 0;
> for (const struct named_register *r = nonpriv_registers; r->name;
> r++) {
> + uint32_t offset;
> +
> if (!(r->engine_mask & engine_bit))
> continue;
> if (!(r->gen_mask & gen_bit))
> continue;
> + if (r->relative && !mmio_base)
> + continue;
> +
> + offset = r->offset;
> + if (r->relative)
> + offset += mmio_base;
>
> - for (unsigned count = r->count ?: 1, offset = r->offset;
> - count--; offset += 4) {
> + for (unsigned count = r->count ?: 1; count--; offset += 4) {
> *b++ = 0x29 << 23 | (1 + r64b); /* LRM */
> *b++ = offset;
> reloc[n].target_handle = obj[0].handle; @@ -479,6
> +503,7 @@ static void dump_regs(int fd,
> const int gen = intel_gen(intel_get_drm_devid(fd));
> const unsigned int gen_bit = 1 << gen;
> const unsigned int engine_bit = ENGINE(e->class, e->instance);
> + const uint32_t mmio_base = gem_engine_mmio_base(fd, e->name);
> unsigned int regs_size;
> uint32_t *out;
>
> @@ -489,26 +514,36 @@ static void dump_regs(int fd,
> gem_set_domain(fd, regs, I915_GEM_DOMAIN_CPU, 0);
>
> for (const struct named_register *r = nonpriv_registers; r->name;
> r++) {
> + uint32_t offset;
> +
> if (!(r->engine_mask & engine_bit))
> continue;
> if (!(r->gen_mask & gen_bit))
> continue;
> + if (r->relative && !mmio_base)
> + continue;
> +
> + offset = r->offset;
> + if (r->relative)
> + offset += mmio_base;
>
> if (r->count <= 1) {
> igt_debug("0x%04x (%s): 0x%08x\n",
> - r->offset, r->name, out[r->offset/4]);
> + offset, r->name, out[offset / 4]);
> } else {
> for (unsigned x = 0; x < r->count; x++)
> igt_debug("0x%04x (%s[%d]): 0x%08x\n",
> - r->offset+4*x, r->name, x,
> - out[r->offset/4 + x]);
> + offset + 4 * x, r->name, x,
> + out[offset / 4 + x]);
> }
> }
> munmap(out, regs_size);
> }
>
> -static void compare_regs(int fd, uint32_t A, uint32_t B, const char *who)
> +static void compare_regs(int fd, const struct intel_execution_engine2 *e,
> + uint32_t A, uint32_t B, const char *who)
> {
> + const uint32_t mmio_base = gem_engine_mmio_base(fd, e->name);
> unsigned int num_errors;
> unsigned int regs_size;
> uint32_t *a, *b;
> @@ -532,11 +567,11 @@ static void compare_regs(int fd, uint32_t A,
> uint32_t B, const char *who)
> if (a[n] == b[n])
> continue;
>
> - if (ignore_register(offset))
> + if (ignore_register(offset, mmio_base))
> continue;
>
> mask = ~0u;
> - r = lookup_register(offset);
> + r = lookup_register(offset, mmio_base);
> if (r && r->masked)
> mask >>= 16;
> if (r && r->ignore_bits)
> @@ -547,7 +582,7 @@ static void compare_regs(int fd, uint32_t A, uint32_t B,
> const char *who)
>
> igt_warn("Register 0x%04x (%s): A=%08x B=%08x\n",
> offset,
> - register_name(offset, buf, sizeof(buf)),
> + register_name(offset, mmio_base, buf, sizeof(buf)),
> a[n] & mask, b[n] & mask);
> num_errors++;
> }
> @@ -638,7 +673,7 @@ static void nonpriv(int fd,
>
> igt_spin_free(fd, spin);
>
> - compare_regs(fd, tmpl, regs[1], "nonpriv read/writes");
> + compare_regs(fd, e, tmpl, regs[1], "nonpriv read/writes");
>
> for (int n = 0; n < ARRAY_SIZE(regs); n++)
> gem_close(fd, regs[n]);
> @@ -708,8 +743,9 @@ static void isolation(int fd,
> igt_spin_free(fd, spin);
>
> if (!(flags & DIRTY1))
> - compare_regs(fd, regs[0], tmp, "two reads of the
> same ctx");
> - compare_regs(fd, regs[0], regs[1], "two virgin contexts");
> + compare_regs(fd, e, regs[0], tmp,
> + "two reads of the same ctx");
> + compare_regs(fd, e, regs[0], regs[1], "two virgin contexts");
>
> for (int n = 0; n < ARRAY_SIZE(ctx); n++) {
> gem_close(fd, regs[n]);
> @@ -829,13 +865,13 @@ static void preservation(int fd,
> char buf[80];
>
> snprintf(buf, sizeof(buf), "dirty %x context\n", values[v]);
> - compare_regs(fd, regs[v][0], regs[v][1], buf);
> + compare_regs(fd, e, regs[v][0], regs[v][1], buf);
>
> gem_close(fd, regs[v][0]);
> gem_close(fd, regs[v][1]);
> gem_context_destroy(fd, ctx[v]);
> }
> - compare_regs(fd, regs[num_values][0], regs[num_values][1],
> "clean");
> + compare_regs(fd, e, regs[num_values][0], regs[num_values][1],
> +"clean");
> gem_context_destroy(fd, ctx[num_values]); }
>
> --
> 2.24.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
