Re: [igt-dev] [PATCH i-g-t 7/8] perf_pmu: Use dynamic subtests

[Katarzyna Dec <katarzyna.dec@intel.com>]

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.

Have a nice day!
Kasia :)
> 
> Signed-off-by: Petri Latvala <petri.latvala@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev