Message ID | 20200130204124.29907-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [i-g-t] tests/i915/gem_ctx_persistence: Convert engine subtests to dynamic | expand |
On 30/01/2020 20:41, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Converts all per-engine tests into dynamic subtests and in the process: > > * Put back I915_EXEC_BSD legacy coverage. > * Remove one added static engine list usage. > * Compact code by driving two groups of the name/func table. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > --- > tests/i915/gem_ctx_persistence.c | 97 +++++++++++++------------------- > 1 file changed, 39 insertions(+), 58 deletions(-) > > diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c > index 8b9633b214ff..0d5b22d2b162 100644 > --- a/tests/i915/gem_ctx_persistence.c > +++ b/tests/i915/gem_ctx_persistence.c > @@ -759,7 +759,20 @@ static void smoketest(int i915) > > igt_main > { > - const struct intel_execution_engine2 *e; > + struct { > + const char *name; > + void (*func)(int fd, unsigned int engine); > + } *test, tests[] = { > + { "persistence", test_persistence }, > + { "cleanup", test_nonpersistent_cleanup }, > + { "queued", test_nonpersistent_queued }, > + { "mixed", test_nonpersistent_mixed }, > + { "mixed-process", test_process_mixed }, > + { "hostile", test_nonpersistent_hostile }, > + { "hostile-preempt", test_nonpersistent_hostile_preempt }, > + { "hang", test_nonpersistent_hang }, > + { NULL, NULL }, > + }; > int i915; > > igt_fixture { > @@ -792,72 +805,40 @@ igt_main > igt_subtest("hang") > test_nohangcheck_hang(i915); > > - __for_each_static_engine(e) { > - igt_subtest_group { > - igt_fixture { > - gem_require_ring(i915, e->flags); > - gem_require_contexts(i915); > - } > - > - igt_subtest_f("legacy-%s-persistence", e->name) > - test_persistence(i915, e->flags); > - > - igt_subtest_f("legacy-%s-cleanup", e->name) > - test_nonpersistent_cleanup(i915, e->flags); > - > - igt_subtest_f("legacy-%s-queued", e->name) > - test_nonpersistent_queued(i915, e->flags); > - > - igt_subtest_f("legacy-%s-mixed", e->name) > - test_nonpersistent_mixed(i915, e->flags); > - > - igt_subtest_f("legacy-%s-mixed-process", e->name) > - test_process_mixed(i915, e->flags); > - > - igt_subtest_f("legacy-%s-hostile", e->name) > - test_nonpersistent_hostile(i915, e->flags); > + igt_subtest("smoketest") > + smoketest(i915); > > - igt_subtest_f("legacy-%s-hostile-preempt", e->name) > - test_nonpersistent_hostile_preempt(i915, > - e->flags); > + igt_subtest_group { > + igt_fixture > + gem_require_contexts(i915); > + > + for (test = tests; test->name; test++) { > + igt_subtest_with_dynamic_f("legacy-engines-%s", > + test->name) { > + for_each_engine(e, i915) { > + igt_dynamic_f("%s", e->name) > + test->func(i915, eb_ring(e)); > + } > + } > } > } > > - __for_each_physical_engine(i915, e) { > - igt_subtest_group { > - igt_fixture > - gem_require_contexts(i915); > - > - igt_subtest_f("%s-persistence", e->name) > - test_persistence(i915, e->flags); > - > - igt_subtest_f("%s-cleanup", e->name) > - test_nonpersistent_cleanup(i915, e->flags); > - > - igt_subtest_f("%s-queued", e->name) > - test_nonpersistent_queued(i915, e->flags); > - > - igt_subtest_f("%s-mixed", e->name) > - test_nonpersistent_mixed(i915, e->flags); > - > - igt_subtest_f("%s-mixed-process", e->name) > - test_process_mixed(i915, e->flags); > + igt_subtest_group { > + const struct intel_execution_engine2 *e; > > - igt_subtest_f("%s-hostile", e->name) > - test_nonpersistent_hostile(i915, e->flags); > + igt_fixture > + gem_require_contexts(i915); > > - igt_subtest_f("%s-hostile-preempt", e->name) > - test_nonpersistent_hostile_preempt(i915, > - e->flags); > - > - igt_subtest_f("%s-hang", e->name) > - test_nonpersistent_hang(i915, e->flags); > + for (test = tests; test->name; test++) { > + igt_subtest_with_dynamic_f("engines-%s", test->name) { > + __for_each_physical_engine(i915, e) { > + igt_dynamic_f("%s", e->name) > + test->func(i915, e->flags); > + } > + } > } > } > > - igt_subtest("smoketest") > - smoketest(i915); I also moved this one to before the default context is configured with engine map, since it uses legacy for_each_physical_engine, and is therefore confused as to engine selection. But perhaps better would be to leave it last and convert to __for_each_physical_engine. In the spirit of a smoke test is to exercise all engines after all. Regards, Tvrtko > - > igt_fixture { > close(i915); > } >
Quoting Tvrtko Ursulin (2020-01-30 20:41:24) > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > Converts all per-engine tests into dynamic subtests and in the process: > > * Put back I915_EXEC_BSD legacy coverage. > * Remove one added static engine list usage. > * Compact code by driving two groups of the name/func table. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> > --- > tests/i915/gem_ctx_persistence.c | 97 +++++++++++++------------------- > 1 file changed, 39 insertions(+), 58 deletions(-) > > diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c > index 8b9633b214ff..0d5b22d2b162 100644 > --- a/tests/i915/gem_ctx_persistence.c > +++ b/tests/i915/gem_ctx_persistence.c > @@ -759,7 +759,20 @@ static void smoketest(int i915) > > igt_main > { > - const struct intel_execution_engine2 *e; > + struct { > + const char *name; > + void (*func)(int fd, unsigned int engine); > + } *test, tests[] = { > + { "persistence", test_persistence }, > + { "cleanup", test_nonpersistent_cleanup }, > + { "queued", test_nonpersistent_queued }, > + { "mixed", test_nonpersistent_mixed }, > + { "mixed-process", test_process_mixed }, > + { "hostile", test_nonpersistent_hostile }, > + { "hostile-preempt", test_nonpersistent_hostile_preempt }, > + { "hang", test_nonpersistent_hang }, > + { NULL, NULL }, Heh, that's what I was tempted to suggest but did want to seem like I was requesting too much :) > + }; > int i915; > > igt_fixture { > @@ -792,72 +805,40 @@ igt_main > igt_subtest("hang") > test_nohangcheck_hang(i915); > > - __for_each_static_engine(e) { > - igt_subtest_group { > - igt_fixture { > - gem_require_ring(i915, e->flags); > - gem_require_contexts(i915); > - } > - > - igt_subtest_f("legacy-%s-persistence", e->name) > - test_persistence(i915, e->flags); > - > - igt_subtest_f("legacy-%s-cleanup", e->name) > - test_nonpersistent_cleanup(i915, e->flags); > - > - igt_subtest_f("legacy-%s-queued", e->name) > - test_nonpersistent_queued(i915, e->flags); > - > - igt_subtest_f("legacy-%s-mixed", e->name) > - test_nonpersistent_mixed(i915, e->flags); > - > - igt_subtest_f("legacy-%s-mixed-process", e->name) > - test_process_mixed(i915, e->flags); > - > - igt_subtest_f("legacy-%s-hostile", e->name) > - test_nonpersistent_hostile(i915, e->flags); > + igt_subtest("smoketest") > + smoketest(i915); > > - igt_subtest_f("legacy-%s-hostile-preempt", e->name) > - test_nonpersistent_hostile_preempt(i915, > - e->flags); > + igt_subtest_group { > + igt_fixture > + gem_require_contexts(i915); > + > + for (test = tests; test->name; test++) { > + igt_subtest_with_dynamic_f("legacy-engines-%s", > + test->name) { > + for_each_engine(e, i915) { Don't we need a if (!gem_has_ring(i915, eb_ring(e)) continue; here? > + igt_dynamic_f("%s", e->name) > + test->func(i915, eb_ring(e)); > + } > + } > } > } > > - __for_each_physical_engine(i915, e) { > - igt_subtest_group { > - igt_fixture > - gem_require_contexts(i915); > - > - igt_subtest_f("%s-persistence", e->name) > - test_persistence(i915, e->flags); > - > - igt_subtest_f("%s-cleanup", e->name) > - test_nonpersistent_cleanup(i915, e->flags); > - > - igt_subtest_f("%s-queued", e->name) > - test_nonpersistent_queued(i915, e->flags); > - > - igt_subtest_f("%s-mixed", e->name) > - test_nonpersistent_mixed(i915, e->flags); > - > - igt_subtest_f("%s-mixed-process", e->name) > - test_process_mixed(i915, e->flags); > + igt_subtest_group { > + const struct intel_execution_engine2 *e; > > - igt_subtest_f("%s-hostile", e->name) > - test_nonpersistent_hostile(i915, e->flags); > + igt_fixture > + gem_require_contexts(i915); > > - igt_subtest_f("%s-hostile-preempt", e->name) > - test_nonpersistent_hostile_preempt(i915, > - e->flags); > - > - igt_subtest_f("%s-hang", e->name) > - test_nonpersistent_hang(i915, e->flags); > + for (test = tests; test->name; test++) { > + igt_subtest_with_dynamic_f("engines-%s", test->name) { > + __for_each_physical_engine(i915, e) { > + igt_dynamic_f("%s", e->name) > + test->func(i915, e->flags); > + } > + } > } Then I had a plan to add the pathological engines[] here. -Chris
Quoting Tvrtko Ursulin (2020-01-30 20:44:57) > > On 30/01/2020 20:41, Tvrtko Ursulin wrote: > > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > > > - igt_subtest("smoketest") > > - smoketest(i915); > > I also moved this one to before the default context is configured with > engine map, since it uses legacy for_each_physical_engine, and is > therefore confused as to engine selection. > > But perhaps better would be to leave it last and convert to > __for_each_physical_engine. In the spirit of a smoke test is to exercise > all engines after all. Earlier was fine by me. It did make a bit of sense to get the smoketest running earlier, but later is also fine. -Chris
On 30/01/2020 20:47, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2020-01-30 20:41:24) >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> Converts all per-engine tests into dynamic subtests and in the process: >> >> * Put back I915_EXEC_BSD legacy coverage. >> * Remove one added static engine list usage. >> * Compact code by driving two groups of the name/func table. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> Cc: Bommu Krishnaiah <krishnaiah.bommu@intel.com> >> --- >> tests/i915/gem_ctx_persistence.c | 97 +++++++++++++------------------- >> 1 file changed, 39 insertions(+), 58 deletions(-) >> >> diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c >> index 8b9633b214ff..0d5b22d2b162 100644 >> --- a/tests/i915/gem_ctx_persistence.c >> +++ b/tests/i915/gem_ctx_persistence.c >> @@ -759,7 +759,20 @@ static void smoketest(int i915) >> >> igt_main >> { >> - const struct intel_execution_engine2 *e; >> + struct { >> + const char *name; >> + void (*func)(int fd, unsigned int engine); >> + } *test, tests[] = { >> + { "persistence", test_persistence }, >> + { "cleanup", test_nonpersistent_cleanup }, >> + { "queued", test_nonpersistent_queued }, >> + { "mixed", test_nonpersistent_mixed }, >> + { "mixed-process", test_process_mixed }, >> + { "hostile", test_nonpersistent_hostile }, >> + { "hostile-preempt", test_nonpersistent_hostile_preempt }, >> + { "hang", test_nonpersistent_hang }, >> + { NULL, NULL }, > > Heh, that's what I was tempted to suggest but did want to seem like I > was requesting too much :) Yeah, much less typing, especially with dynamic subtests. > >> + }; >> int i915; >> >> igt_fixture { >> @@ -792,72 +805,40 @@ igt_main >> igt_subtest("hang") >> test_nohangcheck_hang(i915); >> >> - __for_each_static_engine(e) { >> - igt_subtest_group { >> - igt_fixture { >> - gem_require_ring(i915, e->flags); >> - gem_require_contexts(i915); >> - } >> - >> - igt_subtest_f("legacy-%s-persistence", e->name) >> - test_persistence(i915, e->flags); >> - >> - igt_subtest_f("legacy-%s-cleanup", e->name) >> - test_nonpersistent_cleanup(i915, e->flags); >> - >> - igt_subtest_f("legacy-%s-queued", e->name) >> - test_nonpersistent_queued(i915, e->flags); >> - >> - igt_subtest_f("legacy-%s-mixed", e->name) >> - test_nonpersistent_mixed(i915, e->flags); >> - >> - igt_subtest_f("legacy-%s-mixed-process", e->name) >> - test_process_mixed(i915, e->flags); >> - >> - igt_subtest_f("legacy-%s-hostile", e->name) >> - test_nonpersistent_hostile(i915, e->flags); >> + igt_subtest("smoketest") >> + smoketest(i915); >> >> - igt_subtest_f("legacy-%s-hostile-preempt", e->name) >> - test_nonpersistent_hostile_preempt(i915, >> - e->flags); >> + igt_subtest_group { >> + igt_fixture >> + gem_require_contexts(i915); >> + >> + for (test = tests; test->name; test++) { >> + igt_subtest_with_dynamic_f("legacy-engines-%s", >> + test->name) { >> + for_each_engine(e, i915) { > > Don't we need a > if (!gem_has_ring(i915, eb_ring(e)) continue; > here? It's built in to for_each_engine, yay for dynamic subtests! :) > >> + igt_dynamic_f("%s", e->name) >> + test->func(i915, eb_ring(e)); >> + } >> + } >> } >> } >> >> - __for_each_physical_engine(i915, e) { >> - igt_subtest_group { >> - igt_fixture >> - gem_require_contexts(i915); >> - >> - igt_subtest_f("%s-persistence", e->name) >> - test_persistence(i915, e->flags); >> - >> - igt_subtest_f("%s-cleanup", e->name) >> - test_nonpersistent_cleanup(i915, e->flags); >> - >> - igt_subtest_f("%s-queued", e->name) >> - test_nonpersistent_queued(i915, e->flags); >> - >> - igt_subtest_f("%s-mixed", e->name) >> - test_nonpersistent_mixed(i915, e->flags); >> - >> - igt_subtest_f("%s-mixed-process", e->name) >> - test_process_mixed(i915, e->flags); >> + igt_subtest_group { >> + const struct intel_execution_engine2 *e; >> >> - igt_subtest_f("%s-hostile", e->name) >> - test_nonpersistent_hostile(i915, e->flags); >> + igt_fixture >> + gem_require_contexts(i915); >> >> - igt_subtest_f("%s-hostile-preempt", e->name) >> - test_nonpersistent_hostile_preempt(i915, >> - e->flags); >> - >> - igt_subtest_f("%s-hang", e->name) >> - test_nonpersistent_hang(i915, e->flags); >> + for (test = tests; test->name; test++) { >> + igt_subtest_with_dynamic_f("engines-%s", test->name) { >> + __for_each_physical_engine(i915, e) { >> + igt_dynamic_f("%s", e->name) >> + test->func(i915, e->flags); >> + } >> + } >> } > > Then I had a plan to add the pathological engines[] here. Will see later what you exactly mean. Regards, Tvrtko
Quoting Tvrtko Ursulin (2020-01-30 20:56:23) > > On 30/01/2020 20:47, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2020-01-30 20:41:24) > > Then I had a plan to add the pathological engines[] here. > > Will see later what you exactly mean. for_each_gem_engine(ce, __context_engines_static(ctx), it) { struct intel_engine_cs *engine; if (intel_context_set_banned(ce)) continue; engine = active_engine(ce); That's the bit I want to try and confuse. So 1..64xrcs0, 1...64xVirtual and make sure it kills the right active context. I hope I had such nastiness in mind while writing, so hopefully there's no hole here. -Chris
diff --git a/tests/i915/gem_ctx_persistence.c b/tests/i915/gem_ctx_persistence.c index 8b9633b214ff..0d5b22d2b162 100644 --- a/tests/i915/gem_ctx_persistence.c +++ b/tests/i915/gem_ctx_persistence.c @@ -759,7 +759,20 @@ static void smoketest(int i915) igt_main { - const struct intel_execution_engine2 *e; + struct { + const char *name; + void (*func)(int fd, unsigned int engine); + } *test, tests[] = { + { "persistence", test_persistence }, + { "cleanup", test_nonpersistent_cleanup }, + { "queued", test_nonpersistent_queued }, + { "mixed", test_nonpersistent_mixed }, + { "mixed-process", test_process_mixed }, + { "hostile", test_nonpersistent_hostile }, + { "hostile-preempt", test_nonpersistent_hostile_preempt }, + { "hang", test_nonpersistent_hang }, + { NULL, NULL }, + }; int i915; igt_fixture { @@ -792,72 +805,40 @@ igt_main igt_subtest("hang") test_nohangcheck_hang(i915); - __for_each_static_engine(e) { - igt_subtest_group { - igt_fixture { - gem_require_ring(i915, e->flags); - gem_require_contexts(i915); - } - - igt_subtest_f("legacy-%s-persistence", e->name) - test_persistence(i915, e->flags); - - igt_subtest_f("legacy-%s-cleanup", e->name) - test_nonpersistent_cleanup(i915, e->flags); - - igt_subtest_f("legacy-%s-queued", e->name) - test_nonpersistent_queued(i915, e->flags); - - igt_subtest_f("legacy-%s-mixed", e->name) - test_nonpersistent_mixed(i915, e->flags); - - igt_subtest_f("legacy-%s-mixed-process", e->name) - test_process_mixed(i915, e->flags); - - igt_subtest_f("legacy-%s-hostile", e->name) - test_nonpersistent_hostile(i915, e->flags); + igt_subtest("smoketest") + smoketest(i915); - igt_subtest_f("legacy-%s-hostile-preempt", e->name) - test_nonpersistent_hostile_preempt(i915, - e->flags); + igt_subtest_group { + igt_fixture + gem_require_contexts(i915); + + for (test = tests; test->name; test++) { + igt_subtest_with_dynamic_f("legacy-engines-%s", + test->name) { + for_each_engine(e, i915) { + igt_dynamic_f("%s", e->name) + test->func(i915, eb_ring(e)); + } + } } } - __for_each_physical_engine(i915, e) { - igt_subtest_group { - igt_fixture - gem_require_contexts(i915); - - igt_subtest_f("%s-persistence", e->name) - test_persistence(i915, e->flags); - - igt_subtest_f("%s-cleanup", e->name) - test_nonpersistent_cleanup(i915, e->flags); - - igt_subtest_f("%s-queued", e->name) - test_nonpersistent_queued(i915, e->flags); - - igt_subtest_f("%s-mixed", e->name) - test_nonpersistent_mixed(i915, e->flags); - - igt_subtest_f("%s-mixed-process", e->name) - test_process_mixed(i915, e->flags); + igt_subtest_group { + const struct intel_execution_engine2 *e; - igt_subtest_f("%s-hostile", e->name) - test_nonpersistent_hostile(i915, e->flags); + igt_fixture + gem_require_contexts(i915); - igt_subtest_f("%s-hostile-preempt", e->name) - test_nonpersistent_hostile_preempt(i915, - e->flags); - - igt_subtest_f("%s-hang", e->name) - test_nonpersistent_hang(i915, e->flags); + for (test = tests; test->name; test++) { + igt_subtest_with_dynamic_f("engines-%s", test->name) { + __for_each_physical_engine(i915, e) { + igt_dynamic_f("%s", e->name) + test->func(i915, e->flags); + } + } } } - igt_subtest("smoketest") - smoketest(i915); - igt_fixture { close(i915); }