[RFC i-g-t] igt: Test tagging support

[RFC i-g-t] igt: Test tagging support

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@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@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.

This RFC implementation automatically adds "gem" and "kms" tags
to test binaries prefixed with those strings respectively.

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@intel.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Petri Latvala <petri.latvala@intel.com>
Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC i-g-t] igt: Test tagging support

[Daniel Vetter]

On Tue, Jun 27, 2017 at 12:59:33PM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@intel.com>
> Cc: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Petri Latvala <petri.latvala@intel.com>
> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC i-g-t] igt: Test tagging support

[Tvrtko Ursulin]

On 27/06/2017 14:18, Daniel Vetter wrote:
> On Tue, Jun 27, 2017 at 12:59:33PM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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.

Tags in names are not as flexible and clear. When do tags begin and 
names start, how ugly it will look with names getting long etc.

>> 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.

Yep, and so I said early in the commit message. Here I forgot to explain 
that this gem/kms tag adding I've put in the RFC is just for tests who 
do not specify their own tags.

I've mentioned tags like gem, kms, guc, flip, semaphore, bz12345678, 
gt4e, etc..

> 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.

I think it gives us both. You simply tag a new test with either 
basic/extended/stress and it is automatically included in the correct run.

> 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

It does my introducing a stress keyword and it even marks 
gem_concurrent_ with it.

> 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).

Yes it is some work, but imho not so much.

Start with simply converting the current CI set which is not a long 
list. Then have an intern convert the extended list to tags. :) At this 
point we all work together to extend the extended list and tune the 
tests at the same time. This work is needed anyway if we want to 
increase coverage and fix tests, which we do want. So while we are doing 
that we can just tag them appropriately.

Tvrtko

> -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@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Petri Latvala <petri.latvala@intel.com>
>> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
>> Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg@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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC i-g-t] igt: Test tagging support

[Daniel Vetter]

On Tue, Jun 27, 2017 at 03:03:08PM +0100, Tvrtko Ursulin wrote:
> 
> On 27/06/2017 14:18, Daniel Vetter wrote:
> > On Tue, Jun 27, 2017 at 12:59:33PM +0100, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@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@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.
> 
> Tags in names are not as flexible and clear. When do tags begin and names
> start, how ugly it will look with names getting long etc.

It might be ugly, but if we end up with 2 incomplete/ugly ways to tag
tests, then we're worse of. You're RFC looks like that.
> 
> > > 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.
> 
> Yep, and so I said early in the commit message. Here I forgot to explain
> that this gem/kms tag adding I've put in the RFC is just for tests who do
> not specify their own tags.
> 
> I've mentioned tags like gem, kms, guc, flip, semaphore, bz12345678, gt4e,
> etc..
> 
> > 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.
> 
> I think it gives us both. You simply tag a new test with either
> basic/extended/stress and it is automatically included in the correct run.
> 
> > 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
> 
> It does my introducing a stress keyword and it even marks gem_concurrent_
> with it.

What about kms_frontbuffer_tracking? We already have 2 ways to mark stress
tests, now you add a third. And yours has the problem that testcases are
still run, even if tagged as "stress".

> > 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).
> 
> Yes it is some work, but imho not so much.
> 
> Start with simply converting the current CI set which is not a long list.
> Then have an intern convert the extended list to tags. :) At this point we
> all work together to extend the extended list and tune the tests at the same
> time. This work is needed anyway if we want to increase coverage and fix
> tests, which we do want. So while we are doing that we can just tag them
> appropriately.

Not sure we need an intern, how exactly is an intern going to make good
judgement calls when not even most developers know what all the tests do?
I certainly don't ...

I think this would work much better as an improvement if we reduce scope.
I think managing BAT as an explicit list is doable, and I don't think we
need to add/remove tests all that often (BAT is supposed to check whether
we can run validation runs, not validate itself).

The current accute problem we have is that we can filter out stress tests
and make them not run by default, without ending up with an unmanageable
whitelist. I think just introducing the stress tag (as a #define, not as a
random string, and with documentation) would be an actual real
improvement. At least if it's used to unify the other two ways we have to
auto-delist stress tests (without that it just makes things worse).

For general tagging we atm have the naming scheme, plus the feature
profile json (it's starting to get used I hope soonish). So if you want to
switch that over to tags we'd also need to figure out how to
integrate/replace the feature json stuff.

Just adding tags because tags sounds like a cool idea on top of all the
other things we do already still doesn't sound appealing to me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC i-g-t] igt: Test tagging support

[Jani Nikula]

On Thu, 29 Jun 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> Just adding tags because tags sounds like a cool idea on top of all the
> other things we do already still doesn't sound appealing to me.

Agreed.

So I always thought it was silly when we relied on test names to select
tests to run or exclude in the past. I'm equally suspicious of adding
any sorts of categories or tags within the test sources. I don't think a
test source file should be changed if the category changes. Thus I think
maintaining the categories outside of the tests is a good idea.

Currently we maintain the categories as test lists. I think you could
call them tags just as well. Them being lists in files is just an
implementation detail. If a test is included in a test list, then it's
so tagged. If we wanted to, we could add more logic to running the tests
based on which lists the tests are in. For example, "in extended and not
in stress".

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC i-g-t] igt: Test tagging support

[Tvrtko Ursulin]

On 29/06/2017 09:49, Daniel Vetter wrote:
> On Tue, Jun 27, 2017 at 03:03:08PM +0100, Tvrtko Ursulin wrote:
>>
>> On 27/06/2017 14:18, Daniel Vetter wrote:
>>> On Tue, Jun 27, 2017 at 12:59:33PM +0100, Tvrtko Ursulin wrote:
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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.
>>
>> Tags in names are not as flexible and clear. When do tags begin and names
>> start, how ugly it will look with names getting long etc.
> 
> It might be ugly, but if we end up with 2 incomplete/ugly ways to tag
> tests, then we're worse of. You're RFC looks like that.

Damn it, the RFC only looks like that because it only converted a couple 
of tests cases and not all of them. Not because I am proposing to have 
two parallel systems.

>>>> 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.
>>
>> Yep, and so I said early in the commit message. Here I forgot to explain
>> that this gem/kms tag adding I've put in the RFC is just for tests who do
>> not specify their own tags.
>>
>> I've mentioned tags like gem, kms, guc, flip, semaphore, bz12345678, gt4e,
>> etc..
>>
>>> 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.
>>
>> I think it gives us both. You simply tag a new test with either
>> basic/extended/stress and it is automatically included in the correct run.
>>
>>> 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
>>
>> It does my introducing a stress keyword and it even marks gem_concurrent_
>> with it.
> 
> What about kms_frontbuffer_tracking? We already have 2 ways to mark stress
> tests, now you add a third. And yours has the problem that testcases are

Is the problem that I did not convert kms_frontbuffer_tracking in the 
sketch of a tagging system or something else?

> still run, even if tagged as "stress".

?? "run-tests --include basic,extended --exclude stress" is an example 
of the proposal for the new system. So I don't get this criticism. 
Tagging tests with "stress" exactly enables you not to run them.

> 
>>> 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).
>>
>> Yes it is some work, but imho not so much.
>>
>> Start with simply converting the current CI set which is not a long list.
>> Then have an intern convert the extended list to tags. :) At this point we
>> all work together to extend the extended list and tune the tests at the same
>> time. This work is needed anyway if we want to increase coverage and fix
>> tests, which we do want. So while we are doing that we can just tag them
>> appropriately.
> 
> Not sure we need an intern, how exactly is an intern going to make good
> judgement calls when not even most developers know what all the tests do?
> I certainly don't ...

Intern was simply a response to the criticism that converting the 
existing testslists to tags is a big job. It is a mindless job for a 
first phase which brings feature parity to where we are today. All tests 
which are mentioned in the extended test list simply needed to be 
converted to igt_gem|kms_subtest("extended", "test-name").

Adding additional fine-tuned tags is an additional job on top which is 
something we don't have today anyway. So that one is a job for developers.

> 
> I think this would work much better as an improvement if we reduce scope.
> I think managing BAT as an explicit list is doable, and I don't think we
> need to add/remove tests all that often (BAT is supposed to check whether
> we can run validation runs, not validate itself).
> 
> The current accute problem we have is that we can filter out stress tests
> and make them not run by default, without ending up with an unmanageable
> whitelist. I think just introducing the stress tag (as a #define, not as a
> random string, and with documentation) would be an actual real
> improvement. At least if it's used to unify the other two ways we have to
> auto-delist stress tests (without that it just makes things worse).
> 
> For general tagging we atm have the naming scheme, plus the feature
> profile json (it's starting to get used I hope soonish). So if you want to
> switch that over to tags we'd also need to figure out how to
> integrate/replace the feature json stuff.
> 
> Just adding tags because tags sounds like a cool idea on top of all the
> other things we do already still doesn't sound appealing to me.

It is not supposed to be on top, but to unify and replace.

But OK, I've tried to sell it and no one is liking is so fine. The only 
thing that bothers me that to me it sounds like criticism is missing the 
point, but I don't know how else to try and explain the system.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC i-g-t] igt: Test tagging support

[Tvrtko Ursulin]

On 29/06/2017 10:07, Jani Nikula wrote:
> On Thu, 29 Jun 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
>> Just adding tags because tags sounds like a cool idea on top of all the
>> other things we do already still doesn't sound appealing to me.
> 
> Agreed.

Well it's not on top, but as a replacement, but anyway.

> So I always thought it was silly when we relied on test names to select
> tests to run or exclude in the past. I'm equally suspicious of adding
> any sorts of categories or tags within the test sources. I don't think a
> test source file should be changed if the category changes. Thus I think
> maintaining the categories outside of the tests is a good idea.
> 
> Currently we maintain the categories as test lists. I think you could
> call them tags just as well. Them being lists in files is just an
> implementation detail. If a test is included in a test list, then it's
> so tagged. If we wanted to, we could add more logic to running the tests
> based on which lists the tests are in. For example, "in extended and not
> in stress".

We started with the premise that tests lists are bad. :) If we have 
multiple lists we will need someone to keep an eye on test changes and 
keep all the lists up to date. To me that has a higher chance of failing 
that having tags together with the code. It is easier to remember if it 
stares at you from the screen while you are editing the test.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [RFC i-g-t] igt: Test tagging support

[Daniel Vetter]

On Thu, Jun 29, 2017 at 10:23:03AM +0100, Tvrtko Ursulin wrote:
> 
> On 29/06/2017 09:49, Daniel Vetter wrote:
> > On Tue, Jun 27, 2017 at 03:03:08PM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 27/06/2017 14:18, Daniel Vetter wrote:
> > > > On Tue, Jun 27, 2017 at 12:59:33PM +0100, Tvrtko Ursulin wrote:
> > > > > From: Tvrtko Ursulin <tvrtko.ursulin@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@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.
> > > 
> > > Tags in names are not as flexible and clear. When do tags begin and names
> > > start, how ugly it will look with names getting long etc.
> > 
> > It might be ugly, but if we end up with 2 incomplete/ugly ways to tag
> > tests, then we're worse of. You're RFC looks like that.
> 
> Damn it, the RFC only looks like that because it only converted a couple of
> tests cases and not all of them. Not because I am proposing to have two
> parallel systems.

It's not just the current tests, you also get to re-train an entire team
of 20 people ...

> > > > > 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.
> > > 
> > > Yep, and so I said early in the commit message. Here I forgot to explain
> > > that this gem/kms tag adding I've put in the RFC is just for tests who do
> > > not specify their own tags.
> > > 
> > > I've mentioned tags like gem, kms, guc, flip, semaphore, bz12345678, gt4e,
> > > etc..
> > > 
> > > > 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.
> > > 
> > > I think it gives us both. You simply tag a new test with either
> > > basic/extended/stress and it is automatically included in the correct run.
> > > 
> > > > 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
> > > 
> > > It does my introducing a stress keyword and it even marks gem_concurrent_
> > > with it.
> > 
> > What about kms_frontbuffer_tracking? We already have 2 ways to mark stress
> > tests, now you add a third. And yours has the problem that testcases are
> 
> Is the problem that I did not convert kms_frontbuffer_tracking in the sketch
> of a tagging system or something else?
> 
> > still run, even if tagged as "stress".
> 
> ?? "run-tests --include basic,extended --exclude stress" is an example of
> the proposal for the new system. So I don't get this criticism. Tagging
> tests with "stress" exactly enables you not to run them.

So if I run piglit igt.py, the stress tests won't run? That's the thing
I'm looking for, everything else I'm decidedly meh on (except I want ot
make sure we do actually have the people to do the massive amount of work,
and I'm not sold on the benefits of tags in general for everything).

> > > > 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).
> > > 
> > > Yes it is some work, but imho not so much.
> > > 
> > > Start with simply converting the current CI set which is not a long list.
> > > Then have an intern convert the extended list to tags. :) At this point we
> > > all work together to extend the extended list and tune the tests at the same
> > > time. This work is needed anyway if we want to increase coverage and fix
> > > tests, which we do want. So while we are doing that we can just tag them
> > > appropriately.
> > 
> > Not sure we need an intern, how exactly is an intern going to make good
> > judgement calls when not even most developers know what all the tests do?
> > I certainly don't ...
> 
> Intern was simply a response to the criticism that converting the existing
> testslists to tags is a big job. It is a mindless job for a first phase
> which brings feature parity to where we are today. All tests which are
> mentioned in the extended test list simply needed to be converted to
> igt_gem|kms_subtest("extended", "test-name").
> 
> Adding additional fine-tuned tags is an additional job on top which is
> something we don't have today anyway. So that one is a job for developers.

Who's going to do all that? You're proposing a massive rework here of how
igt test enumeration works ...

> > I think managing BAT as an explicit list is doable, and I don't think we
> > need to add/remove tests all that often (BAT is supposed to check whether
> > we can run validation runs, not validate itself).
> > 
> > The current accute problem we have is that we can filter out stress tests
> > and make them not run by default, without ending up with an unmanageable
> > whitelist. I think just introducing the stress tag (as a #define, not as a
> > random string, and with documentation) would be an actual real
> > improvement. At least if it's used to unify the other two ways we have to
> > auto-delist stress tests (without that it just makes things worse).
> > 
> > For general tagging we atm have the naming scheme, plus the feature
> > profile json (it's starting to get used I hope soonish). So if you want to
> > switch that over to tags we'd also need to figure out how to
> > integrate/replace the feature json stuff.
> > 
> > Just adding tags because tags sounds like a cool idea on top of all the
> > other things we do already still doesn't sound appealing to me.
> 
> It is not supposed to be on top, but to unify and replace.
> 
> But OK, I've tried to sell it and no one is liking is so fine. The only
> thing that bothers me that to me it sounds like criticism is missing the
> point, but I don't know how else to try and explain the system.

You've proposed to convert the add-hoc tagging + add-how hiding of stress
tests into tags. That's a massive project, you need the entire team on
board pretty much.

I don't think tags are bad, but much smaller scope will make it much more
likely they're actually going to be an improvement. And then we can go
from there ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx