From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by gabe.freedesktop.org (Postfix) with ESMTPS id 5C27D6E3FE for ; Wed, 22 Apr 2020 14:34:02 +0000 (UTC) Date: Wed, 22 Apr 2020 20:02:34 +0530 From: "Melkaveri, Arjun" Message-ID: <20200422143233.GA12392@arjun-NUC8i7BEH> References: <20200414175136.13719-1-arjun.melkaveri@intel.com> <878d05aa-efdc-2f2c-963e-6393d5698f9f@linux.intel.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <878d05aa-efdc-2f2c-963e-6393d5698f9f@linux.intel.com> Subject: Re: [igt-dev] [PATCH] [PATCH i-g-t][V7]tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" To: Tvrtko Ursulin Cc: igt-dev@lists.freedesktop.org List-ID: On Wed, Apr 22, 2020 at 02:13:44PM +0100, Tvrtko Ursulin wrote: > > 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 > > Cc: Ursulin Tvrtko > > Signed-off-by: sai gowtham > > Signed-off-by: Arjun Melkaveri > > --- > > 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 > > #include > > +#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. > Will modify this in next patch > > +#define NSEC_PER_MSEC (1000 * 1000ull) > > Unused. > Will remove this > > +#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. > I am changing this to use intel_engine_data, to remove usage of local variable. > > + 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. > I was acting on the reviews comments . by following 28e25ad1f1f987450f017d7f99548d2d7727d388 commit . Will clean up do_test ,as needed for this test. > > +} > > + > > 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? > I removed it for other test group. will modify this too . > > + 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