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

[igt-dev] [PATCH i-g-t 1/5] 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 6d6ecf01..0b714ce3 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 f98dd29f..ceb10cd0 100644
--- a/lib/igt_kmod.h
+++ b/lib/igt_kmod.h
@@ -50,7 +50,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;
@@ -63,19 +63,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 v3 i-g-t 0/5] Introduce KUnit

[Dominik Karol Piatkowski]

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

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

Dominik Karol Piatkowski (1):
  Change logic of ktap parser to run on a thread

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

 lib/igt_kmod.c       | 110 +++++++++++--
 lib/igt_kmod.h       |  14 +-
 lib/igt_ktap.c       | 382 +++++++++++++++++++++++++++++++++++++++++++
 lib/igt_ktap.h       |  34 ++++
 lib/meson.build      |   1 +
 tests/drm_buddy.c    |   4 +-
 tests/drm_mm.c       |   4 +-
 tests/kms_selftest.c |   8 +
 8 files changed, 538 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/5] 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/5] 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/5] 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 768ce90b..45b9626a 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -87,6 +87,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/5] 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 2052b115..46d0142f 100644
--- a/tests/drm_mm.c
+++ b/tests/drm_mm.c
@@ -28,5 +28,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/5] 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

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 |  26 +++++----
 lib/igt_ktap.c | 148 ++++++++++++++++++++++++++++++++-----------------
 lib/igt_ktap.h |   5 +-
 3 files changed, 118 insertions(+), 61 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 21e801bd..45a0c6ed 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -758,7 +758,6 @@ 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;
@@ -771,12 +770,13 @@ 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");
@@ -804,19 +804,25 @@ int igt_kunit(const char *module_name, const char *opts)
 
 	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
 
+	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);
 	}
 
-	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);
 
+	ret = ktap_parser_stop();
+
+	if (ret != 0)
+		ret = IGT_EXIT_ABORT;
+
 	if (ret == 0)
 		igt_success();
 
diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
index 117598fa..090f98b7 100644
--- a/lib/igt_ktap.c
+++ b/lib/igt_ktap.c
@@ -5,11 +5,16 @@
 
 #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"
 
+
+
 static int log_to_end(enum igt_log_level level, FILE *f,
 		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
 
@@ -118,7 +123,10 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
 		igt_info("%s", version_rptr);
 
 		if (fgets(record, BUF_LEN, fp) == NULL) {
-			igt_warn("kmsg truncated: unknown error (%m)\n");
+			if (ferror(fp))
+				igt_warn("kmsg read failed: %s\n", strerror(errno));
+			else
+				igt_warn("kmsg truncated: unknown error (%m)\n");
 			return -2;
 		}
 	}
@@ -253,6 +261,13 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
 	return 1;
 }
 
+struct ktap_parser_args {
+	FILE *fp;
+	bool is_builtin;
+	volatile bool is_running;
+	int ret;
+} ktap_args;
+
 /**
  * igt_ktap_parser:
  * @fp: FILE pointer
@@ -264,71 +279,104 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
  *
  * Returns: IGT default codes
  */
-int igt_ktap_parser(FILE *fp, char *record, bool is_builtin)
+void *igt_ktap_parser(void *unused)
 {
+	FILE *fp = ktap_args.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 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;
-		}
+igt_ktap_parser_start:
+	while (ktap_args.is_running) {
+		test_name[0] = '\0';
+		test_name[BUF_LEN] = '\0';
 
-		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:
+		while(!feof(fp)) {
 			if (fgets(record, BUF_LEN, fp) == NULL) {
-				igt_warn("kmsg truncated: unknown error (%m)\n");
-				return -2;
+				if (ferror(fp))
+					igt_warn("kmsg read failed: %s\n", strerror(errno));
+
+				if (!ktap_args.is_running)
+					return NULL;
 			}
-			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;
+		while (sublevel >= 0) {
+			switch (find_next_tap_subtest(fp, record, is_builtin)) {
+			case -2:
+				/* no more data to read */
+				goto igt_ktap_parser_start;
+			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) {
+					if (ferror(fp))
+						igt_warn("kmsg read failed: %s\n", strerror(errno));
+					else
+						igt_warn("kmsg truncated: unknown error (%m)\n");
+					goto igt_ktap_parser_start;
+				}
+				found_tests = true;
+				sublevel++;
+				break;
+			}
+
+			switch (parse_kmsg_for_tap(fp, record, test_name)) {
+			case -2:
+				goto igt_ktap_parser_start;
+			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;
+		ktap_args.ret = IGT_EXIT_FAILURE;
+	else
+		ktap_args.ret = IGT_EXIT_SUCCESS;
+
+	return NULL;
+}
+
+static pthread_t ktap_parser_thread;
+
+void ktap_parser_start(FILE *fp, bool is_builtin)
+{
+	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 IGT_EXIT_SUCCESS;
+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..ee6689c1 100644
--- a/lib/igt_ktap.h
+++ b/lib/igt_ktap.h
@@ -26,6 +26,9 @@
 
 #define BUF_LEN 4096
 
-int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
+void *igt_ktap_parser(void *unused);
+
+void ktap_parser_start(FILE *fp, bool is_builtin);
+int ktap_parser_stop(void);
 
 #endif /* IGT_KTAP_H */
-- 
2.34.1

[igt-dev] ✗ GitLab.Pipeline: warning for Introduce KUnit (rev3)

[Patchwork]

== Series Details ==

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

== Summary ==

Pipeline status: FAILED.

see https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/837634 for the overview.

test:ninja-test has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/38666969):
  369/373 assembler test/rnde-intsrc              OK       0.01 s 
  370/373 assembler test/rndz                     OK       0.01 s 
  371/373 assembler test/lzd                      OK       0.01 s 
  372/373 assembler test/not                      OK       0.01 s 
  373/373 assembler test/immediate                OK       0.01 s 
  
  Ok:                  366
  Expected Fail:         4
  Fail:                  3
  Unexpected Pass:       0
  Skipped:               0
  Timeout:               0
  
  Full log written to /builds/gfx-ci/igt-ci-tags/build/meson-logs/testlog.txt
  section_end:1679646619:step_script
  section_start:1679646619:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1679646620:cleanup_file_variables
  ERROR: Job failed: exit code 1
  

test:ninja-test-clang has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/38666970):
  369/373 assembler test/rnde-intsrc              OK       0.01 s 
  370/373 assembler test/rndz                     OK       0.01 s 
  371/373 assembler test/lzd                      OK       0.01 s 
  372/373 assembler test/not                      OK       0.01 s 
  373/373 assembler test/immediate                OK       0.01 s 
  
  Ok:                  366
  Expected Fail:         4
  Fail:                  3
  Unexpected Pass:       0
  Skipped:               0
  Timeout:               0
  
  Full log written to /builds/gfx-ci/igt-ci-tags/build/meson-logs/testlog.txt
  section_end:1679646619:step_script
  section_start:1679646619:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1679646619:cleanup_file_variables
  ERROR: Job failed: exit code 1

== Logs ==

For more details see: https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/pipelines/837634

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

[Patchwork]

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from IGT_7216 -> IGTPW_8677
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_8677 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_8677, 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_8677/index.html

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

  Missing    (1): fi-snb-2520m 

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1:
    - bat-dg2-8:          [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/bat-dg2-8/igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8677/bat-dg2-8/igt@kms_pipe_crc_basic@compare-crc-sanitycheck@pipe-d-dp-1.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - bat-rpls-1:         NOTRUN -> [ABORT][3] ([i915#6687] / [i915#7978])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8677/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_selftest@live@execlists:
    - fi-bsw-n3050:       [PASS][4] -> [ABORT][5] ([i915#7911] / [i915#7913])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/fi-bsw-n3050/igt@i915_selftest@live@execlists.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8677/fi-bsw-n3050/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@hangcheck:
    - fi-skl-guc:         [PASS][6] -> [DMESG-WARN][7] ([i915#8073])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/fi-skl-guc/igt@i915_selftest@live@hangcheck.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8677/fi-skl-guc/igt@i915_selftest@live@hangcheck.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/IGT_7216/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8677/bat-dg2-11/igt@i915_selftest@live@migrate.html
    - bat-atsm-1:         [DMESG-FAIL][10] ([i915#7699]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/bat-atsm-1/igt@i915_selftest@live@migrate.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8677/bat-atsm-1/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-1:         [ABORT][12] ([i915#4983]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7216/bat-rpls-1/igt@i915_selftest@live@reset.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8677/bat-rpls-1/igt@i915_selftest@live@reset.html

  
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978
  [i915#8073]: https://gitlab.freedesktop.org/drm/intel/issues/8073


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

  * CI: CI-20190529 -> None
  * IGT: IGT_7216 -> IGTPW_8677

  CI-20190529: 20190529
  CI_DRM_12907: 3e6be7c63e438996c88d6ba51a7d3025c56086d0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8677: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8677/index.html
  IGT_7216: 0682c2b07c7eab2bf384899c3da3cc7f08083847 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git


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

-igt@drm_buddy@all-tests
-igt@drm_mm@all-tests

== Logs ==

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

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

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

[Janusz Krzysztofik]

Hi Dominik,

I can see some progress, but still quite a few issues.

If you and/or other reviewers think my concerns are not important from the PoV 
of Xe validation requirements then please feel free to ignore my comments.

On Friday, 24 March 2023 09:11:20 CEST Dominik Karol Piatkowski 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
> 
> 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 |  26 +++++----
>  lib/igt_ktap.c | 148 ++++++++++++++++++++++++++++++++-----------------
>  lib/igt_ktap.h |   5 +-
>  3 files changed, 118 insertions(+), 61 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 21e801bd..45a0c6ed 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -758,7 +758,6 @@ 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;
> @@ -771,12 +770,13 @@ 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;
> +		}

Please note that since igt_require() is still called inside igt_ktest_begin() 
and can potentially long jump out of your igt_fixture section body, your 
attempt to return an error code from igt_kunit() if some requirements are not 
met can still fail.  If that happens, igt_kunit() will proceed, though all 
following igt_fixture and igt_subtest section bodies will be omitted.  
Moreover, omitted not only inside igt_kunit(), but also outside of it, after 
it returns to its caller, until the program exits.  I believe that's not what 
we intend.

>  
>  	if (tst.kmsg < 0) {
>  		igt_warn("Could not open /dev/kmsg\n");
> @@ -804,19 +804,25 @@ int igt_kunit(const char *module_name, const char *opts)
>  
>  	is_builtin = kmod_module_get_initstate(kunit_kmod) == KMOD_MODULE_BUILTIN;
>  
> +	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);

This igt_fail() (and not only this one) still seems to be called from outside 
of any igt_fixture / igt_subtest sections.

>  	}
>  
> -	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);
>  
> +	ret = ktap_parser_stop();
> +
> +	if (ret != 0)
> +		ret = IGT_EXIT_ABORT;
> +
>  	if (ret == 0)
>  		igt_success();
>  
> diff --git a/lib/igt_ktap.c b/lib/igt_ktap.c
> index 117598fa..090f98b7 100644
> --- a/lib/igt_ktap.c
> +++ b/lib/igt_ktap.c
> @@ -5,11 +5,16 @@
>  
>  #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"
>  
> +
> +
>  static int log_to_end(enum igt_log_level level, FILE *f,
>  		      char *record, const char *format, ...) __attribute__((format(printf, 4, 5)));
>  
> @@ -118,7 +123,10 @@ static int find_next_tap_subtest(FILE *fp, char *record, bool is_builtin)
>  		igt_info("%s", version_rptr);
>  
>  		if (fgets(record, BUF_LEN, fp) == NULL) {

As Petri suggested in his comment to one of previous versions of the series, 
using line buffered input is not needed for reading from /dev/kmesg (which I 
didn't know when I suggested to switch to it before, still when Isabella was 
working on this -- my fault, sorry).  Before you decide whether to keep using 
line buffered input or switch to raw read(), please have a look at 
lib/igt_kmod.c:kmsg_dump() for an example of how we've been reading log 
records from /dev/kmsg so far, and at commit 98783313b8b3 ("lib/kmod: 
reimplement kmsg_dump()") for some justification.

Some of below comments apply to current line buffered reading, if you decide 
to keep it.

> -			igt_warn("kmsg truncated: unknown error (%m)\n");
> +			if (ferror(fp))
> +				igt_warn("kmsg read failed: %s\n", strerror(errno));
> +			else
> +				igt_warn("kmsg truncated: unknown error (%m)\n");
>  			return -2;
>  		}
>  	}
> @@ -253,6 +261,13 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
>  	return 1;
>  }
>  
> +struct ktap_parser_args {
> +	FILE *fp;
> +	bool is_builtin;
> +	volatile bool is_running;
> +	int ret;
> +} ktap_args;
> +
>  /**
>   * igt_ktap_parser:
>   * @fp: FILE pointer
> @@ -264,71 +279,104 @@ static int parse_kmsg_for_tap(FILE *fp, char *record, char *test_name)
>   *
>   * Returns: IGT default codes
>   */
> -int igt_ktap_parser(FILE *fp, char *record, bool is_builtin)
> +void *igt_ktap_parser(void *unused)
>  {
> +	FILE *fp = ktap_args.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 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;
> -		}
> +igt_ktap_parser_start:
> +	while (ktap_args.is_running) {
> +		test_name[0] = '\0';
> +		test_name[BUF_LEN] = '\0';
>  
> -		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:
> +		while(!feof(fp)) {

Since we are reading from /dev/kmsg, which is a special character device file, 
I think we never get EOF.

>  			if (fgets(record, BUF_LEN, fp) == NULL) {
> -				igt_warn("kmsg truncated: unknown error (%m)\n");
> -				return -2;
> +				if (ferror(fp))
> +					igt_warn("kmsg read failed: %s\n", strerror(errno));

Moreover, I think we always get an error if no data is ready for reading.  
Then we will get a lot of noise from here since we are using non-blocking 
read.  I think we should try to use blocking mode, or switch to poll() or an 
equivalent, whichever works best for reading from a character device special 
file.

> +
> +				if (!ktap_args.is_running)
> +					return NULL;
>  			}
> -			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;
> +		while (sublevel >= 0) {
> +			switch (find_next_tap_subtest(fp, record, is_builtin)) {
> +			case -2:
> +				/* no more data to read */
> +				goto igt_ktap_parser_start;
> +			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) {
> +					if (ferror(fp))
> +						igt_warn("kmsg read failed: %s\n", strerror(errno));
> +					else
> +						igt_warn("kmsg truncated: unknown error (%m)\n");
> +					goto igt_ktap_parser_start;
> +				}
> +				found_tests = true;
> +				sublevel++;
> +				break;
> +			}
> +
> +			switch (parse_kmsg_for_tap(fp, record, test_name)) {
> +			case -2:
> +				goto igt_ktap_parser_start;
> +			case -1:
> +				sublevel--;
> +				failed_tests = true;
> +				igt_subtest(test_name)

AFAICT, igt_subtest sections can be used only in the main thread.  I think 
that if we are going to parse the kernel log for ktap messages from a sub-
thread then we need to establish a communication channel for passing progress 
to the main thread.  The main thread in turn should enter an igt_subtest 
section when start of a new subtest is reported bu the ktap parser, then wait 
for the subtest result to be reported and either leave the section on success 
or igt_fail() on failure.

> +					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;
> +			}
>  		}

While ktap_args.is_running can help to force the parser to exit if something 
goes wrong, I think we should be able to leave it when we detect end of ktap 
data (sublevel < 0 ?).

Thanks,
Janusz

>  	}
>  
>  	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;
> +
> +void ktap_parser_start(FILE *fp, bool is_builtin)
> +{
> +	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 IGT_EXIT_SUCCESS;
> +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..ee6689c1 100644
> --- a/lib/igt_ktap.h
> +++ b/lib/igt_ktap.h
> @@ -26,6 +26,9 @@
>  
>  #define BUF_LEN 4096
>  
> -int igt_ktap_parser(FILE *fp, char *record, bool is_builtin);
> +void *igt_ktap_parser(void *unused);
> +
> +void ktap_parser_start(FILE *fp, bool is_builtin);
> +int ktap_parser_stop(void);
>  
>  #endif /* IGT_KTAP_H */
>