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

[RFC i-g-t v2] 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.

Current method of implying tags from test/subtest names has a
couple of problems one of which is where some names can use
the same token for different meanings. (One example is the
"default" token.) It also creates a name clash between naming
and tagging.

Test tags are strings of 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, basic, guc, flip,
semaphore, bz12345678, gt4e, etc..

At runtime this would look something like this (abbreviated for
readability):

  root@e31:~/intel-gpu-tools# tests/gem_sync --list-subtests-with-tags
  ...
  forked-each TAGS="gem "
  forked-store-each TAGS="gem "
  basic-all TAGS="gem basic "
  basic-store-all TAGS="gem basic "

  root@e31:~/intel-gpu-tools# tests/gem_concurrent_blit --list-subtests-with-tags
  ...
  16MiB-swap-gpuX-render-write-read-bcs-bomb TAGS="gem stress "
  16MiB-swap-gpuX-render-write-read-rcs-bomb TAGS="gem stress "
  16MiB-swap-gpuX-render-gpu-read-after-write-bomb TAGS="gem stress "

  root@e31:~/intel-gpu-tools# tests/kms_flip --list-subtests-with-tags | grep basic
  basic-plain-flip TAGS="kms basic "
  basic-flip-vs-dpms TAGS="kms basic "

Test runner can then enable usages like:

  ./run-tests --include gem --exclude stress

Which I think could really be useful and more flexible than name
based selection.

This RFC implementation automatically adds various tags based on
both binary and subtest names. This is to enable easy transition
to the tag based system.

Idea is that all current tests get some useful tags with minimum
amount of work. Unfortunately this doesn't work for test binaries
which at the moment do not support the subtests enumeration so
those are the ones which would need identifying and converting.

Source code usage, for tests which would want to override the
default tag generation would look something like:

	igt_gem_subtest("basic", "each")
		...

	igt_gem_subtest("manual", "special-setup-test")
		...

        igt_gem_stress_subtest_f("tag", "name-%s", ...) {
	        ...

We can of course polish the details of the API if the idea will
be deemed interesting.

v2: More default tags for untagged tests for easier migration.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 lib/igt_core.c             | 118 ++++++++++++++++++++++++++++++++++++++++++---
 lib/igt_core.h             |  29 +++++++++--
 tests/gem_concurrent_all.c |  34 ++++++-------
 3 files changed, 154 insertions(+), 27 deletions(-)

diff --git a/lib/igt_core.c b/lib/igt_core.c
index c0488e944cec..d140196cad13 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -253,10 +253,12 @@ 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;
+static char *subtest_tags;
+static bool subtest_tags_allocated;
 static struct timespec subtest_time;
 static clockid_t igt_clock = (clockid_t)-1;
 static bool in_fixture = false;
@@ -276,6 +278,7 @@ bool test_child;
 
 enum {
  OPT_LIST_SUBTESTS,
+ OPT_LIST_SUBTESTS_TAGS,
  OPT_RUN_SUBTEST,
  OPT_DESCRIPTION,
  OPT_DEBUG,
@@ -599,6 +602,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"
@@ -709,6 +713,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},
@@ -803,6 +808,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);
@@ -935,19 +946,100 @@ void igt_simple_init_parse_opts(int *argc, char **argv,
 		    extra_opt_handler, handler_data);
 }
 
+#define __IGT_TAGS_MAX (4096)
+
+static char * __get_tag_str(char *tags)
+{
+	if (tags)
+		return tags;
+
+	tags = malloc(__IGT_TAGS_MAX);
+	igt_assert(tags);
+
+	memset(tags, 0, __IGT_TAGS_MAX);
+
+	return tags;
+}
+
+static char * __add_tag(char *tags, unsigned int *pos, const char *tag)
+{
+	size_t left = __IGT_TAGS_MAX - *pos;
+	char _tag[128];
+	int ret;
+
+	if (!tag)
+		return tags;
+
+	tags = __get_tag_str(tags);
+
+	sprintf(_tag, "%s ", tag);
+	if (!strncmp(tags, _tag, strlen(_tag)))
+		return tags;
+
+	sprintf(_tag, " %s ", tag);
+	if (strstr(tags, _tag))
+		return tags;
+
+	ret = snprintf(tags + *pos, left, "%s ", tag);
+	igt_assert(ret > 0 && ret < left);
+
+	*pos += ret;
+
+	return tags;
+}
+
 /*
  * Note: Testcases which use these helpers MUST NOT output anything to stdout
  * 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)
 {
+	unsigned int tag_idx = 0;
+	char *_tags = NULL;
+	const char *tag;
 	int i;
 
 	assert(!in_subtest);
+	assert(!subtest_tags);
 	assert(!in_fixture);
 	assert(test_with_subtests);
 
+	if (!tags) {
+		if (!strncmp(command_str, "gem_", 4))
+			tag = "gem";
+		else if (!strncmp(command_str, "prime_", 4))
+			tag = "gem";
+		else if (!strncmp(command_str, "kms_", 4))
+			tag = "kms";
+		else if (!strncmp(command_str, "pm_", 3))
+			tag = "pm";
+		else if (!strncmp(command_str, "drv_", 3))
+			tag = "drv";
+		else if (!strncmp(command_str, "debugfs_", 3))
+			tag = "drv";
+		else if (!strncmp(command_str, "core_", 4))
+			tag = "drm";
+		else if (!strncmp(command_str, "drm_", 4))
+			tag = "drm";
+		else if (!strncmp(command_str, "sw_", 4))
+			tag = "external";
+		else
+			tag = NULL;
+
+		_tags = __add_tag(_tags, &tag_idx, tag);
+
+		if (!strncmp(subtest_name, "basic-", 6))
+			tag = "basic";
+		else
+			tag = NULL;
+
+		_tags = __add_tag(_tags, &tag_idx, tag);
+
+		tags = _tags;
+		subtest_tags_allocated = _tags;
+	}
+
 	/* 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] != '-'
@@ -958,7 +1050,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;
 	}
 
@@ -983,7 +1078,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 = (char *)tags;
+
+	return true;
 }
 
 /**
@@ -1034,7 +1133,12 @@ static void exit_subtest(const char *result)
 	       (!__igt_plain_output) ? "\x1b[0m" : "");
 	fflush(stdout);
 
-	in_subtest = NULL;
+	if (subtest_tags_allocated)
+		free(subtest_tags);
+
+	in_subtest = subtest_tags = NULL;
+	subtest_tags_allocated = false;
+
 	siglongjmp(igt_subtest_jmpbuf, 1);
 }
 
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 1619a9d65400..6c55c04b7de0 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -147,7 +147,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
 
 /**
@@ -160,6 +160,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
@@ -171,14 +194,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,
-- 
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 v2] igt: Test tagging support

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-07-21 11:20:05)
> 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.
> 
> Current method of implying tags from test/subtest names has a
> couple of problems one of which is where some names can use
> the same token for different meanings. (One example is the
> "default" token.) It also creates a name clash between naming
> and tagging.
> 
> Test tags are strings of 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, basic, guc, flip,
> semaphore, bz12345678, gt4e, etc..
> 
> At runtime this would look something like this (abbreviated for
> readability):
> 
>   root@e31:~/intel-gpu-tools# tests/gem_sync --list-subtests-with-tags
>   ...
>   forked-each TAGS="gem "
>   forked-store-each TAGS="gem "
>   basic-all TAGS="gem basic "
>   basic-store-all TAGS="gem basic "
> 
>   root@e31:~/intel-gpu-tools# tests/gem_concurrent_blit --list-subtests-with-tags
>   ...
>   16MiB-swap-gpuX-render-write-read-bcs-bomb TAGS="gem stress "
>   16MiB-swap-gpuX-render-write-read-rcs-bomb TAGS="gem stress "
>   16MiB-swap-gpuX-render-gpu-read-after-write-bomb TAGS="gem stress "
> 
>   root@e31:~/intel-gpu-tools# tests/kms_flip --list-subtests-with-tags | grep basic
>   basic-plain-flip TAGS="kms basic "
>   basic-flip-vs-dpms TAGS="kms basic "
> 
> Test runner can then enable usages like:
> 
>   ./run-tests --include gem --exclude stress
> 
> Which I think could really be useful and more flexible than name
> based selection.
> 
> This RFC implementation automatically adds various tags based on
> both binary and subtest names. This is to enable easy transition
> to the tag based system.
> 
> Idea is that all current tests get some useful tags with minimum
> amount of work. Unfortunately this doesn't work for test binaries
> which at the moment do not support the subtests enumeration so
> those are the ones which would need identifying and converting.
> 
> Source code usage, for tests which would want to override the
> default tag generation would look something like:
> 
>         igt_gem_subtest("basic", "each")
>                 ...
> 
>         igt_gem_subtest("manual", "special-setup-test")
>                 ...
> 
>         igt_gem_stress_subtest_f("tag", "name-%s", ...) {
>                 ...
> 
> We can of course polish the details of the API if the idea will
> be deemed interesting.
> 
> v2: More default tags for untagged tests for easier migration.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  lib/igt_core.c             | 118 ++++++++++++++++++++++++++++++++++++++++++---
>  lib/igt_core.h             |  29 +++++++++--
>  tests/gem_concurrent_all.c |  34 ++++++-------
>  3 files changed, 154 insertions(+), 27 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index c0488e944cec..d140196cad13 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -253,10 +253,12 @@ 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;
> +static char *subtest_tags;
> +static bool subtest_tags_allocated;
>  static struct timespec subtest_time;
>  static clockid_t igt_clock = (clockid_t)-1;
>  static bool in_fixture = false;
> @@ -276,6 +278,7 @@ bool test_child;
>  
>  enum {
>   OPT_LIST_SUBTESTS,
> + OPT_LIST_SUBTESTS_TAGS,
>   OPT_RUN_SUBTEST,
>   OPT_DESCRIPTION,
>   OPT_DEBUG,
> @@ -599,6 +602,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"
> @@ -709,6 +713,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},
> @@ -803,6 +808,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);
> @@ -935,19 +946,100 @@ void igt_simple_init_parse_opts(int *argc, char **argv,
>                     extra_opt_handler, handler_data);
>  }
>  
> +#define __IGT_TAGS_MAX (4096)
> +
> +static char * __get_tag_str(char *tags)
> +{
> +       if (tags)
> +               return tags;
> +
> +       tags = malloc(__IGT_TAGS_MAX);
> +       igt_assert(tags);
> +
> +       memset(tags, 0, __IGT_TAGS_MAX);
> +
> +       return tags;
> +}
> +
> +static char * __add_tag(char *tags, unsigned int *pos, const char *tag)
> +{
> +       size_t left = __IGT_TAGS_MAX - *pos;
> +       char _tag[128];
> +       int ret;
> +
> +       if (!tag)
> +               return tags;
> +
> +       tags = __get_tag_str(tags);
> +
> +       sprintf(_tag, "%s ", tag);
> +       if (!strncmp(tags, _tag, strlen(_tag)))
> +               return tags;
> +
> +       sprintf(_tag, " %s ", tag);
> +       if (strstr(tags, _tag))
> +               return tags;
> +
> +       ret = snprintf(tags + *pos, left, "%s ", tag);
> +       igt_assert(ret > 0 && ret < left);
> +
> +       *pos += ret;
> +
> +       return tags;
> +}
> +
>  /*
>   * Note: Testcases which use these helpers MUST NOT output anything to stdout
>   * 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)
>  {
> +       unsigned int tag_idx = 0;
> +       char *_tags = NULL;
> +       const char *tag;
>         int i;
>  
>         assert(!in_subtest);
> +       assert(!subtest_tags);
>         assert(!in_fixture);
>         assert(test_with_subtests);
>  
> +       if (!tags) {
> +               if (!strncmp(command_str, "gem_", 4))
> +                       tag = "gem";
> +               else if (!strncmp(command_str, "prime_", 4))
> +                       tag = "gem";
> +               else if (!strncmp(command_str, "kms_", 4))
> +                       tag = "kms";
> +               else if (!strncmp(command_str, "pm_", 3))
> +                       tag = "pm";
> +               else if (!strncmp(command_str, "drv_", 3))
> +                       tag = "drv";
> +               else if (!strncmp(command_str, "debugfs_", 3))
> +                       tag = "drv";
> +               else if (!strncmp(command_str, "core_", 4))
> +                       tag = "drm";
> +               else if (!strncmp(command_str, "drm_", 4))
> +                       tag = "drm";
> +               else if (!strncmp(command_str, "sw_", 4))
> +                       tag = "external";
> +               else
> +                       tag = NULL;
> +
> +               _tags = __add_tag(_tags, &tag_idx, tag);
> +
> +               if (!strncmp(subtest_name, "basic-", 6))
> +                       tag = "basic";
> +               else
> +                       tag = NULL;
> +
> +               _tags = __add_tag(_tags, &tag_idx, tag);
> +
> +               tags = _tags;
> +               subtest_tags_allocated = _tags;
> +       }
> +
>         /* 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] != '-'
> @@ -958,7 +1050,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;
>         }
>  
> @@ -983,7 +1078,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 = (char *)tags;
> +
> +       return true;
>  }
>  
>  /**
> @@ -1034,7 +1133,12 @@ static void exit_subtest(const char *result)
>                (!__igt_plain_output) ? "\x1b[0m" : "");
>         fflush(stdout);
>  
> -       in_subtest = NULL;
> +       if (subtest_tags_allocated)
> +               free(subtest_tags);
> +
> +       in_subtest = subtest_tags = NULL;
> +       subtest_tags_allocated = false;
> +
>         siglongjmp(igt_subtest_jmpbuf, 1);
>  }
>  
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 1619a9d65400..6c55c04b7de0 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -147,7 +147,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
>  
>  /**
> @@ -160,6 +160,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
> @@ -171,14 +194,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) {

They are not all stress tests. So you want to be able to build the tags
dynamically... Similarly they offer different types of "stress", you
probably don't want to lump the hang tests in amongst the plain
concurrency tests, and you probably want the swapping tests separated
etc. Stress is missing the point.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 21/07/2017 11:36, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-07-21 11:20:05)
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

[snip]

>> --- 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) {
> 
> They are not all stress tests. So you want to be able to build the tags
> dynamically... Similarly they offer different types of "stress", you
> probably don't want to lump the hang tests in amongst thes plain
> concurrency tests, and you probably want the swapping tests separated
> etc. Stress is missing the point.

Dynamic tags are doable. If you just wanted to include "stress" 
dynamically current RFC can already do that.

	igt_gem_subtest_f(is_stress ? "stress" : "", name, ...)

If you wanted a dynamic set of multiple tags that could be added as well 
I guess. Like a flag based control of "stress", "swapping", "hang", 
"basic", or something. How nice or ugly API depends on the actual 
requirements.

Or if you think that the test lists are a better way to handle all this 
then that is also fine by me.

Regards,

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

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

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-07-21 12:08:00)
> 
> On 21/07/2017 11:36, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-07-21 11:20:05)
> >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> [snip]
> 
> >> --- 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) {
> > 
> > They are not all stress tests. So you want to be able to build the tags
> > dynamically... Similarly they offer different types of "stress", you
> > probably don't want to lump the hang tests in amongst thes plain
> > concurrency tests, and you probably want the swapping tests separated
> > etc. Stress is missing the point.
> 
> Dynamic tags are doable. If you just wanted to include "stress" 
> dynamically current RFC can already do that.
> 
>         igt_gem_subtest_f(is_stress ? "stress" : "", name, ...)
> 
> If you wanted a dynamic set of multiple tags that could be added as well 
> I guess. Like a flag based control of "stress", "swapping", "hang", 
> "basic", or something. How nice or ugly API depends on the actual 
> requirements.

hang, swap, shrink, gtt, wc, cpu, pwrite, pread, contexts, fds, prime,
dmabuf and many more when you start looking for the complete set of
tags/keywords/categories.

Currently, we have tags (keywords) embedded into the test/subtest name.
(One way of looking at it, every test would be a unique combination of
tags.) Being able to filter tests on those tags is definitely lacking.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Tvrtko Ursulin]

On 21/07/2017 12:27, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-07-21 12:08:00)
>>
>> On 21/07/2017 11:36, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-07-21 11:20:05)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> [snip]
>>
>>>> --- 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) {
>>>
>>> They are not all stress tests. So you want to be able to build the tags
>>> dynamically... Similarly they offer different types of "stress", you
>>> probably don't want to lump the hang tests in amongst thes plain
>>> concurrency tests, and you probably want the swapping tests separated
>>> etc. Stress is missing the point.
>>
>> Dynamic tags are doable. If you just wanted to include "stress"
>> dynamically current RFC can already do that.
>>
>>          igt_gem_subtest_f(is_stress ? "stress" : "", name, ...)
>>
>> If you wanted a dynamic set of multiple tags that could be added as well
>> I guess. Like a flag based control of "stress", "swapping", "hang",
>> "basic", or something. How nice or ugly API depends on the actual
>> requirements.
> 
> hang, swap, shrink, gtt, wc, cpu, pwrite, pread, contexts, fds, prime,
> dmabuf and many more when you start looking for the complete set of
> tags/keywords/categories.

I would like having rich tags as well.

> Currently, we have tags (keywords) embedded into the test/subtest name.
> (One way of looking at it, every test would be a unique combination of
> tags.) Being able to filter tests on those tags is definitely lacking.

I don't think every test is an unique combination of tags. For example 
tagged with "gem pread" we woud have tests doing small reads, large 
reads, overflowing reads etc. I would invent new tags for all these cases.

Anyway, I am not sure what is the high level message here. Do you like 
the general idea but find the implementation lacking, or think something 
completely different would be the easier route to the equally functional 
end result?

I like tags because they separate from the test naming namespace. I used 
the example of "default" which means different things in the current 
test names. But as I said it is just a proposal.

And gem_concurrent_* is probably on of the hardest ones to elegantly 
handle since the name is generated from so many parts.

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

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

[Martin Peres]

On 21/07/17 14:27, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-07-21 12:08:00)
>>
>> On 21/07/2017 11:36, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-07-21 11:20:05)
>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> [snip]
>>
>>>> --- 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) {
>>>
>>> They are not all stress tests. So you want to be able to build the tags
>>> dynamically... Similarly they offer different types of "stress", you
>>> probably don't want to lump the hang tests in amongst thes plain
>>> concurrency tests, and you probably want the swapping tests separated
>>> etc. Stress is missing the point.
>>
>> Dynamic tags are doable. If you just wanted to include "stress"
>> dynamically current RFC can already do that.
>>
>>          igt_gem_subtest_f(is_stress ? "stress" : "", name, ...)
>>
>> If you wanted a dynamic set of multiple tags that could be added as well
>> I guess. Like a flag based control of "stress", "swapping", "hang",
>> "basic", or something. How nice or ugly API depends on the actual
>> requirements.
> 
> hang, swap, shrink, gtt, wc, cpu, pwrite, pread, contexts, fds, prime,
> dmabuf and many more when you start looking for the complete set of
> tags/keywords/categories.

This is why I would rather use the execution time of tests as a way to 
tag the tests. What I want is to have a couple of options that brings me 
the best coverage in a certain amount of time:
  - BAT: 70% coverage in 10 minutes
  - FULL: 95% coverage in 6 hours
  - Stress: 99% coverage in 1 year

What do the tags hang, swap, shrink, gtt, etc.. are supposed to bring to me?

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

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

[Tvrtko Ursulin]

On 21/07/2017 13:02, Martin Peres wrote:
> On 21/07/17 14:27, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2017-07-21 12:08:00)
>>>
>>> On 21/07/2017 11:36, Chris Wilson wrote:
>>>> Quoting Tvrtko Ursulin (2017-07-21 11:20:05)
>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> [snip]
>>>
>>>>> --- 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) {
>>>>
>>>> They are not all stress tests. So you want to be able to build the tags
>>>> dynamically... Similarly they offer different types of "stress", you
>>>> probably don't want to lump the hang tests in amongst thes plain
>>>> concurrency tests, and you probably want the swapping tests separated
>>>> etc. Stress is missing the point.
>>>
>>> Dynamic tags are doable. If you just wanted to include "stress"
>>> dynamically current RFC can already do that.
>>>
>>>          igt_gem_subtest_f(is_stress ? "stress" : "", name, ...)
>>>
>>> If you wanted a dynamic set of multiple tags that could be added as well
>>> I guess. Like a flag based control of "stress", "swapping", "hang",
>>> "basic", or something. How nice or ugly API depends on the actual
>>> requirements.
>>
>> hang, swap, shrink, gtt, wc, cpu, pwrite, pread, contexts, fds, prime,
>> dmabuf and many more when you start looking for the complete set of
>> tags/keywords/categories.
> 
> This is why I would rather use the execution time of tests as a way to 
> tag the tests. What I want is to have a couple of options that brings me 
> the best coverage in a certain amount of time:
>   - BAT: 70% coverage in 10 minutes
>   - FULL: 95% coverage in 6 hours
>   - Stress: 99% coverage in 1 year
> 
> What do the tags hang, swap, shrink, gtt, etc.. are supposed to bring to 
> me?

Two things. First is for the "full" run, CI, developers, whoever:

run-tests --exclude stress

And then for the developers ("Making IGT runnable by CI and 
developers"!), for example after some refactoring in specific areas of 
the code base:

run-tests --include gem --include pread
run-tests --include kms --include flip --exclude hang

Examples only...

Regards,

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

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

[Martin Peres]

On 21/07/17 15:17, Tvrtko Ursulin wrote:
> 
> On 21/07/2017 13:02, Martin Peres wrote:
>> On 21/07/17 14:27, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-07-21 12:08:00)
>>>>
>>>> On 21/07/2017 11:36, Chris Wilson wrote:
>>>>> Quoting Tvrtko Ursulin (2017-07-21 11:20:05)
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>
>>>> [snip]
>>>>
>>>>>> --- 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) {
>>>>>
>>>>> They are not all stress tests. So you want to be able to build the 
>>>>> tags
>>>>> dynamically... Similarly they offer different types of "stress", you
>>>>> probably don't want to lump the hang tests in amongst thes plain
>>>>> concurrency tests, and you probably want the swapping tests separated
>>>>> etc. Stress is missing the point.
>>>>
>>>> Dynamic tags are doable. If you just wanted to include "stress"
>>>> dynamically current RFC can already do that.
>>>>
>>>>          igt_gem_subtest_f(is_stress ? "stress" : "", name, ...)
>>>>
>>>> If you wanted a dynamic set of multiple tags that could be added as 
>>>> well
>>>> I guess. Like a flag based control of "stress", "swapping", "hang",
>>>> "basic", or something. How nice or ugly API depends on the actual
>>>> requirements.
>>>
>>> hang, swap, shrink, gtt, wc, cpu, pwrite, pread, contexts, fds, prime,
>>> dmabuf and many more when you start looking for the complete set of
>>> tags/keywords/categories.
>>
>> This is why I would rather use the execution time of tests as a way to 
>> tag the tests. What I want is to have a couple of options that brings 
>> me the best coverage in a certain amount of time:
>>   - BAT: 70% coverage in 10 minutes
>>   - FULL: 95% coverage in 6 hours
>>   - Stress: 99% coverage in 1 year
>>
>> What do the tags hang, swap, shrink, gtt, etc.. are supposed to bring 
>> to me?
> 
> Two things. First is for the "full" run, CI, developers, whoever:
> 
> run-tests --exclude stress
> 
> And then for the developers ("Making IGT runnable by CI and 
> developers"!), for example after some refactoring in specific areas of 
> the code base:
> 
> run-tests --include gem --include pread
> run-tests --include kms --include flip --exclude hang
> 
> Examples only...

I see, and I like that! So tests are annotated with many tags, that 
makes sense :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Martin Peres (2017-07-21 13:02:47)
> On 21/07/17 14:27, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-07-21 12:08:00)
> >>
> >> On 21/07/2017 11:36, Chris Wilson wrote:
> >>> Quoting Tvrtko Ursulin (2017-07-21 11:20:05)
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >> [snip]
> >>
> >>>> --- 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) {
> >>>
> >>> They are not all stress tests. So you want to be able to build the tags
> >>> dynamically... Similarly they offer different types of "stress", you
> >>> probably don't want to lump the hang tests in amongst thes plain
> >>> concurrency tests, and you probably want the swapping tests separated
> >>> etc. Stress is missing the point.
> >>
> >> Dynamic tags are doable. If you just wanted to include "stress"
> >> dynamically current RFC can already do that.
> >>
> >>          igt_gem_subtest_f(is_stress ? "stress" : "", name, ...)
> >>
> >> If you wanted a dynamic set of multiple tags that could be added as well
> >> I guess. Like a flag based control of "stress", "swapping", "hang",
> >> "basic", or something. How nice or ugly API depends on the actual
> >> requirements.
> > 
> > hang, swap, shrink, gtt, wc, cpu, pwrite, pread, contexts, fds, prime,
> > dmabuf and many more when you start looking for the complete set of
> > tags/keywords/categories.
> 
> This is why I would rather use the execution time of tests as a way to 
> tag the tests. What I want is to have a couple of options that brings me 
> the best coverage in a certain amount of time:
>   - BAT: 70% coverage in 10 minutes
>   - FULL: 95% coverage in 6 hours
>   - Stress: 99% coverage in 1 year
> 
> What do the tags hang, swap, shrink, gtt, etc.. are supposed to bring to me?

Nothing. CI should be focused purely on bang per buck. Until we can
measure coverage in a meaningful way, evaluating tests is entirely
arbitrary and worse we have no rigorous means for developing tests.

My dream would be for the system to recognise which tests provide
coverage for a patch and rank them by relevance and then run until death
or timeout. Without such introspection, the best we can do is to have
the developer supply the selection via a common system of tags, one
approximation would be to supply tags based on altered files and
functions. Hmm, that seems doable.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Fri, Jul 21, 2017 at 2:02 PM, Martin Peres
<martin.peres@linux.intel.com> wrote:
> This is why I would rather use the execution time of tests as a way to tag
> the tests. What I want is to have a couple of options that brings me the
> best coverage in a certain amount of time:
>  - BAT: 70% coverage in 10 minutes
>  - FULL: 95% coverage in 6 hours
>  - Stress: 99% coverage in 1 year
>
> What do the tags hang, swap, shrink, gtt, etc.. are supposed to bring to me?

+1 on not adding random piles of tags. Imo we only need two really:
- BAT, for the "is it garbage or can we throw serious amounts of
machine time at this patch" question.
- Stress-tests, to exclude from the normal regression run. Right now
the only thing we can do with stress-tests is have them for developers
to run locally when they're chasing a bug.
- Untagged = default = full igt run.

If we add piles of tags then we're just recreating the naming mess we
have in the testnames already. I don't see what value that would
provide.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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 v2] igt: Test tagging support

[Daniel Vetter]

On Fri, Jul 21, 2017 at 2:59 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> My dream would be for the system to recognise which tests provide
> coverage for a patch and rank them by relevance and then run until death
> or timeout. Without such introspection, the best we can do is to have
> the developer supply the selection via a common system of tags, one
> approximation would be to supply tags based on altered files and
> functions. Hmm, that seems doable.

In principle I agree, in practice I haven't seen such an oracle that
actually works. There's not just the coverage problem, but there's a
huge process problem: If our oracle isn't perfect at constructing the
test set, or if it's timeout based, there's a good chance that even
the same (or very similar patch) will run different sets of testcases.
That adds noise to CI results, and noise trains developers to ignore
it (and no amount of reminding to look at detailed results will fix
that).

The other problem is that we need to be able to compare those test
runs against a baseline. That means we must run the full regression
test set on drm-tip, or pre-merge can't give you results. And drm-tip
updates a few times a day already. We could probably batch more, but
we must have enough machine time to get through a few full runs,
without even taking pre-merge testing into account.

That means even the full set can't really take more than perhaps 6-8h,
even when you have a perfect oracle that selects the test set for each
patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 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 v2] igt: Test tagging support

[Jani Nikula]

On Fri, 21 Jul 2017, Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> 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.
>
> Current method of implying tags from test/subtest names has a
> couple of problems one of which is where some names can use
> the same token for different meanings. (One example is the
> "default" token.) It also creates a name clash between naming
> and tagging.

I'll say this once more, and then I'll shut up.

I think it's fundamentally the wrong thing to do to put the tags in the
test sources themselves. It should be enough to look at the commit churn
we had to move tests to/from BAT before we had test lists to be
convinced. You shouldn't have to modify the tests to tag/untag them.

You'll also find it'll be an interesting and needed feature to be able
to have tags that aren't committed to the public repo. This would fail
completely if you needed to have commits touching all the tests you need
to tag. And we don't want to maintain both the test list and the tagging
thing.

This is the "I told you so" mail I'll reference in the future.


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