[Intel-gfx] [RFC i-g-t] igt: Test tagging support
Daniel Vetter
daniel at ffwll.ch
Tue Jun 27 13:18:45 UTC 2017
On Tue, Jun 27, 2017 at 12:59:33PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>
> Proposal to add test tags as a replacement for separate test
> list which can be difficult to maintain and get out of date.
>
> Putting this maintanenace inline with tests makes it easier
> to remember to update the, now implicit, lists, and also enables
> richer test selection possibilities for the test runner.
>
> Test tags are string tokens separated by spaces meaning of which
> we decide by creating a convetion and helped by the suitable
> helper macros.
>
> For example tags can be things like: gem, kms, guc, flip,
> semaphore, bz12345678, gt4e, etc..
>
> At runtime this would look something like this:
>
> root at e31:~/intel-gpu-tools# tests/gem_sync --list-subtests-with-tags
> default TAGS="gem "
> store-default TAGS="gem "
> many-default TAGS="gem "
> forked-default TAGS="gem "
> forked-store-default TAGS="gem "
> render TAGS="gem "
> store-render TAGS="gem "
> many-render TAGS="gem "
> forked-render TAGS="gem "
> forked-store-render TAGS="gem "
> bsd TAGS="gem "
> store-bsd TAGS="gem "
> many-bsd TAGS="gem "
> forked-bsd TAGS="gem "
> forked-store-bsd TAGS="gem "
> bsd1 TAGS="gem "
> store-bsd1 TAGS="gem "
> many-bsd1 TAGS="gem "
> forked-bsd1 TAGS="gem "
> forked-store-bsd1 TAGS="gem "
> bsd2 TAGS="gem "
> store-bsd2 TAGS="gem "
> many-bsd2 TAGS="gem "
> forked-bsd2 TAGS="gem "
> forked-store-bsd2 TAGS="gem "
> blt TAGS="gem "
> store-blt TAGS="gem "
> many-blt TAGS="gem "
> forked-blt TAGS="gem "
> forked-store-blt TAGS="gem "
> vebox TAGS="gem "
> store-vebox TAGS="gem "
> many-vebox TAGS="gem "
> forked-vebox TAGS="gem "
> forked-store-vebox TAGS="gem "
> each TAGS="gem basic"
> store-each TAGS="gem basic"
> many-each TAGS="gem basic"
> forked-each TAGS="gem basic"
> forked-store-each TAGS="gem "
> all TAGS="gem basic"
> store-all TAGS="gem basic"
> forked-all TAGS="gem extended"
> forked-store-all TAGS="gem extended"
>
> Test runner can then enable usages like:
>
> ./run-tests --include gem --exclude kms,stress
>
> Which I think could really be useful and more flexible than name
> based selection.
How is this different from name-based pattern matching? We could just
throw these tags into the names, which is pretty much what we do already.
> This RFC implementation automatically adds "gem" and "kms" tags
> to test binaries prefixed with those strings respectively.
Why do you want to filter for gem or kms only? If you want to locally test
for a feature, I'd expect you want a more focused test selection, using
naming patterns/exclusions. Or maybe just one test binary that you run.
For CI this doesn't help us, since it doesn't give us a way to both
a) make sure new tests are by default in the extended/full/CI/whatever you
want to call it run
b) exclude the massive pile of stress tests we currently have and can't
run for logistical reasons.
Adding some tags for stress tests, or nasty debug-hunting mode is what I
think we need, so that we can exclude them from default runs. Currently we
have two examples of those:
- kms_frontbuffer_tracking --show-hidden
- gem_concurrent_blt vs _all
Your patch doesn't address this, and since we have 2 independent
inventions of some solution for this, it does seem to be a real problem. I
think tags would be a good way to fix this, but tags just to have more
structured names seems like lots of work for little gain (and we're not
really short on work to do).
-Daniel
>
> I've converted gem_concurrent_all and gem_sync as examples only.
>
> Source code usage looks like:
>
> igt_gem_subtest("basic", "each")
> sync_ring(fd, ~0u, 1, 5);
>
> igt_gem_subtest("extended", "forked-all")
> sync_all(fd, ncpus, 150);
>
> We can of course polish the details of the API if the idea will
> be deemed interesting.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Cc: Daniel Vetter <daniel.vetter at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Petri Latvala <petri.latvala at intel.com>
> Cc: Tomi Sarvela <tomi.p.sarvela at intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg at intel.com>
> ---
> lib/igt_core.c | 38 +++++++++++++++++++++++++++++++-------
> lib/igt_core.h | 29 ++++++++++++++++++++++++++---
> tests/gem_concurrent_all.c | 34 +++++++++++++++++-----------------
> tests/gem_sync.c | 28 ++++++++++++++--------------
> 4 files changed, 88 insertions(+), 41 deletions(-)
>
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 9a5ed40eeb22..37cd261daf2b 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -231,10 +231,10 @@ static unsigned int exit_handler_count;
> const char *igt_interactive_debug;
>
> /* subtests helpers */
> -static bool list_subtests = false;
> -static char *run_single_subtest = NULL;
> +static bool list_subtests, list_subtests_tags;
> +static char *run_single_subtest;
> static bool run_single_subtest_found = false;
> -static const char *in_subtest = NULL;
> +static const char *in_subtest, *subtest_tags;
> static struct timespec subtest_time;
> static clockid_t igt_clock = (clockid_t)-1;
> static bool in_fixture = false;
> @@ -254,6 +254,7 @@ bool test_child;
>
> enum {
> OPT_LIST_SUBTESTS,
> + OPT_LIST_SUBTESTS_TAGS,
> OPT_RUN_SUBTEST,
> OPT_DESCRIPTION,
> OPT_DEBUG,
> @@ -571,6 +572,7 @@ static void print_usage(const char *help_str, bool output_on_stderr)
>
> fprintf(f, "Usage: %s [OPTIONS]\n", command_str);
> fprintf(f, " --list-subtests\n"
> + " --list-subtest-with-tags\n"
> " --run-subtest <pattern>\n"
> " --debug[=log-domain]\n"
> " --interactive-debug[=domain]\n"
> @@ -603,6 +605,7 @@ static int common_init(int *argc, char **argv,
> int c, option_index = 0, i, x;
> static struct option long_options[] = {
> {"list-subtests", 0, 0, OPT_LIST_SUBTESTS},
> + {"list-subtests-with-tags", 0, 0, OPT_LIST_SUBTESTS_TAGS},
> {"run-subtest", 1, 0, OPT_RUN_SUBTEST},
> {"help-description", 0, 0, OPT_DESCRIPTION},
> {"debug", optional_argument, 0, OPT_DEBUG},
> @@ -714,6 +717,12 @@ static int common_init(int *argc, char **argv,
> if (!run_single_subtest)
> list_subtests = true;
> break;
> + case OPT_LIST_SUBTESTS_TAGS:
> + if (!run_single_subtest) {
> + list_subtests = true;
> + list_subtests_tags = true;
> + }
> + break;
> case OPT_RUN_SUBTEST:
> if (!list_subtests)
> run_single_subtest = strdup(optarg);
> @@ -847,14 +856,22 @@ void igt_simple_init_parse_opts(int *argc, char **argv,
> * outside of places protected by igt_run_subtest checks - the piglit
> * runner adds every line to the subtest list.
> */
> -bool __igt_run_subtest(const char *subtest_name)
> +bool __igt_run_subtest(const char *subtest_name, const char *tags)
> {
> int i;
>
> assert(!in_subtest);
> + assert(!subtest_tags);
> assert(!in_fixture);
> assert(test_with_subtests);
>
> + if (!tags) {
> + if (!strncmp(command_str, "gem", 3))
> + tags = "gem";
> + else if (!strncmp(command_str, "kms", 3))
> + tags = "kms";
> + }
> +
> /* check the subtest name only contains a-z, A-Z, 0-9, '-' and '_' */
> for (i = 0; subtest_name[i] != '\0'; i++)
> if (subtest_name[i] != '_' && subtest_name[i] != '-'
> @@ -865,7 +882,10 @@ bool __igt_run_subtest(const char *subtest_name)
> }
>
> if (list_subtests) {
> - printf("%s\n", subtest_name);
> + if (list_subtests_tags && tags)
> + printf("%s TAGS=\"%s\"\n", subtest_name, tags);
> + else
> + printf("%s\n", subtest_name);
> return false;
> }
>
> @@ -890,7 +910,11 @@ bool __igt_run_subtest(const char *subtest_name)
> _igt_log_buffer_reset();
>
> gettime(&subtest_time);
> - return (in_subtest = subtest_name);
> +
> + in_subtest = subtest_name;
> + subtest_tags = tags;
> +
> + return true;
> }
>
> /**
> @@ -941,7 +965,7 @@ static void exit_subtest(const char *result)
> (!__igt_plain_output) ? "\x1b[0m" : "");
> fflush(stdout);
>
> - in_subtest = NULL;
> + in_subtest = subtest_tags = NULL;
> siglongjmp(igt_subtest_jmpbuf, 1);
> }
>
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index a2ed972078bd..b6ca5d315f08 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -145,7 +145,7 @@ int igt_subtest_init_parse_opts(int *argc, char **argv,
> #define igt_subtest_init(argc, argv) \
> igt_subtest_init_parse_opts(&argc, argv, NULL, NULL, NULL, NULL, NULL);
>
> -bool __igt_run_subtest(const char *subtest_name);
> +bool __igt_run_subtest(const char *subtest_name, const char *subtest_tags);
> #define __igt_tokencat2(x, y) x ## y
>
> /**
> @@ -158,6 +158,29 @@ bool __igt_run_subtest(const char *subtest_name);
> */
> #define igt_tokencat(x, y) __igt_tokencat2(x, y)
>
> +#define igt_tagged_subtest(tags, name) \
> + for (; __igt_run_subtest((name), (tags)) && \
> + (sigsetjmp(igt_subtest_jmpbuf, 1) == 0); \
> + igt_success())
> +
> +#define igt_gem_subtest(tags, name) igt_tagged_subtest("gem "tags, name)
> +
> +#define __igt_tagged_subtest_f(tags, tmp, format...) \
> + for (char tmp [256]; \
> + snprintf( tmp , sizeof( tmp ), format), \
> + __igt_run_subtest(tmp, tags) && \
> + (sigsetjmp(igt_subtest_jmpbuf, 1) == 0); \
> + igt_success())
> +
> +#define igt_tagged_subtest_f(tags, f...) \
> + __igt_tagged_subtest_f(tags, igt_tokencat(__tmpchar, __LINE__), f)
> +
> +#define igt_gem_subtest_f(tags, f...) \
> + igt_tagged_subtest_f("gem "tags, f)
> +
> +#define igt_gem_stress_subtest_f(tags, f...) \
> + igt_tagged_subtest_f("gem stress "tags, f)
> +
> /**
> * igt_subtest:
> * @name: name of the subtest
> @@ -169,14 +192,14 @@ bool __igt_run_subtest(const char *subtest_name);
> *
> * This is a simpler version of igt_subtest_f()
> */
> -#define igt_subtest(name) for (; __igt_run_subtest((name)) && \
> +#define igt_subtest(name) for (; __igt_run_subtest((name), NULL) && \
> (sigsetjmp(igt_subtest_jmpbuf, 1) == 0); \
> igt_success())
> #define __igt_subtest_f(tmp, format...) \
> for (char tmp [256]; \
> snprintf( tmp , sizeof( tmp ), \
> format), \
> - __igt_run_subtest( tmp ) && \
> + __igt_run_subtest(tmp, NULL) && \
> (sigsetjmp(igt_subtest_jmpbuf, 1) == 0); \
> igt_success())
>
> diff --git a/tests/gem_concurrent_all.c b/tests/gem_concurrent_all.c
> index 201b491bc245..eef0c8878081 100644
> --- a/tests/gem_concurrent_all.c
> +++ b/tests/gem_concurrent_all.c
> @@ -1492,47 +1492,47 @@ run_mode(const char *prefix,
> igt_subtest_group {
> igt_fixture p->require();
>
> - igt_subtest_f("%s-%s-%s-sanitycheck0%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-sanitycheck0%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers, do_basic0,
> p->copy, h->hang);
> }
>
> - igt_subtest_f("%s-%s-%s-sanitycheck1%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-sanitycheck1%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers, do_basic1,
> p->copy, h->hang);
> }
>
> - igt_subtest_f("%s-%s-%s-sanitycheckN%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-sanitycheckN%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers, do_basicN,
> p->copy, h->hang);
> }
>
> /* try to overwrite the source values */
> - igt_subtest_f("%s-%s-%s-overwrite-source-one%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-overwrite-source-one%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_overwrite_source__one,
> p->copy, h->hang);
> }
>
> - igt_subtest_f("%s-%s-%s-overwrite-source%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-overwrite-source%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_overwrite_source,
> p->copy, h->hang);
> }
>
> - igt_subtest_f("%s-%s-%s-overwrite-source-read-bcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-overwrite-source-read-bcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_overwrite_source_read_bcs,
> p->copy, h->hang);
> }
>
> - igt_subtest_f("%s-%s-%s-overwrite-source-read-rcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-overwrite-source-read-rcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> igt_require(rendercopy);
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> @@ -1540,7 +1540,7 @@ run_mode(const char *prefix,
> p->copy, h->hang);
> }
>
> - igt_subtest_f("%s-%s-%s-overwrite-source-rev%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-overwrite-source-rev%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_overwrite_source__rev,
> @@ -1548,21 +1548,21 @@ run_mode(const char *prefix,
> }
>
> /* try to intermix copies with GPU copies*/
> - igt_subtest_f("%s-%s-%s-intermix-rcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-intermix-rcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> igt_require(rendercopy);
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_intermix_rcs,
> p->copy, h->hang);
> }
> - igt_subtest_f("%s-%s-%s-intermix-bcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-intermix-bcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> igt_require(rendercopy);
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_intermix_bcs,
> p->copy, h->hang);
> }
> - igt_subtest_f("%s-%s-%s-intermix-both%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-intermix-both%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> igt_require(rendercopy);
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> @@ -1571,7 +1571,7 @@ run_mode(const char *prefix,
> }
>
> /* try to read the results before the copy completes */
> - igt_subtest_f("%s-%s-%s-early-read%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-early-read%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_early_read,
> @@ -1579,13 +1579,13 @@ run_mode(const char *prefix,
> }
>
> /* concurrent reads */
> - igt_subtest_f("%s-%s-%s-read-read-bcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-read-read-bcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_read_read_bcs,
> p->copy, h->hang);
> }
> - igt_subtest_f("%s-%s-%s-read-read-rcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-read-read-rcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> igt_require(rendercopy);
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> @@ -1594,13 +1594,13 @@ run_mode(const char *prefix,
> }
>
> /* split copying between rings */
> - igt_subtest_f("%s-%s-%s-write-read-bcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-write-read-bcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_write_read_bcs,
> p->copy, h->hang);
> }
> - igt_subtest_f("%s-%s-%s-write-read-rcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-write-read-rcs%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> igt_require(rendercopy);
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> @@ -1609,7 +1609,7 @@ run_mode(const char *prefix,
> }
>
> /* and finally try to trick the kernel into loosing the pending write */
> - igt_subtest_f("%s-%s-%s-gpu-read-after-write%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> + igt_gem_stress_subtest_f("", "%s-%s-%s-gpu-read-after-write%s%s", prefix, mode->name, p->prefix, suffix, h->suffix) {
> buffers_create(&buffers);
> run_wrap_func(&buffers,
> do_gpu_read_after_write,
> diff --git a/tests/gem_sync.c b/tests/gem_sync.c
> index 706462bc0ac7..36ce3c2880b1 100644
> --- a/tests/gem_sync.c
> +++ b/tests/gem_sync.c
> @@ -730,36 +730,36 @@ igt_main
> }
>
> for (e = intel_execution_engines; e->name; e++) {
> - igt_subtest_f("%s", e->name)
> + igt_gem_subtest_f("", "%s", e->name)
> sync_ring(fd, e->exec_id | e->flags, 1, 150);
> - igt_subtest_f("store-%s", e->name)
> + igt_gem_subtest_f("", "store-%s", e->name)
> store_ring(fd, e->exec_id | e->flags, 1, 150);
> - igt_subtest_f("many-%s", e->name)
> + igt_gem_subtest_f("", "many-%s", e->name)
> store_many(fd, e->exec_id | e->flags, 150);
> - igt_subtest_f("forked-%s", e->name)
> + igt_gem_subtest_f("", "forked-%s", e->name)
> sync_ring(fd, e->exec_id | e->flags, ncpus, 150);
> - igt_subtest_f("forked-store-%s", e->name)
> + igt_gem_subtest_f("", "forked-store-%s", e->name)
> store_ring(fd, e->exec_id | e->flags, ncpus, 150);
> }
>
> - igt_subtest("basic-each")
> + igt_gem_subtest("basic", "each")
> sync_ring(fd, ~0u, 1, 5);
> - igt_subtest("basic-store-each")
> + igt_gem_subtest("basic", "store-each")
> store_ring(fd, ~0u, 1, 5);
> - igt_subtest("basic-many-each")
> + igt_gem_subtest("basic", "many-each")
> store_many(fd, ~0u, 5);
> - igt_subtest("forked-each")
> + igt_gem_subtest("basic", "forked-each")
> sync_ring(fd, ~0u, ncpus, 150);
> - igt_subtest("forked-store-each")
> + igt_gem_subtest("", "forked-store-each")
> store_ring(fd, ~0u, ncpus, 150);
>
> - igt_subtest("basic-all")
> + igt_gem_subtest("basic", "all")
> sync_all(fd, 1, 5);
> - igt_subtest("basic-store-all")
> + igt_gem_subtest("basic", "store-all")
> store_all(fd, 1, 5);
> - igt_subtest("forked-all")
> + igt_gem_subtest("extended", "forked-all")
> sync_all(fd, ncpus, 150);
> - igt_subtest("forked-store-all")
> + igt_gem_subtest("extended", "forked-store-all")
> store_all(fd, ncpus, 150);
>
> igt_fixture {
> --
> 2.9.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list