[i-g-t] Revert "tests/i915: Use engine query interface for gem_ctx_isolation/persistence"
diff mbox series

Message ID 20191207010835.2100418-1-chris@chris-wilson.co.uk
State New
Headers show
Series
  • [i-g-t] Revert "tests/i915: Use engine query interface for gem_ctx_isolation/persistence"
Related show

Commit Message

Chris Wilson Dec. 7, 2019, 1:08 a.m. UTC
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(-)

Comments

Andi Shyti Dec. 7, 2019, 5:42 p.m. UTC | #1
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
Andi Shyti Dec. 7, 2019, 5:43 p.m. UTC | #2
> > 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
Chris Wilson Dec. 7, 2019, 5:46 p.m. UTC | #3
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
Tvrtko Ursulin Dec. 9, 2019, 9:25 a.m. UTC | #4
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);
>

Patch
diff mbox series

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);