[igt-dev] [PATCH i-g-t 7/8] perf_pmu: Use dynamic subtests
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Nov 8 16:27:15 UTC 2019
On 08/11/2019 16:13, Katarzyna Dec wrote:
> On Thu, Oct 24, 2019 at 02:05:45PM +0300, Petri Latvala wrote:
>> Instead of generating a subtest for each engine in a static list,
>> convert to dynamic subtests, with one dynamic subtest per actually
>> present physical engine.
>>
>> In addition, convert comments to igt_describe calls and generally
>> refactor the lot to a series of simpler loops.
>
> Tvrko, is there a chance you could look at dynamic engines usage here and give
> r-b? It will allow us to merge whole series.
It slipped off my radar that there is actual recent series to look at. :I
Anyway, it looks cool and I think it improves things on more on one
front. I also understand that the --dynamic-subtest allows me to isolate
one engine so that's good. On this basis:
Acked-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
Regards,
Tvrtko
P.S. I trust Petri that the mechanical bits of the conversion are fine,
or CI would have complained.
>
> Have a nice day!
> Kasia :)
>>
>> Signed-off-by: Petri Latvala <petri.latvala at intel.com>
>> ---
>> tests/perf_pmu.c | 244 ++++++++++++++++++++---------------------------
>> 1 file changed, 105 insertions(+), 139 deletions(-)
>>
>> diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
>> index e2bd2cc5..6a4a600e 100644
>> --- a/tests/perf_pmu.c
>> +++ b/tests/perf_pmu.c
>> @@ -270,7 +270,7 @@ static void end_spin(int fd, igt_spin_t *spin, unsigned int flags)
>> }
>>
>> static void
>> -single(int gem_fd, const struct intel_execution_engine2 *e, unsigned int flags)
>> +single(int gem_fd, const struct intel_execution_engine2 *e, unsigned int num_engines, unsigned int flags)
>> {
>> unsigned long slept;
>> igt_spin_t *spin;
>> @@ -606,7 +606,7 @@ all_busy_check_all(int gem_fd, const unsigned int num_engines,
>> }
>>
>> static void
>> -no_sema(int gem_fd, const struct intel_execution_engine2 *e, unsigned int flags)
>> +no_sema(int gem_fd, const struct intel_execution_engine2 *e, unsigned int num_engines, unsigned int flags)
>> {
>> igt_spin_t *spin;
>> uint64_t val[2][2];
>> @@ -646,7 +646,7 @@ no_sema(int gem_fd, const struct intel_execution_engine2 *e, unsigned int flags)
>>
>> static void
>> sema_wait(int gem_fd, const struct intel_execution_engine2 *e,
>> - unsigned int flags)
>> + unsigned int num_engines, unsigned int flags)
>> {
>> struct drm_i915_gem_relocation_entry reloc[2] = {};
>> struct drm_i915_gem_exec_object2 obj[2] = {};
>> @@ -809,6 +809,7 @@ __sema_busy(int gem_fd, int pmu,
>> static void
>> sema_busy(int gem_fd,
>> const struct intel_execution_engine2 *e,
>> + unsigned int num_engines,
>> unsigned int flags)
>> {
>> const struct intel_execution_engine2 *signal;
>> @@ -1770,6 +1771,27 @@ igt_main
>> int fd = -1;
>> struct intel_execution_engine2 *e;
>> unsigned int i;
>> + const unsigned int pct[] = { 2, 50, 98 };
>> + struct {
>> + void (*func)(int, const struct intel_execution_engine2 *, unsigned int, unsigned int);
>> + const char *name;
>> + unsigned int flags;
>> + const char *desc;
>> + } busyidle_tests[] = {
>> + { single, "idle", 0, "Test that engines show no load when idle." },
>> + { single, "busy", TEST_BUSY, "Test that a single engine reports load correctly." },
>> + { single, "busy-idle", TEST_BUSY | TEST_TRAILING_IDLE, "Test that a single engine reports load correctly." },
>> + { busy_check_all, "busy-check-all", TEST_BUSY, "Test that when one engine is loaded, others report no load." },
>> + { busy_check_all, "busy-idle-check-all", TEST_BUSY | TEST_TRAILING_IDLE, "Test that one one engine is loaded, others report no load." },
>> + { most_busy_check_all, "most-busy-check-all", TEST_BUSY, "Test that when all except one engine are loaded all loads are correctly reported." },
>> + { most_busy_check_all, "most-busy-idle-check-all", TEST_BUSY | TEST_TRAILING_IDLE, "Test that when all except one engine are loaded all loads are correctly reported." },
>> + { no_sema, "idle-no-semaphores", 0, "Test that semaphore counters report no activity on idle engines." },
>> + { no_sema, "busy-no-semaphores", TEST_BUSY, "Test that semaphore counters report no activity on busy engines." },
>> + { no_sema, "busy-idle-no-semaphores", TEST_BUSY | TEST_TRAILING_IDLE, "Test that semaphore counters report no activity on idle or busy engines." },
>> + { sema_wait, "semaphore-wait", TEST_BUSY, "Test that semaphore waits are correctly reported." },
>> + { sema_wait, "semaphore-wait-idle", TEST_BUSY | TEST_TRAILING_IDLE, "Test that semaphore waits are correctly reported." },
>> + { sema_busy, "semaphore-busy", 0, "Test that semaphore time and busy time difference is under a threshold." },
>> + };
>>
>> igt_fixture {
>> fd = drm_open_driver_master(DRIVER_INTEL);
>> @@ -1781,144 +1803,89 @@ igt_main
>> num_engines++;
>> }
>>
>> - /**
>> - * Test invalid access via perf API is rejected.
>> - */
>> + igt_describe("Test that invalid access via perf API is rejected.");
>> igt_subtest("invalid-init")
>> invalid_init();
>>
>> - __for_each_physical_engine(fd, e) {
>> - const unsigned int pct[] = { 2, 50, 98 };
>> -
>> - /**
>> - * Test that a single engine metric can be initialized or it
>> - * is correctly rejected.
>> - */
>> - igt_subtest_f("init-busy-%s", e->name)
>> - init(fd, e, I915_SAMPLE_BUSY);
>> -
>> - igt_subtest_f("init-wait-%s", e->name)
>> - init(fd, e, I915_SAMPLE_WAIT);
>> -
>> - igt_subtest_f("init-sema-%s", e->name)
>> - init(fd, e, I915_SAMPLE_SEMA);
>> -
>> - /**
>> - * Test that engines show no load when idle.
>> - */
>> - igt_subtest_f("idle-%s", e->name)
>> - single(fd, e, 0);
>> -
>> - /**
>> - * Test that a single engine reports load correctly.
>> - */
>> - igt_subtest_f("busy-%s", e->name)
>> - single(fd, e, TEST_BUSY);
>> - igt_subtest_f("busy-idle-%s", e->name)
>> - single(fd, e, TEST_BUSY | TEST_TRAILING_IDLE);
>> -
>> - /**
>> - * Test that when one engine is loaded other report no
>> - * load.
>> - */
>> - igt_subtest_f("busy-check-all-%s", e->name)
>> - busy_check_all(fd, e, num_engines, TEST_BUSY);
>> - igt_subtest_f("busy-idle-check-all-%s", e->name)
>> - busy_check_all(fd, e, num_engines,
>> - TEST_BUSY | TEST_TRAILING_IDLE);
>> -
>> - /**
>> - * Test that when all except one engine are loaded all
>> - * loads are correctly reported.
>> - */
>> - igt_subtest_f("most-busy-check-all-%s", e->name)
>> - most_busy_check_all(fd, e, num_engines,
>> - TEST_BUSY);
>> - igt_subtest_f("most-busy-idle-check-all-%s", e->name)
>> - most_busy_check_all(fd, e, num_engines,
>> - TEST_BUSY |
>> - TEST_TRAILING_IDLE);
>> -
>> - /**
>> - * Test that semphore counters report no activity on
>> - * idle or busy engines.
>> - */
>> - igt_subtest_f("idle-no-semaphores-%s", e->name)
>> - no_sema(fd, e, 0);
>> -
>> - igt_subtest_f("busy-no-semaphores-%s", e->name)
>> - no_sema(fd, e, TEST_BUSY);
>> -
>> - igt_subtest_f("busy-idle-no-semaphores-%s", e->name)
>> - no_sema(fd, e, TEST_BUSY | TEST_TRAILING_IDLE);
>> -
>> - /**
>> - * Test that semaphore waits are correctly reported.
>> - */
>> - igt_subtest_f("semaphore-wait-%s", e->name)
>> - sema_wait(fd, e, TEST_BUSY);
>> -
>> - igt_subtest_f("semaphore-wait-idle-%s", e->name)
>> - sema_wait(fd, e,
>> - TEST_BUSY | TEST_TRAILING_IDLE);
>> + igt_describe("Test that a single engine metric can be initialized "
>> + "or it is correctly rejected.");
>> + igt_subtest_with_dynamic_subsubtests("init-busy")
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + init(fd, e, I915_SAMPLE_BUSY);
>>
>> - igt_subtest_f("semaphore-busy-%s", e->name)
>> - sema_busy(fd, e, 0);
>> + igt_describe("Test that a single engine metric can be initialized "
>> + "or it is correctly rejected.");
>> + igt_subtest_with_dynamic_subsubtests("init-wait")
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + init(fd, e, I915_SAMPLE_WAIT);
>>
>> - /**
>> - * Check that two perf clients do not influence each
>> - * others observations.
>> - */
>> - igt_subtest_f("multi-client-%s", e->name)
>> - multi_client(fd, e);
>> + igt_describe("Test that a single engine metric can be initialized "
>> + "or it is correctly rejected.");
>> + igt_subtest_with_dynamic_subsubtests("init-sema")
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + init(fd, e, I915_SAMPLE_SEMA);
>> +
>> + for (i = 0; i < ARRAY_SIZE(busyidle_tests); i++) {
>> + igt_describe(busyidle_tests[i].desc);
>> + igt_subtest_with_dynamic_subsubtests(busyidle_tests[i].name)
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + busyidle_tests[i].func(fd, e, num_engines, busyidle_tests[i].flags);
>> + }
>>
>> - /**
>> - * Check that reported usage is correct when PMU is
>> - * enabled after the batch is running.
>> - */
>> - igt_subtest_f("busy-start-%s", e->name)
>> - busy_start(fd, e);
>> + igt_describe("Check that two perf clients do not influence each other's observations.");
>> + igt_subtest_with_dynamic_subsubtests("multi-client")
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + multi_client(fd, e);
>>
>> - /**
>> - * Check that reported usage is correct when PMU is
>> - * enabled after two batches are running.
>> - */
>> - igt_subtest_f("busy-double-start-%s", e->name) {
>> - gem_require_contexts(fd);
>> - busy_double_start(fd, e);
>> - }
>> + igt_describe("Check that reported usage is correct when PMU is enabled after the batch is running.");
>> + igt_subtest_with_dynamic_subsubtests("busy-start")
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + busy_start(fd, e);
>>
>> - /**
>> - * Check that the PMU can be safely enabled in face of
>> - * interrupt-heavy engine load.
>> - */
>> - igt_subtest_f("enable-race-%s", e->name)
>> - test_enable_race(fd, e);
>> + igt_describe("Check that reported usage is correct when PMU is enabled after two batches are running.");
>> + igt_subtest_with_dynamic_subsubtests("busy-double-start") {
>> + gem_require_contexts(fd);
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + busy_double_start(fd, e);
>> + }
>>
>> - /**
>> - * Check engine busyness accuracy is as expected.
>> - */
>> - for (i = 0; i < ARRAY_SIZE(pct); i++) {
>> - igt_subtest_f("busy-accuracy-%u-%s",
>> - pct[i], e->name)
>> - accuracy(fd, e, pct[i], 10);
>> - }
>> + igt_describe("Check that the PMU can be safely enabled in face of interrupt-heavy engine load.");
>> + igt_subtest_with_dynamic_subsubtests("enable-race")
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + test_enable_race(fd, e);
>> +
>> + for (i = 0; i < ARRAY_SIZE(pct); i++) {
>> + igt_describe("Check engine busyness accuracy is as expected.");
>> + igt_subtest_with_dynamic_subsubtests_f("busy-accuracy-%u", pct[i])
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + accuracy(fd, e, pct[i], 10);
>> + }
>>
>> - igt_subtest_f("busy-hang-%s", e->name) {
>> - igt_hang_t hang = igt_allow_hang(fd, 0, 0);
>> + igt_subtest_with_dynamic_subsubtests("busy-hang")
>> + __for_each_physical_engine(fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name) {
>> + igt_hang_t hang = igt_allow_hang(fd, 0, 0);
>>
>> - single(fd, e, TEST_BUSY | FLAG_HANG);
>> + single(fd, e, 0, TEST_BUSY | FLAG_HANG);
>>
>> - igt_disallow_hang(fd, hang);
>> - }
>> + igt_disallow_hang(fd, hang);
>> + }
>>
>> - /**
>> - * Test that event waits are correctly reported.
>> - */
>> - if (e->class == I915_ENGINE_CLASS_RENDER)
>> - igt_subtest_f("event-wait-%s", e->name)
>> - event_wait(fd, e);
>> - }
>> + igt_describe("Test that event waits are correctly reported.");
>> + igt_subtest_with_dynamic_subsubtests("event-wait")
>> + __for_each_physical_engine(fd, e)
>> + if (e->class == I915_ENGINE_CLASS_RENDER)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + event_wait(fd, e);
>>
>> /**
>> * Test that when all engines are loaded all loads are
>> @@ -1975,9 +1942,7 @@ igt_main
>> igt_subtest("rc6-runtime-pm-long")
>> test_rc6(fd, TEST_RUNTIME_PM | FLAG_LONG);
>>
>> - /**
>> - * Check render nodes are counted.
>> - */
>> + igt_describe("Check that render nodes are counted.");
>> igt_subtest_group {
>> int render_fd = -1;
>>
>> @@ -1988,14 +1953,15 @@ igt_main
>> gem_quiescent_gpu(fd);
>> }
>>
>> - __for_each_physical_engine(render_fd, e) {
>> - igt_subtest_f("render-node-busy-%s", e->name)
>> - single(render_fd, e, TEST_BUSY);
>> - igt_subtest_f("render-node-busy-idle-%s",
>> - e->name)
>> - single(render_fd, e,
>> - TEST_BUSY | TEST_TRAILING_IDLE);
>> - }
>> + igt_subtest_with_dynamic_subsubtests("render-node-busy")
>> + __for_each_physical_engine(render_fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + single(render_fd, e, 0, TEST_BUSY);
>> +
>> + igt_subtest_with_dynamic_subsubtests("render-node-busy-idle")
>> + __for_each_physical_engine(render_fd, e)
>> + igt_dynamic_subsubtest_f("%s", e->name)
>> + single(render_fd, e, 0, TEST_BUSY | TEST_TRAILING_IDLE);
>>
>> igt_fixture {
>> close(render_fd);
>> --
>> 2.19.1
>>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
>
More information about the igt-dev
mailing list