Re: [igt-dev] [Intel-gfx] [PATCH i-g-t] tests/i915/gem_ctx_persistence: Convert engine subtests to dynamic

Tvrtko Ursulin
2020-01-30T20:56:23Z

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