Re: [igt-dev] [PATCH] [PATCH i-g-t][V7]tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 14/04/2020 18:51, Arjun Melkaveri wrote:
> Replaced the legacy for_each_engine* defines with the ones
> implemented in the gem_engine_topology library.
> 
> Used  gem_context_clone_with_engines
> to make sure that engine index was potentially created
> based on a  default context with engine map configured.
> 
> Added Legacy engine coverage for sync_ring and sync_all.
> 
> Divided tests into static for ALL_Engine test cases
> and dynamic for multiple engine .
> 
> V7:
> Initializing engine and engine name with
> maximum supported engines value.

You lost the rest of the change log, not terribly important though.

> 
> Cc: Dec Katarzyna <katarzyna.dec@intel.com>
> Cc: Ursulin Tvrtko <tvrtko.ursulin@intel.com>
> Signed-off-by: sai gowtham <sai.gowtham.ch@intel.com>
> Signed-off-by: Arjun Melkaveri <arjun.melkaveri@intel.com>
> ---
>   tests/i915/gem_sync.c | 310 ++++++++++++++++++++++++++----------------
>   1 file changed, 195 insertions(+), 115 deletions(-)
> 
> diff --git a/tests/i915/gem_sync.c b/tests/i915/gem_sync.c
> index 2ef55ecc..f1a87f59 100644
> --- a/tests/i915/gem_sync.c
> +++ b/tests/i915/gem_sync.c
> @@ -24,6 +24,9 @@
>   #include <time.h>
>   #include <pthread.h>
>   
> +#include "igt_debugfs.h"
> +#include "igt_dummyload.h"
> +#include "igt_gt.h"
>   #include "igt.h"
>   #include "igt_sysfs.h"
>   
> @@ -37,6 +40,10 @@
>   #define MIN_PRIO LOCAL_I915_CONTEXT_MIN_USER_PRIORITY
>   
>   #define ENGINE_MASK  (I915_EXEC_RING_MASK | LOCAL_I915_EXEC_BSD_MASK)
> +#define RESET_TIMEOUT_MS (2 * MSEC_PER_SEC)
> +static unsigned long reset_timeout_ms = RESET_TIMEOUT_MS;

I've asked in one of the previous rounds what's this for? It's only set 
in do_test and never acted upon.

> +#define NSEC_PER_MSEC (1000 * 1000ull)

Unused.

> +#define EXECBUF_MAX_ENGINES (I915_EXEC_RING_MASK + 1)
>   
>   IGT_TEST_DESCRIPTION("Basic check of ring<->ring write synchronisation.");
>   
> @@ -78,24 +85,35 @@ out:
>   	return ts.tv_sec + 1e-9*ts.tv_nsec;
>   }
>   
> +static void cleanup(int i915)
> +{
> +	igt_drop_caches_set(i915,
> +			    /* cancel everything */
> +			    DROP_RESET_ACTIVE | DROP_RESET_SEQNO |
> +			    /* cleanup */
> +			    DROP_ACTIVE | DROP_RETIRE | DROP_IDLE | DROP_FREED);
> +	igt_require_gem(i915);
> +}
> +
> +
>   static void
>   sync_ring(int fd, unsigned ring, int num_children, int timeout)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +		__for_each_physical_engine(fd, e2) {
> +			names[num_engines] = strdup(e2->name);
> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   
>   		num_children *= num_engines;
>   	} else {
> -		gem_require_ring(fd, ring);
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
>   	}
> @@ -139,7 +157,7 @@ sync_ring(int fd, unsigned ring, int num_children, int timeout)
>   }
>   
>   static void
> -idle_ring(int fd, unsigned ring, int timeout)
> +idle_ring(int fd, unsigned int ring, int num_children, int timeout)
>   {
>   	const uint32_t bbe = MI_BATCH_BUFFER_END;
>   	struct drm_i915_gem_exec_object2 object;
> @@ -180,23 +198,23 @@ idle_ring(int fd, unsigned ring, int timeout)
>   static void
>   wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

Looks like you need strdup here as well.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   		igt_require(num_engines);
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -290,25 +308,26 @@ wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
>   	igt_assert_eq(intel_detect_and_clear_missed_interrupts(fd), 0);
>   }
>   
> -static void active_ring(int fd, unsigned ring, int timeout)
> +static void active_ring(int fd, unsigned int ring,
> +			int num_children, int timeout)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

And here.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   		igt_require(num_engines);
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -359,23 +378,23 @@ static void active_ring(int fd, unsigned ring, int timeout)
>   static void
>   active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

And here.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   		igt_require(num_engines);
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -493,25 +512,25 @@ active_wakeup_ring(int fd, unsigned ring, int timeout, int wlen)
>   static void
>   store_ring(int fd, unsigned ring, int num_children, int timeout)
>   {
> +	const struct intel_execution_engine2 *e2;
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> -	unsigned engines[16];
> -	const char *names[16];
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

And here.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   
>   		num_children *= num_engines;
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -608,27 +627,27 @@ store_ring(int fd, unsigned ring, int num_children, int timeout)
>   static void
>   switch_ring(int fd, unsigned ring, int num_children, int timeout)
>   {
> +	const struct intel_execution_engine2 *e2;
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> -	unsigned engines[16];
> -	const char *names[16];
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
>   	gem_require_contexts(fd);
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +			names[num_engines] = e2->name;

Ditto.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   
>   		num_children *= num_engines;
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
> @@ -931,10 +950,11 @@ __store_many(int fd, unsigned ring, int timeout, unsigned long *cycles)
>   }
>   
>   static void
> -store_many(int fd, unsigned ring, int timeout)
> +store_many(int fd, unsigned int ring, int num_children, int timeout)
>   {
> +	const struct intel_execution_engine2 *e2;
>   	unsigned long *shared;
> -	const char *names[16];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int n = 0;
>   
>   	shared = mmap(NULL, 4096, PROT_WRITE, MAP_SHARED | MAP_ANON, -1, 0);
> @@ -943,21 +963,20 @@ store_many(int fd, unsigned ring, int timeout)
>   	intel_detect_and_clear_missed_interrupts(fd);
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			if (!gem_can_store_dword(fd, eb_ring(e)))
> +		__for_each_physical_engine(fd, e2) {
> +			if (!gem_class_can_store_dword(fd, e2->class))
>   				continue;
>   
>   			igt_fork(child, 1)
>   				__store_many(fd,
> -					     eb_ring(e),
> +					     e2->flags,
>   					     timeout,
>   					     &shared[n]);
>   
> -			names[n++] = e->name;
> +			names[n++] = e2->name;

Ditto.

>   		}
>   		igt_waitchildren();
>   	} else {
> -		gem_require_ring(fd, ring);
>   		igt_require(gem_can_store_dword(fd, ring));
>   		__store_many(fd, ring, timeout, &shared[n]);
>   		names[n++] = NULL;
> @@ -1025,15 +1044,16 @@ sync_all(int fd, int num_children, int timeout)
>   static void
>   store_all(int fd, int num_children, int timeout)
>   {
> +	const struct intel_execution_engine2 *e;
>   	const int gen = intel_gen(intel_get_drm_devid(fd));
> -	unsigned engines[16];
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   
> -	for_each_physical_engine(e, fd) {
> -		if (!gem_can_store_dword(fd, eb_ring(e)))
> +	__for_each_physical_engine(fd, e) {
> +		if (!gem_class_can_store_dword(fd, e->class))
>   			continue;
>   
> -		engines[num_engines++] = eb_ring(e);
> +		engines[num_engines++] = e->flags;
>   		if (num_engines == ARRAY_SIZE(engines))
>   			break;
>   	}
> @@ -1132,22 +1152,22 @@ store_all(int fd, int num_children, int timeout)
>   static void
>   preempt(int fd, unsigned ring, int num_children, int timeout)
>   {
> -	unsigned engines[16];
> -	const char *names[16];
> +	const struct intel_execution_engine2 *e2;
> +	unsigned int engines[EXECBUF_MAX_ENGINES];
> +	const char *names[EXECBUF_MAX_ENGINES];
>   	int num_engines = 0;
>   	uint32_t ctx[2];
>   
>   	if (ring == ALL_ENGINES) {
> -		for_each_physical_engine(e, fd) {
> -			names[num_engines] = e->name;
> -			engines[num_engines++] = eb_ring(e);
> +		__for_each_physical_engine(fd, e2) {
> +			names[num_engines] = e2->name;

Last one.

> +			engines[num_engines++] = e2->flags;
>   			if (num_engines == ARRAY_SIZE(engines))
>   				break;
>   		}
>   
>   		num_children *= num_engines;
>   	} else {
> -		gem_require_ring(fd, ring);
>   		names[num_engines] = NULL;
>   		engines[num_engines++] = ring;
>   	}
> @@ -1207,76 +1227,99 @@ preempt(int fd, unsigned ring, int num_children, int timeout)
>   	gem_context_destroy(fd, ctx[0]);
>   }
>   
> +static void do_test(void (*test)(int i915, unsigned int engine,
> +		    int num_children, int test_timeout),
> +		    int i915, unsigned int engine, int num_children,
> +		    int test_timeout, const char *name)
> +{
> +#define ATTR "preempt_timeout_ms"
> +	int timeout = -1;
> +
> +	cleanup(i915);
> +
> +	gem_engine_property_scanf(i915, name, ATTR, "%d", &timeout);
> +	if (timeout != -1) {
> +		igt_require(gem_engine_property_printf(i915, name,
> +						       ATTR, "%d", 50) > 0);
> +		reset_timeout_ms = 200;
> +	}
> +
> +	test(i915, engine, num_children, test_timeout);
> +	gem_quiescent_gpu(i915);

For stuff done in this wrapper - I don't really like stuffing in changes 
unrelated to patch title. If it is converting engine iteration it should 
stick to that. If you want to make some other improvements it should be 
a separate patch.

> +}
> +
>   igt_main
>   {
> -	const struct intel_execution_engine *e;
>   	const int ncpus = sysconf(_SC_NPROCESSORS_ONLN);
>   	int fd = -1;
>   
> +	struct {
> +		const char *name;
> +		void (*func)(int fd, unsigned int engine,
> +			     int num_children, int timeout);
> +		int num_children;
> +		int timeout;
> +	} *test, allengine[] = {
> +		{ "basic-each", sync_ring, 1, 2},
> +		{ "basic-store-each", store_ring, 1, 2},
> +		{ "basic-many-each", store_many, 0, 2},
> +		{ "switch-each", switch_ring, 1, 20},
> +		{ "forked-switch-each", switch_ring, ncpus, 20},
> +		{ "forked-each", sync_ring, ncpus, 20},
> +		{ "forked-store-each", store_ring, ncpus, 20},
> +		{ "active-each", active_ring, 0, 20},
> +		{ "wakeup-each", wakeup_ring, 20, 1},
> +		{ "active-wakeup-each", active_wakeup_ring, 20, 1},
> +		{ "double-wakeup-each", wakeup_ring, 20, 2},
> +		{ NULL, NULL },
> +	}, tests[] = {
> +		{ "default", sync_ring, 1, 20},
> +		{ "idle", idle_ring, 0, 20},
> +		{ "active", active_ring, 0, 20},
> +		{ "wakeup", wakeup_ring, 20, 1},
> +		{ "active-wakeup", active_wakeup_ring, 20, 1},
> +		{ "double-wakeup", wakeup_ring, 20, 2},
> +		{ "store", store_ring, 1, 20},
> +		{ "switch", switch_ring, 1, 20},
> +		{ "forked-switch", switch_ring, ncpus, 20},
> +		{ "many", store_many, 0, 20},
> +		{ "forked", sync_ring, ncpus, 20},
> +		{ "forked-store", store_ring, ncpus, 20},
> +		{ NULL, NULL },
> +	};
> +
> +
>   	igt_fixture {
>   		fd = drm_open_driver(DRIVER_INTEL);
>   		igt_require_gem(fd);
>   		gem_submission_print_method(fd);
>   		gem_scheduler_print_capability(fd);
> -

Best to avoid noise.

>   		igt_fork_hang_detector(fd);
>   	}
>   
> -	for (e = intel_execution_engines; e->name; e++) {
> -		igt_subtest_f("%s", e->name)
> -			sync_ring(fd, eb_ring(e), 1, 20);
> -		igt_subtest_f("idle-%s", e->name)
> -			idle_ring(fd, eb_ring(e), 20);
> -		igt_subtest_f("active-%s", e->name)
> -			active_ring(fd, eb_ring(e), 20);
> -		igt_subtest_f("wakeup-%s", e->name)
> -			wakeup_ring(fd, eb_ring(e), 20, 1);
> -		igt_subtest_f("active-wakeup-%s", e->name)
> -			active_wakeup_ring(fd, eb_ring(e), 20, 1);
> -		igt_subtest_f("double-wakeup-%s", e->name)
> -			wakeup_ring(fd, eb_ring(e), 20, 2);
> -		igt_subtest_f("store-%s", e->name)
> -			store_ring(fd, eb_ring(e), 1, 20);
> -		igt_subtest_f("switch-%s", e->name)
> -			switch_ring(fd, eb_ring(e), 1, 20);
> -		igt_subtest_f("forked-switch-%s", e->name)
> -			switch_ring(fd, eb_ring(e), ncpus, 20);
> -		igt_subtest_f("many-%s", e->name)
> -			store_many(fd, eb_ring(e), 20);
> -		igt_subtest_f("forked-%s", e->name)
> -			sync_ring(fd, eb_ring(e), ncpus, 20);
> -		igt_subtest_f("forked-store-%s", e->name)
> -			store_ring(fd, eb_ring(e), ncpus, 20);
> -	}
> +	/* legacy of selecting engines. */
>   
> -	igt_subtest("basic-each")
> -		sync_ring(fd, ALL_ENGINES, 1, 2);
> -	igt_subtest("basic-store-each")
> -		store_ring(fd, ALL_ENGINES, 1, 2);
> -	igt_subtest("basic-many-each")
> -		store_many(fd, ALL_ENGINES, 2);
> -	igt_subtest("switch-each")
> -		switch_ring(fd, ALL_ENGINES, 1, 20);
> -	igt_subtest("forked-switch-each")
> -		switch_ring(fd, ALL_ENGINES, ncpus, 20);
> -	igt_subtest("forked-each")
> -		sync_ring(fd, ALL_ENGINES, ncpus, 20);
> -	igt_subtest("forked-store-each")
> -		store_ring(fd, ALL_ENGINES, ncpus, 20);
> -	igt_subtest("active-each")
> -		active_ring(fd, ALL_ENGINES, 20);
> -	igt_subtest("wakeup-each")
> -		wakeup_ring(fd, ALL_ENGINES, 20, 1);
> -	igt_subtest("active-wakeup-each")
> -		active_wakeup_ring(fd, ALL_ENGINES, 20, 1);
> -	igt_subtest("double-wakeup-each")
> -		wakeup_ring(fd, ALL_ENGINES, 20, 2);
> +	igt_subtest_group {
> +		for (test = tests; test->name; test++) {
> +			igt_subtest_with_dynamic_f("legacy-engines-%s",

Have we not agreed to not add "engines" to test name?

> +						   test->name) {
> +				for_each_physical_engine(e, fd) {
> +					igt_dynamic_f("%s", e->name) {
> +						do_test(test->func,
> +							fd, eb_ring(e),
> +							test->num_children,
> +							test->timeout,
> +							e->full_name);
> +					}
> +				}
> +			}
> +		}
> +	}
>   
>   	igt_subtest("basic-all")
>   		sync_all(fd, 1, 2);
>   	igt_subtest("basic-store-all")
>   		store_all(fd, 1, 2);
> -

Noise as well.

>   	igt_subtest("all")
>   		sync_all(fd, 1, 20);
>   	igt_subtest("store-all")
> @@ -1286,7 +1329,42 @@ igt_main
>   	igt_subtest("forked-store-all")
>   		store_all(fd, ncpus, 20);
>   
> +	/* All Engines */
>   	igt_subtest_group {
> +		for (test = allengine; test->name; test++) {
> +			igt_subtest_f("%s", test->name) {
> +				do_test(test->func,
> +					fd, ALL_ENGINES,
> +					test->num_children,
> +					test->timeout,
> +					test->name);
> +			}
> +		}
> +	}
> +
> +	/* New way of selecting engines. */
> +	igt_subtest_group {
> +		const struct intel_execution_engine2 *e;
> +
> +		for (test = tests; test->name; test++) {
> +			igt_subtest_with_dynamic_f("%s", test->name) {
> +				__for_each_physical_engine(fd, e) {
> +					igt_dynamic_f("%s", e->name) {
> +						do_test(test->func,
> +							fd, e->flags,
> +							test->num_children,
> +							test->timeout,
> +							e->name);
> +					}
> +				}
> +			}
> +		}
> +	}
> +
> +
> +	igt_subtest_group {
> +		const struct intel_execution_engine2 *e;
> +
>   		igt_fixture {

Regards,

Tvrtko

>   			gem_require_contexts(fd);
>   			igt_require(gem_scheduler_has_ctx_priority(fd));
> @@ -1295,10 +1373,12 @@ igt_main
>   
>   		igt_subtest("preempt-all")
>   			preempt(fd, ALL_ENGINES, 1, 20);
> -
> -		for (e = intel_execution_engines; e->name; e++) {
> -			igt_subtest_f("preempt-%s", e->name)
> -				preempt(fd, eb_ring(e), ncpus, 20);
> +		igt_subtest_with_dynamic("preempt") {
> +			__for_each_physical_engine(fd, e) {
> +				/* Requires master for STORE_DWORD on gen4/5 */
> +				igt_dynamic_f("%s", e->name)
> +					preempt(fd, e->flags, ncpus, 20);
> +			}
>   		}
>   	}
>   
> 
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev