[igt-dev] [PATCH] [PATCH i-g-t][V7]tests/i915/gem_sync.c :Added __for_each_physical_engine to utilize all available
Melkaveri, Arjun
arjun.melkaveri at intel.com
Wed Apr 22 14:32:34 UTC 2020
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 <katarzyna.dec at intel.com>
> > Cc: Ursulin Tvrtko <tvrtko.ursulin at intel.com>
> > Signed-off-by: sai gowtham <sai.gowtham.ch at intel.com>
> > Signed-off-by: Arjun Melkaveri <arjun.melkaveri at 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.
>
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);
> > + }
> > }
> > }
> >
More information about the igt-dev
mailing list