[igt-dev] [PATCH v5 i-g-t 0/8] Introduce KUnit

[igt-dev] [PATCH v5 i-g-t 0/8] Introduce KUnit

[Dominik Karol Piatkowski]

This series is a continuation of Isabella's work on introducing
KUnit to IGT.

Sample drm_buddy output:
	Starting subtest: all-tests
	[thread:8003] TAP version 1
	[thread:8003] Executing 6 tests in: drm_buddy
	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_limit
	Dynamic subtest drm_buddy__drm_test_buddy_alloc_limit: SUCCESS (0.000s)
	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_range
	Dynamic subtest drm_buddy__drm_test_buddy_alloc_range: SUCCESS (0.000s)
	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_optimistic
	Dynamic subtest drm_buddy__drm_test_buddy_alloc_optimistic: SUCCESS (0.000s)
	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_pessimistic
	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pessimistic: SUCCESS (0.000s)
	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_smoke
	Dynamic subtest drm_buddy__drm_test_buddy_alloc_smoke: SUCCESS (0.000s)
	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_pathological
	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pathological: SUCCESS (0.000s)
	Starting dynamic subtest: drm_buddy
	Dynamic subtest drm_buddy: SUCCESS (0.000s)
	Subtest all-tests: SUCCESS (0.707s)

The issue of possibility of too many sublevels occurrence is solved
by name concatenation.

Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>

Dominik Karol Piatkowski (2):
  Change logic of ktap parser to run on a thread
  lib/igt_kmod: fix nesting igt_fixture in igt_subtest

Isabella Basso (4):
  lib/igt_kmod: rename kselftest functions to ktest
  lib/igt_kmod.c: check if module is builtin before attempting to unload
    it
  lib/igt_kmod: add compatibility for KUnit
  tests: DRM selftests: switch to KUnit

Mauro Carvalho Chehab (2):
  lib/igt_kmod: place KUnit tests on a subtest
  kunit tests: add an optional name for the selftests

 lib/igt_kmod.c       | 146 +++++++++-
 lib/igt_kmod.h       |  14 +-
 lib/igt_ktap.c       | 615 +++++++++++++++++++++++++++++++++++++++++++
 lib/igt_ktap.h       |  50 ++++
 lib/meson.build      |   1 +
 tests/drm_buddy.c    |   4 +-
 tests/drm_mm.c       |   5 +-
 tests/kms_selftest.c |  19 ++
 8 files changed, 835 insertions(+), 19 deletions(-)
 create mode 100644 lib/igt_ktap.c
 create mode 100644 lib/igt_ktap.h

-- 
2.34.1

[igt-dev] [PATCH i-g-t 1/8] lib/igt_kmod: rename kselftest functions to ktest

[Dominik Karol Piatkowski]

From: Isabella Basso <isabbasso@riseup.net>

This aims at making IGT's structure more general to different kernel
testing frameworks such as KUnit, as they use a lot of the same
functionality.

Reviewed-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Signed-off-by: Isabella Basso <isabbasso@riseup.net>

Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
---
 lib/igt_kmod.c | 22 +++++++++++-----------
 lib/igt_kmod.h | 12 ++++++------
 2 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index ccf0063c..93fa2006 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -749,8 +749,8 @@ static int open_parameters(const char *module_name)
 	return open(path, O_RDONLY);
 }
 
-int igt_kselftest_init(struct igt_kselftest *tst,
-		       const char *module_name)
+int igt_ktest_init(struct igt_ktest *tst,
+		   const char *module_name)
 {
 	int err;
 
@@ -769,7 +769,7 @@ int igt_kselftest_init(struct igt_kselftest *tst,
 	return 0;
 }
 
-int igt_kselftest_begin(struct igt_kselftest *tst)
+int igt_ktest_begin(struct igt_ktest *tst)
 {
 	int err;
 
@@ -784,7 +784,7 @@ int igt_kselftest_begin(struct igt_kselftest *tst)
 	return 0;
 }
 
-int igt_kselftest_execute(struct igt_kselftest *tst,
+int igt_kselftest_execute(struct igt_ktest *tst,
 			  struct igt_kselftest_list *tl,
 			  const char *options,
 			  const char *result)
@@ -822,13 +822,13 @@ int igt_kselftest_execute(struct igt_kselftest *tst,
 	return err;
 }
 
-void igt_kselftest_end(struct igt_kselftest *tst)
+void igt_ktest_end(struct igt_ktest *tst)
 {
 	kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE);
 	close(tst->kmsg);
 }
 
-void igt_kselftest_fini(struct igt_kselftest *tst)
+void igt_ktest_fini(struct igt_ktest *tst)
 {
 	free(tst->module_name);
 	kmod_module_unref(tst->kmod);
@@ -851,15 +851,15 @@ void igt_kselftests(const char *module_name,
 		    const char *result,
 		    const char *filter)
 {
-	struct igt_kselftest tst;
+	struct igt_ktest tst;
 	IGT_LIST_HEAD(tests);
 	struct igt_kselftest_list *tl, *tn;
 
-	if (igt_kselftest_init(&tst, module_name) != 0)
+	if (igt_ktest_init(&tst, module_name) != 0)
 		return;
 
 	igt_fixture
-		igt_require(igt_kselftest_begin(&tst) == 0);
+		igt_require(igt_ktest_begin(&tst) == 0);
 
 	igt_kselftest_get_tests(tst.kmod, filter, &tests);
 	igt_subtest_with_dynamic(filter ?: "all-tests") {
@@ -878,9 +878,9 @@ void igt_kselftests(const char *module_name,
 	}
 
 	igt_fixture {
-		igt_kselftest_end(&tst);
+		igt_ktest_end(&tst);
 		igt_require(!igt_list_empty(&tests));
 	}
 
-	igt_kselftest_fini(&tst);
+	igt_ktest_fini(&tst);
 }
diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
index d05af4a6..ff59f1ec 100644
--- a/lib/igt_kmod.h
+++ b/lib/igt_kmod.h
@@ -76,7 +76,7 @@ void igt_kselftests(const char *module_name,
 		    const char *result_option,
 		    const char *filter);
 
-struct igt_kselftest {
+struct igt_ktest {
 	struct kmod_module *kmod;
 	char *module_name;
 	int kmsg;
@@ -89,19 +89,19 @@ struct igt_kselftest_list {
 	char param[];
 };
 
-int igt_kselftest_init(struct igt_kselftest *tst,
+int igt_ktest_init(struct igt_ktest *tst,
 		       const char *module_name);
-int igt_kselftest_begin(struct igt_kselftest *tst);
+int igt_ktest_begin(struct igt_ktest *tst);
 
 void igt_kselftest_get_tests(struct kmod_module *kmod,
 			     const char *filter,
 			     struct igt_list_head *tests);
-int igt_kselftest_execute(struct igt_kselftest *tst,
+int igt_kselftest_execute(struct igt_ktest *tst,
 			  struct igt_kselftest_list *tl,
 			  const char *module_options,
 			  const char *result);
 
-void igt_kselftest_end(struct igt_kselftest *tst);
-void igt_kselftest_fini(struct igt_kselftest *tst);
+void igt_ktest_end(struct igt_ktest *tst);
+void igt_ktest_fini(struct igt_ktest *tst);
 
 #endif /* IGT_KMOD_H */
-- 
2.34.1

[igt-dev] [PATCH i-g-t 2/8] lib/igt_kmod.c: check if module is builtin before attempting to unload it

[Dominik Karol Piatkowski]

From: Isabella Basso <isabbasso@riseup.net>

This change makes `igt_kmod_unload_r` safer as it checks whether the
module can be unloaded before attempting it.

v2 -> v3:
- Fix commit message
- Make return value clearer

Acked-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Signed-off-by: Isabella Basso <isabbasso@riseup.net>

Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
---
 lib/igt_kmod.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 93fa2006..26d58e29 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -259,6 +259,9 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
 	int err, tries;
 	const char *mod_name = kmod_module_get_name(kmod);
 
+	if (kmod_module_get_initstate(kmod) == KMOD_MODULE_BUILTIN)
+		return 0;
+
 	holders = kmod_module_get_holders(kmod);
 	kmod_list_foreach(pos, holders) {
 		struct kmod_module *it = kmod_module_get_module(pos);
-- 
2.34.1

[igt-dev] [PATCH i-g-t 3/8] lib/igt_kmod: add compatibility for KUnit

[Dominik Karol Piatkowski]

From: Isabella Basso <isabbasso@riseup.net>

This adds functions for both executing the tests as well as parsing (K)TAP
kmsg output, as per the KTAP spec [1].

[1] https://www.kernel.org/doc/html/latest/dev-tools/ktap.html

v1 -> v2:
- refactor igt_kunit function and ktap parser so that we have only one
  parser that we call only once (code size is now less than half the
  size as v1)
- add lookup_value helper
- fix parsing problems
v2 -> v3:
- move ktap parsing functions to own file
- rename to ktap_parser
- get rid of unneeded pointers in igt_kunit
- change return values to allow for subsequent call to igt_kselftests if
  needed
- add docs to parsing functions and helpers
- switch to line buffering
- add line buffering logging helper
- fix kunit module handling
- fix parsing of version lines
- use igt_subtest blocks to improve output handling on the CI
- fix output handling during crashes

Signed-off-by: Isabella Basso <isabbasso@riseup.net>

v3 -> v4:
- handle igt_ktap_parser fail with IGT_EXIT_ABORT code

v4 -> v5:
- added missing newlines in igt_warn
- removed setvbuf

Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
---
 lib/igt_kmod.c  |  79 ++++++++++++
 lib/igt_kmod.h  |   2 +
 lib/igt_ktap.c  | 334 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/igt_ktap.h  |  31 +++++
 lib/meson.build |   1 +
 5 files changed, 447 insertions(+)
 create mode 100644 lib/igt_ktap.c
 create mode 100644 lib/igt_ktap.h

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 26d58e29..21e801bd 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -29,6 +29,7 @@
 #include "igt_aux.h"
 #include "igt_core.h"
 #include "igt_kmod.h"
+#include "igt_ktap.h"
 #include "igt_sysfs.h"
 #include "igt_taints.h"
 
@@ -744,6 +745,84 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
 	kmod_module_info_free_list(pre);
 }
 
+/**
+ * igt_kunit:
+ * @module_name: the name of the module
+ * @opts: options to load the module
+ *
+ * Loads the test module, parses its (k)tap dmesg output, then unloads it
+ *
+ * Returns: IGT default codes
+ */
+int igt_kunit(const char *module_name, const char *opts)
+{
+	struct igt_ktest tst;
+	struct kmod_module *kunit_kmod;
+	char record[BUF_LEN + 1];
+	FILE *f;
+	bool is_builtin;
+	int ret;
+
+	ret = IGT_EXIT_INVALID;
+
+	/* get normalized module name */
+	if (igt_ktest_init(&tst, module_name) != 0) {
+		igt_warn("Unable to initialize ktest for %s\n", module_name);
+		return ret;
+	}
+
+	if (igt_ktest_begin(&tst) != 0) {
+		igt_warn("Unable to begin ktest for %s\n", module_name);
+
+		igt_ktest_fini(&tst);
+		return ret;
+	}
+
+	if (tst.kmsg < 0) {
+		igt_warn("Could not open /dev/kmsg\n");
+		goto unload;
+	}
+
+	if (lseek(tst.kmsg, 0, SEEK_END)) {
+		igt_warn("Could not seek the end of /dev/kmsg\n");
+		goto unload;
+	}
+
+	f = fdopen(tst.kmsg, "r");
+
+	if (f == NULL) {
+		igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
+		goto unload;
+	}
+
+	/* The KUnit module is required for running any KUnit tests */
+	if (igt_kmod_load("kunit", NULL) != 0 ||
+	    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
+		igt_warn("Unable to load KUnit\n");
+		igt_fail(IGT_EXIT_FAILURE);
+	}
+
+	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
+
+	if (igt_kmod_load(module_name, opts) != 0) {
+		igt_warn("Unable to load %s module\n", module_name);
+		igt_fail(IGT_EXIT_FAILURE);
+	}
+
+	ret = igt_ktap_parser(f, record, is_builtin);
+	if (ret != 0)
+		ret = IGT_EXIT_ABORT;
+unload:
+	igt_ktest_end(&tst);
+
+	igt_ktest_fini(&tst);
+
+	if (ret == 0)
+		igt_success();
+
+	return ret;
+}
+
 static int open_parameters(const char *module_name)
 {
 	char path[256];
diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
index ff59f1ec..ce17c714 100644
--- a/lib/igt_kmod.h
+++ b/lib/igt_kmod.h
@@ -71,6 +71,8 @@ static inline int igt_xe_driver_unload(void)
 int igt_amdgpu_driver_load(const char *opts);
 int igt_amdgpu_driver_unload(void);
 
+int igt_kunit(const char *module_name, const char *opts);
+
 void igt_kselftests(const char *module_name,
 		    const char *module_options,
 		    const char *result_option,
diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
new file mode 100644
index 00000000..117598fa
--- /dev/null
+++ b/lib/igt_ktap.c
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2023 Isabella Basso do Amaral <isabbasso@riseup.net>
+ */
+
+#include <ctype.h>
+#include <limits.h>
+
+#include "igt_aux.h"
+#include "igt_core.h"
+#include "igt_ktap.h"
+
+static int log_to_end(enum igt_log_level level, FILE *f,
+		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
+
+/**
+ * log_to_end:
+ * @level: #igt_log_level
+ * @record: record to store the read data
+ * @format: format string
+ * @...: optional arguments used in the format string
+ *
+ * This is an altered version of the generic structured logging helper function
+ * igt_log capable of reading to the end of a given line.
+ *
+ * Returns: 0 for success, or -2 if there's an error reading from the file
+ */
+static int log_to_end(enum igt_log_level level, FILE *f,
+		      char *record, const char *format, ...)
+{
+	va_list args;
+	const char *lend;
+
+	va_start(args, format);
+	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
+	va_end(args);
+
+	lend = strchrnul(record, '\n');
+	while (*lend == '\0') {
+		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
+		if (fgets(record, BUF_LEN, f) == NULL) {
+			igt_warn("kmsg truncated: unknown error (%m)\n");
+			return -2;
+		}
+		lend = strchrnul(record, '\n');
+	}
+	return 0;
+}
+
+/**
+ * lookup_value:
+ * @haystack: the string to search in
+ * @needle: the string to search for
+ *
+ * Returns: the value of the needle in the haystack, or -1 if not found.
+ */
+static long lookup_value(const char *haystack, const char *needle)
+{
+	const char *needle_rptr;
+	char *needle_end;
+	long num;
+
+	needle_rptr = strcasestr(haystack, needle);
+
+	if (needle_rptr == NULL)
+		return -1;
+
+	/* skip search string and whitespaces after it */
+	needle_rptr += strlen(needle);
+
+	num = strtol(needle_rptr, &needle_end, 10);
+
+	if (needle_rptr == needle_end)
+		return -1;
+
+	if (num == LONG_MIN || num == LONG_MAX)
+		return 0;
+
+	return num > 0 ? num : 0;
+}
+
+/**
+ * find_next_tap_subtest:
+ * @fp: FILE pointer
+ * @record: buffer used to read fp
+ * @is_builtin: whether KUnit is built-in or not
+ *
+ * Returns:
+ * 0 if there's missing information
+ * -1 if not found
+ * -2 if there are problems while reading the file.
+ * any other value corresponds to the amount of cases of the next (sub)test
+ */
+static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
+{
+	const char *test_lookup_str, *subtest_lookup_str, *name_rptr, *version_rptr;
+	char test_name[BUF_LEN + 1];
+	long test_count;
+
+	test_name[0] = '\0';
+	test_name[BUF_LEN] = '\0';
+
+	test_lookup_str = " subtest: ";
+	subtest_lookup_str = " test: ";
+
+	/*
+	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
+	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
+	 *
+	 * but actually isn't, as it currently depends on the KUnit module
+	 * being built-in, so we can't rely on it every time
+	 */
+	if (is_builtin) {
+		version_rptr = strcasestr(record, "TAP version ");
+		if (version_rptr == NULL)
+			return -1;
+
+		igt_info("%s", version_rptr);
+
+		if (fgets(record, BUF_LEN, fp) == NULL) {
+			igt_warn("kmsg truncated: unknown error (%m)\n");
+			return -2;
+		}
+	}
+
+	name_rptr = strcasestr(record, test_lookup_str);
+	if (name_rptr != NULL) {
+		name_rptr += strlen(test_lookup_str);
+	} else {
+		name_rptr = strcasestr(record, subtest_lookup_str);
+		if (name_rptr != NULL)
+			name_rptr += strlen(subtest_lookup_str);
+	}
+
+	if (name_rptr == NULL) {
+		if (!is_builtin)
+			/* we've probably found nothing */
+			return -1;
+		igt_info("Missing test name\n");
+	} else {
+		strncpy(test_name, name_rptr, BUF_LEN);
+		if (fgets(record, BUF_LEN, fp) == NULL) {
+			igt_warn("kmsg truncated: unknown error (%m)\n");
+			return -2;
+		}
+		/* now we can be sure we found tests */
+		if (!is_builtin)
+			igt_info("KUnit is not built-in, skipping version check...\n");
+	}
+
+	/*
+	 * total test count will almost always appear as 0..N at the beginning
+	 * of a run, so we use it to reliably identify a new run
+	 */
+	test_count = lookup_value(record, "..");
+
+	if (test_count <= 0) {
+		igt_info("Missing test count\n");
+		if (test_name[0] == '\0')
+			return 0;
+		if (log_to_end(IGT_LOG_INFO, fp, record,
+				"Running some tests in: %s",
+				test_name) < 0)
+			return -2;
+		return 0;
+	} else if (test_name[0] == '\0') {
+		igt_info("Running %ld tests...\n", test_count);
+		return 0;
+	}
+
+	if (log_to_end(IGT_LOG_INFO, fp, record,
+			"Executing %ld tests in: %s",
+			test_count, test_name) < 0)
+		return -2;
+
+	return test_count;
+}
+
+/**
+ * find_next_tap_test:
+ * @fp: FILE pointer
+ * @record: buffer used to read fp
+ * @test_name: buffer to store the test name
+ *
+ * Returns:
+ * 1 if no results were found
+ * 0 if a test succeded
+ * -1 if a test failed
+ * -2 if there are problems reading the file
+ */
+static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
+{
+	const char *lstart, *ok_lookup_str, *nok_lookup_str,
+	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
+	char *test_name_end;
+
+	ok_lookup_str = "ok ";
+	nok_lookup_str = "not ok ";
+
+	lstart = strchrnul(record, ';');
+
+	if (*lstart == '\0') {
+		igt_warn("kmsg truncated: output malformed (%m)\n");
+		return -2;
+	}
+
+	lstart++;
+	while (isspace(*lstart))
+		lstart++;
+
+	nok_rptr = strstr(lstart, nok_lookup_str);
+	if (nok_rptr != NULL) {
+		nok_rptr += strlen(nok_lookup_str);
+		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
+			nok_rptr++;
+		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
+		while (!isspace(*test_name_end))
+			test_name_end++;
+		*test_name_end = '\0';
+		if (log_to_end(IGT_LOG_WARN, fp, record,
+			       "%s", lstart) < 0)
+			return -2;
+		return -1;
+	}
+
+	comment_start = strchrnul(lstart, '#');
+
+	/* check if we're still in a subtest */
+	if (*comment_start != '\0') {
+		comment_start++;
+		value_parse_start = comment_start;
+
+		if (lookup_value(value_parse_start, "fail: ") > 0) {
+			if (log_to_end(IGT_LOG_WARN, fp, record,
+				       "%s", lstart) < 0)
+				return -2;
+			return -1;
+		}
+	}
+
+	ok_rptr = strstr(lstart, ok_lookup_str);
+	if (ok_rptr != NULL) {
+		ok_rptr += strlen(ok_lookup_str);
+		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
+			ok_rptr++;
+		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
+		while (!isspace(*test_name_end))
+			test_name_end++;
+		*test_name_end = '\0';
+		return 0;
+	}
+
+	return 1;
+}
+
+/**
+ * igt_ktap_parser:
+ * @fp: FILE pointer
+ * @record: buffer used to read fp
+ * @is_builtin: whether the KUnit module is built-in or not
+ *
+ * This function parses the output of a ktap script and prints the test results,
+ * as well as any other output to stdout.
+ *
+ * Returns: IGT default codes
+ */
+int igt_ktap_parser(FILE *fp, char *record, bool is_builtin)
+{
+	char test_name[BUF_LEN + 1];
+	bool failed_tests, found_tests;
+	int sublevel = 0;
+
+	test_name[0] = '\0';
+	test_name[BUF_LEN] = '\0';
+
+	failed_tests = false;
+	found_tests = false;
+
+	while (sublevel >= 0) {
+		if (fgets(record, BUF_LEN, fp) == NULL) {
+			if (!found_tests)
+				igt_warn("kmsg truncated: unknown error (%m)\n");
+			break;
+		}
+
+		switch (find_next_tap_subtest(fp, record, is_builtin)) {
+		case -2:
+			/* no more data to read */
+			return IGT_EXIT_FAILURE;
+		case -1:
+			/* no test found, so we keep parsing */
+			break;
+		case 0:
+			/*
+			 * tests found, but they're missing info, so we might
+			 * have read into test output
+			 */
+			found_tests = true;
+			sublevel++;
+			break;
+		default:
+			if (fgets(record, BUF_LEN, fp) == NULL) {
+				igt_warn("kmsg truncated: unknown error (%m)\n");
+				return -2;
+			}
+			found_tests = true;
+			sublevel++;
+			break;
+		}
+
+		switch (parse_kmsg_for_tap(fp, record, test_name)) {
+		case -2:
+			return IGT_EXIT_FAILURE;
+		case -1:
+			sublevel--;
+			failed_tests = true;
+			igt_subtest(test_name)
+				igt_fail(IGT_EXIT_FAILURE);
+			test_name[0] = '\0';
+			break;
+		case 0: /* fallthrough */
+			igt_subtest(test_name)
+				igt_success();
+			test_name[0] = '\0';
+		default:
+			break;
+		}
+	}
+
+	if (failed_tests || !found_tests)
+		return IGT_EXIT_FAILURE;
+
+	return IGT_EXIT_SUCCESS;
+}
diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
new file mode 100644
index 00000000..b2f69df2
--- /dev/null
+++ b/lib/igt_ktap.h
@@ -0,0 +1,31 @@
+/*
+ * Copyright © 2022 Isabella Basso do Amaral <isabbasso@riseup.net>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#ifndef IGT_KTAP_H
+#define IGT_KTAP_H
+
+#define BUF_LEN 4096
+
+int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
+
+#endif /* IGT_KTAP_H */
diff --git a/lib/meson.build b/lib/meson.build
index 55efdc83..1b33ea91 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -88,6 +88,7 @@ lib_sources = [
 	'igt_store.c',
 	'uwildmat/uwildmat.c',
 	'igt_kmod.c',
+	'igt_ktap.c',
 	'igt_panfrost.c',
 	'igt_v3d.c',
 	'igt_vc4.c',
-- 
2.34.1

[igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Dominik Karol Piatkowski]

From: Isabella Basso <isabbasso@riseup.net>

As the DRM selftests are now using KUnit [1], update IGT tests as well.

[1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/

Signed-off-by: Isabella Basso <isabbasso@riseup.net>

v1 -> v2:
- drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
  with code other than IGT_EXIT_ABORT
- kms_selftest: move igt_kunit tests to separate subtests
- kms_selftest: fallback to igt_kselftests if all subtests failed

v2 -> v3:
- expose all subtests

Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
---
 tests/drm_buddy.c    | 4 +++-
 tests/drm_mm.c       | 4 +++-
 tests/kms_selftest.c | 8 ++++++++
 3 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
index 06876e0c..3261f0d6 100644
--- a/tests/drm_buddy.c
+++ b/tests/drm_buddy.c
@@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's buddy allocator (struct drm_bu
 
 igt_main
 {
-	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
+	int ret = igt_kunit("drm_buddy_test", NULL);
+	if (ret != 0 && ret != IGT_EXIT_ABORT)
+		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
 }
diff --git a/tests/drm_mm.c b/tests/drm_mm.c
index 0bce7139..88f76a57 100644
--- a/tests/drm_mm.c
+++ b/tests/drm_mm.c
@@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's range manager (struct drm_mm)"
 
 igt_main
 {
-	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
+	int ret = igt_kunit("drm_mm_test", NULL);
+	if (ret != 0 && ret != IGT_EXIT_ABORT)
+		igt_kselftests("test-drm_mm", NULL, NULL, NULL);
 }
diff --git a/tests/kms_selftest.c b/tests/kms_selftest.c
index abc4bfe9..b27f60fb 100644
--- a/tests/kms_selftest.c
+++ b/tests/kms_selftest.c
@@ -28,5 +28,13 @@ IGT_TEST_DESCRIPTION("Basic sanity check of KMS selftests.");
 
 igt_main
 {
+	static const char *kunit_subtests[] = { "drm_cmdline_parser_test", "drm_damage_helper_test",
+						"drm_dp_mst_helper_test", "drm_format_helper_test",
+						"drm_format_test", "drm_framebuffer_test",
+						"drm_plane_helper_test", NULL };
+
+	for (int i = 0; kunit_subtests[i] != NULL; i++)
+		igt_kunit(kunit_subtests[i], NULL);
+
 	igt_kselftests("test-drm_modeset", NULL, NULL, NULL);
 }
-- 
2.34.1

[igt-dev] [PATCH i-g-t 5/8] Change logic of ktap parser to run on a thread

[Dominik Karol Piatkowski]

The ktap parser should be listening and parsing messages as the tests
are executed, and not after the end of module load.

v1 -> v2:
- fix coding style
- remove usleep
- add error check logic
- follow the structure of igt_kselftests more closely

v2 -> v3:
- fixed sublevel issues by rewriting tap parser flow

Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
---
 lib/igt_kmod.c |  99 ++++++++----
 lib/igt_ktap.c | 431 ++++++++++++++++++++++++++++++++++++++++---------
 lib/igt_ktap.h |  21 ++-
 3 files changed, 440 insertions(+), 111 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 21e801bd..8cb9cb2e 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -758,10 +758,11 @@ int igt_kunit(const char *module_name, const char *opts)
 {
 	struct igt_ktest tst;
 	struct kmod_module *kunit_kmod;
-	char record[BUF_LEN + 1];
 	FILE *f;
 	bool is_builtin;
 	int ret;
+	struct ktap_test_results *results;
+	struct ktap_test_results_element *temp;
 
 	ret = IGT_EXIT_INVALID;
 
@@ -771,55 +772,83 @@ int igt_kunit(const char *module_name, const char *opts)
 		return ret;
 	}
 
-	if (igt_ktest_begin(&tst) != 0) {
-		igt_warn("Unable to begin ktest for %s\n", module_name);
+	igt_fixture {
+		if (igt_ktest_begin(&tst) != 0) {
+			igt_warn("Unable to begin ktest for %s\n", module_name);
 
-		igt_ktest_fini(&tst);
-		return ret;
-	}
+			igt_ktest_fini(&tst);
+			return ret;
+		}
 
-	if (tst.kmsg < 0) {
-		igt_warn("Could not open /dev/kmsg\n");
-		goto unload;
-	}
+		if (tst.kmsg < 0) {
+			igt_warn("Could not open /dev/kmsg\n");
+			goto unload;
+		}
 
-	if (lseek(tst.kmsg, 0, SEEK_END)) {
-		igt_warn("Could not seek the end of /dev/kmsg\n");
-		goto unload;
-	}
+		if (lseek(tst.kmsg, 0, SEEK_END)) {
+			igt_warn("Could not seek the end of /dev/kmsg\n");
+			goto unload;
+		}
 
-	f = fdopen(tst.kmsg, "r");
+		f = fdopen(tst.kmsg, "r");
 
-	if (f == NULL) {
-		igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
-		goto unload;
-	}
+		if (f == NULL) {
+			igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
+			goto unload;
+		}
+
+		/* The KUnit module is required for running any KUnit tests */
+		if (igt_kmod_load("kunit", NULL) != 0 ||
+		    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
+			igt_warn("Unable to load KUnit\n");
+			igt_fail(IGT_EXIT_FAILURE);
+		}
 
-	/* The KUnit module is required for running any KUnit tests */
-	if (igt_kmod_load("kunit", NULL) != 0 ||
-	    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
-		igt_warn("Unable to load KUnit\n");
-		igt_fail(IGT_EXIT_FAILURE);
+		is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
+
+		results = ktap_parser_start(f, is_builtin);
+
+		if (igt_kmod_load(module_name, opts) != 0) {
+			igt_warn("Unable to load %s module\n", module_name);
+			ret = ktap_parser_stop();
+			igt_fail(IGT_EXIT_FAILURE);
+		}
 	}
 
-	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
+	while (READ_ONCE(results->still_running) || READ_ONCE(results->head) != NULL)
+	{
+		if (READ_ONCE(results->head) != NULL) {
+			pthread_mutex_lock(&results->mutex);
 
-	if (igt_kmod_load(module_name, opts) != 0) {
-		igt_warn("Unable to load %s module\n", module_name);
-		igt_fail(IGT_EXIT_FAILURE);
+			igt_subtest(results->head->test_name) {
+				if (READ_ONCE(results->head->passed))
+					igt_success();
+				else
+					igt_fail(IGT_EXIT_FAILURE);
+			}
+
+			temp = results->head;
+			results->head = results->head->next;
+			free(temp);
+
+			pthread_mutex_unlock(&results->mutex);
+		}
 	}
 
-	ret = igt_ktap_parser(f, record, is_builtin);
-	if (ret != 0)
-		ret = IGT_EXIT_ABORT;
 unload:
-	igt_ktest_end(&tst);
+	igt_fixture {
+		igt_ktest_end(&tst);
 
-	igt_ktest_fini(&tst);
+		igt_ktest_fini(&tst);
+
+		ret = ktap_parser_stop();
 
-	if (ret == 0)
-		igt_success();
+		if (ret != 0)
+			ret = IGT_EXIT_ABORT;
 
+		if (ret == 0)
+			igt_success();
+	}
 	return ret;
 }
 
diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
index 117598fa..d8dec52d 100644
--- a/lib/igt_ktap.c
+++ b/lib/igt_ktap.c
@@ -5,11 +5,25 @@
 
 #include <ctype.h>
 #include <limits.h>
+#include <libkmod.h>
+#include <pthread.h>
+#include <errno.h>
 
 #include "igt_aux.h"
 #include "igt_core.h"
 #include "igt_ktap.h"
 
+#define DELIMITER "__"
+
+struct ktap_parser_args {
+	FILE *fp;
+	bool is_builtin;
+	volatile bool is_running;
+	int ret;
+} ktap_args;
+
+static struct ktap_test_results results;
+
 static int log_to_end(enum igt_log_level level, FILE *f,
 		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
 
@@ -30,6 +44,14 @@ static int log_to_end(enum igt_log_level level, FILE *f,
 {
 	va_list args;
 	const char *lend;
+	int f_fd = fileno(f);
+
+	/* Cutoff after newline character, in order to not display garbage */
+	char *cutoff = strchr(record, '\n');
+	if (cutoff) {
+		if (cutoff - record < BUF_LEN)
+			cutoff[1] = '\0';
+	}
 
 	va_start(args, format);
 	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
@@ -38,10 +60,29 @@ static int log_to_end(enum igt_log_level level, FILE *f,
 	lend = strchrnul(record, '\n');
 	while (*lend == '\0') {
 		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
-		if (fgets(record, BUF_LEN, f) == NULL) {
+
+		while (read(f_fd, record, BUF_LEN) < 0) {
+			if (!READ_ONCE(ktap_args.is_running)) {
+				igt_warn("ktap parser stopped\n");
+				return -2;
+			}
+
+			if (errno == EINTR)
+				continue;
+
+			if (errno == EPIPE) {
+				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
+				return -2;
+			}
+
+			if (errno == EAGAIN)
+				/* No records available */
+				continue;
+
 			igt_warn("kmsg truncated: unknown error (%m)\n");
 			return -2;
 		}
+
 		lend = strchrnul(record, '\n');
 	}
 	return 0;
@@ -65,7 +106,7 @@ static long lookup_value(const char *haystack, const char *needle)
 	if (needle_rptr == NULL)
 		return -1;
 
-	/* skip search string and whitespaces after it */
+	/* Skip search string and whitespaces after it */
 	needle_rptr += strlen(needle);
 
 	num = strtol(needle_rptr, &needle_end, 10);
@@ -79,6 +120,41 @@ static long lookup_value(const char *haystack, const char *needle)
 	return num > 0 ? num : 0;
 }
 
+/**
+ * tap_version_present:
+ * @record: buffer with tap data
+ * @print_info: whether tap version should be printed or not
+ *
+ * Returns:
+ * 0 if not found
+ * 1 if found
+ */
+static int tap_version_present(char* record, bool print_info)
+{
+	/*
+	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
+	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
+	 *
+	 * but actually isn't, as it currently depends on the KUnit module
+	 * being built-in, so we can't rely on it every time
+	 */
+	const char *version_rptr = strcasestr(record, "TAP version ");
+	char *cutoff;
+
+	if (version_rptr == NULL)
+		return 0;
+
+	/* Cutoff after newline character, in order to not display garbage */
+	cutoff = strchr(version_rptr, '\n');
+	if (cutoff)
+		cutoff[0] = '\0';
+
+	if (print_info)
+		igt_info("%s\n", version_rptr);
+
+	return 1;
+}
+
 /**
  * find_next_tap_subtest:
  * @fp: FILE pointer
@@ -91,11 +167,12 @@ static long lookup_value(const char *haystack, const char *needle)
  * -2 if there are problems while reading the file.
  * any other value corresponds to the amount of cases of the next (sub)test
  */
-static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
+static int find_next_tap_subtest(FILE *fp, char *record, char *test_name, bool is_builtin)
 {
-	const char *test_lookup_str, *subtest_lookup_str, *name_rptr, *version_rptr;
-	char test_name[BUF_LEN + 1];
+	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
 	long test_count;
+	int fp_fd = fileno(fp);
+	char *cutoff;
 
 	test_name[0] = '\0';
 	test_name[BUF_LEN] = '\0';
@@ -103,21 +180,28 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
 	test_lookup_str = " subtest: ";
 	subtest_lookup_str = " test: ";
 
-	/*
-	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
-	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
-	 *
-	 * but actually isn't, as it currently depends on the KUnit module
-	 * being built-in, so we can't rely on it every time
-	 */
+	if (!tap_version_present(record, true))
+		return -1;
+
 	if (is_builtin) {
-		version_rptr = strcasestr(record, "TAP version ");
-		if (version_rptr == NULL)
-			return -1;
+		while (read(fp_fd, record, BUF_LEN) < 0) {
+			if (!READ_ONCE(ktap_args.is_running)) {
+				igt_warn("ktap parser stopped\n");
+				return -2;
+			}
+
+			if (errno == EINTR)
+				continue;
 
-		igt_info("%s", version_rptr);
+			if (errno == EPIPE) {
+				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
+				return -2;
+			}
+
+			if (errno == EAGAIN)
+				/* No records available */
+				continue;
 
-		if (fgets(record, BUF_LEN, fp) == NULL) {
 			igt_warn("kmsg truncated: unknown error (%m)\n");
 			return -2;
 		}
@@ -134,22 +218,45 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
 
 	if (name_rptr == NULL) {
 		if (!is_builtin)
-			/* we've probably found nothing */
+			/* We've probably found nothing */
 			return -1;
 		igt_info("Missing test name\n");
 	} else {
 		strncpy(test_name, name_rptr, BUF_LEN);
-		if (fgets(record, BUF_LEN, fp) == NULL) {
+		/* Cutoff after newline character, in order to not display garbage */
+		cutoff = strchr(test_name, '\n');
+		if (cutoff)
+			cutoff[0] = '\0';
+
+		while (read(fp_fd, record, BUF_LEN) < 0) {
+			if (!READ_ONCE(ktap_args.is_running)) {
+				igt_warn("ktap parser stopped\n");
+				return -2;
+			}
+
+			if (errno == EINTR)
+				continue;
+
+			if (errno == EPIPE) {
+				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
+				return -2;
+			}
+
+			if (errno == EAGAIN)
+				/* No records available */
+				continue;
+
 			igt_warn("kmsg truncated: unknown error (%m)\n");
 			return -2;
 		}
-		/* now we can be sure we found tests */
+
+		/* Now we can be sure we found tests */
 		if (!is_builtin)
 			igt_info("KUnit is not built-in, skipping version check...\n");
 	}
 
 	/*
-	 * total test count will almost always appear as 0..N at the beginning
+	 * Total test count will almost always appear as 0..N at the beginning
 	 * of a run, so we use it to reliably identify a new run
 	 */
 	test_count = lookup_value(record, "..");
@@ -159,7 +266,7 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
 		if (test_name[0] == '\0')
 			return 0;
 		if (log_to_end(IGT_LOG_INFO, fp, record,
-				"Running some tests in: %s",
+				"Running some tests in: %s\n",
 				test_name) < 0)
 			return -2;
 		return 0;
@@ -169,7 +276,7 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
 	}
 
 	if (log_to_end(IGT_LOG_INFO, fp, record,
-			"Executing %ld tests in: %s",
+			"Executing %ld tests in: %s\n",
 			test_count, test_name) < 0)
 		return -2;
 
@@ -177,7 +284,7 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
 }
 
 /**
- * find_next_tap_test:
+ * parse_kmsg_for_tap:
  * @fp: FILE pointer
  * @record: buffer used to read fp
  * @test_name: buffer to store the test name
@@ -225,7 +332,7 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
 
 	comment_start = strchrnul(lstart, '#');
 
-	/* check if we're still in a subtest */
+	/* Check if we're still in a subtest */
 	if (*comment_start != '\0') {
 		comment_start++;
 		value_parse_start = comment_start;
@@ -254,81 +361,255 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
 }
 
 /**
- * igt_ktap_parser:
+ * parse_tap_level:
  * @fp: FILE pointer
- * @record: buffer used to read fp
+ * @base_test_name: test_name from upper recursion level
+ * @test_count: test_count of this level
+ * @failed_tests: top level failed_tests pointer
+ * @found_tests: top level found_tests pointer
  * @is_builtin: whether the KUnit module is built-in or not
  *
- * This function parses the output of a ktap script and prints the test results,
- * as well as any other output to stdout.
- *
- * Returns: IGT default codes
+ * Returns:
+ * 0 if succeded
+ * -1 if error occurred
  */
-int igt_ktap_parser(FILE *fp, char *record, bool is_builtin)
+static int parse_tap_level(FILE *fp, char *base_test_name, int test_count, bool *failed_tests,
+			   bool *found_tests, bool is_builtin)
 {
+	int fp_fd = fileno(fp);
+	char record[BUF_LEN + 1];
+	struct ktap_test_results_element *r, *temp;
+	int internal_test_count;
 	char test_name[BUF_LEN + 1];
-	bool failed_tests, found_tests;
-	int sublevel = 0;
+	char base_test_name_for_next_level[BUF_LEN + 1];
 
-	test_name[0] = '\0';
-	test_name[BUF_LEN] = '\0';
+	for (int i = 0; i < test_count; i++) {
+		while (read(fp_fd, record, BUF_LEN) < 0) {
+			if (!READ_ONCE(ktap_args.is_running)) {
+				igt_warn("ktap parser stopped\n");
+				return -1;
+			}
 
-	failed_tests = false;
-	found_tests = false;
+			if (errno == EINTR)
+				continue;
 
-	while (sublevel >= 0) {
-		if (fgets(record, BUF_LEN, fp) == NULL) {
-			if (!found_tests)
-				igt_warn("kmsg truncated: unknown error (%m)\n");
-			break;
+			if (errno == EAGAIN)
+				/* No records available */
+				continue;
+
+			if (errno == EPIPE) {
+				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
+				return -1;
+			}
+
+			igt_warn("kmsg truncated: unknown error (%m)\n");
+			return -1;
 		}
 
-		switch (find_next_tap_subtest(fp, record, is_builtin)) {
-		case -2:
-			/* no more data to read */
-			return IGT_EXIT_FAILURE;
-		case -1:
-			/* no test found, so we keep parsing */
-			break;
-		case 0:
-			/*
-			 * tests found, but they're missing info, so we might
-			 * have read into test output
-			 */
-			found_tests = true;
-			sublevel++;
-			break;
-		default:
-			if (fgets(record, BUF_LEN, fp) == NULL) {
-				igt_warn("kmsg truncated: unknown error (%m)\n");
-				return -2;
+		/* Sublevel found */
+		if (tap_version_present(record, false))
+		{
+			internal_test_count = find_next_tap_subtest(fp, record, test_name,
+								    is_builtin);
+			switch (internal_test_count) {
+			case -2:
+				/* No more data to read */
+				return -1;
+			case -1:
+				/* No test found */
+				return -1;
+			case 0:
+				/* Tests found, but they're missing info */
+				*found_tests = true;
+				return -1;
+			default:
+				*found_tests = true;
+
+				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
+				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
+				    base_test_name_for_next_level[0])
+					strncat(base_test_name_for_next_level, DELIMITER,
+						BUF_LEN - strlen(base_test_name_for_next_level));
+				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
+				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
+
+				if (parse_tap_level(fp, base_test_name_for_next_level,
+						    internal_test_count, failed_tests, found_tests,
+						    is_builtin) == -1)
+					return -1;
+				break;
 			}
-			found_tests = true;
-			sublevel++;
-			break;
 		}
 
 		switch (parse_kmsg_for_tap(fp, record, test_name)) {
 		case -2:
-			return IGT_EXIT_FAILURE;
+			return -1;
 		case -1:
-			sublevel--;
-			failed_tests = true;
-			igt_subtest(test_name)
-				igt_fail(IGT_EXIT_FAILURE);
+			*failed_tests = true;
+
+			r = malloc(sizeof(*r));
+
+			memcpy(r->test_name, base_test_name, BUF_LEN);
+			if (strlen(r->test_name) < BUF_LEN - 1)
+				if (r->test_name[0])
+					strncat(r->test_name, DELIMITER,
+						BUF_LEN - strlen(r->test_name));
+			memcpy(r->test_name + strlen(r->test_name), test_name,
+			       BUF_LEN - strlen(r->test_name));
+			r->test_name[BUF_LEN] = '\0';
+
+			r->passed = false;
+			r->next = NULL;
+
+			pthread_mutex_lock(&results.mutex);
+			if (results.head == NULL) {
+				results.head = r;
+			} else {
+				temp = results.head;
+				while (temp->next != NULL)
+					temp = temp->next;
+				temp->next = r;
+			}
+			pthread_mutex_unlock(&results.mutex);
+
 			test_name[0] = '\0';
 			break;
-		case 0: /* fallthrough */
-			igt_subtest(test_name)
-				igt_success();
+		case 0:
+			r = malloc(sizeof(*r));
+
+			memcpy(r->test_name, base_test_name, BUF_LEN);
+			if (strlen(r->test_name) < BUF_LEN - 1)
+				if (r->test_name[0])
+					strncat(r->test_name, DELIMITER,
+						BUF_LEN - strlen(r->test_name));
+			memcpy(r->test_name + strlen(r->test_name), test_name,
+			       BUF_LEN - strlen(r->test_name));
+			r->test_name[BUF_LEN] = '\0';
+
+			r->passed = true;
+			r->next = NULL;
+
+			pthread_mutex_lock(&results.mutex);
+			if (results.head == NULL) {
+				results.head = r;
+			} else {
+				temp = results.head;
+				while (temp->next != NULL)
+					temp = temp->next;
+				temp->next = r;
+			}
+			pthread_mutex_unlock(&results.mutex);
+
 			test_name[0] = '\0';
+			break;
 		default:
 			break;
 		}
 	}
+	return 0;
+}
+
+/**
+ * igt_ktap_parser:
+ *
+ * This function parses the output of a ktap script and passes it to main thread.
+ */
+void *igt_ktap_parser(void *unused)
+{
+	FILE *fp = ktap_args.fp;
+	int fp_fd = fileno(fp);
+	char record[BUF_LEN + 1];
+	bool is_builtin = ktap_args.is_builtin;
+	char test_name[BUF_LEN + 1];
+	bool failed_tests, found_tests;
+	int test_count;
+
+	failed_tests = false;
+	found_tests = false;
+
+	if (!READ_ONCE(ktap_args.is_running))
+		goto igt_ktap_parser_end;
+
+igt_ktap_parser_start:
+	test_name[0] = '\0';
+	test_name[BUF_LEN] = '\0';
+
+	while (read(fp_fd, record, BUF_LEN) < 0) {
+		if (!READ_ONCE(ktap_args.is_running)) {
+			igt_warn("ktap parser stopped\n");
+			goto igt_ktap_parser_end;
+		}
+
+		if (errno == EAGAIN)
+			/* No records available */
+			continue;
+
+		if (errno == EINTR)
+			continue;
+
+		if (errno == EPIPE) {
+			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
+			goto igt_ktap_parser_end;
+		}
+	}
+
+	test_count = find_next_tap_subtest(fp, record, test_name, is_builtin);
+
+	switch (test_count) {
+	case -2:
+		/* Problems while reading the file */
+		goto igt_ktap_parser_end;
+	case -1:
+		/* No test found */
+		goto igt_ktap_parser_start;
+	case 0:
+		/* Tests found, but they're missing info */
+		found_tests = true;
+		goto igt_ktap_parser_end;
+	default:
+		found_tests = true;
+
+		if (parse_tap_level(fp, test_name, test_count, &failed_tests, &found_tests,
+				    is_builtin) == -1)
+			goto igt_ktap_parser_end;
+
+		break;
+	}
+
+	/* Parse topmost level */
+	test_name[0] = '\0';
+	parse_tap_level(fp, test_name, test_count, &failed_tests, &found_tests, is_builtin);
+
+igt_ktap_parser_end:
+	results.still_running = false;
 
 	if (failed_tests || !found_tests)
-		return IGT_EXIT_FAILURE;
+		ktap_args.ret = IGT_EXIT_FAILURE;
+	else
+		ktap_args.ret = IGT_EXIT_SUCCESS;
+
+	return NULL;
+}
+
+static pthread_t ktap_parser_thread;
+
+struct ktap_test_results *ktap_parser_start(FILE *fp, bool is_builtin)
+{
+	results.head = NULL;
+	pthread_mutex_init(&results.mutex, NULL);
+	results.still_running = true;
 
-	return IGT_EXIT_SUCCESS;
+	ktap_args.fp = fp;
+	ktap_args.is_builtin = is_builtin;
+	ktap_args.is_running = true;
+	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
+
+	return &results;
+}
+
+int ktap_parser_stop(void)
+{
+	ktap_args.is_running = false;
+	pthread_join(ktap_parser_thread, NULL);
+	return ktap_args.ret;
 }
diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
index b2f69df2..34fe0957 100644
--- a/lib/igt_ktap.h
+++ b/lib/igt_ktap.h
@@ -26,6 +26,25 @@
 
 #define BUF_LEN 4096
 
-int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
+#include <pthread.h>
+
+void *igt_ktap_parser(void *unused);
+
+typedef struct ktap_test_results_element {
+	char test_name[BUF_LEN + 1];
+	bool passed;
+	struct ktap_test_results_element *next;
+} ktap_test_results_element;
+
+struct ktap_test_results {
+	ktap_test_results_element *head;
+	pthread_mutex_t mutex;
+	bool still_running;
+};
+
+
+
+struct ktap_test_results *ktap_parser_start(FILE *fp, bool is_builtin);
+int ktap_parser_stop(void);
 
 #endif /* IGT_KTAP_H */
-- 
2.34.1

[igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Dominik Karol Piatkowski]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

There's a hidden bug at KUnit implementation: as it doesn't
place tests inside a subtest, trying to use it with igt_main
causes a crash:

	$ ./build/tests/drm_mm --list
	skipping is allowed only in fixtures, subtests or igt_simple_main
	please refer to lib/igt_core documentation
	drm_mm: ../lib/igt_core.c:437: internal_assert: Assertion `0' failed.
	Received signal SIGABRT.
	Stack trace:
	 #0 [fatal_sig_handler+0x17b]
	 #1 [__sigaction+0x50]
	 #2 [__pthread_kill_implementation+0x114]
	 #3 [gsignal+0x1e]
	 #4 [abort+0xdf]
	 #5 [__assert_fail_base.cold+0xe]
	 #6 [__assert_fail+0x47]
	 #7 [internal_assert+0xe5]
	 #8 [igt_skip+0x169]
	 #9 [__igt_skip_check+0x1bb]
	 #10 [igt_ktest_begin+0xa6]
	 #11 [igt_kunit+0x70]
	 #12 [main+0x2a]
	 #13 [__libc_start_call_main+0x7a]
	 #14 [__libc_start_main+0x8b]
	 #15 [_start+0x25]

Fix it by using igt_subtests() before actually implememnting
KUnit logic.

After the patch, it should now report subtests:

	$ ./build/tests/drm_mm --list
	all-tests

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
---
 lib/igt_kmod.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 8cb9cb2e..1309ab21 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -754,7 +754,7 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
  *
  * Returns: IGT default codes
  */
-int igt_kunit(const char *module_name, const char *opts)
+static int __igt_kunit(const char *module_name, const char *opts)
 {
 	struct igt_ktest tst;
 	struct kmod_module *kunit_kmod;
@@ -852,6 +852,20 @@ unload:
 	return ret;
 }
 
+int igt_kunit(const char *module_name, const char *opts)
+{
+	/*
+	 * We need to use igt_subtest here, as otherwise it may crash with:
+	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
+	 * if used on igt_main. This is also needed in order to provide
+	 * proper namespace for dynamic subtests, with is required for CI
+	 * and for documentation.
+	 */
+	igt_subtest_with_dynamic("all-tests")
+		return __igt_kunit(module_name, opts);
+	return 0;
+}
+
 static int open_parameters(const char *module_name)
 {
 	char path[256];
-- 
2.34.1

[igt-dev] [PATCH i-g-t 7/8] kunit tests: add an optional name for the selftests

[Dominik Karol Piatkowski]

From: Mauro Carvalho Chehab <mchehab@kernel.org>

When multiple KUnit tests are called by the same program, it is
interesting to group them with a name. This would allow IGT
namespace to better refer to the KUnit tests and will give some
filtering capability to it.

After those changes, the IGT kUnit tests will be better named:

	$ for i in kms_selftest drm_buddy drm_mm; do echo $i:; build/tests/$i --list; echo; done
	kms_selftest:
	drm_cmdline
	drm_damage
	drm_dp_mst
	drm_format_helper
	drm_format
	framebuffer
	drm_plane
	all-tests

	drm_buddy:
	all-tests

	drm_mm:
	all-tests

Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
---
 lib/igt_kmod.c       |  7 +++++--
 lib/igt_kmod.h       |  2 +-
 tests/drm_buddy.c    |  2 +-
 tests/drm_mm.c       |  3 ++-
 tests/kms_selftest.c | 23 +++++++++++++++++------
 5 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 1309ab21..c62eb97a 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -852,7 +852,7 @@ unload:
 	return ret;
 }
 
-int igt_kunit(const char *module_name, const char *opts)
+int igt_kunit(const char *module_name, const char *name, const char *opts)
 {
 	/*
 	 * We need to use igt_subtest here, as otherwise it may crash with:
@@ -861,7 +861,10 @@ int igt_kunit(const char *module_name, const char *opts)
 	 * proper namespace for dynamic subtests, with is required for CI
 	 * and for documentation.
 	 */
-	igt_subtest_with_dynamic("all-tests")
+	if (name == NULL)
+		name = "all-tests";
+
+	igt_subtest_with_dynamic(name)
 		return __igt_kunit(module_name, opts);
 	return 0;
 }
diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
index ce17c714..24895547 100644
--- a/lib/igt_kmod.h
+++ b/lib/igt_kmod.h
@@ -71,7 +71,7 @@ static inline int igt_xe_driver_unload(void)
 int igt_amdgpu_driver_load(const char *opts);
 int igt_amdgpu_driver_unload(void);
 
-int igt_kunit(const char *module_name, const char *opts);
+int igt_kunit(const char *module_name, const char *name, const char *opts);
 
 void igt_kselftests(const char *module_name,
 		    const char *module_options,
diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
index 3261f0d6..09feaf63 100644
--- a/tests/drm_buddy.c
+++ b/tests/drm_buddy.c
@@ -10,7 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's buddy allocator (struct drm_bu
 
 igt_main
 {
-	int ret = igt_kunit("drm_buddy_test", NULL);
+	int ret = igt_kunit("drm_buddy_test", NULL, NULL);
 	if (ret != 0 && ret != IGT_EXIT_ABORT)
 		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
 }
diff --git a/tests/drm_mm.c b/tests/drm_mm.c
index 88f76a57..ada8cb93 100644
--- a/tests/drm_mm.c
+++ b/tests/drm_mm.c
@@ -156,7 +156,8 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's range manager (struct drm_mm)"
 
 igt_main
 {
-	int ret = igt_kunit("drm_mm_test", NULL);
+	int ret = igt_kunit("drm_mm_test", NULL, NULL);
+
 	if (ret != 0 && ret != IGT_EXIT_ABORT)
 		igt_kselftests("test-drm_mm", NULL, NULL, NULL);
 }
diff --git a/tests/kms_selftest.c b/tests/kms_selftest.c
index b27f60fb..d83e5ff4 100644
--- a/tests/kms_selftest.c
+++ b/tests/kms_selftest.c
@@ -26,15 +26,26 @@
 
 IGT_TEST_DESCRIPTION("Basic sanity check of KMS selftests.");
 
+struct kms_kunittests {
+	const char *kunit;
+	const char *name;
+};
+
 igt_main
 {
-	static const char *kunit_subtests[] = { "drm_cmdline_parser_test", "drm_damage_helper_test",
-						"drm_dp_mst_helper_test", "drm_format_helper_test",
-						"drm_format_test", "drm_framebuffer_test",
-						"drm_plane_helper_test", NULL };
+	static const struct kms_kunittests kunit_subtests[] = {
+		{ "drm_cmdline_parser_test",	"drm_cmdline" },
+		{ "drm_damage_helper_test",	"drm_damage" },
+		{ "drm_dp_mst_helper_test",	"drm_dp_mst" },
+		{ "drm_format_helper_test",	"drm_format_helper" },
+		{ "drm_format_test",		"drm_format" },
+		{ "drm_framebuffer_test",	"framebuffer" },
+		{ "drm_plane_helper_test",	"drm_plane" },
+		{ NULL, NULL}
+	};
 
-	for (int i = 0; kunit_subtests[i] != NULL; i++)
-		igt_kunit(kunit_subtests[i], NULL);
+	for (int i = 0; kunit_subtests[i].kunit != NULL; i++)
+		igt_kunit(kunit_subtests[i].kunit, kunit_subtests[i].name, NULL);
 
 	igt_kselftests("test-drm_modeset", NULL, NULL, NULL);
 }
-- 
2.34.1

[igt-dev] [PATCH i-g-t 8/8] lib/igt_kmod: fix nesting igt_fixture in igt_subtest

[Dominik Karol Piatkowski]

Fix the following issue:
	$ ./build/tests/drm_buddy
	Starting subtest: all-tests
	nesting igt_fixture in igt_subtest is invalid
	please refer to lib/igt_core documentation

Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
---
 lib/igt_kmod.c | 80 ++++++++++++++++++++++++--------------------------
 1 file changed, 38 insertions(+), 42 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index c62eb97a..2c0cc026 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -772,47 +772,45 @@ static int __igt_kunit(const char *module_name, const char *opts)
 		return ret;
 	}
 
-	igt_fixture {
-		if (igt_ktest_begin(&tst) != 0) {
-			igt_warn("Unable to begin ktest for %s\n", module_name);
+	if (igt_ktest_begin(&tst) != 0) {
+		igt_warn("Unable to begin ktest for %s\n", module_name);
 
-			igt_ktest_fini(&tst);
-			return ret;
-		}
+		igt_ktest_fini(&tst);
+		return ret;
+	}
 
-		if (tst.kmsg < 0) {
-			igt_warn("Could not open /dev/kmsg\n");
-			goto unload;
-		}
+	if (tst.kmsg < 0) {
+		igt_warn("Could not open /dev/kmsg\n");
+		goto unload;
+	}
 
-		if (lseek(tst.kmsg, 0, SEEK_END)) {
-			igt_warn("Could not seek the end of /dev/kmsg\n");
-			goto unload;
-		}
+	if (lseek(tst.kmsg, 0, SEEK_END)) {
+		igt_warn("Could not seek the end of /dev/kmsg\n");
+		goto unload;
+	}
 
-		f = fdopen(tst.kmsg, "r");
+	f = fdopen(tst.kmsg, "r");
 
-		if (f == NULL) {
-			igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
-			goto unload;
-		}
+	if (f == NULL) {
+		igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
+		goto unload;
+	}
 
-		/* The KUnit module is required for running any KUnit tests */
-		if (igt_kmod_load("kunit", NULL) != 0 ||
-		    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
-			igt_warn("Unable to load KUnit\n");
-			igt_fail(IGT_EXIT_FAILURE);
-		}
+	/* The KUnit module is required for running any KUnit tests */
+	if (igt_kmod_load("kunit", NULL) != 0 ||
+	    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
+		igt_warn("Unable to load KUnit\n");
+		igt_fail(IGT_EXIT_FAILURE);
+	}
 
-		is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
+	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
 
-		results = ktap_parser_start(f, is_builtin);
+	results = ktap_parser_start(f, is_builtin);
 
-		if (igt_kmod_load(module_name, opts) != 0) {
-			igt_warn("Unable to load %s module\n", module_name);
-			ret = ktap_parser_stop();
-			igt_fail(IGT_EXIT_FAILURE);
-		}
+	if (igt_kmod_load(module_name, opts) != 0) {
+		igt_warn("Unable to load %s module\n", module_name);
+		ret = ktap_parser_stop();
+		igt_fail(IGT_EXIT_FAILURE);
 	}
 
 	while (READ_ONCE(results->still_running) || READ_ONCE(results->head) != NULL)
@@ -820,7 +818,7 @@ static int __igt_kunit(const char *module_name, const char *opts)
 		if (READ_ONCE(results->head) != NULL) {
 			pthread_mutex_lock(&results->mutex);
 
-			igt_subtest(results->head->test_name) {
+			igt_dynamic(results->head->test_name) {
 				if (READ_ONCE(results->head->passed))
 					igt_success();
 				else
@@ -836,19 +834,17 @@ static int __igt_kunit(const char *module_name, const char *opts)
 	}
 
 unload:
-	igt_fixture {
-		igt_ktest_end(&tst);
+	igt_ktest_end(&tst);
 
-		igt_ktest_fini(&tst);
+	igt_ktest_fini(&tst);
 
-		ret = ktap_parser_stop();
+	ret = ktap_parser_stop();
 
-		if (ret != 0)
-			ret = IGT_EXIT_ABORT;
+	if (ret != 0)
+		ret = IGT_EXIT_ABORT;
 
-		if (ret == 0)
-			igt_success();
-	}
+	if (ret == 0)
+		igt_success();
 	return ret;
 }
 
-- 
2.34.1

Re: [igt-dev] [PATCH i-g-t 1/8] lib/igt_kmod: rename kselftest functions to ktest

[Mauro Carvalho Chehab]

On Mon,  5 Jun 2023 12:47:09 +0200
Dominik Karol Piatkowski <dominik.karol.piatkowski@intel.com> wrote:

> From: Isabella Basso <isabbasso@riseup.net>
> 
> This aims at making IGT's structure more general to different kernel
> testing frameworks such as KUnit, as they use a lot of the same
> functionality.
> 
> Reviewed-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Reviewed-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> ---
>  lib/igt_kmod.c | 22 +++++++++++-----------
>  lib/igt_kmod.h | 12 ++++++------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index ccf0063c..93fa2006 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -749,8 +749,8 @@ static int open_parameters(const char *module_name)
>  	return open(path, O_RDONLY);
>  }
>  
> -int igt_kselftest_init(struct igt_kselftest *tst,
> -		       const char *module_name)
> +int igt_ktest_init(struct igt_ktest *tst,
> +		   const char *module_name)
>  {
>  	int err;
>  
> @@ -769,7 +769,7 @@ int igt_kselftest_init(struct igt_kselftest *tst,
>  	return 0;
>  }
>  
> -int igt_kselftest_begin(struct igt_kselftest *tst)
> +int igt_ktest_begin(struct igt_ktest *tst)
>  {
>  	int err;
>  
> @@ -784,7 +784,7 @@ int igt_kselftest_begin(struct igt_kselftest *tst)
>  	return 0;
>  }
>  
> -int igt_kselftest_execute(struct igt_kselftest *tst,
> +int igt_kselftest_execute(struct igt_ktest *tst,
>  			  struct igt_kselftest_list *tl,
>  			  const char *options,
>  			  const char *result)
> @@ -822,13 +822,13 @@ int igt_kselftest_execute(struct igt_kselftest *tst,
>  	return err;
>  }
>  
> -void igt_kselftest_end(struct igt_kselftest *tst)
> +void igt_ktest_end(struct igt_ktest *tst)
>  {
>  	kmod_module_remove_module(tst->kmod, KMOD_REMOVE_FORCE);
>  	close(tst->kmsg);
>  }
>  
> -void igt_kselftest_fini(struct igt_kselftest *tst)
> +void igt_ktest_fini(struct igt_ktest *tst)
>  {
>  	free(tst->module_name);
>  	kmod_module_unref(tst->kmod);
> @@ -851,15 +851,15 @@ void igt_kselftests(const char *module_name,
>  		    const char *result,
>  		    const char *filter)
>  {
> -	struct igt_kselftest tst;
> +	struct igt_ktest tst;
>  	IGT_LIST_HEAD(tests);
>  	struct igt_kselftest_list *tl, *tn;
>  
> -	if (igt_kselftest_init(&tst, module_name) != 0)
> +	if (igt_ktest_init(&tst, module_name) != 0)
>  		return;
>  
>  	igt_fixture
> -		igt_require(igt_kselftest_begin(&tst) == 0);
> +		igt_require(igt_ktest_begin(&tst) == 0);
>  
>  	igt_kselftest_get_tests(tst.kmod, filter, &tests);
>  	igt_subtest_with_dynamic(filter ?: "all-tests") {
> @@ -878,9 +878,9 @@ void igt_kselftests(const char *module_name,
>  	}
>  
>  	igt_fixture {
> -		igt_kselftest_end(&tst);
> +		igt_ktest_end(&tst);
>  		igt_require(!igt_list_empty(&tests));
>  	}
>  
> -	igt_kselftest_fini(&tst);
> +	igt_ktest_fini(&tst);
>  }
> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> index d05af4a6..ff59f1ec 100644
> --- a/lib/igt_kmod.h
> +++ b/lib/igt_kmod.h
> @@ -76,7 +76,7 @@ void igt_kselftests(const char *module_name,
>  		    const char *result_option,
>  		    const char *filter);
>  
> -struct igt_kselftest {
> +struct igt_ktest {
>  	struct kmod_module *kmod;
>  	char *module_name;
>  	int kmsg;
> @@ -89,19 +89,19 @@ struct igt_kselftest_list {
>  	char param[];
>  };
>  
> -int igt_kselftest_init(struct igt_kselftest *tst,
> +int igt_ktest_init(struct igt_ktest *tst,
>  		       const char *module_name);
> -int igt_kselftest_begin(struct igt_kselftest *tst);
> +int igt_ktest_begin(struct igt_ktest *tst);
>  
>  void igt_kselftest_get_tests(struct kmod_module *kmod,
>  			     const char *filter,
>  			     struct igt_list_head *tests);
> -int igt_kselftest_execute(struct igt_kselftest *tst,
> +int igt_kselftest_execute(struct igt_ktest *tst,
>  			  struct igt_kselftest_list *tl,
>  			  const char *module_options,
>  			  const char *result);
>  
> -void igt_kselftest_end(struct igt_kselftest *tst);
> -void igt_kselftest_fini(struct igt_kselftest *tst);
> +void igt_ktest_end(struct igt_ktest *tst);
> +void igt_ktest_fini(struct igt_ktest *tst);
>  
>  #endif /* IGT_KMOD_H */

Re: [igt-dev] [PATCH i-g-t 2/8] lib/igt_kmod.c: check if module is builtin before attempting to unload it

[Mauro Carvalho Chehab]

On Mon,  5 Jun 2023 12:47:10 +0200
Dominik Karol Piatkowski <dominik.karol.piatkowski@intel.com> wrote:

> From: Isabella Basso <isabbasso@riseup.net>
> 
> This change makes `igt_kmod_unload_r` safer as it checks whether the
> module can be unloaded before attempting it.
> 
> v2 -> v3:
> - Fix commit message
> - Make return value clearer
> 
> Acked-by: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
Acked-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> ---
>  lib/igt_kmod.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 93fa2006..26d58e29 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -259,6 +259,9 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
>  	int err, tries;
>  	const char *mod_name = kmod_module_get_name(kmod);
>  
> +	if (kmod_module_get_initstate(kmod) == KMOD_MODULE_BUILTIN)
> +		return 0;
> +
>  	holders = kmod_module_get_holders(kmod);
>  	kmod_list_foreach(pos, holders) {
>  		struct kmod_module *it = kmod_module_get_module(pos);

Re: [igt-dev] [PATCH i-g-t 3/8] lib/igt_kmod: add compatibility for KUnit

[Mauro Carvalho Chehab]

On Mon,  5 Jun 2023 12:47:11 +0200
Dominik Karol Piatkowski <dominik.karol.piatkowski@intel.com> wrote:

> From: Isabella Basso <isabbasso@riseup.net>
> 
> This adds functions for both executing the tests as well as parsing (K)TAP
> kmsg output, as per the KTAP spec [1].
> 
> [1] https://www.kernel.org/doc/html/latest/dev-tools/ktap.html
> 
> v1 -> v2:
> - refactor igt_kunit function and ktap parser so that we have only one
>   parser that we call only once (code size is now less than half the
>   size as v1)
> - add lookup_value helper
> - fix parsing problems
> v2 -> v3:
> - move ktap parsing functions to own file
> - rename to ktap_parser
> - get rid of unneeded pointers in igt_kunit
> - change return values to allow for subsequent call to igt_kselftests if
>   needed
> - add docs to parsing functions and helpers
> - switch to line buffering
> - add line buffering logging helper
> - fix kunit module handling
> - fix parsing of version lines
> - use igt_subtest blocks to improve output handling on the CI
> - fix output handling during crashes
> 
> Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> 
> v3 -> v4:
> - handle igt_ktap_parser fail with IGT_EXIT_ABORT code
> 
> v4 -> v5:
> - added missing newlines in igt_warn
> - removed setvbuf
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>

Acked-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  lib/igt_kmod.c  |  79 ++++++++++++
>  lib/igt_kmod.h  |   2 +
>  lib/igt_ktap.c  | 334 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_ktap.h  |  31 +++++
>  lib/meson.build |   1 +
>  5 files changed, 447 insertions(+)
>  create mode 100644 lib/igt_ktap.c
>  create mode 100644 lib/igt_ktap.h
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 26d58e29..21e801bd 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -29,6 +29,7 @@
>  #include "igt_aux.h"
>  #include "igt_core.h"
>  #include "igt_kmod.h"
> +#include "igt_ktap.h"
>  #include "igt_sysfs.h"
>  #include "igt_taints.h"
>  
> @@ -744,6 +745,84 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
>  	kmod_module_info_free_list(pre);
>  }
>  
> +/**
> + * igt_kunit:
> + * @module_name: the name of the module
> + * @opts: options to load the module
> + *
> + * Loads the test module, parses its (k)tap dmesg output, then unloads it
> + *
> + * Returns: IGT default codes
> + */
> +int igt_kunit(const char *module_name, const char *opts)
> +{
> +	struct igt_ktest tst;
> +	struct kmod_module *kunit_kmod;
> +	char record[BUF_LEN + 1];
> +	FILE *f;
> +	bool is_builtin;
> +	int ret;
> +
> +	ret = IGT_EXIT_INVALID;
> +
> +	/* get normalized module name */
> +	if (igt_ktest_init(&tst, module_name) != 0) {
> +		igt_warn("Unable to initialize ktest for %s\n", module_name);
> +		return ret;
> +	}
> +
> +	if (igt_ktest_begin(&tst) != 0) {
> +		igt_warn("Unable to begin ktest for %s\n", module_name);
> +
> +		igt_ktest_fini(&tst);
> +		return ret;
> +	}
> +
> +	if (tst.kmsg < 0) {
> +		igt_warn("Could not open /dev/kmsg\n");
> +		goto unload;
> +	}
> +
> +	if (lseek(tst.kmsg, 0, SEEK_END)) {
> +		igt_warn("Could not seek the end of /dev/kmsg\n");
> +		goto unload;
> +	}
> +
> +	f = fdopen(tst.kmsg, "r");
> +
> +	if (f == NULL) {
> +		igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> +		goto unload;
> +	}
> +
> +	/* The KUnit module is required for running any KUnit tests */
> +	if (igt_kmod_load("kunit", NULL) != 0 ||
> +	    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> +		igt_warn("Unable to load KUnit\n");
> +		igt_fail(IGT_EXIT_FAILURE);
> +	}
> +
> +	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> +
> +	if (igt_kmod_load(module_name, opts) != 0) {
> +		igt_warn("Unable to load %s module\n", module_name);
> +		igt_fail(IGT_EXIT_FAILURE);
> +	}
> +
> +	ret = igt_ktap_parser(f, record, is_builtin);
> +	if (ret != 0)
> +		ret = IGT_EXIT_ABORT;
> +unload:
> +	igt_ktest_end(&tst);
> +
> +	igt_ktest_fini(&tst);
> +
> +	if (ret == 0)
> +		igt_success();
> +
> +	return ret;
> +}
> +
>  static int open_parameters(const char *module_name)
>  {
>  	char path[256];
> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> index ff59f1ec..ce17c714 100644
> --- a/lib/igt_kmod.h
> +++ b/lib/igt_kmod.h
> @@ -71,6 +71,8 @@ static inline int igt_xe_driver_unload(void)
>  int igt_amdgpu_driver_load(const char *opts);
>  int igt_amdgpu_driver_unload(void);
>  
> +int igt_kunit(const char *module_name, const char *opts);
> +
>  void igt_kselftests(const char *module_name,
>  		    const char *module_options,
>  		    const char *result_option,
> diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> new file mode 100644
> index 00000000..117598fa
> --- /dev/null
> +++ b/lib/igt_ktap.c
> @@ -0,0 +1,334 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Isabella Basso do Amaral <isabbasso@riseup.net>
> + */
> +
> +#include <ctype.h>
> +#include <limits.h>
> +
> +#include "igt_aux.h"
> +#include "igt_core.h"
> +#include "igt_ktap.h"
> +
> +static int log_to_end(enum igt_log_level level, FILE *f,
> +		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
> +
> +/**
> + * log_to_end:
> + * @level: #igt_log_level
> + * @record: record to store the read data
> + * @format: format string
> + * @...: optional arguments used in the format string
> + *
> + * This is an altered version of the generic structured logging helper function
> + * igt_log capable of reading to the end of a given line.
> + *
> + * Returns: 0 for success, or -2 if there's an error reading from the file
> + */
> +static int log_to_end(enum igt_log_level level, FILE *f,
> +		      char *record, const char *format, ...)
> +{
> +	va_list args;
> +	const char *lend;
> +
> +	va_start(args, format);
> +	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
> +	va_end(args);
> +
> +	lend = strchrnul(record, '\n');
> +	while (*lend == '\0') {
> +		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
> +		if (fgets(record, BUF_LEN, f) == NULL) {
> +			igt_warn("kmsg truncated: unknown error (%m)\n");
> +			return -2;
> +		}
> +		lend = strchrnul(record, '\n');
> +	}
> +	return 0;
> +}
> +
> +/**
> + * lookup_value:
> + * @haystack: the string to search in
> + * @needle: the string to search for
> + *
> + * Returns: the value of the needle in the haystack, or -1 if not found.
> + */
> +static long lookup_value(const char *haystack, const char *needle)
> +{
> +	const char *needle_rptr;
> +	char *needle_end;
> +	long num;
> +
> +	needle_rptr = strcasestr(haystack, needle);
> +
> +	if (needle_rptr == NULL)
> +		return -1;
> +
> +	/* skip search string and whitespaces after it */
> +	needle_rptr += strlen(needle);
> +
> +	num = strtol(needle_rptr, &needle_end, 10);
> +
> +	if (needle_rptr == needle_end)
> +		return -1;
> +
> +	if (num == LONG_MIN || num == LONG_MAX)
> +		return 0;
> +
> +	return num > 0 ? num : 0;
> +}
> +
> +/**
> + * find_next_tap_subtest:
> + * @fp: FILE pointer
> + * @record: buffer used to read fp
> + * @is_builtin: whether KUnit is built-in or not
> + *
> + * Returns:
> + * 0 if there's missing information
> + * -1 if not found
> + * -2 if there are problems while reading the file.
> + * any other value corresponds to the amount of cases of the next (sub)test
> + */
> +static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
> +{
> +	const char *test_lookup_str, *subtest_lookup_str, *name_rptr, *version_rptr;
> +	char test_name[BUF_LEN + 1];
> +	long test_count;
> +
> +	test_name[0] = '\0';
> +	test_name[BUF_LEN] = '\0';
> +
> +	test_lookup_str = " subtest: ";
> +	subtest_lookup_str = " test: ";
> +
> +	/*
> +	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> +	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
> +	 *
> +	 * but actually isn't, as it currently depends on the KUnit module
> +	 * being built-in, so we can't rely on it every time
> +	 */
> +	if (is_builtin) {
> +		version_rptr = strcasestr(record, "TAP version ");
> +		if (version_rptr == NULL)
> +			return -1;
> +
> +		igt_info("%s", version_rptr);
> +
> +		if (fgets(record, BUF_LEN, fp) == NULL) {
> +			igt_warn("kmsg truncated: unknown error (%m)\n");
> +			return -2;
> +		}
> +	}
> +
> +	name_rptr = strcasestr(record, test_lookup_str);
> +	if (name_rptr != NULL) {
> +		name_rptr += strlen(test_lookup_str);
> +	} else {
> +		name_rptr = strcasestr(record, subtest_lookup_str);
> +		if (name_rptr != NULL)
> +			name_rptr += strlen(subtest_lookup_str);
> +	}
> +
> +	if (name_rptr == NULL) {
> +		if (!is_builtin)
> +			/* we've probably found nothing */
> +			return -1;
> +		igt_info("Missing test name\n");
> +	} else {
> +		strncpy(test_name, name_rptr, BUF_LEN);
> +		if (fgets(record, BUF_LEN, fp) == NULL) {
> +			igt_warn("kmsg truncated: unknown error (%m)\n");
> +			return -2;
> +		}
> +		/* now we can be sure we found tests */
> +		if (!is_builtin)
> +			igt_info("KUnit is not built-in, skipping version check...\n");
> +	}
> +
> +	/*
> +	 * total test count will almost always appear as 0..N at the beginning
> +	 * of a run, so we use it to reliably identify a new run
> +	 */
> +	test_count = lookup_value(record, "..");
> +
> +	if (test_count <= 0) {
> +		igt_info("Missing test count\n");
> +		if (test_name[0] == '\0')
> +			return 0;
> +		if (log_to_end(IGT_LOG_INFO, fp, record,
> +				"Running some tests in: %s",
> +				test_name) < 0)
> +			return -2;
> +		return 0;
> +	} else if (test_name[0] == '\0') {
> +		igt_info("Running %ld tests...\n", test_count);
> +		return 0;
> +	}
> +
> +	if (log_to_end(IGT_LOG_INFO, fp, record,
> +			"Executing %ld tests in: %s",
> +			test_count, test_name) < 0)
> +		return -2;
> +
> +	return test_count;
> +}
> +
> +/**
> + * find_next_tap_test:
> + * @fp: FILE pointer
> + * @record: buffer used to read fp
> + * @test_name: buffer to store the test name
> + *
> + * Returns:
> + * 1 if no results were found
> + * 0 if a test succeded
> + * -1 if a test failed
> + * -2 if there are problems reading the file
> + */
> +static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
> +{
> +	const char *lstart, *ok_lookup_str, *nok_lookup_str,
> +	      *ok_rptr, *nok_rptr, *comment_start, *value_parse_start;
> +	char *test_name_end;
> +
> +	ok_lookup_str = "ok ";
> +	nok_lookup_str = "not ok ";
> +
> +	lstart = strchrnul(record, ';');
> +
> +	if (*lstart == '\0') {
> +		igt_warn("kmsg truncated: output malformed (%m)\n");
> +		return -2;
> +	}
> +
> +	lstart++;
> +	while (isspace(*lstart))
> +		lstart++;
> +
> +	nok_rptr = strstr(lstart, nok_lookup_str);
> +	if (nok_rptr != NULL) {
> +		nok_rptr += strlen(nok_lookup_str);
> +		while (isdigit(*nok_rptr) || isspace(*nok_rptr) || *nok_rptr == '-')
> +			nok_rptr++;
> +		test_name_end = strncpy(test_name, nok_rptr, BUF_LEN);
> +		while (!isspace(*test_name_end))
> +			test_name_end++;
> +		*test_name_end = '\0';
> +		if (log_to_end(IGT_LOG_WARN, fp, record,
> +			       "%s", lstart) < 0)
> +			return -2;
> +		return -1;
> +	}
> +
> +	comment_start = strchrnul(lstart, '#');
> +
> +	/* check if we're still in a subtest */
> +	if (*comment_start != '\0') {
> +		comment_start++;
> +		value_parse_start = comment_start;
> +
> +		if (lookup_value(value_parse_start, "fail: ") > 0) {
> +			if (log_to_end(IGT_LOG_WARN, fp, record,
> +				       "%s", lstart) < 0)
> +				return -2;
> +			return -1;
> +		}
> +	}
> +
> +	ok_rptr = strstr(lstart, ok_lookup_str);
> +	if (ok_rptr != NULL) {
> +		ok_rptr += strlen(ok_lookup_str);
> +		while (isdigit(*ok_rptr) || isspace(*ok_rptr) || *ok_rptr == '-')
> +			ok_rptr++;
> +		test_name_end = strncpy(test_name, ok_rptr, BUF_LEN);
> +		while (!isspace(*test_name_end))
> +			test_name_end++;
> +		*test_name_end = '\0';
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +/**
> + * igt_ktap_parser:
> + * @fp: FILE pointer
> + * @record: buffer used to read fp
> + * @is_builtin: whether the KUnit module is built-in or not
> + *
> + * This function parses the output of a ktap script and prints the test results,
> + * as well as any other output to stdout.
> + *
> + * Returns: IGT default codes
> + */
> +int igt_ktap_parser(FILE *fp, char *record, bool is_builtin)
> +{
> +	char test_name[BUF_LEN + 1];
> +	bool failed_tests, found_tests;
> +	int sublevel = 0;
> +
> +	test_name[0] = '\0';
> +	test_name[BUF_LEN] = '\0';
> +
> +	failed_tests = false;
> +	found_tests = false;
> +
> +	while (sublevel >= 0) {
> +		if (fgets(record, BUF_LEN, fp) == NULL) {
> +			if (!found_tests)
> +				igt_warn("kmsg truncated: unknown error (%m)\n");
> +			break;
> +		}
> +
> +		switch (find_next_tap_subtest(fp, record, is_builtin)) {
> +		case -2:
> +			/* no more data to read */
> +			return IGT_EXIT_FAILURE;
> +		case -1:
> +			/* no test found, so we keep parsing */
> +			break;
> +		case 0:
> +			/*
> +			 * tests found, but they're missing info, so we might
> +			 * have read into test output
> +			 */
> +			found_tests = true;
> +			sublevel++;
> +			break;
> +		default:
> +			if (fgets(record, BUF_LEN, fp) == NULL) {
> +				igt_warn("kmsg truncated: unknown error (%m)\n");
> +				return -2;
> +			}
> +			found_tests = true;
> +			sublevel++;
> +			break;
> +		}
> +
> +		switch (parse_kmsg_for_tap(fp, record, test_name)) {
> +		case -2:
> +			return IGT_EXIT_FAILURE;
> +		case -1:
> +			sublevel--;
> +			failed_tests = true;
> +			igt_subtest(test_name)
> +				igt_fail(IGT_EXIT_FAILURE);
> +			test_name[0] = '\0';
> +			break;
> +		case 0: /* fallthrough */
> +			igt_subtest(test_name)
> +				igt_success();
> +			test_name[0] = '\0';
> +		default:
> +			break;
> +		}
> +	}
> +
> +	if (failed_tests || !found_tests)
> +		return IGT_EXIT_FAILURE;
> +
> +	return IGT_EXIT_SUCCESS;
> +}
> diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> new file mode 100644
> index 00000000..b2f69df2
> --- /dev/null
> +++ b/lib/igt_ktap.h
> @@ -0,0 +1,31 @@
> +/*
> + * Copyright © 2022 Isabella Basso do Amaral <isabbasso@riseup.net>
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef IGT_KTAP_H
> +#define IGT_KTAP_H
> +
> +#define BUF_LEN 4096
> +
> +int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
> +
> +#endif /* IGT_KTAP_H */
> diff --git a/lib/meson.build b/lib/meson.build
> index 55efdc83..1b33ea91 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -88,6 +88,7 @@ lib_sources = [
>  	'igt_store.c',
>  	'uwildmat/uwildmat.c',
>  	'igt_kmod.c',
> +	'igt_ktap.c',
>  	'igt_panfrost.c',
>  	'igt_v3d.c',
>  	'igt_vc4.c',

Re: [igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Mauro Carvalho Chehab]

On Mon,  5 Jun 2023 12:47:12 +0200
Dominik Karol Piatkowski <dominik.karol.piatkowski@intel.com> wrote:

> From: Isabella Basso <isabbasso@riseup.net>
> 
> As the DRM selftests are now using KUnit [1], update IGT tests as well.
> 
> [1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/
> 
> Signed-off-by: Isabella Basso <isabbasso@riseup.net>

Acked-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> 
> v1 -> v2:
> - drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
>   with code other than IGT_EXIT_ABORT
> - kms_selftest: move igt_kunit tests to separate subtests
> - kms_selftest: fallback to igt_kselftests if all subtests failed
> 
> v2 -> v3:
> - expose all subtests
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> ---
>  tests/drm_buddy.c    | 4 +++-
>  tests/drm_mm.c       | 4 +++-
>  tests/kms_selftest.c | 8 ++++++++
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
> index 06876e0c..3261f0d6 100644
> --- a/tests/drm_buddy.c
> +++ b/tests/drm_buddy.c
> @@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's buddy allocator (struct drm_bu
>  
>  igt_main
>  {
> -	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> +	int ret = igt_kunit("drm_buddy_test", NULL);
> +	if (ret != 0 && ret != IGT_EXIT_ABORT)

Nit: Kernel style asks for a blank like after variable initialization.

> +		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
>  }
> diff --git a/tests/drm_mm.c b/tests/drm_mm.c
> index 0bce7139..88f76a57 100644
> --- a/tests/drm_mm.c
> +++ b/tests/drm_mm.c
> @@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's range manager (struct drm_mm)"
>  
>  igt_main
>  {
> -	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> +	int ret = igt_kunit("drm_mm_test", NULL);
> +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> +		igt_kselftests("test-drm_mm", NULL, NULL, NULL);
>  }
> diff --git a/tests/kms_selftest.c b/tests/kms_selftest.c
> index abc4bfe9..b27f60fb 100644
> --- a/tests/kms_selftest.c
> +++ b/tests/kms_selftest.c
> @@ -28,5 +28,13 @@ IGT_TEST_DESCRIPTION("Basic sanity check of KMS selftests.");
>  
>  igt_main
>  {
> +	static const char *kunit_subtests[] = { "drm_cmdline_parser_test", "drm_damage_helper_test",
> +						"drm_dp_mst_helper_test", "drm_format_helper_test",
> +						"drm_format_test", "drm_framebuffer_test",
> +						"drm_plane_helper_test", NULL };
> +
> +	for (int i = 0; kunit_subtests[i] != NULL; i++)
> +		igt_kunit(kunit_subtests[i], NULL);
> +
>  	igt_kselftests("test-drm_modeset", NULL, NULL, NULL);
>  }

Re: [igt-dev] [PATCH i-g-t 5/8] Change logic of ktap parser to run on a thread

[Mauro Carvalho Chehab]

On Mon,  5 Jun 2023 12:47:13 +0200
Dominik Karol Piatkowski <dominik.karol.piatkowski@intel.com> wrote:

> The ktap parser should be listening and parsing messages as the tests
> are executed, and not after the end of module load.
> 
> v1 -> v2:
> - fix coding style
> - remove usleep
> - add error check logic
> - follow the structure of igt_kselftests more closely
> 
> v2 -> v3:
> - fixed sublevel issues by rewriting tap parser flow
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>

Acked-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  lib/igt_kmod.c |  99 ++++++++----
>  lib/igt_ktap.c | 431 ++++++++++++++++++++++++++++++++++++++++---------
>  lib/igt_ktap.h |  21 ++-
>  3 files changed, 440 insertions(+), 111 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 21e801bd..8cb9cb2e 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -758,10 +758,11 @@ int igt_kunit(const char *module_name, const char *opts)
>  {
>  	struct igt_ktest tst;
>  	struct kmod_module *kunit_kmod;
> -	char record[BUF_LEN + 1];
>  	FILE *f;
>  	bool is_builtin;
>  	int ret;
> +	struct ktap_test_results *results;
> +	struct ktap_test_results_element *temp;
>  
>  	ret = IGT_EXIT_INVALID;
>  
> @@ -771,55 +772,83 @@ int igt_kunit(const char *module_name, const char *opts)
>  		return ret;
>  	}
>  
> -	if (igt_ktest_begin(&tst) != 0) {
> -		igt_warn("Unable to begin ktest for %s\n", module_name);
> +	igt_fixture {
> +		if (igt_ktest_begin(&tst) != 0) {
> +			igt_warn("Unable to begin ktest for %s\n", module_name);
>  
> -		igt_ktest_fini(&tst);
> -		return ret;
> -	}
> +			igt_ktest_fini(&tst);
> +			return ret;
> +		}
>  
> -	if (tst.kmsg < 0) {
> -		igt_warn("Could not open /dev/kmsg\n");
> -		goto unload;
> -	}
> +		if (tst.kmsg < 0) {
> +			igt_warn("Could not open /dev/kmsg\n");
> +			goto unload;
> +		}
>  
> -	if (lseek(tst.kmsg, 0, SEEK_END)) {
> -		igt_warn("Could not seek the end of /dev/kmsg\n");
> -		goto unload;
> -	}
> +		if (lseek(tst.kmsg, 0, SEEK_END)) {
> +			igt_warn("Could not seek the end of /dev/kmsg\n");
> +			goto unload;
> +		}
>  
> -	f = fdopen(tst.kmsg, "r");
> +		f = fdopen(tst.kmsg, "r");
>  
> -	if (f == NULL) {
> -		igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> -		goto unload;
> -	}
> +		if (f == NULL) {
> +			igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> +			goto unload;
> +		}
> +
> +		/* The KUnit module is required for running any KUnit tests */
> +		if (igt_kmod_load("kunit", NULL) != 0 ||
> +		    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> +			igt_warn("Unable to load KUnit\n");
> +			igt_fail(IGT_EXIT_FAILURE);
> +		}
>  
> -	/* The KUnit module is required for running any KUnit tests */
> -	if (igt_kmod_load("kunit", NULL) != 0 ||
> -	    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> -		igt_warn("Unable to load KUnit\n");
> -		igt_fail(IGT_EXIT_FAILURE);
> +		is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> +
> +		results = ktap_parser_start(f, is_builtin);
> +
> +		if (igt_kmod_load(module_name, opts) != 0) {
> +			igt_warn("Unable to load %s module\n", module_name);
> +			ret = ktap_parser_stop();
> +			igt_fail(IGT_EXIT_FAILURE);
> +		}
>  	}
>  
> -	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> +	while (READ_ONCE(results->still_running) || READ_ONCE(results->head) != NULL)
> +	{
> +		if (READ_ONCE(results->head) != NULL) {
> +			pthread_mutex_lock(&results->mutex);
>  
> -	if (igt_kmod_load(module_name, opts) != 0) {
> -		igt_warn("Unable to load %s module\n", module_name);
> -		igt_fail(IGT_EXIT_FAILURE);
> +			igt_subtest(results->head->test_name) {
> +				if (READ_ONCE(results->head->passed))
> +					igt_success();
> +				else
> +					igt_fail(IGT_EXIT_FAILURE);
> +			}
> +
> +			temp = results->head;
> +			results->head = results->head->next;
> +			free(temp);
> +
> +			pthread_mutex_unlock(&results->mutex);
> +		}
>  	}
>  
> -	ret = igt_ktap_parser(f, record, is_builtin);
> -	if (ret != 0)
> -		ret = IGT_EXIT_ABORT;
>  unload:
> -	igt_ktest_end(&tst);
> +	igt_fixture {
> +		igt_ktest_end(&tst);
>  
> -	igt_ktest_fini(&tst);
> +		igt_ktest_fini(&tst);
> +
> +		ret = ktap_parser_stop();
>  
> -	if (ret == 0)
> -		igt_success();
> +		if (ret != 0)
> +			ret = IGT_EXIT_ABORT;
>  
> +		if (ret == 0)
> +			igt_success();
> +	}
>  	return ret;
>  }
>  
> diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> index 117598fa..d8dec52d 100644
> --- a/lib/igt_ktap.c
> +++ b/lib/igt_ktap.c
> @@ -5,11 +5,25 @@
>  
>  #include <ctype.h>
>  #include <limits.h>
> +#include <libkmod.h>
> +#include <pthread.h>
> +#include <errno.h>
>  
>  #include "igt_aux.h"
>  #include "igt_core.h"
>  #include "igt_ktap.h"
>  
> +#define DELIMITER "__"
> +
> +struct ktap_parser_args {
> +	FILE *fp;
> +	bool is_builtin;
> +	volatile bool is_running;
> +	int ret;
> +} ktap_args;
> +
> +static struct ktap_test_results results;
> +
>  static int log_to_end(enum igt_log_level level, FILE *f,
>  		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
>  
> @@ -30,6 +44,14 @@ static int log_to_end(enum igt_log_level level, FILE *f,
>  {
>  	va_list args;
>  	const char *lend;
> +	int f_fd = fileno(f);
> +
> +	/* Cutoff after newline character, in order to not display garbage */
> +	char *cutoff = strchr(record, '\n');
> +	if (cutoff) {
> +		if (cutoff - record < BUF_LEN)
> +			cutoff[1] = '\0';
> +	}
>  
>  	va_start(args, format);
>  	igt_vlog(IGT_LOG_DOMAIN, level, format, args);
> @@ -38,10 +60,29 @@ static int log_to_end(enum igt_log_level level, FILE *f,
>  	lend = strchrnul(record, '\n');
>  	while (*lend == '\0') {
>  		igt_log(IGT_LOG_DOMAIN, level, "%s", record);
> -		if (fgets(record, BUF_LEN, f) == NULL) {
> +
> +		while (read(f_fd, record, BUF_LEN) < 0) {
> +			if (!READ_ONCE(ktap_args.is_running)) {
> +				igt_warn("ktap parser stopped\n");
> +				return -2;
> +			}
> +
> +			if (errno == EINTR)
> +				continue;
> +
> +			if (errno == EPIPE) {
> +				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> +				return -2;
> +			}
> +
> +			if (errno == EAGAIN)
> +				/* No records available */
> +				continue;
> +
>  			igt_warn("kmsg truncated: unknown error (%m)\n");
>  			return -2;
>  		}
> +
>  		lend = strchrnul(record, '\n');
>  	}
>  	return 0;
> @@ -65,7 +106,7 @@ static long lookup_value(const char *haystack, const char *needle)
>  	if (needle_rptr == NULL)
>  		return -1;
>  
> -	/* skip search string and whitespaces after it */
> +	/* Skip search string and whitespaces after it */
>  	needle_rptr += strlen(needle);
>  
>  	num = strtol(needle_rptr, &needle_end, 10);
> @@ -79,6 +120,41 @@ static long lookup_value(const char *haystack, const char *needle)
>  	return num > 0 ? num : 0;
>  }
>  
> +/**
> + * tap_version_present:
> + * @record: buffer with tap data
> + * @print_info: whether tap version should be printed or not
> + *
> + * Returns:
> + * 0 if not found
> + * 1 if found
> + */
> +static int tap_version_present(char* record, bool print_info)
> +{
> +	/*
> +	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> +	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
> +	 *
> +	 * but actually isn't, as it currently depends on the KUnit module
> +	 * being built-in, so we can't rely on it every time
> +	 */
> +	const char *version_rptr = strcasestr(record, "TAP version ");
> +	char *cutoff;
> +
> +	if (version_rptr == NULL)
> +		return 0;
> +
> +	/* Cutoff after newline character, in order to not display garbage */
> +	cutoff = strchr(version_rptr, '\n');
> +	if (cutoff)
> +		cutoff[0] = '\0';
> +
> +	if (print_info)
> +		igt_info("%s\n", version_rptr);
> +
> +	return 1;
> +}
> +
>  /**
>   * find_next_tap_subtest:
>   * @fp: FILE pointer
> @@ -91,11 +167,12 @@ static long lookup_value(const char *haystack, const char *needle)
>   * -2 if there are problems while reading the file.
>   * any other value corresponds to the amount of cases of the next (sub)test
>   */
> -static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
> +static int find_next_tap_subtest(FILE *fp, char *record, char *test_name, bool is_builtin)
>  {
> -	const char *test_lookup_str, *subtest_lookup_str, *name_rptr, *version_rptr;
> -	char test_name[BUF_LEN + 1];
> +	const char *test_lookup_str, *subtest_lookup_str, *name_rptr;
>  	long test_count;
> +	int fp_fd = fileno(fp);
> +	char *cutoff;
>  
>  	test_name[0] = '\0';
>  	test_name[BUF_LEN] = '\0';
> @@ -103,21 +180,28 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
>  	test_lookup_str = " subtest: ";
>  	subtest_lookup_str = " test: ";
>  
> -	/*
> -	 * "(K)TAP version XX" should be the first line on all (sub)tests as per
> -	 * https://kernel.org/doc/html/latest/dev-tools/ktap.html#version-lines
> -	 *
> -	 * but actually isn't, as it currently depends on the KUnit module
> -	 * being built-in, so we can't rely on it every time
> -	 */
> +	if (!tap_version_present(record, true))
> +		return -1;
> +
>  	if (is_builtin) {
> -		version_rptr = strcasestr(record, "TAP version ");
> -		if (version_rptr == NULL)
> -			return -1;
> +		while (read(fp_fd, record, BUF_LEN) < 0) {
> +			if (!READ_ONCE(ktap_args.is_running)) {
> +				igt_warn("ktap parser stopped\n");
> +				return -2;
> +			}
> +
> +			if (errno == EINTR)
> +				continue;
>  
> -		igt_info("%s", version_rptr);
> +			if (errno == EPIPE) {
> +				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> +				return -2;
> +			}
> +
> +			if (errno == EAGAIN)
> +				/* No records available */
> +				continue;
>  
> -		if (fgets(record, BUF_LEN, fp) == NULL) {
>  			igt_warn("kmsg truncated: unknown error (%m)\n");
>  			return -2;
>  		}
> @@ -134,22 +218,45 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
>  
>  	if (name_rptr == NULL) {
>  		if (!is_builtin)
> -			/* we've probably found nothing */
> +			/* We've probably found nothing */
>  			return -1;
>  		igt_info("Missing test name\n");
>  	} else {
>  		strncpy(test_name, name_rptr, BUF_LEN);
> -		if (fgets(record, BUF_LEN, fp) == NULL) {
> +		/* Cutoff after newline character, in order to not display garbage */
> +		cutoff = strchr(test_name, '\n');
> +		if (cutoff)
> +			cutoff[0] = '\0';
> +
> +		while (read(fp_fd, record, BUF_LEN) < 0) {
> +			if (!READ_ONCE(ktap_args.is_running)) {
> +				igt_warn("ktap parser stopped\n");
> +				return -2;
> +			}
> +
> +			if (errno == EINTR)
> +				continue;
> +
> +			if (errno == EPIPE) {
> +				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> +				return -2;
> +			}
> +
> +			if (errno == EAGAIN)
> +				/* No records available */
> +				continue;
> +
>  			igt_warn("kmsg truncated: unknown error (%m)\n");
>  			return -2;
>  		}
> -		/* now we can be sure we found tests */
> +
> +		/* Now we can be sure we found tests */
>  		if (!is_builtin)
>  			igt_info("KUnit is not built-in, skipping version check...\n");
>  	}
>  
>  	/*
> -	 * total test count will almost always appear as 0..N at the beginning
> +	 * Total test count will almost always appear as 0..N at the beginning
>  	 * of a run, so we use it to reliably identify a new run
>  	 */
>  	test_count = lookup_value(record, "..");
> @@ -159,7 +266,7 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
>  		if (test_name[0] == '\0')
>  			return 0;
>  		if (log_to_end(IGT_LOG_INFO, fp, record,
> -				"Running some tests in: %s",
> +				"Running some tests in: %s\n",
>  				test_name) < 0)
>  			return -2;
>  		return 0;
> @@ -169,7 +276,7 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
>  	}
>  
>  	if (log_to_end(IGT_LOG_INFO, fp, record,
> -			"Executing %ld tests in: %s",
> +			"Executing %ld tests in: %s\n",
>  			test_count, test_name) < 0)
>  		return -2;
>  
> @@ -177,7 +284,7 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
>  }
>  
>  /**
> - * find_next_tap_test:
> + * parse_kmsg_for_tap:
>   * @fp: FILE pointer
>   * @record: buffer used to read fp
>   * @test_name: buffer to store the test name
> @@ -225,7 +332,7 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
>  
>  	comment_start = strchrnul(lstart, '#');
>  
> -	/* check if we're still in a subtest */
> +	/* Check if we're still in a subtest */
>  	if (*comment_start != '\0') {
>  		comment_start++;
>  		value_parse_start = comment_start;
> @@ -254,81 +361,255 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
>  }
>  
>  /**
> - * igt_ktap_parser:
> + * parse_tap_level:
>   * @fp: FILE pointer
> - * @record: buffer used to read fp
> + * @base_test_name: test_name from upper recursion level
> + * @test_count: test_count of this level
> + * @failed_tests: top level failed_tests pointer
> + * @found_tests: top level found_tests pointer
>   * @is_builtin: whether the KUnit module is built-in or not
>   *
> - * This function parses the output of a ktap script and prints the test results,
> - * as well as any other output to stdout.
> - *
> - * Returns: IGT default codes
> + * Returns:
> + * 0 if succeded
> + * -1 if error occurred
>   */
> -int igt_ktap_parser(FILE *fp, char *record, bool is_builtin)
> +static int parse_tap_level(FILE *fp, char *base_test_name, int test_count, bool *failed_tests,
> +			   bool *found_tests, bool is_builtin)
>  {
> +	int fp_fd = fileno(fp);
> +	char record[BUF_LEN + 1];
> +	struct ktap_test_results_element *r, *temp;
> +	int internal_test_count;
>  	char test_name[BUF_LEN + 1];
> -	bool failed_tests, found_tests;
> -	int sublevel = 0;
> +	char base_test_name_for_next_level[BUF_LEN + 1];
>  
> -	test_name[0] = '\0';
> -	test_name[BUF_LEN] = '\0';
> +	for (int i = 0; i < test_count; i++) {
> +		while (read(fp_fd, record, BUF_LEN) < 0) {
> +			if (!READ_ONCE(ktap_args.is_running)) {
> +				igt_warn("ktap parser stopped\n");
> +				return -1;
> +			}
>  
> -	failed_tests = false;
> -	found_tests = false;
> +			if (errno == EINTR)
> +				continue;
>  
> -	while (sublevel >= 0) {
> -		if (fgets(record, BUF_LEN, fp) == NULL) {
> -			if (!found_tests)
> -				igt_warn("kmsg truncated: unknown error (%m)\n");
> -			break;
> +			if (errno == EAGAIN)
> +				/* No records available */
> +				continue;
> +
> +			if (errno == EPIPE) {
> +				igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> +				return -1;
> +			}
> +
> +			igt_warn("kmsg truncated: unknown error (%m)\n");
> +			return -1;
>  		}
>  
> -		switch (find_next_tap_subtest(fp, record, is_builtin)) {
> -		case -2:
> -			/* no more data to read */
> -			return IGT_EXIT_FAILURE;
> -		case -1:
> -			/* no test found, so we keep parsing */
> -			break;
> -		case 0:
> -			/*
> -			 * tests found, but they're missing info, so we might
> -			 * have read into test output
> -			 */
> -			found_tests = true;
> -			sublevel++;
> -			break;
> -		default:
> -			if (fgets(record, BUF_LEN, fp) == NULL) {
> -				igt_warn("kmsg truncated: unknown error (%m)\n");
> -				return -2;
> +		/* Sublevel found */
> +		if (tap_version_present(record, false))
> +		{
> +			internal_test_count = find_next_tap_subtest(fp, record, test_name,
> +								    is_builtin);
> +			switch (internal_test_count) {
> +			case -2:
> +				/* No more data to read */
> +				return -1;
> +			case -1:
> +				/* No test found */
> +				return -1;
> +			case 0:
> +				/* Tests found, but they're missing info */
> +				*found_tests = true;
> +				return -1;
> +			default:
> +				*found_tests = true;
> +
> +				memcpy(base_test_name_for_next_level, base_test_name, BUF_LEN);
> +				if (strlen(base_test_name_for_next_level) < BUF_LEN - 1 &&
> +				    base_test_name_for_next_level[0])
> +					strncat(base_test_name_for_next_level, DELIMITER,
> +						BUF_LEN - strlen(base_test_name_for_next_level));
> +				memcpy(base_test_name_for_next_level + strlen(base_test_name_for_next_level),
> +				       test_name, BUF_LEN - strlen(base_test_name_for_next_level));
> +
> +				if (parse_tap_level(fp, base_test_name_for_next_level,
> +						    internal_test_count, failed_tests, found_tests,
> +						    is_builtin) == -1)
> +					return -1;
> +				break;
>  			}
> -			found_tests = true;
> -			sublevel++;
> -			break;
>  		}
>  
>  		switch (parse_kmsg_for_tap(fp, record, test_name)) {
>  		case -2:
> -			return IGT_EXIT_FAILURE;
> +			return -1;
>  		case -1:
> -			sublevel--;
> -			failed_tests = true;
> -			igt_subtest(test_name)
> -				igt_fail(IGT_EXIT_FAILURE);
> +			*failed_tests = true;
> +
> +			r = malloc(sizeof(*r));
> +
> +			memcpy(r->test_name, base_test_name, BUF_LEN);
> +			if (strlen(r->test_name) < BUF_LEN - 1)
> +				if (r->test_name[0])
> +					strncat(r->test_name, DELIMITER,
> +						BUF_LEN - strlen(r->test_name));
> +			memcpy(r->test_name + strlen(r->test_name), test_name,
> +			       BUF_LEN - strlen(r->test_name));
> +			r->test_name[BUF_LEN] = '\0';
> +
> +			r->passed = false;
> +			r->next = NULL;
> +
> +			pthread_mutex_lock(&results.mutex);
> +			if (results.head == NULL) {
> +				results.head = r;
> +			} else {
> +				temp = results.head;
> +				while (temp->next != NULL)
> +					temp = temp->next;
> +				temp->next = r;
> +			}
> +			pthread_mutex_unlock(&results.mutex);
> +
>  			test_name[0] = '\0';
>  			break;
> -		case 0: /* fallthrough */
> -			igt_subtest(test_name)
> -				igt_success();
> +		case 0:
> +			r = malloc(sizeof(*r));
> +
> +			memcpy(r->test_name, base_test_name, BUF_LEN);
> +			if (strlen(r->test_name) < BUF_LEN - 1)
> +				if (r->test_name[0])
> +					strncat(r->test_name, DELIMITER,
> +						BUF_LEN - strlen(r->test_name));
> +			memcpy(r->test_name + strlen(r->test_name), test_name,
> +			       BUF_LEN - strlen(r->test_name));
> +			r->test_name[BUF_LEN] = '\0';
> +
> +			r->passed = true;
> +			r->next = NULL;
> +
> +			pthread_mutex_lock(&results.mutex);
> +			if (results.head == NULL) {
> +				results.head = r;
> +			} else {
> +				temp = results.head;
> +				while (temp->next != NULL)
> +					temp = temp->next;
> +				temp->next = r;
> +			}
> +			pthread_mutex_unlock(&results.mutex);
> +
>  			test_name[0] = '\0';
> +			break;
>  		default:
>  			break;
>  		}
>  	}
> +	return 0;
> +}
> +
> +/**
> + * igt_ktap_parser:
> + *
> + * This function parses the output of a ktap script and passes it to main thread.
> + */
> +void *igt_ktap_parser(void *unused)
> +{
> +	FILE *fp = ktap_args.fp;
> +	int fp_fd = fileno(fp);
> +	char record[BUF_LEN + 1];
> +	bool is_builtin = ktap_args.is_builtin;
> +	char test_name[BUF_LEN + 1];
> +	bool failed_tests, found_tests;
> +	int test_count;
> +
> +	failed_tests = false;
> +	found_tests = false;
> +
> +	if (!READ_ONCE(ktap_args.is_running))
> +		goto igt_ktap_parser_end;
> +
> +igt_ktap_parser_start:
> +	test_name[0] = '\0';
> +	test_name[BUF_LEN] = '\0';
> +
> +	while (read(fp_fd, record, BUF_LEN) < 0) {
> +		if (!READ_ONCE(ktap_args.is_running)) {
> +			igt_warn("ktap parser stopped\n");
> +			goto igt_ktap_parser_end;
> +		}
> +
> +		if (errno == EAGAIN)
> +			/* No records available */
> +			continue;
> +
> +		if (errno == EINTR)
> +			continue;
> +
> +		if (errno == EPIPE) {
> +			igt_warn("kmsg truncated: too many messages. You may want to increase log_buf_len in kmcdline\n");
> +			goto igt_ktap_parser_end;
> +		}
> +	}
> +
> +	test_count = find_next_tap_subtest(fp, record, test_name, is_builtin);
> +
> +	switch (test_count) {
> +	case -2:
> +		/* Problems while reading the file */
> +		goto igt_ktap_parser_end;
> +	case -1:
> +		/* No test found */
> +		goto igt_ktap_parser_start;
> +	case 0:
> +		/* Tests found, but they're missing info */
> +		found_tests = true;
> +		goto igt_ktap_parser_end;
> +	default:
> +		found_tests = true;
> +
> +		if (parse_tap_level(fp, test_name, test_count, &failed_tests, &found_tests,
> +				    is_builtin) == -1)
> +			goto igt_ktap_parser_end;
> +
> +		break;
> +	}
> +
> +	/* Parse topmost level */
> +	test_name[0] = '\0';
> +	parse_tap_level(fp, test_name, test_count, &failed_tests, &found_tests, is_builtin);
> +
> +igt_ktap_parser_end:
> +	results.still_running = false;
>  
>  	if (failed_tests || !found_tests)
> -		return IGT_EXIT_FAILURE;
> +		ktap_args.ret = IGT_EXIT_FAILURE;
> +	else
> +		ktap_args.ret = IGT_EXIT_SUCCESS;
> +
> +	return NULL;
> +}
> +
> +static pthread_t ktap_parser_thread;
> +
> +struct ktap_test_results *ktap_parser_start(FILE *fp, bool is_builtin)
> +{
> +	results.head = NULL;
> +	pthread_mutex_init(&results.mutex, NULL);
> +	results.still_running = true;
>  
> -	return IGT_EXIT_SUCCESS;
> +	ktap_args.fp = fp;
> +	ktap_args.is_builtin = is_builtin;
> +	ktap_args.is_running = true;
> +	pthread_create(&ktap_parser_thread, NULL, igt_ktap_parser, NULL);
> +
> +	return &results;
> +}
> +
> +int ktap_parser_stop(void)
> +{
> +	ktap_args.is_running = false;
> +	pthread_join(ktap_parser_thread, NULL);
> +	return ktap_args.ret;
>  }
> diff --git a/lib/igt_ktap.h b/lib/igt_ktap.h
> index b2f69df2..34fe0957 100644
> --- a/lib/igt_ktap.h
> +++ b/lib/igt_ktap.h
> @@ -26,6 +26,25 @@
>  
>  #define BUF_LEN 4096
>  
> -int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
> +#include <pthread.h>
> +
> +void *igt_ktap_parser(void *unused);
> +
> +typedef struct ktap_test_results_element {
> +	char test_name[BUF_LEN + 1];
> +	bool passed;
> +	struct ktap_test_results_element *next;
> +} ktap_test_results_element;
> +
> +struct ktap_test_results {
> +	ktap_test_results_element *head;
> +	pthread_mutex_t mutex;
> +	bool still_running;
> +};
> +
> +
> +
> +struct ktap_test_results *ktap_parser_start(FILE *fp, bool is_builtin);
> +int ktap_parser_stop(void);
>  
>  #endif /* IGT_KTAP_H */

Re: [igt-dev] [PATCH i-g-t 8/8] lib/igt_kmod: fix nesting igt_fixture in igt_subtest

[Mauro Carvalho Chehab]

On Mon,  5 Jun 2023 12:47:16 +0200
Dominik Karol Piatkowski <dominik.karol.piatkowski@intel.com> wrote:

> Fix the following issue:
> 	$ ./build/tests/drm_buddy
> 	Starting subtest: all-tests
> 	nesting igt_fixture in igt_subtest is invalid
> 	please refer to lib/igt_core documentation
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>

Acked-by: Mauro Carvalho Chehab <mchehab@kernel.org>

> ---
>  lib/igt_kmod.c | 80 ++++++++++++++++++++++++--------------------------
>  1 file changed, 38 insertions(+), 42 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index c62eb97a..2c0cc026 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -772,47 +772,45 @@ static int __igt_kunit(const char *module_name, const char *opts)
>  		return ret;
>  	}
>  
> -	igt_fixture {
> -		if (igt_ktest_begin(&tst) != 0) {
> -			igt_warn("Unable to begin ktest for %s\n", module_name);
> +	if (igt_ktest_begin(&tst) != 0) {
> +		igt_warn("Unable to begin ktest for %s\n", module_name);
>  
> -			igt_ktest_fini(&tst);
> -			return ret;
> -		}
> +		igt_ktest_fini(&tst);
> +		return ret;
> +	}
>  
> -		if (tst.kmsg < 0) {
> -			igt_warn("Could not open /dev/kmsg\n");
> -			goto unload;
> -		}
> +	if (tst.kmsg < 0) {
> +		igt_warn("Could not open /dev/kmsg\n");
> +		goto unload;
> +	}
>  
> -		if (lseek(tst.kmsg, 0, SEEK_END)) {
> -			igt_warn("Could not seek the end of /dev/kmsg\n");
> -			goto unload;
> -		}
> +	if (lseek(tst.kmsg, 0, SEEK_END)) {
> +		igt_warn("Could not seek the end of /dev/kmsg\n");
> +		goto unload;
> +	}
>  
> -		f = fdopen(tst.kmsg, "r");
> +	f = fdopen(tst.kmsg, "r");
>  
> -		if (f == NULL) {
> -			igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> -			goto unload;
> -		}
> +	if (f == NULL) {
> +		igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> +		goto unload;
> +	}
>  
> -		/* The KUnit module is required for running any KUnit tests */
> -		if (igt_kmod_load("kunit", NULL) != 0 ||
> -		    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> -			igt_warn("Unable to load KUnit\n");
> -			igt_fail(IGT_EXIT_FAILURE);
> -		}
> +	/* The KUnit module is required for running any KUnit tests */
> +	if (igt_kmod_load("kunit", NULL) != 0 ||
> +	    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> +		igt_warn("Unable to load KUnit\n");
> +		igt_fail(IGT_EXIT_FAILURE);
> +	}
>  
> -		is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> +	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
>  
> -		results = ktap_parser_start(f, is_builtin);
> +	results = ktap_parser_start(f, is_builtin);
>  
> -		if (igt_kmod_load(module_name, opts) != 0) {
> -			igt_warn("Unable to load %s module\n", module_name);
> -			ret = ktap_parser_stop();
> -			igt_fail(IGT_EXIT_FAILURE);
> -		}
> +	if (igt_kmod_load(module_name, opts) != 0) {
> +		igt_warn("Unable to load %s module\n", module_name);
> +		ret = ktap_parser_stop();
> +		igt_fail(IGT_EXIT_FAILURE);
>  	}
>  
>  	while (READ_ONCE(results->still_running) || READ_ONCE(results->head) != NULL)
> @@ -820,7 +818,7 @@ static int __igt_kunit(const char *module_name, const char *opts)
>  		if (READ_ONCE(results->head) != NULL) {
>  			pthread_mutex_lock(&results->mutex);
>  
> -			igt_subtest(results->head->test_name) {
> +			igt_dynamic(results->head->test_name) {
>  				if (READ_ONCE(results->head->passed))
>  					igt_success();
>  				else
> @@ -836,19 +834,17 @@ static int __igt_kunit(const char *module_name, const char *opts)
>  	}
>  
>  unload:
> -	igt_fixture {
> -		igt_ktest_end(&tst);
> +	igt_ktest_end(&tst);
>  
> -		igt_ktest_fini(&tst);
> +	igt_ktest_fini(&tst);
>  
> -		ret = ktap_parser_stop();
> +	ret = ktap_parser_stop();
>  
> -		if (ret != 0)
> -			ret = IGT_EXIT_ABORT;
> +	if (ret != 0)
> +		ret = IGT_EXIT_ABORT;
>  
> -		if (ret == 0)
> -			igt_success();
> -	}
> +	if (ret == 0)
> +		igt_success();
>  	return ret;
>  }
>  

[igt-dev] ✓ Fi.CI.BAT: success for Introduce KUnit (rev5)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4877 bytes --]

== Series Details ==

Series: Introduce KUnit (rev5)
URL   : https://patchwork.freedesktop.org/series/114612/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13228 -> IGTPW_9102
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/index.html

Participating hosts (37 -> 36)
------------------------------

  Missing    (1): fi-snb-2520m 

Known issues
------------

  Here are the changes found in IGTPW_9102 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_pm_backlight@basic-brightness@edp-1:
    - bat-rplp-1:         NOTRUN -> [ABORT][1] ([i915#7077])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/bat-rplp-1/igt@i915_pm_backlight@basic-brightness@edp-1.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-2:         [PASS][2] -> [ABORT][3] ([i915#4983] / [i915#7461] / [i915#7913] / [i915#8347])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/bat-rpls-2/igt@i915_selftest@live@reset.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/bat-rpls-2/igt@i915_selftest@live@reset.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1:
    - bat-dg2-8:          [PASS][4] -> [FAIL][5] ([i915#7932])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence@pipe-d-dp-1.html

  * igt@kms_pipe_crc_basic@read-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][6] ([i915#1845] / [i915#5354])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/bat-dg2-11/igt@kms_pipe_crc_basic@read-crc-frame-sequence.html

  * igt@kms_setmode@basic-clone-single-crtc:
    - bat-rplp-1:         NOTRUN -> [SKIP][7] ([i915#3555] / [i915#4579])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/bat-rplp-1/igt@kms_setmode@basic-clone-single-crtc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@migrate:
    - bat-dg2-11:         [DMESG-WARN][8] ([i915#7699]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/bat-dg2-11/igt@i915_selftest@live@migrate.html

  
#### Warnings ####

  * igt@kms_psr@primary_mmap_gtt:
    - bat-rplp-1:         [ABORT][10] ([i915#8442]) -> [SKIP][11] ([i915#1072])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/bat-rplp-1/igt@kms_psr@primary_mmap_gtt.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/bat-rplp-1/igt@kms_psr@primary_mmap_gtt.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#4423]: https://gitlab.freedesktop.org/drm/intel/issues/4423
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#7059]: https://gitlab.freedesktop.org/drm/intel/issues/7059
  [i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347
  [i915#8442]: https://gitlab.freedesktop.org/drm/intel/issues/8442
  [i915#8497]: https://gitlab.freedesktop.org/drm/intel/issues/8497


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7318 -> IGTPW_9102

  CI-20190529: 20190529
  CI_DRM_13228: e1c7a0f8d1ad2af33f6c71852b9f2d85a7db32ac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_9102: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/index.html
  IGT_7318: c2d8ef8b9397d0976959f29dc1dd7c8a698d65fe @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git


Testlist changes
----------------

+igt@kms_selftest@drm_cmdline
+igt@kms_selftest@drm_damage
+igt@kms_selftest@drm_dp_mst
+igt@kms_selftest@drm_format
+igt@kms_selftest@drm_format_helper
+igt@kms_selftest@drm_plane
+igt@kms_selftest@framebuffer

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/index.html

[-- Attachment #2: Type: text/html, Size: 5385 bytes --]

Re: [igt-dev] [PATCH i-g-t 8/8] lib/igt_kmod: fix nesting igt_fixture in igt_subtest

[Janusz Krzysztofik]

On Monday, 5 June 2023 12:47:16 CEST Dominik Karol Piatkowski wrote:
> Fix the following issue:
> 	$ ./build/tests/drm_buddy
> 	Starting subtest: all-tests
> 	nesting igt_fixture in igt_subtest is invalid
> 	please refer to lib/igt_core documentation

AFAICS, this patch reverts a lot of changes introduced by PATCH 5/5 "Change 
logic of ktap parser to run on a thread".  Then, I think it should be squashed 
with the latter.  Ideally, PATCH 6/8 "lib/igt_kmod: place KUnit tests on a 
subtest", which looks like a fix for the sqashed 5+8, should be squashed with 
5/8 either to provide a really working solution, I believe.

Thanks,
Janusz

> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> ---
>  lib/igt_kmod.c | 80 ++++++++++++++++++++++++--------------------------
>  1 file changed, 38 insertions(+), 42 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index c62eb97a..2c0cc026 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -772,47 +772,45 @@ static int __igt_kunit(const char *module_name, const char *opts)
>  		return ret;
>  	}
>  
> -	igt_fixture {
> -		if (igt_ktest_begin(&tst) != 0) {
> -			igt_warn("Unable to begin ktest for %s\n", module_name);
> +	if (igt_ktest_begin(&tst) != 0) {
> +		igt_warn("Unable to begin ktest for %s\n", module_name);
>  
> -			igt_ktest_fini(&tst);
> -			return ret;
> -		}
> +		igt_ktest_fini(&tst);
> +		return ret;
> +	}
>  
> -		if (tst.kmsg < 0) {
> -			igt_warn("Could not open /dev/kmsg\n");
> -			goto unload;
> -		}
> +	if (tst.kmsg < 0) {
> +		igt_warn("Could not open /dev/kmsg\n");
> +		goto unload;
> +	}
>  
> -		if (lseek(tst.kmsg, 0, SEEK_END)) {
> -			igt_warn("Could not seek the end of /dev/kmsg\n");
> -			goto unload;
> -		}
> +	if (lseek(tst.kmsg, 0, SEEK_END)) {
> +		igt_warn("Could not seek the end of /dev/kmsg\n");
> +		goto unload;
> +	}
>  
> -		f = fdopen(tst.kmsg, "r");
> +	f = fdopen(tst.kmsg, "r");
>  
> -		if (f == NULL) {
> -			igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> -			goto unload;
> -		}
> +	if (f == NULL) {
> +		igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> +		goto unload;
> +	}
>  
> -		/* The KUnit module is required for running any KUnit tests */
> -		if (igt_kmod_load("kunit", NULL) != 0 ||
> -		    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> -			igt_warn("Unable to load KUnit\n");
> -			igt_fail(IGT_EXIT_FAILURE);
> -		}
> +	/* The KUnit module is required for running any KUnit tests */
> +	if (igt_kmod_load("kunit", NULL) != 0 ||
> +	    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> +		igt_warn("Unable to load KUnit\n");
> +		igt_fail(IGT_EXIT_FAILURE);
> +	}
>  
> -		is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> +	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
>  
> -		results = ktap_parser_start(f, is_builtin);
> +	results = ktap_parser_start(f, is_builtin);
>  
> -		if (igt_kmod_load(module_name, opts) != 0) {
> -			igt_warn("Unable to load %s module\n", module_name);
> -			ret = ktap_parser_stop();
> -			igt_fail(IGT_EXIT_FAILURE);
> -		}
> +	if (igt_kmod_load(module_name, opts) != 0) {
> +		igt_warn("Unable to load %s module\n", module_name);
> +		ret = ktap_parser_stop();
> +		igt_fail(IGT_EXIT_FAILURE);
>  	}
>  
>  	while (READ_ONCE(results->still_running) || READ_ONCE(results->head) != NULL)
> @@ -820,7 +818,7 @@ static int __igt_kunit(const char *module_name, const char *opts)
>  		if (READ_ONCE(results->head) != NULL) {
>  			pthread_mutex_lock(&results->mutex);
>  
> -			igt_subtest(results->head->test_name) {
> +			igt_dynamic(results->head->test_name) {
>  				if (READ_ONCE(results->head->passed))
>  					igt_success();
>  				else
> @@ -836,19 +834,17 @@ static int __igt_kunit(const char *module_name, const char *opts)
>  	}
>  
>  unload:
> -	igt_fixture {
> -		igt_ktest_end(&tst);
> +	igt_ktest_end(&tst);
>  
> -		igt_ktest_fini(&tst);
> +	igt_ktest_fini(&tst);
>  
> -		ret = ktap_parser_stop();
> +	ret = ktap_parser_stop();
>  
> -		if (ret != 0)
> -			ret = IGT_EXIT_ABORT;
> +	if (ret != 0)
> +		ret = IGT_EXIT_ABORT;
>  
> -		if (ret == 0)
> -			igt_success();
> -	}
> +	if (ret == 0)
> +		igt_success();
>  	return ret;
>  }
>  
> 

Re: [igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Janusz Krzysztofik]

Hi Mauro, Dominik,

On Monday, 5 June 2023 12:47:14 CEST Dominik Karol Piatkowski wrote:
> From: Mauro Carvalho Chehab <mchehab@kernel.org>
> 
> There's a hidden bug at KUnit implementation: as it doesn't
> place tests inside a subtest, trying to use it with igt_main
> causes a crash:
> 
> 	$ ./build/tests/drm_mm --list
> 	skipping is allowed only in fixtures, subtests or igt_simple_main
> 	please refer to lib/igt_core documentation
> 	drm_mm: ../lib/igt_core.c:437: internal_assert: Assertion `0' failed.
> 	Received signal SIGABRT.
> 	Stack trace:
> 	 #0 [fatal_sig_handler+0x17b]
> 	 #1 [__sigaction+0x50]
> 	 #2 [__pthread_kill_implementation+0x114]
> 	 #3 [gsignal+0x1e]
> 	 #4 [abort+0xdf]
> 	 #5 [__assert_fail_base.cold+0xe]
> 	 #6 [__assert_fail+0x47]
> 	 #7 [internal_assert+0xe5]
> 	 #8 [igt_skip+0x169]
> 	 #9 [__igt_skip_check+0x1bb]
> 	 #10 [igt_ktest_begin+0xa6]
> 	 #11 [igt_kunit+0x70]
> 	 #12 [main+0x2a]
> 	 #13 [__libc_start_call_main+0x7a]
> 	 #14 [__libc_start_main+0x8b]
> 	 #15 [_start+0x25]
> 
> Fix it by using igt_subtests() before actually implememnting
> KUnit logic.
> 
> After the patch, it should now report subtests:
> 
> 	$ ./build/tests/drm_mm --list
> 	all-tests
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> ---
>  lib/igt_kmod.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 8cb9cb2e..1309ab21 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -754,7 +754,7 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
>   *
>   * Returns: IGT default codes
>   */
> -int igt_kunit(const char *module_name, const char *opts)
> +static int __igt_kunit(const char *module_name, const char *opts)
>  {
>  	struct igt_ktest tst;
>  	struct kmod_module *kunit_kmod;
> @@ -852,6 +852,20 @@ unload:
>  	return ret;
>  }
>  
> +int igt_kunit(const char *module_name, const char *opts)
> +{
> +	/*
> +	 * We need to use igt_subtest here, as otherwise it may crash with:
> +	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
> +	 * if used on igt_main. This is also needed in order to provide
> +	 * proper namespace for dynamic subtests, with is required for CI
> +	 * and for documentation.
> +	 */
> +	igt_subtest_with_dynamic("all-tests")

Why can't we use module_name as subtest name?

Thanks,
Janusz

> +		return __igt_kunit(module_name, opts);
> +	return 0;
> +}
> +
>  static int open_parameters(const char *module_name)
>  {
>  	char path[256];
> 

Re: [igt-dev] [PATCH v5 i-g-t 0/8] Introduce KUnit

[Janusz Krzysztofik]

On Monday, 5 June 2023 12:47:08 CEST Dominik Karol Piatkowski wrote:
> This series is a continuation of Isabella's work on introducing
> KUnit to IGT.
> 
> Sample drm_buddy output:
> 	Starting subtest: all-tests
> 	[thread:8003] TAP version 1
> 	[thread:8003] Executing 6 tests in: drm_buddy
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_limit
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_limit: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_range
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_range: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_optimistic
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_optimistic: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_pessimistic
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pessimistic: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_smoke
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_smoke: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_pathological
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pathological: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy
> 	Dynamic subtest drm_buddy: SUCCESS (0.000s)
> 	Subtest all-tests: SUCCESS (0.707s)

It would be nice if we could see CI results from the new kunit subtests before 
accepting the series.  Unfortunately, none of the tests seems to have been 
executed within the pre-merge run.  Can we fix that?

Thanks,
Janusz

> 
> The issue of possibility of too many sublevels occurrence is solved
> by name concatenation.
> 
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> 
> Dominik Karol Piatkowski (2):
>   Change logic of ktap parser to run on a thread
>   lib/igt_kmod: fix nesting igt_fixture in igt_subtest
> 
> Isabella Basso (4):
>   lib/igt_kmod: rename kselftest functions to ktest
>   lib/igt_kmod.c: check if module is builtin before attempting to unload
>     it
>   lib/igt_kmod: add compatibility for KUnit
>   tests: DRM selftests: switch to KUnit
> 
> Mauro Carvalho Chehab (2):
>   lib/igt_kmod: place KUnit tests on a subtest
>   kunit tests: add an optional name for the selftests
> 
>  lib/igt_kmod.c       | 146 +++++++++-
>  lib/igt_kmod.h       |  14 +-
>  lib/igt_ktap.c       | 615 +++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_ktap.h       |  50 ++++
>  lib/meson.build      |   1 +
>  tests/drm_buddy.c    |   4 +-
>  tests/drm_mm.c       |   5 +-
>  tests/kms_selftest.c |  19 ++
>  8 files changed, 835 insertions(+), 19 deletions(-)
>  create mode 100644 lib/igt_ktap.c
>  create mode 100644 lib/igt_ktap.h
> 
> 

Re: [igt-dev] [PATCH v5 i-g-t 0/8] Introduce KUnit

[Piatkowski, Dominik Karol]

Hi Janusz,

> -----Original Message-----
> From: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Sent: wtorek, 6 czerwca 2023 09:47
> To: igt-dev@lists.freedesktop.org; Piatkowski, Dominik Karol
> <dominik.karol.piatkowski@intel.com>; Mauro Carvalho Chehab
> <mauro.chehab@linux.intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Subject: Re: [PATCH v5 i-g-t 0/8] Introduce KUnit
> 
> On Monday, 5 June 2023 12:47:08 CEST Dominik Karol Piatkowski wrote:
> > This series is a continuation of Isabella's work on introducing KUnit
> > to IGT.
> >
> > Sample drm_buddy output:
> > 	Starting subtest: all-tests
> > 	[thread:8003] TAP version 1
> > 	[thread:8003] Executing 6 tests in: drm_buddy
> > 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_limit
> > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_limit:
> SUCCESS (0.000s)
> > 	Starting dynamic subtest:
> drm_buddy__drm_test_buddy_alloc_range
> > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_range:
> SUCCESS (0.000s)
> > 	Starting dynamic subtest:
> drm_buddy__drm_test_buddy_alloc_optimistic
> > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_optimistic:
> SUCCESS (0.000s)
> > 	Starting dynamic subtest:
> drm_buddy__drm_test_buddy_alloc_pessimistic
> > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pessimistic:
> SUCCESS (0.000s)
> > 	Starting dynamic subtest:
> drm_buddy__drm_test_buddy_alloc_smoke
> > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_smoke:
> SUCCESS (0.000s)
> > 	Starting dynamic subtest:
> drm_buddy__drm_test_buddy_alloc_pathological
> > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pathological:
> SUCCESS (0.000s)
> > 	Starting dynamic subtest: drm_buddy
> > 	Dynamic subtest drm_buddy: SUCCESS (0.000s)
> > 	Subtest all-tests: SUCCESS (0.707s)
> 
> It would be nice if we could see CI results from the new kunit subtests
> before accepting the series.  Unfortunately, none of the tests seems to have
> been executed within the pre-merge run.  Can we fix that?

AFAIK, Kconfig used by CI does not have required params set (CONFIG_KUNIT and CONFIG_DRM_I915_KUNIT_SELFTESTS). I submitted a merge request to enable these params, but it was not merged yet. The best option I can think of right now, is a trybot run tested with Kconfig patch enabling these params. Would that be ok with you?

Thanks,
Dominik

> 
> Thanks,
> Janusz
> 
> >
> > The issue of possibility of too many sublevels occurrence is solved by
> > name concatenation.
> >
> > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> >
> > Dominik Karol Piatkowski (2):
> >   Change logic of ktap parser to run on a thread
> >   lib/igt_kmod: fix nesting igt_fixture in igt_subtest
> >
> > Isabella Basso (4):
> >   lib/igt_kmod: rename kselftest functions to ktest
> >   lib/igt_kmod.c: check if module is builtin before attempting to unload
> >     it
> >   lib/igt_kmod: add compatibility for KUnit
> >   tests: DRM selftests: switch to KUnit
> >
> > Mauro Carvalho Chehab (2):
> >   lib/igt_kmod: place KUnit tests on a subtest
> >   kunit tests: add an optional name for the selftests
> >
> >  lib/igt_kmod.c       | 146 +++++++++-
> >  lib/igt_kmod.h       |  14 +-
> >  lib/igt_ktap.c       | 615
> +++++++++++++++++++++++++++++++++++++++++++
> >  lib/igt_ktap.h       |  50 ++++
> >  lib/meson.build      |   1 +
> >  tests/drm_buddy.c    |   4 +-
> >  tests/drm_mm.c       |   5 +-
> >  tests/kms_selftest.c |  19 ++
> >  8 files changed, 835 insertions(+), 19 deletions(-)  create mode
> > 100644 lib/igt_ktap.c  create mode 100644 lib/igt_ktap.h
> >
> >
> 
> 
> 

Re: [igt-dev] [PATCH v5 i-g-t 0/8] Introduce KUnit

[Mauro Carvalho Chehab]

On Tue, 6 Jun 2023 07:54:56 +0000
"Piatkowski, Dominik Karol" <dominik.karol.piatkowski@intel.com> wrote:

> Hi Janusz,
> 
> > -----Original Message-----
> > From: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Sent: wtorek, 6 czerwca 2023 09:47
> > To: igt-dev@lists.freedesktop.org; Piatkowski, Dominik Karol
> > <dominik.karol.piatkowski@intel.com>; Mauro Carvalho Chehab
> > <mauro.chehab@linux.intel.com>
> > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Subject: Re: [PATCH v5 i-g-t 0/8] Introduce KUnit
> > 
> > On Monday, 5 June 2023 12:47:08 CEST Dominik Karol Piatkowski wrote:  
> > > This series is a continuation of Isabella's work on introducing KUnit
> > > to IGT.
> > >
> > > Sample drm_buddy output:
> > > 	Starting subtest: all-tests
> > > 	[thread:8003] TAP version 1
> > > 	[thread:8003] Executing 6 tests in: drm_buddy
> > > 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_limit
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_limit:  
> > SUCCESS (0.000s)  
> > > 	Starting dynamic subtest:  
> > drm_buddy__drm_test_buddy_alloc_range  
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_range:  
> > SUCCESS (0.000s)  
> > > 	Starting dynamic subtest:  
> > drm_buddy__drm_test_buddy_alloc_optimistic  
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_optimistic:  
> > SUCCESS (0.000s)  
> > > 	Starting dynamic subtest:  
> > drm_buddy__drm_test_buddy_alloc_pessimistic  
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pessimistic:  
> > SUCCESS (0.000s)  
> > > 	Starting dynamic subtest:  
> > drm_buddy__drm_test_buddy_alloc_smoke  
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_smoke:  
> > SUCCESS (0.000s)  
> > > 	Starting dynamic subtest:  
> > drm_buddy__drm_test_buddy_alloc_pathological  
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pathological:  
> > SUCCESS (0.000s)  
> > > 	Starting dynamic subtest: drm_buddy
> > > 	Dynamic subtest drm_buddy: SUCCESS (0.000s)
> > > 	Subtest all-tests: SUCCESS (0.707s)  
> > 
> > It would be nice if we could see CI results from the new kunit subtests
> > before accepting the series.  Unfortunately, none of the tests seems to have
> > been executed within the pre-merge run.  Can we fix that?  
> 
> AFAIK, Kconfig used by CI does not have required params set (CONFIG_KUNIT and CONFIG_DRM_I915_KUNIT_SELFTESTS). I submitted a merge request to enable these params, but it was not merged yet. The best option I can think of right now, is a trybot run tested with Kconfig patch enabling these params. Would that be ok with you?

Where is such merge request? Let's review/merge it first. Then,
re-trigger the test.

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Mauro Carvalho Chehab]

On Tue, 06 Jun 2023 09:44:00 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

> Hi Mauro, Dominik,
> 
> On Monday, 5 June 2023 12:47:14 CEST Dominik Karol Piatkowski wrote:
> > From: Mauro Carvalho Chehab <mchehab@kernel.org>
> > 
> > There's a hidden bug at KUnit implementation: as it doesn't
> > place tests inside a subtest, trying to use it with igt_main
> > causes a crash:
> > 
> > 	$ ./build/tests/drm_mm --list
> > 	skipping is allowed only in fixtures, subtests or igt_simple_main
> > 	please refer to lib/igt_core documentation
> > 	drm_mm: ../lib/igt_core.c:437: internal_assert: Assertion `0' failed.
> > 	Received signal SIGABRT.
> > 	Stack trace:
> > 	 #0 [fatal_sig_handler+0x17b]
> > 	 #1 [__sigaction+0x50]
> > 	 #2 [__pthread_kill_implementation+0x114]
> > 	 #3 [gsignal+0x1e]
> > 	 #4 [abort+0xdf]
> > 	 #5 [__assert_fail_base.cold+0xe]
> > 	 #6 [__assert_fail+0x47]
> > 	 #7 [internal_assert+0xe5]
> > 	 #8 [igt_skip+0x169]
> > 	 #9 [__igt_skip_check+0x1bb]
> > 	 #10 [igt_ktest_begin+0xa6]
> > 	 #11 [igt_kunit+0x70]
> > 	 #12 [main+0x2a]
> > 	 #13 [__libc_start_call_main+0x7a]
> > 	 #14 [__libc_start_main+0x8b]
> > 	 #15 [_start+0x25]
> > 
> > Fix it by using igt_subtests() before actually implememnting
> > KUnit logic.
> > 
> > After the patch, it should now report subtests:
> > 
> > 	$ ./build/tests/drm_mm --list
> > 	all-tests
> > 
> > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> > ---
> >  lib/igt_kmod.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 8cb9cb2e..1309ab21 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -754,7 +754,7 @@ void igt_kselftest_get_tests(struct kmod_module *kmod,
> >   *
> >   * Returns: IGT default codes
> >   */
> > -int igt_kunit(const char *module_name, const char *opts)
> > +static int __igt_kunit(const char *module_name, const char *opts)
> >  {
> >  	struct igt_ktest tst;
> >  	struct kmod_module *kunit_kmod;
> > @@ -852,6 +852,20 @@ unload:
> >  	return ret;
> >  }
> >  
> > +int igt_kunit(const char *module_name, const char *opts)
> > +{
> > +	/*
> > +	 * We need to use igt_subtest here, as otherwise it may crash with:
> > +	 *  skipping is allowed only in fixtures, subtests or igt_simple_main
> > +	 * if used on igt_main. This is also needed in order to provide
> > +	 * proper namespace for dynamic subtests, with is required for CI
> > +	 * and for documentation.
> > +	 */
> > +	igt_subtest_with_dynamic("all-tests")  
> 
> Why can't we use module_name as subtest name?

We can, but this preserves the old behavior. So, the existing documentation
won't break.

Patch 7/8 allows adjusting the name, as it makes sense that each kUnit
group to be on a different igt dynamic subtest.

Regards,
Mauro

Re: [igt-dev] [PATCH v5 i-g-t 0/8] Introduce KUnit

[Piatkowski, Dominik Karol]

> -----Original Message-----
> From: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> Sent: wtorek, 6 czerwca 2023 10:18
> To: Piatkowski, Dominik Karol <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>; igt-
> dev@lists.freedesktop.org
> Subject: Re: [PATCH v5 i-g-t 0/8] Introduce KUnit
> 
> On Tue, 6 Jun 2023 07:54:56 +0000
> "Piatkowski, Dominik Karol" <dominik.karol.piatkowski@intel.com> wrote:
> 
> > Hi Janusz,
> >
> > > -----Original Message-----
> > > From: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Sent: wtorek, 6 czerwca 2023 09:47
> > > To: igt-dev@lists.freedesktop.org; Piatkowski, Dominik Karol
> > > <dominik.karol.piatkowski@intel.com>; Mauro Carvalho Chehab
> > > <mauro.chehab@linux.intel.com>
> > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Subject: Re: [PATCH v5 i-g-t 0/8] Introduce KUnit
> > >
> > > On Monday, 5 June 2023 12:47:08 CEST Dominik Karol Piatkowski wrote:
> > > > This series is a continuation of Isabella's work on introducing
> > > > KUnit to IGT.
> > > >
> > > > Sample drm_buddy output:
> > > > 	Starting subtest: all-tests
> > > > 	[thread:8003] TAP version 1
> > > > 	[thread:8003] Executing 6 tests in: drm_buddy
> > > > 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_limit
> > > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_limit:
> > > SUCCESS (0.000s)
> > > > 	Starting dynamic subtest:
> > > drm_buddy__drm_test_buddy_alloc_range
> > > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_range:
> > > SUCCESS (0.000s)
> > > > 	Starting dynamic subtest:
> > > drm_buddy__drm_test_buddy_alloc_optimistic
> > > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_optimistic:
> > > SUCCESS (0.000s)
> > > > 	Starting dynamic subtest:
> > > drm_buddy__drm_test_buddy_alloc_pessimistic
> > > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pessimistic:
> > > SUCCESS (0.000s)
> > > > 	Starting dynamic subtest:
> > > drm_buddy__drm_test_buddy_alloc_smoke
> > > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_smoke:
> > > SUCCESS (0.000s)
> > > > 	Starting dynamic subtest:
> > > drm_buddy__drm_test_buddy_alloc_pathological
> > > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pathological:
> > > SUCCESS (0.000s)
> > > > 	Starting dynamic subtest: drm_buddy
> > > > 	Dynamic subtest drm_buddy: SUCCESS (0.000s)
> > > > 	Subtest all-tests: SUCCESS (0.707s)
> > >
> > > It would be nice if we could see CI results from the new kunit
> > > subtests before accepting the series.  Unfortunately, none of the
> > > tests seems to have been executed within the pre-merge run.  Can we
> fix that?
> >
> > AFAIK, Kconfig used by CI does not have required params set
> (CONFIG_KUNIT and CONFIG_DRM_I915_KUNIT_SELFTESTS). I submitted a
> merge request to enable these params, but it was not merged yet. The best
> option I can think of right now, is a trybot run tested with Kconfig patch
> enabling these params. Would that be ok with you?
> 
> Where is such merge request? Let's review/merge it first. Then, re-trigger
> the test.

Hi,

The merge request is here:
gitlab.freedesktop.org/gfx-ci/i915-infra/-/merge_requests/122

Thanks,
Dominik Karol

> 
> Regards,
> Mauro

Re: [igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Janusz Krzysztofik]

On Tuesday, 6 June 2023 10:21:44 CEST Mauro Carvalho Chehab wrote:
> On Tue, 06 Jun 2023 09:44:00 +0200
> Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> 
> > Hi Mauro, Dominik,
> > 
> > On Monday, 5 June 2023 12:47:14 CEST Dominik Karol Piatkowski wrote:
> > > From: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > 
> > > There's a hidden bug at KUnit implementation: as it doesn't
> > > place tests inside a subtest, trying to use it with igt_main
> > > causes a crash:
> > > 
> > > 	$ ./build/tests/drm_mm --list
> > > 	skipping is allowed only in fixtures, subtests or igt_simple_main
> > > 	please refer to lib/igt_core documentation
> > > 	drm_mm: ../lib/igt_core.c:437: internal_assert: Assertion `0' 
failed.
> > > 	Received signal SIGABRT.
> > > 	Stack trace:
> > > 	 #0 [fatal_sig_handler+0x17b]
> > > 	 #1 [__sigaction+0x50]
> > > 	 #2 [__pthread_kill_implementation+0x114]
> > > 	 #3 [gsignal+0x1e]
> > > 	 #4 [abort+0xdf]
> > > 	 #5 [__assert_fail_base.cold+0xe]
> > > 	 #6 [__assert_fail+0x47]
> > > 	 #7 [internal_assert+0xe5]
> > > 	 #8 [igt_skip+0x169]
> > > 	 #9 [__igt_skip_check+0x1bb]
> > > 	 #10 [igt_ktest_begin+0xa6]
> > > 	 #11 [igt_kunit+0x70]
> > > 	 #12 [main+0x2a]
> > > 	 #13 [__libc_start_call_main+0x7a]
> > > 	 #14 [__libc_start_main+0x8b]
> > > 	 #15 [_start+0x25]
> > > 
> > > Fix it by using igt_subtests() before actually implememnting
> > > KUnit logic.
> > > 
> > > After the patch, it should now report subtests:
> > > 
> > > 	$ ./build/tests/drm_mm --list
> > > 	all-tests
> > > 
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > > Signed-off-by: Dominik Karol Piątkowski 
<dominik.karol.piatkowski@intel.com>
> > > ---
> > >  lib/igt_kmod.c | 16 +++++++++++++++-
> > >  1 file changed, 15 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > > index 8cb9cb2e..1309ab21 100644
> > > --- a/lib/igt_kmod.c
> > > +++ b/lib/igt_kmod.c
> > > @@ -754,7 +754,7 @@ void igt_kselftest_get_tests(struct kmod_module 
*kmod,
> > >   *
> > >   * Returns: IGT default codes
> > >   */
> > > -int igt_kunit(const char *module_name, const char *opts)
> > > +static int __igt_kunit(const char *module_name, const char *opts)
> > >  {
> > >  	struct igt_ktest tst;
> > >  	struct kmod_module *kunit_kmod;
> > > @@ -852,6 +852,20 @@ unload:
> > >  	return ret;
> > >  }
> > >  
> > > +int igt_kunit(const char *module_name, const char *opts)
> > > +{
> > > +	/*
> > > +	 * We need to use igt_subtest here, as otherwise it may crash with:
> > > +	 *  skipping is allowed only in fixtures, subtests or 
igt_simple_main
> > > +	 * if used on igt_main. This is also needed in order to provide
> > > +	 * proper namespace for dynamic subtests, with is required for CI
> > > +	 * and for documentation.
> > > +	 */
> > > +	igt_subtest_with_dynamic("all-tests")  
> > 
> > Why can't we use module_name as subtest name?
> 
> We can, but this preserves the old behavior. So, the existing documentation
> won't break.

Where is this old behavior documented?  In the current code I can't find any 
occurrence of "test-all" other than inside igt_kmod.c:igt_ksleftest(), not 
documented in any way.

Wasn't the old behavior a result of a limitation rather than a feature?

Thanks,
Janusz

> 
> Patch 7/8 allows adjusting the name, as it makes sense that each kUnit
> group to be on a different igt dynamic subtest.
> 
> Regards,
> Mauro
> 

Re: [igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Mauro Carvalho Chehab]

On Tue, 06 Jun 2023 10:41:40 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

> > > > +	 * and for documentation.
> > > > +	 */
> > > > +	igt_subtest_with_dynamic("all-tests")    
> > > 
> > > Why can't we use module_name as subtest name?  
> > 
> > We can, but this preserves the old behavior. So, the existing documentation
> > won't break.  
> 
> Where is this old behavior documented?  In the current code I can't find any 
> occurrence of "test-all" other than inside igt_kmod.c:igt_ksleftest(), not 
> documented in any way.
> 
> Wasn't the old behavior a result of a limitation rather than a feature?

Take a look at igt_kselftests():

        igt_subtest_with_dynamic(filter ?: "all-tests") {
                igt_list_for_each_entry_safe(tl, tn, &tests, link) {
                        unsigned long taints;

                        igt_dynamic_f("%s", unfilter(filter, tl->name))
                                igt_kselftest_execute(&tst, tl, options, result);
                        free(tl);

The default when "filter" is not used is "all-tests".

Regards,
Mauro

[igt-dev] ✗ Fi.CI.IGT: failure for Introduce KUnit (rev5)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 17199 bytes --]

== Series Details ==

Series: Introduce KUnit (rev5)
URL   : https://patchwork.freedesktop.org/series/114612/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_13228_full -> IGTPW_9102_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_9102_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_9102_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/index.html

Participating hosts (7 -> 6)
------------------------------

  Missing    (1): shard-dg1 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_9102_full:

### IGT changes ###

#### Possible regressions ####

  * {igt@kms_selftest@drm_cmdline} (NEW):
    - shard-glk:          NOTRUN -> [INCOMPLETE][1] +6 similar issues
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk3/igt@kms_selftest@drm_cmdline.html

  * {igt@kms_selftest@drm_dp_mst} (NEW):
    - {shard-tglu}:       NOTRUN -> [INCOMPLETE][2] +6 similar issues
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-tglu-8/igt@kms_selftest@drm_dp_mst.html

  * {igt@kms_selftest@drm_format} (NEW):
    - {shard-rkl}:        NOTRUN -> [INCOMPLETE][3] +6 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-rkl-2/igt@kms_selftest@drm_format.html
    - shard-apl:          NOTRUN -> [INCOMPLETE][4] +6 similar issues
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-apl4/igt@kms_selftest@drm_format.html
    - shard-snb:          NOTRUN -> [INCOMPLETE][5] +6 similar issues
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-snb6/igt@kms_selftest@drm_format.html

  * igt@kms_vblank@pipe-c-ts-continuation-suspend:
    - shard-glk:          [PASS][6] -> [TIMEOUT][7] +3 similar issues
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk9/igt@kms_vblank@pipe-c-ts-continuation-suspend.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk9/igt@kms_vblank@pipe-c-ts-continuation-suspend.html

  
#### Warnings ####

  * igt@drm_mm@all-tests:
    - shard-apl:          [SKIP][8] ([fdo#109271] / [i915#4579]) -> [INCOMPLETE][9] +1 similar issue
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-apl4/igt@drm_mm@all-tests.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-apl6/igt@drm_mm@all-tests.html
    - shard-snb:          [SKIP][10] ([fdo#109271] / [i915#4579]) -> [INCOMPLETE][11] +1 similar issue
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-snb7/igt@drm_mm@all-tests.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-snb4/igt@drm_mm@all-tests.html
    - shard-glk:          [SKIP][12] ([fdo#109271] / [i915#4579]) -> [INCOMPLETE][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk5/igt@drm_mm@all-tests.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk7/igt@drm_mm@all-tests.html

  * igt@kms_big_fb@y-tiled-16bpp-rotate-90:
    - shard-glk:          [SKIP][14] ([fdo#109271]) -> [INCOMPLETE][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk8/igt@kms_big_fb@y-tiled-16bpp-rotate-90.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk6/igt@kms_big_fb@y-tiled-16bpp-rotate-90.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render:
    - shard-glk:          [SKIP][16] ([fdo#109271]) -> [TIMEOUT][17] +1 similar issue
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk4/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk9/igt@kms_frontbuffer_tracking@fbcpsr-1p-offscren-pri-shrfb-draw-render.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@drm_buddy@all-tests:
    - {shard-tglu}:       [SKIP][18] ([i915#4579] / [i915#6433]) -> [INCOMPLETE][19] +1 similar issue
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-tglu-9/igt@drm_buddy@all-tests.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-tglu-2/igt@drm_buddy@all-tests.html

  * igt@drm_mm@all-tests:
    - {shard-rkl}:        [SKIP][20] ([i915#4579] / [i915#6433]) -> [INCOMPLETE][21] +1 similar issue
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-rkl-2/igt@drm_mm@all-tests.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-rkl-3/igt@drm_mm@all-tests.html

  
New tests
---------

  New tests have been introduced between CI_DRM_13228_full and IGTPW_9102_full:

### New IGT tests (7) ###

  * igt@kms_selftest@drm_cmdline:
    - Statuses : 5 incomplete(s)
    - Exec time: [0.0] s

  * igt@kms_selftest@drm_damage:
    - Statuses : 5 incomplete(s)
    - Exec time: [0.0] s

  * igt@kms_selftest@drm_dp_mst:
    - Statuses : 5 incomplete(s)
    - Exec time: [0.0] s

  * igt@kms_selftest@drm_format:
    - Statuses : 5 incomplete(s)
    - Exec time: [0.0] s

  * igt@kms_selftest@drm_format_helper:
    - Statuses : 5 incomplete(s)
    - Exec time: [0.0] s

  * igt@kms_selftest@drm_plane:
    - Statuses : 5 incomplete(s)
    - Exec time: [0.0] s

  * igt@kms_selftest@framebuffer:
    - Statuses : 5 incomplete(s)
    - Exec time: [0.0] s

  

Known issues
------------

  Here are the changes found in IGTPW_9102_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gen9_exec_parse@allowed-single:
    - shard-glk:          [PASS][22] -> [ABORT][23] ([i915#5566])
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk7/igt@gen9_exec_parse@allowed-single.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk4/igt@gen9_exec_parse@allowed-single.html

  * igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-vga:
    - shard-snb:          NOTRUN -> [SKIP][24] ([fdo#109271] / [i915#4579]) +13 similar issues
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-snb6/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-vga.html

  * igt@i915_pm_rps@reset:
    - shard-snb:          [PASS][25] -> [INCOMPLETE][26] ([i915#7790])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-snb1/igt@i915_pm_rps@reset.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-snb5/igt@i915_pm_rps@reset.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-apl:          [PASS][27] -> [FAIL][28] ([i915#2346])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-apl4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-hdmi-a-1:
    - shard-snb:          NOTRUN -> [SKIP][29] ([fdo#109271]) +37 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-snb1/igt@kms_plane_scaling@planes-upscale-factor-0-25-downscale-factor-0-5@pipe-a-hdmi-a-1.html

  * igt@kms_universal_plane@disable-primary-vs-flip-pipe-d:
    - shard-glk:          NOTRUN -> [SKIP][30] ([fdo#109271]) +2 similar issues
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk5/igt@kms_universal_plane@disable-primary-vs-flip-pipe-d.html

  * igt@perf@enable-disable@0-rcs0:
    - shard-glk:          [PASS][31] -> [TIMEOUT][32] ([i915#8170])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk2/igt@perf@enable-disable@0-rcs0.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk9/igt@perf@enable-disable@0-rcs0.html

  
#### Possible fixes ####

  * igt@gem_barrier_race@remote-request@rcs0:
    - shard-glk:          [ABORT][33] ([i915#7461] / [i915#8211]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk6/igt@gem_barrier_race@remote-request@rcs0.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk8/igt@gem_barrier_race@remote-request@rcs0.html

  * igt@gem_eio@in-flight-contexts-1us:
    - shard-apl:          [TIMEOUT][35] ([i915#3063]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-apl7/igt@gem_eio@in-flight-contexts-1us.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-apl7/igt@gem_eio@in-flight-contexts-1us.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - {shard-tglu}:       [FAIL][37] ([i915#2842]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-tglu-4/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-tglu-5/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_exec_fair@basic-pace@rcs0:
    - {shard-rkl}:        [FAIL][39] ([i915#2842]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-rkl-3/igt@gem_exec_fair@basic-pace@rcs0.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-rkl-1/igt@gem_exec_fair@basic-pace@rcs0.html

  * igt@i915_pm_rpm@modeset-lpsp-stress-no-wait:
    - {shard-rkl}:        [SKIP][41] ([i915#1397]) -> [PASS][42] +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-rkl-6/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-rkl-7/igt@i915_pm_rpm@modeset-lpsp-stress-no-wait.html

  * igt@i915_suspend@basic-s3-without-i915:
    - {shard-rkl}:        [FAIL][43] ([fdo#103375]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-rkl-3/igt@i915_suspend@basic-s3-without-i915.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-rkl-3/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_flip@flip-vs-expired-vblank@b-hdmi-a1:
    - shard-glk:          [FAIL][45] ([i915#2122]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk7/igt@kms_flip@flip-vs-expired-vblank@b-hdmi-a1.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk9/igt@kms_flip@flip-vs-expired-vblank@b-hdmi-a1.html

  * igt@kms_plane_scaling@plane-upscale-with-modifiers-factor-0-25@pipe-a-hdmi-a-2:
    - {shard-rkl}:        [ABORT][47] ([i915#8311]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-rkl-2/igt@kms_plane_scaling@plane-upscale-with-modifiers-factor-0-25@pipe-a-hdmi-a-2.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-rkl-4/igt@kms_plane_scaling@plane-upscale-with-modifiers-factor-0-25@pipe-a-hdmi-a-2.html

  
#### Warnings ####

  * igt@kms_content_protection@mei_interface:
    - shard-apl:          [SKIP][49] ([fdo#109271]) -> [SKIP][50] ([fdo#109271] / [i915#4579])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-apl6/igt@kms_content_protection@mei_interface.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-apl2/igt@kms_content_protection@mei_interface.html
    - shard-snb:          [SKIP][51] ([fdo#109271]) -> [SKIP][52] ([fdo#109271] / [i915#4579])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-snb1/igt@kms_content_protection@mei_interface.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-snb5/igt@kms_content_protection@mei_interface.html
    - shard-glk:          [SKIP][53] ([fdo#109271]) -> [SKIP][54] ([fdo#109271] / [i915#4579])
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13228/shard-glk6/igt@kms_content_protection@mei_interface.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/shard-glk5/igt@kms_content_protection@mei_interface.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103375]: https://bugs.freedesktop.org/show_bug.cgi?id=103375
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109295]: https://bugs.freedesktop.org/show_bug.cgi?id=109295
  [fdo#109300]: https://bugs.freedesktop.org/show_bug.cgi?id=109300
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#110723]: https://bugs.freedesktop.org/show_bug.cgi?id=110723
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1825]: https://gitlab.freedesktop.org/drm/intel/issues/1825
  [i915#2122]: https://gitlab.freedesktop.org/drm/intel/issues/2122
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#3023]: https://gitlab.freedesktop.org/drm/intel/issues/3023
  [i915#3063]: https://gitlab.freedesktop.org/drm/intel/issues/3063
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3291]: https://gitlab.freedesktop.org/drm/intel/issues/3291
  [i915#3323]: https://gitlab.freedesktop.org/drm/intel/issues/3323
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3637]: https://gitlab.freedesktop.org/drm/intel/issues/3637
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3804]: https://gitlab.freedesktop.org/drm/intel/issues/3804
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3989]: https://gitlab.freedesktop.org/drm/intel/issues/3989
  [i915#4070]: https://gitlab.freedesktop.org/drm/intel/issues/4070
  [i915#4098]: https://gitlab.freedesktop.org/drm/intel/issues/4098
  [i915#4103]: https://gitlab.freedesktop.org/drm/intel/issues/4103
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4525]: https://gitlab.freedesktop.org/drm/intel/issues/4525
  [i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#4816]: https://gitlab.freedesktop.org/drm/intel/issues/4816
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#533]: https://gitlab.freedesktop.org/drm/intel/issues/533
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6768]: https://gitlab.freedesktop.org/drm/intel/issues/6768
  [i915#6944]: https://gitlab.freedesktop.org/drm/intel/issues/6944
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7118]: https://gitlab.freedesktop.org/drm/intel/issues/7118
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7790]: https://gitlab.freedesktop.org/drm/intel/issues/7790
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#8170]: https://gitlab.freedesktop.org/drm/intel/issues/8170
  [i915#8211]: https://gitlab.freedesktop.org/drm/intel/issues/8211
  [i915#8234]: https://gitlab.freedesktop.org/drm/intel/issues/8234
  [i915#8311]: https://gitlab.freedesktop.org/drm/intel/issues/8311


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7318 -> IGTPW_9102
  * Piglit: piglit_4509 -> None

  CI-20190529: 20190529
  CI_DRM_13228: e1c7a0f8d1ad2af33f6c71852b9f2d85a7db32ac @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_9102: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/index.html
  IGT_7318: c2d8ef8b9397d0976959f29dc1dd7c8a698d65fe @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_9102/index.html

[-- Attachment #2: Type: text/html, Size: 17000 bytes --]

Re: [igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Janusz Krzysztofik]

On Tuesday, 6 June 2023 11:18:50 CEST Mauro Carvalho Chehab wrote:
> On Tue, 06 Jun 2023 10:41:40 +0200
> Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> 
> > > > > +	 * and for documentation.
> > > > > +	 */
> > > > > +	igt_subtest_with_dynamic("all-tests")    
> > > > 
> > > > Why can't we use module_name as subtest name?  
> > > 
> > > We can, but this preserves the old behavior. So, the existing 
documentation
> > > won't break.  
> > 
> > Where is this old behavior documented?  In the current code I can't find 
any 
> > occurrence of "test-all" other than inside igt_kmod.c:igt_ksleftest(), not 
> > documented in any way.
> > 
> > Wasn't the old behavior a result of a limitation rather than a feature?
> 
> Take a look at igt_kselftests():
> 
>         igt_subtest_with_dynamic(filter ?: "all-tests") {
>                 igt_list_for_each_entry_safe(tl, tn, &tests, link) {
>                         unsigned long taints;
> 
>                         igt_dynamic_f("%s", unfilter(filter, tl->name))
>                                 igt_kselftest_execute(&tst, tl, options, 
result);
>                         free(tl);
> 
> The default when "filter" is not used is "all-tests".

Yes, I did look into igt_kselftest() and did notice the use of "all-tests", 
but for me that's not a documentation of an expected behavior, only a not 
quite fortunate implementation detail, forced by a mix of IGT contraints on 
one hand and implementation specifics of kernel side of i915 selftests on the 
other.

When you call an IGT test with a subtest name specified then only that subtest 
is executed, no matter if that's a selftest or a userspace test.

When you call a userspace IGT test with no subtest name (pattern) specified 
then all subtests are executed and their names displayed.  But when you do the 
same for an IGT selftest then all selftests are executed as one subtest named 
"all-tests".  Do you think that's intentional, not a result of some 
constraints which no longer apply for kunit tests?  Why do you prefer to keep 
the old inconsistent behavior instead of implementing IGT kunit tests to 
behave consistently with userspace IGT tests?

Thanks,
Janusz

> 
> Regards,
> Mauro
> 

Re: [igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Mauro Carvalho Chehab]

On Tue, 06 Jun 2023 12:03:04 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

> > > > > > +	igt_subtest_with_dynamic("all-tests")      
> > > > > 
> > > > > Why can't we use module_name as subtest name?    
> > > > 
> > > > We can, but this preserves the old behavior. So, the existing   
> documentation
> > > > won't break.    
> > > 
> > > Where is this old behavior documented?  In the current code I can't find   
> any 
> > > occurrence of "test-all" other than inside igt_kmod.c:igt_ksleftest(), not 
> > > documented in any way.
> > > 
> > > Wasn't the old behavior a result of a limitation rather than a feature?  
> > 
> > Take a look at igt_kselftests():
> > 
> >         igt_subtest_with_dynamic(filter ?: "all-tests") {
> >                 igt_list_for_each_entry_safe(tl, tn, &tests, link) {
> >                         unsigned long taints;
> > 
> >                         igt_dynamic_f("%s", unfilter(filter, tl->name))
> >                                 igt_kselftest_execute(&tst, tl, options,   
> result);
> >                         free(tl);
> > 
> > The default when "filter" is not used is "all-tests".  
> 
> Yes, I did look into igt_kselftest() and did notice the use of "all-tests", 
> but for me that's not a documentation of an expected behavior, only a not 
> quite fortunate implementation detail, forced by a mix of IGT contraints on 
> one hand and implementation specifics of kernel side of i915 selftests on the 
> other.

No, it is not just that. The tests actually announce that at runtime.
So, if you run the test with --list, you'll see:

	$ ./build/tests/drm_mm --list-subtests
	all-tests

And this is what it is used to check for testplan documentation.

> When you call an IGT test with a subtest name specified then only that subtest 
> is executed, no matter if that's a selftest or a userspace test.
> 
> When you call a userspace IGT test with no subtest name (pattern) specified 
> then all subtests are executed and their names displayed.  But when you do the 
> same for an IGT selftest then all selftests are executed as one subtest named 
> "all-tests".  Do you think that's intentional, not a result of some 
> constraints which no longer apply for kunit tests?  Why do you prefer to keep 
> the old inconsistent behavior instead of implementing IGT kunit tests to 
> behave consistently with userspace IGT tests?

We can use whatever other name, provided that the output of
--list-subtests will be identical to the dynamic subtest. If not,
build will fail when checking for documentation inconsistencies.

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Janusz Krzysztofik]

On Tuesday, 6 June 2023 15:57:27 CEST Mauro Carvalho Chehab wrote:
> On Tue, 06 Jun 2023 12:03:04 +0200
> Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> 
> > > > > > > +	igt_subtest_with_dynamic("all-tests")      
> > > > > > 
> > > > > > Why can't we use module_name as subtest name?    
> > > > > 
> > > > > We can, but this preserves the old behavior. So, the existing   
> > documentation
> > > > > won't break.    
> > > > 
> > > > Where is this old behavior documented?  In the current code I can't find   
> > any 
> > > > occurrence of "test-all" other than inside igt_kmod.c:igt_ksleftest(), not 
> > > > documented in any way.
> > > > 
> > > > Wasn't the old behavior a result of a limitation rather than a feature?  
> > > 
> > > Take a look at igt_kselftests():
> > > 
> > >         igt_subtest_with_dynamic(filter ?: "all-tests") {
> > >                 igt_list_for_each_entry_safe(tl, tn, &tests, link) {
> > >                         unsigned long taints;
> > > 
> > >                         igt_dynamic_f("%s", unfilter(filter, tl->name))
> > >                                 igt_kselftest_execute(&tst, tl, options,   
> > result);
> > >                         free(tl);
> > > 
> > > The default when "filter" is not used is "all-tests".  
> > 
> > Yes, I did look into igt_kselftest() and did notice the use of "all-tests", 
> > but for me that's not a documentation of an expected behavior, only a not 
> > quite fortunate implementation detail, forced by a mix of IGT contraints on 
> > one hand and implementation specifics of kernel side of i915 selftests on the 
> > other.
> 
> No, it is not just that. The tests actually announce that at runtime.
> So, if you run the test with --list, you'll see:
> 
> 	$ ./build/tests/drm_mm --list-subtests
> 	all-tests
> 
> And this is what it is used to check for testplan documentation.
> 
> > When you call an IGT test with a subtest name specified then only that subtest 
> > is executed, no matter if that's a selftest or a userspace test.
> > 
> > When you call a userspace IGT test with no subtest name (pattern) specified 
> > then all subtests are executed and their names displayed.  But when you do the 
> > same for an IGT selftest then all selftests are executed as one subtest named 
> > "all-tests".  Do you think that's intentional, not a result of some 
> > constraints which no longer apply for kunit tests?  Why do you prefer to keep 
> > the old inconsistent behavior instead of implementing IGT kunit tests to 
> > behave consistently with userspace IGT tests?
> 
> We can use whatever other name, provided that the output of
> --list-subtests will be identical to the dynamic subtest. If not,
> build will fail when checking for documentation inconsistencies. 

AFAICU, that checking process compares subtest names found in a documentation, 
here the source code, as pointed by you in one of your previouse answers as 
the source of the documentation, against --list output.  But that sounds 
weird.  What a sophisticated code parser must be used to resolve subtest names 
potentially provided via expressions?  Another compiler?
Or is there another source of documentation, which I already asked you for a 
reference to, and you didn't clearly answered yet, used by the build process 
for that checking?

Thanks,
Janusz
 
> 
> Regards,
> Mauro
> 

Re: [igt-dev] [PATCH i-g-t 6/8] lib/igt_kmod: place KUnit tests on a subtest

[Mauro Carvalho Chehab]

On Tue, 06 Jun 2023 16:22:21 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

> > We can use whatever other name, provided that the output of
> > --list-subtests will be identical to the dynamic subtest. If not,
> > build will fail when checking for documentation inconsistencies.   
> 
> AFAICU, that checking process compares subtest names found in a documentation, 
> here the source code, as pointed by you in one of your previouse answers as 
> the source of the documentation, against --list output.  But that sounds 
> weird.  What a sophisticated code parser must be used to resolve subtest names 
> potentially provided via expressions?  Another compiler?
> Or is there another source of documentation, which I already asked you for a 
> reference to, and you didn't clearly answered yet, used by the build process 
> for that checking?

There's no other source of documentation. The documentation toolset
has a --check tool that calls the binaries using "--list" parameter,
comparing them with the documentation inside the source code.

So, for tests/drm_mm.c file, the documentation is on its source:

	/**
	 * TEST: drm mm
	 * Description: Basic sanity check of DRM's range manager (struct drm_mm)
	 * Feature: mapping
	 * Run type: FULL
	 *
	 * SUBTEST: all-tests
	 *
	 * SUBTEST: all-tests@align
	 * Category: Infrastructure
	 * Description: drm_mm range manager SW validation
	 * Functionality: DRM memory mangemnt
	 * Test category: GEM_Legacy
	...

It will run build/tests/drm_mm --list in order to compare "SUBTEST"
the fields. The ones with "@" are dynamic subtests and are currently
ignored, as --list is currently not capable of reporting dynamic 
subtests.

Now, it is possible to change from "all-tests" to something else, but
such patch also needs to change it inside tests/drm_mm.c.

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Janusz Krzysztofik]

On Monday, 5 June 2023 12:47:12 CEST Dominik Karol Piatkowski wrote:
> From: Isabella Basso <isabbasso@riseup.net>
> 
> As the DRM selftests are now using KUnit [1], update IGT tests as well.
> 
> [1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/
> 
> Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> 
> v1 -> v2:
> - drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
>   with code other than IGT_EXIT_ABORT
> - kms_selftest: move igt_kunit tests to separate subtests
> - kms_selftest: fallback to igt_kselftests if all subtests failed
> 
> v2 -> v3:
> - expose all subtests
> 
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> ---
>  tests/drm_buddy.c    | 4 +++-
>  tests/drm_mm.c       | 4 +++-
>  tests/kms_selftest.c | 8 ++++++++
>  3 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
> index 06876e0c..3261f0d6 100644
> --- a/tests/drm_buddy.c
> +++ b/tests/drm_buddy.c
> @@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's buddy 
allocator (struct drm_bu
>  
>  igt_main
>  {
> -	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> +	int ret = igt_kunit("drm_buddy_test", NULL);
> +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> +		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
>  }
> diff --git a/tests/drm_mm.c b/tests/drm_mm.c
> index 0bce7139..88f76a57 100644
> --- a/tests/drm_mm.c
> +++ b/tests/drm_mm.c
> @@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's range 
manager (struct drm_mm)"
>  
>  igt_main
>  {
> -	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> +	int ret = igt_kunit("drm_mm_test", NULL);
> +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> +		igt_kselftests("test-drm_mm", NULL, NULL, NULL);

My discussion with Mauro about subtest names and their consistency with inline 
documentation has lead me to a question: have we verified if behavior of 
--list-subtests option under such conditional construct is consistent with 
expectations of the testplan tool?

But maybe we should still get back to a design phase and the question of how 
we want these three generic DRM selftests to behave on old and new kernels 
after the change.

Option 1:
We just add kunit variants as new subtests, aside the existing i915-like 
selftest subtests.  Whether kunit or i915-like selftest variants will execute 
or skip depends on availability of required kernel side kunit or selftest 
modules.

Option 2:
Each of the three tests still provides one igt_subtest_with_dynamic().  Which 
dynamic subtests are executed, whether kunit or i915-like selftest or none, 
depends on availability of required kernel modules.

Option 3:
Current approach: provide only kunit subtests on kernels with kunit modules 
and only i915-like sleftest subtests otherwise.  But then, take care of 
--list-subtests option always returning only names of subtests that can be 
executed (for which kernel modules are available).
Aditional assumption for the testplan tool: the same kunit kernel modules 
available when building the testplan will be available when executing it.

Thanks,
Janusz
  
>  }
> diff --git a/tests/kms_selftest.c b/tests/kms_selftest.c
> index abc4bfe9..b27f60fb 100644
> --- a/tests/kms_selftest.c
> +++ b/tests/kms_selftest.c
> @@ -28,5 +28,13 @@ IGT_TEST_DESCRIPTION("Basic sanity check of KMS 
selftests.");
>  
>  igt_main
>  {
> +	static const char *kunit_subtests[] = { "drm_cmdline_parser_test", 
"drm_damage_helper_test",
> +						
"drm_dp_mst_helper_test", "drm_format_helper_test",
> +						"drm_format_test", 
"drm_framebuffer_test",
> +						
"drm_plane_helper_test", NULL };
> +
> +	for (int i = 0; kunit_subtests[i] != NULL; i++)
> +		igt_kunit(kunit_subtests[i], NULL);
> +
>  	igt_kselftests("test-drm_modeset", NULL, NULL, NULL);
>  }
> 

Re: [igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Mauro Carvalho Chehab]

On Wed, 07 Jun 2023 12:24:55 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

> On Monday, 5 June 2023 12:47:12 CEST Dominik Karol Piatkowski wrote:
> > From: Isabella Basso <isabbasso@riseup.net>
> > 
> > As the DRM selftests are now using KUnit [1], update IGT tests as well.
> > 
> > [1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/
> > 
> > Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> > 
> > v1 -> v2:
> > - drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
> >   with code other than IGT_EXIT_ABORT
> > - kms_selftest: move igt_kunit tests to separate subtests
> > - kms_selftest: fallback to igt_kselftests if all subtests failed
> > 
> > v2 -> v3:
> > - expose all subtests
> > 
> > Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> > ---
> >  tests/drm_buddy.c    | 4 +++-
> >  tests/drm_mm.c       | 4 +++-
> >  tests/kms_selftest.c | 8 ++++++++
> >  3 files changed, 14 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
> > index 06876e0c..3261f0d6 100644
> > --- a/tests/drm_buddy.c
> > +++ b/tests/drm_buddy.c
> > @@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's buddy 
> allocator (struct drm_bu
> >  
> >  igt_main
> >  {
> > -	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > +	int ret = igt_kunit("drm_buddy_test", NULL);
> > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > +		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> >  }
> > diff --git a/tests/drm_mm.c b/tests/drm_mm.c
> > index 0bce7139..88f76a57 100644
> > --- a/tests/drm_mm.c
> > +++ b/tests/drm_mm.c
> > @@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's range 
> manager (struct drm_mm)"
> >  
> >  igt_main
> >  {
> > -	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> > +	int ret = igt_kunit("drm_mm_test", NULL);
> > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > +		igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> 
> My discussion with Mauro about subtest names and their consistency with inline 
> documentation has lead me to a question: have we verified if behavior of 
> --list-subtests option under such conditional construct is consistent with 
> expectations of the testplan tool?
> 
> But maybe we should still get back to a design phase and the question of how 
> we want these three generic DRM selftests to behave on old and new kernels 
> after the change.
> 
> Option 1:
> We just add kunit variants as new subtests, aside the existing i915-like 
> selftest subtests.  Whether kunit or i915-like selftest variants will execute 
> or skip depends on availability of required kernel side kunit or selftest 
> modules.
> 
> Option 2:
> Each of the three tests still provides one igt_subtest_with_dynamic().  Which 
> dynamic subtests are executed, whether kunit or i915-like selftest or none, 
> depends on availability of required kernel modules.
> 
> Option 3:
> Current approach: provide only kunit subtests on kernels with kunit modules 
> and only i915-like sleftest subtests otherwise.  But then, take care of 
> --list-subtests option always returning only names of subtests that can be 
> executed (for which kernel modules are available).
> Aditional assumption for the testplan tool: the same kunit kernel modules 
> available when building the testplan will be available when executing it.

It sounds to me that you're over complicating it.

At IGT build time, it doesn't really matter if the tests will run with
KUnit or kselftest. What it matters is that igt dynamic subtest is
properly setup, in a way that --list will display the dynamic subtest(s)
that are part of it.

Looking further, this series touch only 3 tests:

	- tests/drm_buddy.c
	- tests/drm_mm.c
	- tests/kms_selftest.c

The first two are related to some changes that already happened
upstream: DRM core now uses KUnit and don't have support for
selftests.

For KMS, I would expect that the Xe driver will require those to use
KUnit as well, as Xe driver doesn't support selftest. It may either
run as selftest or KUnit for i915. The IGT runtime decision to run 
either with KUnit or via selftest may depend if the Kernel is built
with KUnit support or not.

-

Now, preserving dynamic subtest namespace is particularly needed 
by drm_mm, which has an extensive documentation for the subtests 
provided by DRM core. We need to group the tests there inside
igt_subtest_with_dynamic("all-tests"), in order to preserve the
documentation we have.

An alternative approach would be to change it to some other
name:

	igt_subtest_with_dynamic("some-foo-name")

And then rename the subtests inside tests/drm_mm.c replacing
"all-tests" with "some-foo-name".

I can't see any rationale for doing that, but, if you think it
is worth doing that, feel free to submit a patch after we have
this patch series merged.

Regards,
Mauro

Re: [igt-dev] [PATCH v5 i-g-t 0/8] Introduce KUnit

[Janusz Krzysztofik]

On Tuesday, 6 June 2023 09:54:56 CEST Piatkowski, Dominik Karol wrote:
> Hi Janusz,
> 
> > -----Original Message-----
> > From: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Sent: wtorek, 6 czerwca 2023 09:47
> > To: igt-dev@lists.freedesktop.org; Piatkowski, Dominik Karol
> > <dominik.karol.piatkowski@intel.com>; Mauro Carvalho Chehab
> > <mauro.chehab@linux.intel.com>
> > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > Subject: Re: [PATCH v5 i-g-t 0/8] Introduce KUnit
> > 
> > On Monday, 5 June 2023 12:47:08 CEST Dominik Karol Piatkowski wrote:
> > > This series is a continuation of Isabella's work on introducing KUnit
> > > to IGT.
> > >
> > > Sample drm_buddy output:
> > > 	Starting subtest: all-tests
> > > 	[thread:8003] TAP version 1
> > > 	[thread:8003] Executing 6 tests in: drm_buddy
> > > 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_limit
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_limit:
> > SUCCESS (0.000s)
> > > 	Starting dynamic subtest:
> > drm_buddy__drm_test_buddy_alloc_range
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_range:
> > SUCCESS (0.000s)
> > > 	Starting dynamic subtest:
> > drm_buddy__drm_test_buddy_alloc_optimistic
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_optimistic:
> > SUCCESS (0.000s)
> > > 	Starting dynamic subtest:
> > drm_buddy__drm_test_buddy_alloc_pessimistic
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pessimistic:
> > SUCCESS (0.000s)
> > > 	Starting dynamic subtest:
> > drm_buddy__drm_test_buddy_alloc_smoke
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_smoke:
> > SUCCESS (0.000s)
> > > 	Starting dynamic subtest:
> > drm_buddy__drm_test_buddy_alloc_pathological
> > > 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pathological:
> > SUCCESS (0.000s)
> > > 	Starting dynamic subtest: drm_buddy
> > > 	Dynamic subtest drm_buddy: SUCCESS (0.000s)
> > > 	Subtest all-tests: SUCCESS (0.707s)
> > 
> > It would be nice if we could see CI results from the new kunit subtests
> > before accepting the series.  Unfortunately, none of the tests seems to 
have
> > been executed within the pre-merge run.  Can we fix that?
> 
> AFAIK, Kconfig used by CI does not have required params set (CONFIG_KUNIT 
> and CONFIG_DRM_I915_KUNIT_SELFTESTS). I submitted a merge request to enable 
> these params, but it was not merged yet. The best option I can think of 
> right now, is a trybot run tested with Kconfig patch enabling these params. 
> Would that be ok with you?

I think the reason for no results from CI is that these tests trigger abort if 
kunit modules are missing, at least on my laptop:

jkrzyszt@jkrzyszt-mobl2:~/build/igt$ sudo ./tests/drm_mm
IGT-Version: 1.27.1-gbdd46c438a (x86_64) (Linux: 6.1.31-1-lts x86_64)
Starting subtest: all-tests
(drm_mm:121917) igt_kmod-WARNING: Unable to load KUnit
drm_mm: ../../git/rebasing-scripts/igt/lib/igt_core.c:1808: igt_fail: 
Assertion `_igt_dynamic_tests_executed < 0 || dynamic_failed_one' failed.
Received signal SIGABRT.
Stack trace: 
 #0 [fatal_sig_handler+0x17b]
 #1 [__sigaction+0x50]
 #2 [pthread_key_delete+0x14c]
 #3 [gsignal+0x18]
 #4 [abort+0xd7]
 #5 [<unknown>+0xd7]
 #6 [__assert_fail+0x46]
 #7 [igt_fail+0x121]
 #8 [__igt_kunit+0x29d]
 #9 [igt_kunit+0x6c]
 #10 [main+0x32]
 #11 [__libc_init_first+0x90]
 #12 [__libc_start_main+0x8a]
 #13 [_start+0x25]
Aborted

They should skip, I believe, not abort.

Thanks,
Janusz

> 
> Thanks,
> Dominik
> 
> > 
> > Thanks,
> > Janusz
> > 
> > >
> > > The issue of possibility of too many sublevels occurrence is solved by
> > > name concatenation.
> > >
> > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> > >
> > > Dominik Karol Piatkowski (2):
> > >   Change logic of ktap parser to run on a thread
> > >   lib/igt_kmod: fix nesting igt_fixture in igt_subtest
> > >
> > > Isabella Basso (4):
> > >   lib/igt_kmod: rename kselftest functions to ktest
> > >   lib/igt_kmod.c: check if module is builtin before attempting to unload
> > >     it
> > >   lib/igt_kmod: add compatibility for KUnit
> > >   tests: DRM selftests: switch to KUnit
> > >
> > > Mauro Carvalho Chehab (2):
> > >   lib/igt_kmod: place KUnit tests on a subtest
> > >   kunit tests: add an optional name for the selftests
> > >
> > >  lib/igt_kmod.c       | 146 +++++++++-
> > >  lib/igt_kmod.h       |  14 +-
> > >  lib/igt_ktap.c       | 615
> > +++++++++++++++++++++++++++++++++++++++++++
> > >  lib/igt_ktap.h       |  50 ++++
> > >  lib/meson.build      |   1 +
> > >  tests/drm_buddy.c    |   4 +-
> > >  tests/drm_mm.c       |   5 +-
> > >  tests/kms_selftest.c |  19 ++
> > >  8 files changed, 835 insertions(+), 19 deletions(-)  create mode
> > > 100644 lib/igt_ktap.c  create mode 100644 lib/igt_ktap.h
> > >
> > >
> > 
> > 
> > 
> 
> 

Re: [igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Janusz Krzysztofik]

On Wednesday, 7 June 2023 14:45:41 CEST Mauro Carvalho Chehab wrote:
> On Wed, 07 Jun 2023 12:24:55 +0200
> Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> 
> > On Monday, 5 June 2023 12:47:12 CEST Dominik Karol Piatkowski wrote:
> > > From: Isabella Basso <isabbasso@riseup.net>
> > > 
> > > As the DRM selftests are now using KUnit [1], update IGT tests as well.
> > > 
> > > [1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/
> > > 
> > > Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> > > 
> > > v1 -> v2:
> > > - drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
> > >   with code other than IGT_EXIT_ABORT
> > > - kms_selftest: move igt_kunit tests to separate subtests
> > > - kms_selftest: fallback to igt_kselftests if all subtests failed
> > > 
> > > v2 -> v3:
> > > - expose all subtests
> > > 
> > > Signed-off-by: Dominik Karol Piątkowski 
<dominik.karol.piatkowski@intel.com>
> > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> > > ---
> > >  tests/drm_buddy.c    | 4 +++-
> > >  tests/drm_mm.c       | 4 +++-
> > >  tests/kms_selftest.c | 8 ++++++++
> > >  3 files changed, 14 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
> > > index 06876e0c..3261f0d6 100644
> > > --- a/tests/drm_buddy.c
> > > +++ b/tests/drm_buddy.c
> > > @@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's 
buddy 
> > allocator (struct drm_bu
> > >  
> > >  igt_main
> > >  {
> > > -	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > +	int ret = igt_kunit("drm_buddy_test", NULL);
> > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > +		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > >  }
> > > diff --git a/tests/drm_mm.c b/tests/drm_mm.c
> > > index 0bce7139..88f76a57 100644
> > > --- a/tests/drm_mm.c
> > > +++ b/tests/drm_mm.c
> > > @@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's 
range 
> > manager (struct drm_mm)"
> > >  
> > >  igt_main
> > >  {
> > > -	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> > > +	int ret = igt_kunit("drm_mm_test", NULL);
> > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > +		igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> > 
> > My discussion with Mauro about subtest names and their consistency with 
inline 
> > documentation has lead me to a question: have we verified if behavior of 
> > --list-subtests option under such conditional construct is consistent with 
> > expectations of the testplan tool?
> > 
> > But maybe we should still get back to a design phase and the question of 
how 
> > we want these three generic DRM selftests to behave on old and new kernels 
> > after the change.
> > 
> > Option 1:
> > We just add kunit variants as new subtests, aside the existing i915-like 
> > selftest subtests.  Whether kunit or i915-like selftest variants will 
execute 
> > or skip depends on availability of required kernel side kunit or selftest 
> > modules.
> > 
> > Option 2:
> > Each of the three tests still provides one igt_subtest_with_dynamic().  
Which 
> > dynamic subtests are executed, whether kunit or i915-like selftest or 
none, 
> > depends on availability of required kernel modules.
> > 
> > Option 3:
> > Current approach: provide only kunit subtests on kernels with kunit 
modules 
> > and only i915-like sleftest subtests otherwise.  But then, take care of 
> > --list-subtests option always returning only names of subtests that can be 
> > executed (for which kernel modules are available).
> > Aditional assumption for the testplan tool: the same kunit kernel modules 
> > available when building the testplan will be available when executing it.
> 
> It sounds to me that you're over complicating it.

No, but I was just wrong about --list-subtests behavior for option 3.  It 
always displays the name of the kunit subtest, never of the i915-selftest-like 
subtest, no matter which kernel modules are available.

> 
> At IGT build time, it doesn't really matter if the tests will run with
> KUnit or kselftest. What it matters is that igt dynamic subtest is
> properly setup, in a way that --list will display the dynamic subtest(s)
> that are part of it.
> 
> Looking further, this series touch only 3 tests:
> 
> 	- tests/drm_buddy.c
> 	- tests/drm_mm.c
> 	- tests/kms_selftest.c
> 
> The first two are related to some changes that already happened
> upstream: DRM core now uses KUnit and don't have support for
> selftests.
> 
> For KMS, I would expect that the Xe driver will require those to use
> KUnit as well, as Xe driver doesn't support selftest. It may either
> run as selftest or KUnit for i915. The IGT runtime decision to run 
> either with KUnit or via selftest may depend if the Kernel is built
> with KUnit support or not.
> 
> -
> 
> Now, preserving dynamic subtest namespace is particularly needed 
> by drm_mm, which has an extensive documentation for the subtests 
> provided by DRM core. We need to group the tests there inside
> igt_subtest_with_dynamic("all-tests"), in order to preserve the
> documentation we have.
> 
> An alternative approach would be to change it to some other
> name:
> 
> 	igt_subtest_with_dynamic("some-foo-name")
> 
> And then rename the subtests inside tests/drm_mm.c replacing
> "all-tests" with "some-foo-name".
> 
> I can't see any rationale for doing that, but, if you think it
> is worth doing that, feel free to submit a patch after we have
> this patch series merged.

So we're back to the discussion limited to subtest naming, while I was not 
talking about subtest names, only about the structure of the tests, and for me 
it seems like you missed my points.

Having the whole series applied, we can now observe two different approaches:

Tests drm_mm and drm_buddy implement my option 1.  Command line option 
--list-subtests displays only the name of the kunit subtest.  Its name 
"all-tests" is deliberately the same as that of the i915-like selftest 
subtest, called conditionally if the former returns an error code (which never 
happens with --list-subtests, I believe).  Documentation checking tool, which 
ignores dynamic sub-subtests documentation sections if any, will be happy.  If 
the documentation provides details on individual dynamic sub-subtests then it 
will be correct as long as respective kunit kernel modules provide the same 
set of tests as their old i915-selftest-like counterparts.

OTOH, test kms_selftest implements my option 3.  Command line option 
--list-subtests displays a static list of several kunit subtest names, 
followed by the name of the i915-selftest-like subtest "all-test", which is 
not consistent with drm_mm and drm_buddy.  All subtests, including the i915-
selftest-like one, will have to be documented to make the documentation 
checking tool happy.

Other than that, I think that returning from a subtest body via just return, 
as implemented by patch 6/8, and not via either igt_success() or igt_fail() 
and friends is not supported, then I think we can't predict how the tests 
modified with this series will behave under different conditions.

Thanks,
Janusz

> 
> Regards,
> Mauro
> 

Re: [igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Janusz Krzysztofik]

Auto-correction, sorry.

On Wednesday, 7 June 2023 16:35:32 CEST Janusz Krzysztofik wrote:
> On Wednesday, 7 June 2023 14:45:41 CEST Mauro Carvalho Chehab wrote:
> > On Wed, 07 Jun 2023 12:24:55 +0200
> > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> > 
> > > On Monday, 5 June 2023 12:47:12 CEST Dominik Karol Piatkowski wrote:
> > > > From: Isabella Basso <isabbasso@riseup.net>
> > > > 
> > > > As the DRM selftests are now using KUnit [1], update IGT tests as well.
> > > > 
> > > > [1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/
> > > > 
> > > > Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> > > > 
> > > > v1 -> v2:
> > > > - drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
> > > >   with code other than IGT_EXIT_ABORT
> > > > - kms_selftest: move igt_kunit tests to separate subtests
> > > > - kms_selftest: fallback to igt_kselftests if all subtests failed
> > > > 
> > > > v2 -> v3:
> > > > - expose all subtests
> > > > 
> > > > Signed-off-by: Dominik Karol Piątkowski 
> <dominik.karol.piatkowski@intel.com>
> > > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> > > > ---
> > > >  tests/drm_buddy.c    | 4 +++-
> > > >  tests/drm_mm.c       | 4 +++-
> > > >  tests/kms_selftest.c | 8 ++++++++
> > > >  3 files changed, 14 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
> > > > index 06876e0c..3261f0d6 100644
> > > > --- a/tests/drm_buddy.c
> > > > +++ b/tests/drm_buddy.c
> > > > @@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's 
> buddy 
> > > allocator (struct drm_bu
> > > >  
> > > >  igt_main
> > > >  {
> > > > -	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > > +	int ret = igt_kunit("drm_buddy_test", NULL);
> > > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > > +		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > >  }
> > > > diff --git a/tests/drm_mm.c b/tests/drm_mm.c
> > > > index 0bce7139..88f76a57 100644
> > > > --- a/tests/drm_mm.c
> > > > +++ b/tests/drm_mm.c
> > > > @@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's 
> range 
> > > manager (struct drm_mm)"
> > > >  
> > > >  igt_main
> > > >  {
> > > > -	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> > > > +	int ret = igt_kunit("drm_mm_test", NULL);
> > > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > > +		igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> > > 
> > > My discussion with Mauro about subtest names and their consistency with 
> inline 
> > > documentation has lead me to a question: have we verified if behavior of 
> > > --list-subtests option under such conditional construct is consistent with 
> > > expectations of the testplan tool?
> > > 
> > > But maybe we should still get back to a design phase and the question of 
> how 
> > > we want these three generic DRM selftests to behave on old and new kernels 
> > > after the change.
> > > 
> > > Option 1:
> > > We just add kunit variants as new subtests, aside the existing i915-like 
> > > selftest subtests.  Whether kunit or i915-like selftest variants will 
> execute 
> > > or skip depends on availability of required kernel side kunit or selftest 
> > > modules.
> > > 
> > > Option 2:
> > > Each of the three tests still provides one igt_subtest_with_dynamic().  
> Which 
> > > dynamic subtests are executed, whether kunit or i915-like selftest or 
> none, 
> > > depends on availability of required kernel modules.
> > > 
> > > Option 3:
> > > Current approach: provide only kunit subtests on kernels with kunit 
> modules 
> > > and only i915-like sleftest subtests otherwise.  But then, take care of 
> > > --list-subtests option always returning only names of subtests that can be 
> > > executed (for which kernel modules are available).
> > > Aditional assumption for the testplan tool: the same kunit kernel modules 
> > > available when building the testplan will be available when executing it.
> > 
> > It sounds to me that you're over complicating it.
> 
> No, but I was just wrong about --list-subtests behavior for option 3.  It 
> always displays the name of the kunit subtest, never of the i915-selftest-like 
> subtest, no matter which kernel modules are available.
> 
> > 
> > At IGT build time, it doesn't really matter if the tests will run with
> > KUnit or kselftest. What it matters is that igt dynamic subtest is
> > properly setup, in a way that --list will display the dynamic subtest(s)
> > that are part of it.
> > 
> > Looking further, this series touch only 3 tests:
> > 
> > 	- tests/drm_buddy.c
> > 	- tests/drm_mm.c
> > 	- tests/kms_selftest.c
> > 
> > The first two are related to some changes that already happened
> > upstream: DRM core now uses KUnit and don't have support for
> > selftests.
> > 
> > For KMS, I would expect that the Xe driver will require those to use
> > KUnit as well, as Xe driver doesn't support selftest. It may either
> > run as selftest or KUnit for i915. The IGT runtime decision to run 
> > either with KUnit or via selftest may depend if the Kernel is built
> > with KUnit support or not.
> > 
> > -
> > 
> > Now, preserving dynamic subtest namespace is particularly needed 
> > by drm_mm, which has an extensive documentation for the subtests 
> > provided by DRM core. We need to group the tests there inside
> > igt_subtest_with_dynamic("all-tests"), in order to preserve the
> > documentation we have.
> > 
> > An alternative approach would be to change it to some other
> > name:
> > 
> > 	igt_subtest_with_dynamic("some-foo-name")
> > 
> > And then rename the subtests inside tests/drm_mm.c replacing
> > "all-tests" with "some-foo-name".
> > 
> > I can't see any rationale for doing that, but, if you think it
> > is worth doing that, feel free to submit a patch after we have
> > this patch series merged.
> 
> So we're back to the discussion limited to subtest naming, while I was not 
> talking about subtest names, only about the structure of the tests, and for me 
> it seems like you missed my points.
> 
> Having the whole series applied, we can now observe two different approaches:
> 
> Tests drm_mm and drm_buddy implement my option 1.  

Should read: option 3

> Command line option 
> --list-subtests displays only the name of the kunit subtest.  Its name 
> "all-tests" is deliberately the same as that of the i915-like selftest 
> subtest, called conditionally if the former returns an error code (which never 
> happens with --list-subtests, I believe).  Documentation checking tool, which 
> ignores dynamic sub-subtests documentation sections if any, will be happy.  If 
> the documentation provides details on individual dynamic sub-subtests then it 
> will be correct as long as respective kunit kernel modules provide the same 
> set of tests as their old i915-selftest-like counterparts.
> 
> OTOH, test kms_selftest implements my option 3.  
 
Should read: option 1

> Command line option 
> --list-subtests displays a static list of several kunit subtest names, 
> followed by the name of the i915-selftest-like subtest "all-test", which is 
> not consistent with drm_mm and drm_buddy.  All subtests, including the i915-
> selftest-like one, will have to be documented to make the documentation 
> checking tool happy.
> 
> Other than that, I think that returning from a subtest body via just return, 
> as implemented by patch 6/8, and not via either igt_success() or igt_fail() 
> and friends is not supported, then I think we can't predict how the tests 
> modified with this series will behave under different conditions.
> 
> Thanks,
> Janusz
> 
> > 
> > Regards,
> > Mauro
> > 
> 
> 

Re: [igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Mauro Carvalho Chehab]

On Wed, 07 Jun 2023 16:39:23 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

> Auto-correction, sorry.
> 
> On Wednesday, 7 June 2023 16:35:32 CEST Janusz Krzysztofik wrote:
> > On Wednesday, 7 June 2023 14:45:41 CEST Mauro Carvalho Chehab wrote:  
> > > On Wed, 07 Jun 2023 12:24:55 +0200
> > > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> > >   
> > > > On Monday, 5 June 2023 12:47:12 CEST Dominik Karol Piatkowski wrote:  
> > > > > From: Isabella Basso <isabbasso@riseup.net>
> > > > > 
> > > > > As the DRM selftests are now using KUnit [1], update IGT tests as well.
> > > > > 
> > > > > [1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/
> > > > > 
> > > > > Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> > > > > 
> > > > > v1 -> v2:
> > > > > - drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
> > > > >   with code other than IGT_EXIT_ABORT
> > > > > - kms_selftest: move igt_kunit tests to separate subtests
> > > > > - kms_selftest: fallback to igt_kselftests if all subtests failed
> > > > > 
> > > > > v2 -> v3:
> > > > > - expose all subtests
> > > > > 
> > > > > Signed-off-by: Dominik Karol Piątkowski   
> > <dominik.karol.piatkowski@intel.com>  
> > > > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > > > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> > > > > ---
> > > > >  tests/drm_buddy.c    | 4 +++-
> > > > >  tests/drm_mm.c       | 4 +++-
> > > > >  tests/kms_selftest.c | 8 ++++++++
> > > > >  3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
> > > > > index 06876e0c..3261f0d6 100644
> > > > > --- a/tests/drm_buddy.c
> > > > > +++ b/tests/drm_buddy.c
> > > > > @@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's   
> > buddy   
> > > > allocator (struct drm_bu  
> > > > >  
> > > > >  igt_main
> > > > >  {
> > > > > -	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > > > +	int ret = igt_kunit("drm_buddy_test", NULL);
> > > > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > > > +		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > > >  }
> > > > > diff --git a/tests/drm_mm.c b/tests/drm_mm.c
> > > > > index 0bce7139..88f76a57 100644
> > > > > --- a/tests/drm_mm.c
> > > > > +++ b/tests/drm_mm.c
> > > > > @@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's   
> > range   
> > > > manager (struct drm_mm)"  
> > > > >  
> > > > >  igt_main
> > > > >  {
> > > > > -	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> > > > > +	int ret = igt_kunit("drm_mm_test", NULL);
> > > > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > > > +		igt_kselftests("test-drm_mm", NULL, NULL, NULL);  
> > > > 
> > > > My discussion with Mauro about subtest names and their consistency with   
> > inline   
> > > > documentation has lead me to a question: have we verified if behavior of 
> > > > --list-subtests option under such conditional construct is consistent with 
> > > > expectations of the testplan tool?
> > > > 
> > > > But maybe we should still get back to a design phase and the question of   
> > how   
> > > > we want these three generic DRM selftests to behave on old and new kernels 
> > > > after the change.
> > > > 
> > > > Option 1:
> > > > We just add kunit variants as new subtests, aside the existing i915-like 
> > > > selftest subtests.  Whether kunit or i915-like selftest variants will   
> > execute   
> > > > or skip depends on availability of required kernel side kunit or selftest 
> > > > modules.
> > > > 
> > > > Option 2:
> > > > Each of the three tests still provides one igt_subtest_with_dynamic().    
> > Which   
> > > > dynamic subtests are executed, whether kunit or i915-like selftest or   
> > none,   
> > > > depends on availability of required kernel modules.
> > > > 
> > > > Option 3:
> > > > Current approach: provide only kunit subtests on kernels with kunit   
> > modules   
> > > > and only i915-like sleftest subtests otherwise.  But then, take care of 
> > > > --list-subtests option always returning only names of subtests that can be 
> > > > executed (for which kernel modules are available).
> > > > Aditional assumption for the testplan tool: the same kunit kernel modules 
> > > > available when building the testplan will be available when executing it.  
> > > 
> > > It sounds to me that you're over complicating it.  
> > 
> > No, but I was just wrong about --list-subtests behavior for option 3.  It 
> > always displays the name of the kunit subtest, never of the i915-selftest-like 
> > subtest, no matter which kernel modules are available.

It always display the name that igt_subtest_dynamic() uses. Such
behavior should be preserved after adding support for KUnit.





> >   
> > > 
> > > At IGT build time, it doesn't really matter if the tests will run with
> > > KUnit or kselftest. What it matters is that igt dynamic subtest is
> > > properly setup, in a way that --list will display the dynamic subtest(s)
> > > that are part of it.
> > > 
> > > Looking further, this series touch only 3 tests:
> > > 
> > > 	- tests/drm_buddy.c
> > > 	- tests/drm_mm.c
> > > 	- tests/kms_selftest.c
> > > 
> > > The first two are related to some changes that already happened
> > > upstream: DRM core now uses KUnit and don't have support for
> > > selftests.
> > > 
> > > For KMS, I would expect that the Xe driver will require those to use
> > > KUnit as well, as Xe driver doesn't support selftest. It may either
> > > run as selftest or KUnit for i915. The IGT runtime decision to run 
> > > either with KUnit or via selftest may depend if the Kernel is built
> > > with KUnit support or not.
> > > 
> > > -
> > > 
> > > Now, preserving dynamic subtest namespace is particularly needed 
> > > by drm_mm, which has an extensive documentation for the subtests 
> > > provided by DRM core. We need to group the tests there inside
> > > igt_subtest_with_dynamic("all-tests"), in order to preserve the
> > > documentation we have.
> > > 
> > > An alternative approach would be to change it to some other
> > > name:
> > > 
> > > 	igt_subtest_with_dynamic("some-foo-name")
> > > 
> > > And then rename the subtests inside tests/drm_mm.c replacing
> > > "all-tests" with "some-foo-name".
> > > 
> > > I can't see any rationale for doing that, but, if you think it
> > > is worth doing that, feel free to submit a patch after we have
> > > this patch series merged.  
> > 
> > So we're back to the discussion limited to subtest naming, while I was not 
> > talking about subtest names, only about the structure of the tests, and for me 
> > it seems like you missed my points.
> > 
> > Having the whole series applied, we can now observe two different approaches:
> > 
> > Tests drm_mm and drm_buddy implement my option 1.    
> 
> Should read: option 3

Yes, I missed the point. Could you please reply with a patch showing
what kind of changes are you talking about?

Regards,
Mauro

Re: [igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Janusz Krzysztofik]

On Wednesday, 7 June 2023 17:59:20 CEST Mauro Carvalho Chehab wrote:
> On Wed, 07 Jun 2023 16:39:23 +0200
> Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> 
> > Auto-correction, sorry.
> > 
> > On Wednesday, 7 June 2023 16:35:32 CEST Janusz Krzysztofik wrote:
> > > On Wednesday, 7 June 2023 14:45:41 CEST Mauro Carvalho Chehab wrote:  
> > > > On Wed, 07 Jun 2023 12:24:55 +0200
> > > > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> > > >   
> > > > > On Monday, 5 June 2023 12:47:12 CEST Dominik Karol Piatkowski wrote:  
> > > > > > From: Isabella Basso <isabbasso@riseup.net>
> > > > > > 
> > > > > > As the DRM selftests are now using KUnit [1], update IGT tests as well.
> > > > > > 
> > > > > > [1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/
> > > > > > 
> > > > > > Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> > > > > > 
> > > > > > v1 -> v2:
> > > > > > - drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
> > > > > >   with code other than IGT_EXIT_ABORT
> > > > > > - kms_selftest: move igt_kunit tests to separate subtests
> > > > > > - kms_selftest: fallback to igt_kselftests if all subtests failed
> > > > > > 
> > > > > > v2 -> v3:
> > > > > > - expose all subtests
> > > > > > 
> > > > > > Signed-off-by: Dominik Karol Piątkowski   
> > > <dominik.karol.piatkowski@intel.com>  
> > > > > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > > > > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> > > > > > ---
> > > > > >  tests/drm_buddy.c    | 4 +++-
> > > > > >  tests/drm_mm.c       | 4 +++-
> > > > > >  tests/kms_selftest.c | 8 ++++++++
> > > > > >  3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
> > > > > > index 06876e0c..3261f0d6 100644
> > > > > > --- a/tests/drm_buddy.c
> > > > > > +++ b/tests/drm_buddy.c
> > > > > > @@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's   
> > > buddy   
> > > > > allocator (struct drm_bu  
> > > > > >  
> > > > > >  igt_main
> > > > > >  {
> > > > > > -	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > > > > +	int ret = igt_kunit("drm_buddy_test", NULL);
> > > > > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > > > > +		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > > > >  }
> > > > > > diff --git a/tests/drm_mm.c b/tests/drm_mm.c
> > > > > > index 0bce7139..88f76a57 100644
> > > > > > --- a/tests/drm_mm.c
> > > > > > +++ b/tests/drm_mm.c
> > > > > > @@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's   
> > > range   
> > > > > manager (struct drm_mm)"  
> > > > > >  
> > > > > >  igt_main
> > > > > >  {
> > > > > > -	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> > > > > > +	int ret = igt_kunit("drm_mm_test", NULL);
> > > > > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > > > > +		igt_kselftests("test-drm_mm", NULL, NULL, NULL);  
> > > > > 
> > > > > My discussion with Mauro about subtest names and their consistency with   
> > > inline   
> > > > > documentation has lead me to a question: have we verified if behavior of 
> > > > > --list-subtests option under such conditional construct is consistent with 
> > > > > expectations of the testplan tool?
> > > > > 
> > > > > But maybe we should still get back to a design phase and the question of   
> > > how   
> > > > > we want these three generic DRM selftests to behave on old and new kernels 
> > > > > after the change.
> > > > > 
> > > > > Option 1:
> > > > > We just add kunit variants as new subtests, aside the existing i915-like 
> > > > > selftest subtests.  Whether kunit or i915-like selftest variants will   
> > > execute   
> > > > > or skip depends on availability of required kernel side kunit or selftest 
> > > > > modules.
> > > > > 
> > > > > Option 2:
> > > > > Each of the three tests still provides one igt_subtest_with_dynamic().    
> > > Which   
> > > > > dynamic subtests are executed, whether kunit or i915-like selftest or   
> > > none,   
> > > > > depends on availability of required kernel modules.
> > > > > 
> > > > > Option 3:
> > > > > Current approach: provide only kunit subtests on kernels with kunit   
> > > modules   
> > > > > and only i915-like sleftest subtests otherwise.  But then, take care of 
> > > > > --list-subtests option always returning only names of subtests that can be 
> > > > > executed (for which kernel modules are available).
> > > > > Aditional assumption for the testplan tool: the same kunit kernel modules 
> > > > > available when building the testplan will be available when executing it.  
> > > > 
> > > > It sounds to me that you're over complicating it.  
> > > 
> > > No, but I was just wrong about --list-subtests behavior for option 3.  It 
> > > always displays the name of the kunit subtest, never of the i915-selftest-like 
> > > subtest, no matter which kernel modules are available.
> 
> It always display the name that igt_subtest_dynamic() uses. Such
> behavior should be preserved after adding support for KUnit.

Each instance of igt_subtest_dynamic()?  If there are two, one for kunit, one 
for i915-like sleftest, and the second one is called conditionally, only if 
the first one fails, then what?
The first one never fails with --list-subtests option provided.

> 
> > >   
> > > > 
> > > > At IGT build time, it doesn't really matter if the tests will run with
> > > > KUnit or kselftest. What it matters is that igt dynamic subtest is
> > > > properly setup, in a way that --list will display the dynamic subtest(s)
> > > > that are part of it.
> > > > 
> > > > Looking further, this series touch only 3 tests:
> > > > 
> > > > 	- tests/drm_buddy.c
> > > > 	- tests/drm_mm.c
> > > > 	- tests/kms_selftest.c
> > > > 
> > > > The first two are related to some changes that already happened
> > > > upstream: DRM core now uses KUnit and don't have support for
> > > > selftests.
> > > > 
> > > > For KMS, I would expect that the Xe driver will require those to use
> > > > KUnit as well, as Xe driver doesn't support selftest. It may either
> > > > run as selftest or KUnit for i915. The IGT runtime decision to run 
> > > > either with KUnit or via selftest may depend if the Kernel is built
> > > > with KUnit support or not.
> > > > 
> > > > -
> > > > 
> > > > Now, preserving dynamic subtest namespace is particularly needed 
> > > > by drm_mm, which has an extensive documentation for the subtests 
> > > > provided by DRM core. We need to group the tests there inside
> > > > igt_subtest_with_dynamic("all-tests"), in order to preserve the
> > > > documentation we have.
> > > > 
> > > > An alternative approach would be to change it to some other
> > > > name:
> > > > 
> > > > 	igt_subtest_with_dynamic("some-foo-name")
> > > > 
> > > > And then rename the subtests inside tests/drm_mm.c replacing
> > > > "all-tests" with "some-foo-name".
> > > > 
> > > > I can't see any rationale for doing that, but, if you think it
> > > > is worth doing that, feel free to submit a patch after we have
> > > > this patch series merged.  
> > > 
> > > So we're back to the discussion limited to subtest naming, while I was not 
> > > talking about subtest names, only about the structure of the tests, and for me 
> > > it seems like you missed my points.
> > > 
> > > Having the whole series applied, we can now observe two different approaches:
> > > 
> > > Tests drm_mm and drm_buddy implement my option 1.    
> > 
> > Should read: option 3
> 
> Yes, I missed the point. Could you please reply with a patch showing
> what kind of changes are you talking about?

I'm not sure what you mean.  Would you like me to reply with a patch that 
fixes the issues I've pointed out?  Or would you like me to respond to another 
patch of this series that's more related to my points than this one?

If the former then I don't have a solution ready out of top of my head since 
I'm still not sure how we want to have coexistence of backword compatible 
subtests with new kunit ones designed.

Thanks,
Janusz


> 
> Regards,
> Mauro
> 
> 
> 
> 

Re: [igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit

[Mauro Carvalho Chehab]

On Wed, 07 Jun 2023 19:40:39 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

> On Wednesday, 7 June 2023 17:59:20 CEST Mauro Carvalho Chehab wrote:
> > On Wed, 07 Jun 2023 16:39:23 +0200
> > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> >   
> > > Auto-correction, sorry.
> > > 
> > > On Wednesday, 7 June 2023 16:35:32 CEST Janusz Krzysztofik wrote:  
> > > > On Wednesday, 7 June 2023 14:45:41 CEST Mauro Carvalho Chehab wrote:    
> > > > > On Wed, 07 Jun 2023 12:24:55 +0200
> > > > > Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> > > > >     
> > > > > > On Monday, 5 June 2023 12:47:12 CEST Dominik Karol Piatkowski wrote:    
> > > > > > > From: Isabella Basso <isabbasso@riseup.net>
> > > > > > > 
> > > > > > > As the DRM selftests are now using KUnit [1], update IGT tests as well.
> > > > > > > 
> > > > > > > [1] - https://lore.kernel.org/all/20220708203052.236290-1-maira.canal@usp.br/
> > > > > > > 
> > > > > > > Signed-off-by: Isabella Basso <isabbasso@riseup.net>
> > > > > > > 
> > > > > > > v1 -> v2:
> > > > > > > - drm_buddy|drm_mm: fallback to igt_kselftests if igt_kunit failed
> > > > > > >   with code other than IGT_EXIT_ABORT
> > > > > > > - kms_selftest: move igt_kunit tests to separate subtests
> > > > > > > - kms_selftest: fallback to igt_kselftests if all subtests failed
> > > > > > > 
> > > > > > > v2 -> v3:
> > > > > > > - expose all subtests
> > > > > > > 
> > > > > > > Signed-off-by: Dominik Karol Piątkowski     
> > > > <dominik.karol.piatkowski@intel.com>    
> > > > > > > Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > > > > > Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> > > > > > > ---
> > > > > > >  tests/drm_buddy.c    | 4 +++-
> > > > > > >  tests/drm_mm.c       | 4 +++-
> > > > > > >  tests/kms_selftest.c | 8 ++++++++
> > > > > > >  3 files changed, 14 insertions(+), 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/tests/drm_buddy.c b/tests/drm_buddy.c
> > > > > > > index 06876e0c..3261f0d6 100644
> > > > > > > --- a/tests/drm_buddy.c
> > > > > > > +++ b/tests/drm_buddy.c
> > > > > > > @@ -10,5 +10,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's     
> > > > buddy     
> > > > > > allocator (struct drm_bu    
> > > > > > >  
> > > > > > >  igt_main
> > > > > > >  {
> > > > > > > -	igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > > > > > +	int ret = igt_kunit("drm_buddy_test", NULL);
> > > > > > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > > > > > +		igt_kselftests("test-drm_buddy", NULL, NULL, NULL);
> > > > > > >  }
> > > > > > > diff --git a/tests/drm_mm.c b/tests/drm_mm.c
> > > > > > > index 0bce7139..88f76a57 100644
> > > > > > > --- a/tests/drm_mm.c
> > > > > > > +++ b/tests/drm_mm.c
> > > > > > > @@ -156,5 +156,7 @@ IGT_TEST_DESCRIPTION("Basic sanity check of DRM's     
> > > > range     
> > > > > > manager (struct drm_mm)"    
> > > > > > >  
> > > > > > >  igt_main
> > > > > > >  {
> > > > > > > -	igt_kselftests("test-drm_mm", NULL, NULL, NULL);
> > > > > > > +	int ret = igt_kunit("drm_mm_test", NULL);
> > > > > > > +	if (ret != 0 && ret != IGT_EXIT_ABORT)
> > > > > > > +		igt_kselftests("test-drm_mm", NULL, NULL, NULL);    
> > > > > > 
> > > > > > My discussion with Mauro about subtest names and their consistency with     
> > > > inline     
> > > > > > documentation has lead me to a question: have we verified if behavior of 
> > > > > > --list-subtests option under such conditional construct is consistent with 
> > > > > > expectations of the testplan tool?
> > > > > > 
> > > > > > But maybe we should still get back to a design phase and the question of     
> > > > how     
> > > > > > we want these three generic DRM selftests to behave on old and new kernels 
> > > > > > after the change.
> > > > > > 
> > > > > > Option 1:
> > > > > > We just add kunit variants as new subtests, aside the existing i915-like 
> > > > > > selftest subtests.  Whether kunit or i915-like selftest variants will     
> > > > execute     
> > > > > > or skip depends on availability of required kernel side kunit or selftest 
> > > > > > modules.
> > > > > > 
> > > > > > Option 2:
> > > > > > Each of the three tests still provides one igt_subtest_with_dynamic().      
> > > > Which     
> > > > > > dynamic subtests are executed, whether kunit or i915-like selftest or     
> > > > none,     
> > > > > > depends on availability of required kernel modules.
> > > > > > 
> > > > > > Option 3:
> > > > > > Current approach: provide only kunit subtests on kernels with kunit     
> > > > modules     
> > > > > > and only i915-like sleftest subtests otherwise.  But then, take care of 
> > > > > > --list-subtests option always returning only names of subtests that can be 
> > > > > > executed (for which kernel modules are available).
> > > > > > Aditional assumption for the testplan tool: the same kunit kernel modules 
> > > > > > available when building the testplan will be available when executing it.    
> > > > > 
> > > > > It sounds to me that you're over complicating it.    
> > > > 
> > > > No, but I was just wrong about --list-subtests behavior for option 3.  It 
> > > > always displays the name of the kunit subtest, never of the i915-selftest-like 
> > > > subtest, no matter which kernel modules are available.  
> > 
> > It always display the name that igt_subtest_dynamic() uses. Such
> > behavior should be preserved after adding support for KUnit.  
> 
> Each instance of igt_subtest_dynamic()?  If there are two, one for kunit, one 
> for i915-like sleftest, and the second one is called conditionally, only if 
> the first one fails, then what?
> The first one never fails with --list-subtests option provided.

I don't see any reason to have a fallback. For DRM tests, we should
simply remove selftests, as, afaikt, upstream doesn't support it
anymore. The same applies for Xe and KMS, as we won't have support
for selftests at the Xe driver.

Selftests should only be kept for i915 selftests, which this series
don't touch.

So, what I meant to say is that KUnit tests - after their conversion
from selftest - should use igt_subtest_dynamic(). This is needed in order
for IGT to list KUnit tests as dynamic subtests when using --list parameter.

> 
> >   
> > > >     
> > > > > 
> > > > > At IGT build time, it doesn't really matter if the tests will run with
> > > > > KUnit or kselftest. What it matters is that igt dynamic subtest is
> > > > > properly setup, in a way that --list will display the dynamic subtest(s)
> > > > > that are part of it.
> > > > > 
> > > > > Looking further, this series touch only 3 tests:
> > > > > 
> > > > > 	- tests/drm_buddy.c
> > > > > 	- tests/drm_mm.c
> > > > > 	- tests/kms_selftest.c
> > > > > 
> > > > > The first two are related to some changes that already happened
> > > > > upstream: DRM core now uses KUnit and don't have support for
> > > > > selftests.
> > > > > 
> > > > > For KMS, I would expect that the Xe driver will require those to use
> > > > > KUnit as well, as Xe driver doesn't support selftest. It may either
> > > > > run as selftest or KUnit for i915. The IGT runtime decision to run 
> > > > > either with KUnit or via selftest may depend if the Kernel is built
> > > > > with KUnit support or not.
> > > > > 
> > > > > -
> > > > > 
> > > > > Now, preserving dynamic subtest namespace is particularly needed 
> > > > > by drm_mm, which has an extensive documentation for the subtests 
> > > > > provided by DRM core. We need to group the tests there inside
> > > > > igt_subtest_with_dynamic("all-tests"), in order to preserve the
> > > > > documentation we have.
> > > > > 
> > > > > An alternative approach would be to change it to some other
> > > > > name:
> > > > > 
> > > > > 	igt_subtest_with_dynamic("some-foo-name")
> > > > > 
> > > > > And then rename the subtests inside tests/drm_mm.c replacing
> > > > > "all-tests" with "some-foo-name".
> > > > > 
> > > > > I can't see any rationale for doing that, but, if you think it
> > > > > is worth doing that, feel free to submit a patch after we have
> > > > > this patch series merged.    
> > > > 
> > > > So we're back to the discussion limited to subtest naming, while I was not 
> > > > talking about subtest names, only about the structure of the tests, and for me 
> > > > it seems like you missed my points.
> > > > 
> > > > Having the whole series applied, we can now observe two different approaches:
> > > > 
> > > > Tests drm_mm and drm_buddy implement my option 1.      
> > > 
> > > Should read: option 3  
> > 
> > Yes, I missed the point. Could you please reply with a patch showing
> > what kind of changes are you talking about?  
> 
> I'm not sure what you mean.  Would you like me to reply with a patch that 
> fixes the issues I've pointed out?

Yes, a patch on the top of this series, doing the changes you think
are still needed to get this series merged.

>  Or would you like me to respond to another 
> patch of this series that's more related to my points than this one?
> 
> If the former then I don't have a solution ready out of top of my head since 
> I'm still not sure how we want to have coexistence of backword compatible 
> subtests with new kunit ones designed.
> 
> Thanks,
> Janusz
> 
> 
> > 
> > Regards,
> > Mauro
> > 
> > 
> > 
> >   
> 
> 
> 
> 

Re: [igt-dev] [PATCH i-g-t 8/8] lib/igt_kmod: fix nesting igt_fixture in igt_subtest

[Mauro Carvalho Chehab]

Hi Dominik,


Please add this patch to the KUnit series:

https://patchwork.freedesktop.org/patch/541702/?series=119074&rev=1


It contains the Xe KUnit tests.


Regards,
Mauro

On 6/5/23 12:47, Dominik Karol Piatkowski wrote:
> Fix the following issue:
> 	$ ./build/tests/drm_buddy
> 	Starting subtest: all-tests
> 	nesting igt_fixture in igt_subtest is invalid
> 	please refer to lib/igt_core documentation
>
> Signed-off-by: Dominik Karol Piątkowski <dominik.karol.piatkowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> ---
>   lib/igt_kmod.c | 80 ++++++++++++++++++++++++--------------------------
>   1 file changed, 38 insertions(+), 42 deletions(-)
>
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index c62eb97a..2c0cc026 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -772,47 +772,45 @@ static int __igt_kunit(const char *module_name, const char *opts)
>   		return ret;
>   	}
>   
> -	igt_fixture {
> -		if (igt_ktest_begin(&tst) != 0) {
> -			igt_warn("Unable to begin ktest for %s\n", module_name);
> +	if (igt_ktest_begin(&tst) != 0) {
> +		igt_warn("Unable to begin ktest for %s\n", module_name);
>   
> -			igt_ktest_fini(&tst);
> -			return ret;
> -		}
> +		igt_ktest_fini(&tst);
> +		return ret;
> +	}
>   
> -		if (tst.kmsg < 0) {
> -			igt_warn("Could not open /dev/kmsg\n");
> -			goto unload;
> -		}
> +	if (tst.kmsg < 0) {
> +		igt_warn("Could not open /dev/kmsg\n");
> +		goto unload;
> +	}
>   
> -		if (lseek(tst.kmsg, 0, SEEK_END)) {
> -			igt_warn("Could not seek the end of /dev/kmsg\n");
> -			goto unload;
> -		}
> +	if (lseek(tst.kmsg, 0, SEEK_END)) {
> +		igt_warn("Could not seek the end of /dev/kmsg\n");
> +		goto unload;
> +	}
>   
> -		f = fdopen(tst.kmsg, "r");
> +	f = fdopen(tst.kmsg, "r");
>   
> -		if (f == NULL) {
> -			igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> -			goto unload;
> -		}
> +	if (f == NULL) {
> +		igt_warn("Could not turn /dev/kmsg file descriptor into a FILE pointer\n");
> +		goto unload;
> +	}
>   
> -		/* The KUnit module is required for running any KUnit tests */
> -		if (igt_kmod_load("kunit", NULL) != 0 ||
> -		    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> -			igt_warn("Unable to load KUnit\n");
> -			igt_fail(IGT_EXIT_FAILURE);
> -		}
> +	/* The KUnit module is required for running any KUnit tests */
> +	if (igt_kmod_load("kunit", NULL) != 0 ||
> +	    kmod_module_new_from_name(kmod_ctx(), "kunit", &kunit_kmod) != 0) {
> +		igt_warn("Unable to load KUnit\n");
> +		igt_fail(IGT_EXIT_FAILURE);
> +	}
>   
> -		is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
> +	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
>   
> -		results = ktap_parser_start(f, is_builtin);
> +	results = ktap_parser_start(f, is_builtin);
>   
> -		if (igt_kmod_load(module_name, opts) != 0) {
> -			igt_warn("Unable to load %s module\n", module_name);
> -			ret = ktap_parser_stop();
> -			igt_fail(IGT_EXIT_FAILURE);
> -		}
> +	if (igt_kmod_load(module_name, opts) != 0) {
> +		igt_warn("Unable to load %s module\n", module_name);
> +		ret = ktap_parser_stop();
> +		igt_fail(IGT_EXIT_FAILURE);
>   	}
>   
>   	while (READ_ONCE(results->still_running) || READ_ONCE(results->head) != NULL)
> @@ -820,7 +818,7 @@ static int __igt_kunit(const char *module_name, const char *opts)
>   		if (READ_ONCE(results->head) != NULL) {
>   			pthread_mutex_lock(&results->mutex);
>   
> -			igt_subtest(results->head->test_name) {
> +			igt_dynamic(results->head->test_name) {
>   				if (READ_ONCE(results->head->passed))
>   					igt_success();
>   				else
> @@ -836,19 +834,17 @@ static int __igt_kunit(const char *module_name, const char *opts)
>   	}
>   
>   unload:
> -	igt_fixture {
> -		igt_ktest_end(&tst);
> +	igt_ktest_end(&tst);
>   
> -		igt_ktest_fini(&tst);
> +	igt_ktest_fini(&tst);
>   
> -		ret = ktap_parser_stop();
> +	ret = ktap_parser_stop();
>   
> -		if (ret != 0)
> -			ret = IGT_EXIT_ABORT;
> +	if (ret != 0)
> +		ret = IGT_EXIT_ABORT;
>   
> -		if (ret == 0)
> -			igt_success();
> -	}
> +	if (ret == 0)
> +		igt_success();
>   	return ret;
>   }
>   

Re: [igt-dev] [PATCH v5 i-g-t 0/8] Introduce KUnit

[Janusz Krzysztofik]

Hi Dominik,

On Monday, 5 June 2023 12:47:08 CEST Dominik Karol Piatkowski wrote:
> This series is a continuation of Isabella's work on introducing
> KUnit to IGT.
> 
> Sample drm_buddy output:
> 	Starting subtest: all-tests
> 	[thread:8003] TAP version 1
> 	[thread:8003] Executing 6 tests in: drm_buddy
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_limit

Does TAP version 1 allow for more than one top level section starting with 
"Executing N tests in: <name>"?  If only one was expected then we could 
shorten dynamic sub-subtest names by dropping the initial top level <name> 
component from concatenated sub-subtest names, and maybe even use the top 
level <name> as IGT subtest name ("Starting subtest: <name>") 
(automatically?).

Besides, IGT traditionally uses dashes in subtest names, not underscores.  I'm 
not voting for replacing all underscores obtained from ktap with dashes, only 
using dashes as concatenation marks (where applicable) instead of your double 
underscores.

> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_limit: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_range
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_range: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_optimistic
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_optimistic: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_pessimistic
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pessimistic: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_smoke
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_smoke: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy__drm_test_buddy_alloc_pathological
> 	Dynamic subtest drm_buddy__drm_test_buddy_alloc_pathological: SUCCESS (0.000s)
> 	Starting dynamic subtest: drm_buddy
> 	Dynamic subtest drm_buddy: SUCCESS (0.000s)

Having dynamic subtest names as concatenations of names from all levels of 
ktap subtest nesting (a consequence of only one dynamic sub-subtest nesting 
level expected by IGT users, including CI), I'm not sure if we should still 
report higher levels as separate subtests if we have already reported those 
nested inside them.  While CI can still be happy with that, bug 
classification, categorization and reporting as well as passrate calculation 
will be affected, I'm afraid.  Do we accept that impact?

Thanks,
Janusz


> 	Subtest all-tests: SUCCESS (0.707s)
> 
> The issue of possibility of too many sublevels occurrence is solved
> by name concatenation.
> 
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>
> 
> Dominik Karol Piatkowski (2):
>   Change logic of ktap parser to run on a thread
>   lib/igt_kmod: fix nesting igt_fixture in igt_subtest
> 
> Isabella Basso (4):
>   lib/igt_kmod: rename kselftest functions to ktest
>   lib/igt_kmod.c: check if module is builtin before attempting to unload
>     it
>   lib/igt_kmod: add compatibility for KUnit
>   tests: DRM selftests: switch to KUnit
> 
> Mauro Carvalho Chehab (2):
>   lib/igt_kmod: place KUnit tests on a subtest
>   kunit tests: add an optional name for the selftests
> 
>  lib/igt_kmod.c       | 146 +++++++++-
>  lib/igt_kmod.h       |  14 +-
>  lib/igt_ktap.c       | 615 +++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_ktap.h       |  50 ++++
>  lib/meson.build      |   1 +
>  tests/drm_buddy.c    |   4 +-
>  tests/drm_mm.c       |   5 +-
>  tests/kms_selftest.c |  19 ++
>  8 files changed, 835 insertions(+), 19 deletions(-)
>  create mode 100644 lib/igt_ktap.c
>  create mode 100644 lib/igt_ktap.h
> 
>