| Message ID | 20191207010835.2100418-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t] Revert "tests/i915: Use engine query interface for gem_ctx_isolation/persistence" | expand |
Hi Chris, On Sat, Dec 07, 2019 at 01:08:35AM +0000, Chris Wilson wrote: > This reverts commit 343aae776a58a67fa153825385e6fe90e3185c5b. > > __for_each_physical_engine() reprograms the context, invalidating the > use of e->flags to select engines, necessitating e->index instead. > Withot also fixing up the engine selection, the result is that random > engines were being used to read registers from the intended engine. > This does not end well. So, the problem here is that with __for_each_physical_engine() we "reprogram the context" which means that there is a re-mapping of engines in it. Have I understood correctly? Doesn't that happen only in the case when the context has no engines in it? > @@ -877,7 +876,8 @@ igt_main > igt_skip_on(gen > LAST_KNOWN_GEN); > } > > - __for_each_physical_engine(fd, e) { > + for (const struct intel_execution_engine2 *e = intel_execution_engines2; > + e->name; e++) { __for_each_static_engine() ? Andi
> > This reverts commit 343aae776a58a67fa153825385e6fe90e3185c5b. [...] > > - __for_each_physical_engine(fd, e) { > > + for (const struct intel_execution_engine2 *e = intel_execution_engines2; > > + e->name; e++) { > > __for_each_static_engine() ? oh yes, that's a revert! ignore it. Andi
Quoting Andi Shyti (2019-12-07 17:42:39) > Hi Chris, > > On Sat, Dec 07, 2019 at 01:08:35AM +0000, Chris Wilson wrote: > > This reverts commit 343aae776a58a67fa153825385e6fe90e3185c5b. > > > > __for_each_physical_engine() reprograms the context, invalidating the > > use of e->flags to select engines, necessitating e->index instead. > > Withot also fixing up the engine selection, the result is that random > > engines were being used to read registers from the intended engine. > > This does not end well. > > So, the problem here is that with __for_each_physical_engine() we > "reprogram the context" which means that there is a re-mapping of > engines in it. Have I understood correctly? > > Doesn't that happen only in the case when the context has no > engines in it? iirc, it does something like init_engine_map and sets a consistent engine layout on the context -- but switches execbuf over to index mode. > > @@ -877,7 +876,8 @@ igt_main > > igt_skip_on(gen > LAST_KNOWN_GEN); > > } > > > > - __for_each_physical_engine(fd, e) { > > + for (const struct intel_execution_engine2 *e = intel_execution_engines2; > > + e->name; e++) { > > __for_each_static_engine() ? I'm just doing a simple revert so we can try again :) -Chris
On 07/12/2019 01:08, Chris Wilson wrote: > This reverts commit 343aae776a58a67fa153825385e6fe90e3185c5b. > > __for_each_physical_engine() reprograms the context, invalidating the > use of e->flags to select engines, necessitating e->index instead. > Withot also fixing up the engine selection, the result is that random > engines were being used to read registers from the intended engine. > This does not end well. Argh, an oversight.. test(s) needs to be looked at in detail and converted fully. On a cursory glance a sprinkle of gem_context_set_all_engines on internally created contexts is needed to align with the passed in e and e->flags. Acked-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Stuart Summers <stuart.summers@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > tests/i915/gem_ctx_isolation.c | 4 ++-- > tests/i915/gem_ctx_persistence.c | 2 +- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c > index 9435209e9..6aa27133c 100644 > --- a/tests/i915/gem_ctx_isolation.c > +++ b/tests/i915/gem_ctx_isolation.c > @@ -856,7 +856,6 @@ static unsigned int __has_context_isolation(int fd) > > igt_main > { > - struct intel_execution_engine2 *e; > unsigned int has_context_isolation = 0; > int fd = -1; > > @@ -877,7 +876,8 @@ igt_main > igt_skip_on(gen > LAST_KNOWN_GEN); > } > > - __for_each_physical_engine(fd, e) { > + for (const struct intel_execution_engine2 *e = intel_execution_engines2; > + e->name; e++) { > igt_subtest_group { > igt_fixture { > igt_require(has_context_isolation & (1 << e->class)); > diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c > index 30772159b..d68431ae0 100644 > --- a/tests/i915/gem_ctx_persistence.c > +++ b/tests/i915/gem_ctx_persistence.c > @@ -727,7 +727,7 @@ igt_main > igt_subtest("hangcheck") > test_nohangcheck_hostile(i915); > > - __for_each_physical_engine(i915, e) { > + __for_each_static_engine(e) { > igt_subtest_group { > igt_fixture { > gem_require_ring(i915, e->flags); >
diff --git a/tests/i915/gem_ctx_isolation.c b/tests/i915/gem_ctx_isolation.c index 9435209e9..6aa27133c 100644 --- a/tests/i915/gem_ctx_isolation.c +++ b/tests/i915/gem_ctx_isolation.c @@ -856,7 +856,6 @@ static unsigned int __has_context_isolation(int fd) igt_main { - struct intel_execution_engine2 *e; unsigned int has_context_isolation = 0; int fd = -1; @@ -877,7 +876,8 @@ igt_main igt_skip_on(gen > LAST_KNOWN_GEN); } - __for_each_physical_engine(fd, e) { + for (const struct intel_execution_engine2 *e = intel_execution_engines2; + e->name; e++) { igt_subtest_group { igt_fixture { igt_require(has_context_isolation & (1 << e->class)); diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c index 30772159b..d68431ae0 100644 --- a/tests/i915/gem_ctx_persistence.c +++ b/tests/i915/gem_ctx_persistence.c @@ -727,7 +727,7 @@ igt_main igt_subtest("hangcheck") test_nohangcheck_hostile(i915); - __for_each_physical_engine(i915, e) { + __for_each_static_engine(e) { igt_subtest_group { igt_fixture { gem_require_ring(i915, e->flags);
This reverts commit 343aae776a58a67fa153825385e6fe90e3185c5b. __for_each_physical_engine() reprograms the context, invalidating the use of e->flags to select engines, necessitating e->index instead. Withot also fixing up the engine selection, the result is that random engines were being used to read registers from the intended engine. This does not end well. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> Cc: Stuart Summers <stuart.summers@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> --- tests/i915/gem_ctx_isolation.c | 4 ++-- tests/i915/gem_ctx_persistence.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-)