[RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[John.C.Harrison]

From: johnharr <johnharr@invalid-email.com>

The list of supported kunit tests is currently hard coded. Which means
that adding a new test to the kernel also requires patches to IGT as
well.

The list of available kunit tests is already exported by the kernel.
So there is no need to bake a list into the IGT source code. So, add
support for querying the test list dynamically.

NB: Currently, the test list can only be queried by root but the IGT
build system queries all tests at compile time. In theory this should
not be a problem. However, the kunit helper code is all written to run
inside a test and not inside the prep code, which means that when it
fails to open the root only interfaces, it calls 'skip'. And skip is
not allowed outside of running a test. Hence the build fails with:
  skipping is allowed only in fixtures, subtests or igt_simple_main

And of course, putting all the query code inside an igt_fixture block
means it doesn't get run as part of --show-testlist or --list-subtests.

Reworking the kunit code to fail cleanly rather than skipping
everywhere seems much more invasive. It is also not clear why the
kunit code is considered 'library' when it is execlusively used from
the kunit test alone!? So posting this as an RFC before going too far
down the possible rabbit hole.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 lib/igt_kmod.c              | 68 +++++++++++++++++++++++++++++++++++++
 lib/igt_kmod.h              |  6 ++++
 tests/intel/xe_live_ktest.c | 60 ++++++++++++++++----------------
 3 files changed, 103 insertions(+), 31 deletions(-)

diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
index 464c0dcf484a..69c1254b397c 100644
--- a/lib/igt_kmod.c
+++ b/lib/igt_kmod.c
@@ -1414,6 +1414,74 @@ static void __igt_kunit(struct igt_ktest *tst,
 	}
 }
 
+/**
+ * igt_kunit:
+ * @module_name: the name of the module
+ * @suite: the name of test suite to be executed, also used as subtest name;
+ *	   if NULL then test cases from all test suites provided by the module
+ *	   are executed as dynamic sub-subtests of one IGT subtest, which name
+ *	   is derived from the module name by cutting off its optional trailing
+ *	   _test or _kunit suffix
+ * @opts: options to load the module
+ *
+ * Loads the test module, parses its (k)tap dmesg output, then unloads it
+ */
+void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names)
+{
+	char debugfs_path[PATH_MAX] = { '\0', };
+	struct igt_ktest tst = { .kmsg = -1, };
+	struct igt_ktap_results *ktap = NULL;
+	const char *subtest;
+	char *suite_name = NULL, *case_name = NULL;
+	DIR *debugfs_dir = NULL;
+	IGT_LIST_HEAD(tests);
+
+	subtest = strdup(module_name);
+	if (!igt_debug_on(!subtest)) {
+		char *suffix = strstr(subtest, "_test");
+
+		if (!suffix)
+			suffix = strstr(subtest, "_kunit");
+
+		if (suffix)
+			*suffix = '\0';
+	}
+
+	igt_require(subtest);
+
+	igt_require(!igt_ktest_init(&tst, module_name));
+	igt_require(!igt_ktest_begin(&tst));
+
+	igt_ignore_warn(igt_kmod_load("kunit", NULL));
+
+	kunit_debugfs_path(debugfs_path);
+	igt_info("kunit path: %s\n", debugfs_path);
+	if (igt_debug_on(!*debugfs_path) ||
+	    !kunit_get_tests(&tests, &tst, NULL, opts, debugfs_path, &debugfs_dir, &ktap)) {
+		igt_info("kunit - no tests found!?\n");
+	} else {
+		struct igt_ktap_result *t;
+		igt_info("kunit tests:\n");
+		igt_list_for_each_entry(t, &tests, link) {
+			struct igt_kunit_names *name = malloc(sizeof(*name));
+			name->suite = strdup(t->suite_name);
+			name->sub = strdup(t->case_name);
+			igt_info("  %s / %s\n", t->suite_name, t->case_name);
+			igt_list_add(&name->link, names);
+		}
+	}
+
+	igt_ktap_free(&ktap);
+
+	kunit_results_free(&tests, &suite_name, &case_name);
+
+	if (debugfs_dir)
+		closedir(debugfs_dir);
+
+	igt_ktest_end(&tst);
+	igt_ktest_fini(&tst);
+}
+
 /**
  * igt_kunit:
  * @module_name: the name of the module
diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
index efb46da128d4..2f608eb69d48 100644
--- a/lib/igt_kmod.h
+++ b/lib/igt_kmod.h
@@ -71,7 +71,13 @@ static inline int igt_xe_driver_unload(void)
 int igt_amdgpu_driver_load(const char *opts);
 int igt_amdgpu_driver_unload(void);
 
+struct igt_kunit_names {
+	char *suite;
+	char *sub;
+	struct igt_list_head link;
+};
 void igt_kunit(const char *module_name, const char *name, const char *opts);
+void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names);
 
 void igt_kselftests(const char *module_name,
 		    const char *module_options,
diff --git a/tests/intel/xe_live_ktest.c b/tests/intel/xe_live_ktest.c
index 4376d5df7751..13265e87ba9d 100644
--- a/tests/intel/xe_live_ktest.c
+++ b/tests/intel/xe_live_ktest.c
@@ -9,40 +9,38 @@
  * Sub-category: kunit
  * Functionality: kunit test
  * Test category: functionality test
- *
- * SUBTEST: xe_bo
- * Description:
- *	Kernel dynamic selftests to check if GPU buffer objects are
- *	being handled properly.
- * Functionality: bo
- *
- * SUBTEST: xe_dma_buf
- * Description: Kernel dynamic selftests for dmabuf functionality.
- * Functionality: dmabuf test
- *
- * SUBTEST: xe_migrate
- * Description:
- *	Kernel dynamic selftests to check if page table migrations
- *	are working properly.
- * Functionality: migrate
- *
- * SUBTEST: xe_mocs
- * Description:
- *	Kernel dynamic selftests to check mocs configuration.
- * Functionality: mocs configuration
  */
 
-static const char *live_tests[] = {
-	"xe_bo",
-	"xe_dma_buf",
-	"xe_migrate",
-	"xe_mocs",
-};
-
 igt_main
 {
-	int i;
+	IGT_LIST_HEAD(names);
+	IGT_LIST_HEAD(done);
+	struct igt_kunit_names *name, *cmp;
+
+	igt_kunit_get_test_names("xe_live_test", NULL, &names);
+
+	while (!igt_list_empty(&names)) {
+		bool found = false;
+
+		name = igt_list_first_entry(&names, name, link);
+		igt_list_del(&name->link);
+
+		/*
+		 * Retuned list is sub-tests.
+		 * So, need filter out duplicated top level names.
+		 */
+		igt_list_for_each_entry(cmp, &done, link) {
+			if (strcmp(name->suite, cmp->suite) != 0)
+				continue;
+
+			found = true;
+		}
+
+		if (found)
+			continue;
+
+		igt_list_add(&name->link, &done);
 
-	for (i = 0; i < ARRAY_SIZE(live_tests); i++)
-		igt_kunit("xe_live_test", live_tests[i], NULL);
+		igt_kunit("xe_live_test", name->suite, NULL);
+	}
 }
-- 
2.46.0

✗ Fi.CI.BUILD: failure for kunit: Remove hard-coded test list from xe_live_ktest

[Patchwork]

== Series Details ==

Series: kunit: Remove hard-coded test list from xe_live_ktest
URL   : https://patchwork.freedesktop.org/series/137735/
State : failure

== Summary ==

IGT patchset build failed on latest successful build
af25d174eb6445242565a014f99bf3f419552c04 tests/amd_queue_reset: add ability to skip subtest

Tail of build.log:
[1720/1767] Linking target tools/intel_reg_checker.
[1721/1767] Linking target tools/lsgpu.
[1722/1767] Generating gem_stress.testlist with a meson_exe.py custom command.
[1723/1767] Linking target tools/intel_watermark.
[1724/1767] Linking target runner/testdata/abort-simple.
[1725/1767] Linking target runner/testdata/no-subtests.
[1726/1767] Linking target tools/xe-perf/xe-perf-reader.
[1727/1767] Linking target runner/testdata/dynamic.
[1728/1767] Linking target tools/xe-perf/xe-perf-recorder.
[1729/1767] Linking target tools/amd_hdmi_compliance.
[1730/1767] Linking target tools/intel_gvtg_test.
[1731/1767] Linking target tools/intel_vbt_decode.
[1732/1767] Linking target runner/igt_results.
[1733/1767] Linking target runner/testdata/successtest.
[1734/1767] Linking target tools/intel_dp_compliance.
[1735/1767] Linking target runner/igt_comms_decoder.
[1736/1767] Generating xe_live_ktest.testlist with a meson_exe.py custom command.
FAILED: tests/xe_live_ktest.testlist 
/usr/bin/meson --internal exe --capture tests/xe_live_ktest.testlist -- /opt/igt/build/tests/xe_live_ktest --show-testlist
skipping is allowed only in fixtures, subtests or igt_simple_main
please refer to lib/igt_core documentation
xe_live_ktest: ../../../usr/src/igt-gpu-tools/lib/igt_core.c:450: internal_assert: Assertion `0' failed.
Received signal SIGABRT.
Stack trace: 
 #0 [fatal_sig_handler+0x10f]
 #1 [killpg+0x40]
 #2 [gsignal+0xcb]
 #3 [abort+0x12b]
 #4 [<unknown>+0x12b]
 #5 [__assert_fail+0x46]
 #6 [internal_assert+0x106]
 #7 [igt_skip+0x185]
 #8 [__igt_skip_check+0x1d7]
 #9 [igt_ktest_begin+0x9e]
 #10 [igt_kunit_get_test_names+0x124]
 #11 [main+0x75]
 #12 [__libc_start_main+0xf3]
 #13 [_start+0x2e]
[1737/1767] Linking target runner/testdata/skippers.
[1738/1767] Linking target runner/testdata/abort.
[1739/1767] Linking target runner/testdata/abort-fixture.
[1740/1767] Linking target runner/igt_runner.
[1741/1767] Linking target runner/testdata/abort-dynamic.
[1742/1767] Linking target runner/runner_json_test.
[1743/1767] Linking target runner/igt_resume.
[1744/1767] Compiling C object 'lib/76b5a35@@i915_perf@sha/meson-generated_.._i915_perf_metrics_acmgt1.c.o'.
[1745/1767] Compiling C object 'runner/527aa9f@@runner_test@exe/runner_tests.c.o'.
[1746/1767] Compiling C object 'lib/76b5a35@@i915_perf@sha/meson-generated_.._i915_perf_metrics_acmgt2.c.o'.
[1747/1767] Compiling C object 'lib/76b5a35@@i915_perf@sha/meson-generated_.._i915_perf_metrics_acmgt3.c.o'.
ninja: build stopped: subcommand failed.

✗ GitLab.Pipeline: warning for kunit: Remove hard-coded test list from xe_live_ktest

[Patchwork]

== Series Details ==

Series: kunit: Remove hard-coded test list from xe_live_ktest
URL   : https://patchwork.freedesktop.org/series/137735/
State : warning

== Summary ==

Pipeline status: FAILED.

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

test:ninja-test has failed (https://gitlab.freedesktop.org/gfx-ci/igt-ci-tags/-/jobs/62678117):
  431/435 assembler test/rnde-intsrc              OK       0.02 s 
  432/435 assembler test/rndz                     OK       0.02 s 
  433/435 assembler test/lzd                      OK       0.02 s 
  434/435 assembler test/not                      OK       0.02 s 
  435/435 assembler test/immediate                OK       0.01 s 
  
  Ok:                  430
  Expected Fail:         4
  Fail:                  1
  Unexpected Pass:       0
  Skipped:               0
  Timeout:               0
  
  Full log written to /builds/gfx-ci/igt-ci-tags/build/meson-logs/testlog.txt
  section_end:1724450659:step_script
  section_start:1724450659:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1724450659: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/62678116):
  431/435 assembler test/rnde-intsrc              OK       0.02 s 
  432/435 assembler test/rndz                     OK       0.02 s 
  433/435 assembler test/lzd                      OK       0.02 s 
  434/435 assembler test/not                      OK       0.02 s 
  435/435 assembler test/immediate                OK       0.02 s 
  
  Ok:                  430
  Expected Fail:         4
  Fail:                  1
  Unexpected Pass:       0
  Skipped:               0
  Timeout:               0
  
  Full log written to /builds/gfx-ci/igt-ci-tags/build/meson-logs/testlog.txt
  section_end:1724450644:step_script
  section_start:1724450644:cleanup_file_variables
  Cleaning up project directory and file based variables
  section_end:1724450644:cleanup_file_variables
  ERROR: Job failed: exit code 1

== Logs ==

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

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[Kamil Konieczny]

Hi John.C.Harrison,
On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
> From: johnharr <johnharr@invalid-email.com>
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Again invalid e-mail here.

> 
> The list of supported kunit tests is currently hard coded. Which means
> that adding a new test to the kernel also requires patches to IGT as
> well.
> 
> The list of available kunit tests is already exported by the kernel.
> So there is no need to bake a list into the IGT source code. So, add
> support for querying the test list dynamically.
> 
> NB: Currently, the test list can only be queried by root but the IGT
> build system queries all tests at compile time. In theory this should
> not be a problem. However, the kunit helper code is all written to run
> inside a test and not inside the prep code, which means that when it
> fails to open the root only interfaces, it calls 'skip'. And skip is
> not allowed outside of running a test. Hence the build fails with:
>   skipping is allowed only in fixtures, subtests or igt_simple_main

Looks like we should fix it, move out skips from kunit libs.

> 
> And of course, putting all the query code inside an igt_fixture block
> means it doesn't get run as part of --show-testlist or --list-subtests.
> 
> Reworking the kunit code to fail cleanly rather than skipping
> everywhere seems much more invasive. It is also not clear why the
> kunit code is considered 'library' when it is execlusively used from
> the kunit test alone!? So posting this as an RFC before going too far
> down the possible rabbit hole.
> 

As I see this, it is a lib because some other vendor (like amdpgu)
could use it.

+cc Janusz
Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>

btw could you split this into two patches, one for lib/* and
one for tests/intel/xe_live_ktest ?

> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  lib/igt_kmod.c              | 68 +++++++++++++++++++++++++++++++++++++
>  lib/igt_kmod.h              |  6 ++++
>  tests/intel/xe_live_ktest.c | 60 ++++++++++++++++----------------
>  3 files changed, 103 insertions(+), 31 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 464c0dcf484a..69c1254b397c 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -1414,6 +1414,74 @@ static void __igt_kunit(struct igt_ktest *tst,
>  	}
>  }
>  
> +/**
> + * igt_kunit:
> + * @module_name: the name of the module
> + * @suite: the name of test suite to be executed, also used as subtest name;
> + *	   if NULL then test cases from all test suites provided by the module
> + *	   are executed as dynamic sub-subtests of one IGT subtest, which name
> + *	   is derived from the module name by cutting off its optional trailing
> + *	   _test or _kunit suffix
> + * @opts: options to load the module
> + *
> + * Loads the test module, parses its (k)tap dmesg output, then unloads it
> + */
> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names)
> +{
> +	char debugfs_path[PATH_MAX] = { '\0', };
> +	struct igt_ktest tst = { .kmsg = -1, };
> +	struct igt_ktap_results *ktap = NULL;
> +	const char *subtest;
> +	char *suite_name = NULL, *case_name = NULL;
> +	DIR *debugfs_dir = NULL;
> +	IGT_LIST_HEAD(tests);
> +
> +	subtest = strdup(module_name);
> +	if (!igt_debug_on(!subtest)) {
> +		char *suffix = strstr(subtest, "_test");
> +
> +		if (!suffix)
> +			suffix = strstr(subtest, "_kunit");
> +
> +		if (suffix)
> +			*suffix = '\0';
> +	}
> +
> +	igt_require(subtest);
> +
> +	igt_require(!igt_ktest_init(&tst, module_name));
> +	igt_require(!igt_ktest_begin(&tst));
> +
> +	igt_ignore_warn(igt_kmod_load("kunit", NULL));
> +
> +	kunit_debugfs_path(debugfs_path);
> +	igt_info("kunit path: %s\n", debugfs_path);
> +	if (igt_debug_on(!*debugfs_path) ||
> +	    !kunit_get_tests(&tests, &tst, NULL, opts, debugfs_path, &debugfs_dir, &ktap)) {
> +		igt_info("kunit - no tests found!?\n");
> +	} else {
> +		struct igt_ktap_result *t;
> +		igt_info("kunit tests:\n");
> +		igt_list_for_each_entry(t, &tests, link) {
> +			struct igt_kunit_names *name = malloc(sizeof(*name));
> +			name->suite = strdup(t->suite_name);
> +			name->sub = strdup(t->case_name);
> +			igt_info("  %s / %s\n", t->suite_name, t->case_name);
> +			igt_list_add(&name->link, names);
> +		}
> +	}
> +
> +	igt_ktap_free(&ktap);
> +
> +	kunit_results_free(&tests, &suite_name, &case_name);
> +
> +	if (debugfs_dir)
> +		closedir(debugfs_dir);
> +
> +	igt_ktest_end(&tst);
> +	igt_ktest_fini(&tst);
> +}
> +
>  /**
>   * igt_kunit:
>   * @module_name: the name of the module
> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> index efb46da128d4..2f608eb69d48 100644
> --- a/lib/igt_kmod.h
> +++ b/lib/igt_kmod.h
> @@ -71,7 +71,13 @@ static inline int igt_xe_driver_unload(void)
>  int igt_amdgpu_driver_load(const char *opts);
>  int igt_amdgpu_driver_unload(void);
>  
> +struct igt_kunit_names {
> +	char *suite;
> +	char *sub;
> +	struct igt_list_head link;
> +};
>  void igt_kunit(const char *module_name, const char *name, const char *opts);
> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names);
>  
>  void igt_kselftests(const char *module_name,
>  		    const char *module_options,
> diff --git a/tests/intel/xe_live_ktest.c b/tests/intel/xe_live_ktest.c
> index 4376d5df7751..13265e87ba9d 100644
> --- a/tests/intel/xe_live_ktest.c
> +++ b/tests/intel/xe_live_ktest.c
> @@ -9,40 +9,38 @@
>   * Sub-category: kunit
>   * Functionality: kunit test
>   * Test category: functionality test
> - *
> - * SUBTEST: xe_bo
> - * Description:
> - *	Kernel dynamic selftests to check if GPU buffer objects are
> - *	being handled properly.
> - * Functionality: bo
> - *
> - * SUBTEST: xe_dma_buf
> - * Description: Kernel dynamic selftests for dmabuf functionality.
> - * Functionality: dmabuf test
> - *
> - * SUBTEST: xe_migrate
> - * Description:
> - *	Kernel dynamic selftests to check if page table migrations
> - *	are working properly.
> - * Functionality: migrate
> - *
> - * SUBTEST: xe_mocs
> - * Description:
> - *	Kernel dynamic selftests to check mocs configuration.
> - * Functionality: mocs configuration
>   */
>  

Does that mean we will not have any subtests here? The point
into moving subtests names into documentation was to have a testlist
generated during compilation, so it will not depend on running
test with --list option. Adding also Katarzyna to cc.
Cc: Katarzyna Piecielska <katarzyna.piecielska@intel.com>

Regards,
Kamil

> -static const char *live_tests[] = {
> -	"xe_bo",
> -	"xe_dma_buf",
> -	"xe_migrate",
> -	"xe_mocs",
> -};
> -
>  igt_main
>  {
> -	int i;
> +	IGT_LIST_HEAD(names);
> +	IGT_LIST_HEAD(done);
> +	struct igt_kunit_names *name, *cmp;
> +
> +	igt_kunit_get_test_names("xe_live_test", NULL, &names);
> +
> +	while (!igt_list_empty(&names)) {
> +		bool found = false;
> +
> +		name = igt_list_first_entry(&names, name, link);
> +		igt_list_del(&name->link);
> +
> +		/*
> +		 * Retuned list is sub-tests.
> +		 * So, need filter out duplicated top level names.
> +		 */
> +		igt_list_for_each_entry(cmp, &done, link) {
> +			if (strcmp(name->suite, cmp->suite) != 0)
> +				continue;
> +
> +			found = true;
> +		}
> +
> +		if (found)
> +			continue;
> +
> +		igt_list_add(&name->link, &done);
>  
> -	for (i = 0; i < ARRAY_SIZE(live_tests); i++)
> -		igt_kunit("xe_live_test", live_tests[i], NULL);
> +		igt_kunit("xe_live_test", name->suite, NULL);
> +	}
>  }
> -- 
> 2.46.0
> 

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[Janusz Krzysztofik]

Hi John, Kamil,

On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
> Hi John.C.Harrison,
> On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
> > From: johnharr <johnharr@invalid-email.com>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Again invalid e-mail here.
> 
> > 
> > The list of supported kunit tests is currently hard coded. 

That pattern originates from i915_selftest, where there are 3 stable subtests, 
"mock", "live" and "perf", each of them executing a (possibly long) list of 
dynamic sub-subtests actually provided by the module for each category.  Also, 
IIRC there was a separate module for each Xe test suite before, each mapped to 
a separate IGT test, later merged into one module with multiple suites and one 
test with multiple corresponding subtests.

> > Which means
> > that adding a new test to the kernel also requires patches to IGT as
> > well.
> > 
> > The list of available kunit tests is already exported by the kernel.
> > So there is no need to bake a list into the IGT source code. So, add
> > support for querying the test list dynamically.
> > 
> > NB: Currently, the test list can only be queried by root but the IGT
> > build system queries all tests at compile time. In theory this should
> > not be a problem. However, the kunit helper code is all written to run
> > inside a test and not inside the prep code, which means that when it
> > fails to open the root only interfaces, it calls 'skip'. And skip is
> > not allowed outside of running a test. Hence the build fails with:
> >   skipping is allowed only in fixtures, subtests or igt_simple_main
> 
> Looks like we should fix it, move out skips from kunit libs.

I suggest you consider a different approach: for a module, call igt_kunit() 
only once, with NULL suite argument.  As a result, you'll get results from one 
IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-subtests 
called "<test_suite>-<test_case>", one for each test case provided by the 
module.

> 
> > 
> > And of course, putting all the query code inside an igt_fixture block
> > means it doesn't get run as part of --show-testlist or --list-subtests.
> > 
> > Reworking the kunit code to fail cleanly rather than skipping
> > everywhere seems much more invasive. It is also not clear why the
> > kunit code is considered 'library' when it is execlusively used from
> > the kunit test alone!? 

Current users:
	igt$ grep -rl igt_kunit tests/
	tests/intel/xe_live_ktest.c
	tests/kms_selftest.c
	tests/drm_mm.c
	tests/drm_buddy.c

The other 3 were first converted from i915_selftest standard to KUnit still 
before Xe KUnit IGT tests started appearing.

Thanks,
Janusz

> > So posting this as an RFC before going too far
> > down the possible rabbit hole.
> > 
> 
> As I see this, it is a lib because some other vendor (like amdpgu)
> could use it.
> 
> +cc Janusz
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> 
> btw could you split this into two patches, one for lib/* and
> one for tests/intel/xe_live_ktest ?
> 
> > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > ---
> >  lib/igt_kmod.c              | 68 +++++++++++++++++++++++++++++++++++++
> >  lib/igt_kmod.h              |  6 ++++
> >  tests/intel/xe_live_ktest.c | 60 ++++++++++++++++----------------
> >  3 files changed, 103 insertions(+), 31 deletions(-)
> > 
> > diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > index 464c0dcf484a..69c1254b397c 100644
> > --- a/lib/igt_kmod.c
> > +++ b/lib/igt_kmod.c
> > @@ -1414,6 +1414,74 @@ static void __igt_kunit(struct igt_ktest *tst,
> >  	}
> >  }
> >  
> > +/**
> > + * igt_kunit:
> > + * @module_name: the name of the module
> > + * @suite: the name of test suite to be executed, also used as subtest name;
> > + *	   if NULL then test cases from all test suites provided by the module
> > + *	   are executed as dynamic sub-subtests of one IGT subtest, which name
> > + *	   is derived from the module name by cutting off its optional trailing
> > + *	   _test or _kunit suffix
> > + * @opts: options to load the module
> > + *
> > + * Loads the test module, parses its (k)tap dmesg output, then unloads it
> > + */
> > +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names)
> > +{
> > +	char debugfs_path[PATH_MAX] = { '\0', };
> > +	struct igt_ktest tst = { .kmsg = -1, };
> > +	struct igt_ktap_results *ktap = NULL;
> > +	const char *subtest;
> > +	char *suite_name = NULL, *case_name = NULL;
> > +	DIR *debugfs_dir = NULL;
> > +	IGT_LIST_HEAD(tests);
> > +
> > +	subtest = strdup(module_name);
> > +	if (!igt_debug_on(!subtest)) {
> > +		char *suffix = strstr(subtest, "_test");
> > +
> > +		if (!suffix)
> > +			suffix = strstr(subtest, "_kunit");
> > +
> > +		if (suffix)
> > +			*suffix = '\0';
> > +	}
> > +
> > +	igt_require(subtest);
> > +
> > +	igt_require(!igt_ktest_init(&tst, module_name));
> > +	igt_require(!igt_ktest_begin(&tst));
> > +
> > +	igt_ignore_warn(igt_kmod_load("kunit", NULL));
> > +
> > +	kunit_debugfs_path(debugfs_path);
> > +	igt_info("kunit path: %s\n", debugfs_path);
> > +	if (igt_debug_on(!*debugfs_path) ||
> > +	    !kunit_get_tests(&tests, &tst, NULL, opts, debugfs_path, &debugfs_dir, &ktap)) {
> > +		igt_info("kunit - no tests found!?\n");
> > +	} else {
> > +		struct igt_ktap_result *t;
> > +		igt_info("kunit tests:\n");
> > +		igt_list_for_each_entry(t, &tests, link) {
> > +			struct igt_kunit_names *name = malloc(sizeof(*name));
> > +			name->suite = strdup(t->suite_name);
> > +			name->sub = strdup(t->case_name);
> > +			igt_info("  %s / %s\n", t->suite_name, t->case_name);
> > +			igt_list_add(&name->link, names);
> > +		}
> > +	}
> > +
> > +	igt_ktap_free(&ktap);
> > +
> > +	kunit_results_free(&tests, &suite_name, &case_name);
> > +
> > +	if (debugfs_dir)
> > +		closedir(debugfs_dir);
> > +
> > +	igt_ktest_end(&tst);
> > +	igt_ktest_fini(&tst);
> > +}
> > +
> >  /**
> >   * igt_kunit:
> >   * @module_name: the name of the module
> > diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> > index efb46da128d4..2f608eb69d48 100644
> > --- a/lib/igt_kmod.h
> > +++ b/lib/igt_kmod.h
> > @@ -71,7 +71,13 @@ static inline int igt_xe_driver_unload(void)
> >  int igt_amdgpu_driver_load(const char *opts);
> >  int igt_amdgpu_driver_unload(void);
> >  
> > +struct igt_kunit_names {
> > +	char *suite;
> > +	char *sub;
> > +	struct igt_list_head link;
> > +};
> >  void igt_kunit(const char *module_name, const char *name, const char *opts);
> > +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names);
> >  
> >  void igt_kselftests(const char *module_name,
> >  		    const char *module_options,
> > diff --git a/tests/intel/xe_live_ktest.c b/tests/intel/xe_live_ktest.c
> > index 4376d5df7751..13265e87ba9d 100644
> > --- a/tests/intel/xe_live_ktest.c
> > +++ b/tests/intel/xe_live_ktest.c
> > @@ -9,40 +9,38 @@
> >   * Sub-category: kunit
> >   * Functionality: kunit test
> >   * Test category: functionality test
> > - *
> > - * SUBTEST: xe_bo
> > - * Description:
> > - *	Kernel dynamic selftests to check if GPU buffer objects are
> > - *	being handled properly.
> > - * Functionality: bo
> > - *
> > - * SUBTEST: xe_dma_buf
> > - * Description: Kernel dynamic selftests for dmabuf functionality.
> > - * Functionality: dmabuf test
> > - *
> > - * SUBTEST: xe_migrate
> > - * Description:
> > - *	Kernel dynamic selftests to check if page table migrations
> > - *	are working properly.
> > - * Functionality: migrate
> > - *
> > - * SUBTEST: xe_mocs
> > - * Description:
> > - *	Kernel dynamic selftests to check mocs configuration.
> > - * Functionality: mocs configuration
> >   */
> >  
> 
> Does that mean we will not have any subtests here? The point
> into moving subtests names into documentation was to have a testlist
> generated during compilation, so it will not depend on running
> test with --list option. Adding also Katarzyna to cc.
> Cc: Katarzyna Piecielska <katarzyna.piecielska@intel.com>
> 
> Regards,
> Kamil
> 
> > -static const char *live_tests[] = {
> > -	"xe_bo",
> > -	"xe_dma_buf",
> > -	"xe_migrate",
> > -	"xe_mocs",
> > -};
> > -
> >  igt_main
> >  {
> > -	int i;
> > +	IGT_LIST_HEAD(names);
> > +	IGT_LIST_HEAD(done);
> > +	struct igt_kunit_names *name, *cmp;
> > +
> > +	igt_kunit_get_test_names("xe_live_test", NULL, &names);
> > +
> > +	while (!igt_list_empty(&names)) {
> > +		bool found = false;
> > +
> > +		name = igt_list_first_entry(&names, name, link);
> > +		igt_list_del(&name->link);
> > +
> > +		/*
> > +		 * Retuned list is sub-tests.
> > +		 * So, need filter out duplicated top level names.
> > +		 */
> > +		igt_list_for_each_entry(cmp, &done, link) {
> > +			if (strcmp(name->suite, cmp->suite) != 0)
> > +				continue;
> > +
> > +			found = true;
> > +		}
> > +
> > +		if (found)
> > +			continue;
> > +
> > +		igt_list_add(&name->link, &done);
> >  
> > -	for (i = 0; i < ARRAY_SIZE(live_tests); i++)
> > -		igt_kunit("xe_live_test", live_tests[i], NULL);
> > +		igt_kunit("xe_live_test", name->suite, NULL);
> > +	}
> >  }
> 

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[John Harrison]

On 8/26/2024 04:46, Kamil Konieczny wrote:
> Hi John.C.Harrison,
> On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
>> From: johnharr <johnharr@invalid-email.com>
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Again invalid e-mail here.
Hmm. I don't get where this is coming from! Running 'git config --list | 
grep email' gives me a whole bunch of different email settings, aliases, 
target lists, etc. but nothing at all mentions 'invalid'. Creating 
patches from any of my other trees does not have this issue. Doesn't 
make sense!

>
>> The list of supported kunit tests is currently hard coded. Which means
>> that adding a new test to the kernel also requires patches to IGT as
>> well.
>>
>> The list of available kunit tests is already exported by the kernel.
>> So there is no need to bake a list into the IGT source code. So, add
>> support for querying the test list dynamically.
>>
>> NB: Currently, the test list can only be queried by root but the IGT
>> build system queries all tests at compile time. In theory this should
>> not be a problem. However, the kunit helper code is all written to run
>> inside a test and not inside the prep code, which means that when it
>> fails to open the root only interfaces, it calls 'skip'. And skip is
>> not allowed outside of running a test. Hence the build fails with:
>>    skipping is allowed only in fixtures, subtests or igt_simple_main
> Looks like we should fix it, move out skips from kunit libs.
>
>> And of course, putting all the query code inside an igt_fixture block
>> means it doesn't get run as part of --show-testlist or --list-subtests.
>>
>> Reworking the kunit code to fail cleanly rather than skipping
>> everywhere seems much more invasive. It is also not clear why the
>> kunit code is considered 'library' when it is execlusively used from
>> the kunit test alone!? So posting this as an RFC before going too far
>> down the possible rabbit hole.
>>
> As I see this, it is a lib because some other vendor (like amdpgu)
> could use it.
>
> +cc Janusz
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>
> btw could you split this into two patches, one for lib/* and
> one for tests/intel/xe_live_ktest ?
Not really. The library changes re-work the interface. So splitting into 
two patches means that the test would not compile after the first patch 
is applied.

>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   lib/igt_kmod.c              | 68 +++++++++++++++++++++++++++++++++++++
>>   lib/igt_kmod.h              |  6 ++++
>>   tests/intel/xe_live_ktest.c | 60 ++++++++++++++++----------------
>>   3 files changed, 103 insertions(+), 31 deletions(-)
>>
>> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>> index 464c0dcf484a..69c1254b397c 100644
>> --- a/lib/igt_kmod.c
>> +++ b/lib/igt_kmod.c
>> @@ -1414,6 +1414,74 @@ static void __igt_kunit(struct igt_ktest *tst,
>>   	}
>>   }
>>   
>> +/**
>> + * igt_kunit:
>> + * @module_name: the name of the module
>> + * @suite: the name of test suite to be executed, also used as subtest name;
>> + *	   if NULL then test cases from all test suites provided by the module
>> + *	   are executed as dynamic sub-subtests of one IGT subtest, which name
>> + *	   is derived from the module name by cutting off its optional trailing
>> + *	   _test or _kunit suffix
>> + * @opts: options to load the module
>> + *
>> + * Loads the test module, parses its (k)tap dmesg output, then unloads it
>> + */
>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names)
>> +{
>> +	char debugfs_path[PATH_MAX] = { '\0', };
>> +	struct igt_ktest tst = { .kmsg = -1, };
>> +	struct igt_ktap_results *ktap = NULL;
>> +	const char *subtest;
>> +	char *suite_name = NULL, *case_name = NULL;
>> +	DIR *debugfs_dir = NULL;
>> +	IGT_LIST_HEAD(tests);
>> +
>> +	subtest = strdup(module_name);
>> +	if (!igt_debug_on(!subtest)) {
>> +		char *suffix = strstr(subtest, "_test");
>> +
>> +		if (!suffix)
>> +			suffix = strstr(subtest, "_kunit");
>> +
>> +		if (suffix)
>> +			*suffix = '\0';
>> +	}
>> +
>> +	igt_require(subtest);
>> +
>> +	igt_require(!igt_ktest_init(&tst, module_name));
>> +	igt_require(!igt_ktest_begin(&tst));
>> +
>> +	igt_ignore_warn(igt_kmod_load("kunit", NULL));
>> +
>> +	kunit_debugfs_path(debugfs_path);
>> +	igt_info("kunit path: %s\n", debugfs_path);
>> +	if (igt_debug_on(!*debugfs_path) ||
>> +	    !kunit_get_tests(&tests, &tst, NULL, opts, debugfs_path, &debugfs_dir, &ktap)) {
>> +		igt_info("kunit - no tests found!?\n");
>> +	} else {
>> +		struct igt_ktap_result *t;
>> +		igt_info("kunit tests:\n");
>> +		igt_list_for_each_entry(t, &tests, link) {
>> +			struct igt_kunit_names *name = malloc(sizeof(*name));
>> +			name->suite = strdup(t->suite_name);
>> +			name->sub = strdup(t->case_name);
>> +			igt_info("  %s / %s\n", t->suite_name, t->case_name);
>> +			igt_list_add(&name->link, names);
>> +		}
>> +	}
>> +
>> +	igt_ktap_free(&ktap);
>> +
>> +	kunit_results_free(&tests, &suite_name, &case_name);
>> +
>> +	if (debugfs_dir)
>> +		closedir(debugfs_dir);
>> +
>> +	igt_ktest_end(&tst);
>> +	igt_ktest_fini(&tst);
>> +}
>> +
>>   /**
>>    * igt_kunit:
>>    * @module_name: the name of the module
>> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
>> index efb46da128d4..2f608eb69d48 100644
>> --- a/lib/igt_kmod.h
>> +++ b/lib/igt_kmod.h
>> @@ -71,7 +71,13 @@ static inline int igt_xe_driver_unload(void)
>>   int igt_amdgpu_driver_load(const char *opts);
>>   int igt_amdgpu_driver_unload(void);
>>   
>> +struct igt_kunit_names {
>> +	char *suite;
>> +	char *sub;
>> +	struct igt_list_head link;
>> +};
>>   void igt_kunit(const char *module_name, const char *name, const char *opts);
>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names);
>>   
>>   void igt_kselftests(const char *module_name,
>>   		    const char *module_options,
>> diff --git a/tests/intel/xe_live_ktest.c b/tests/intel/xe_live_ktest.c
>> index 4376d5df7751..13265e87ba9d 100644
>> --- a/tests/intel/xe_live_ktest.c
>> +++ b/tests/intel/xe_live_ktest.c
>> @@ -9,40 +9,38 @@
>>    * Sub-category: kunit
>>    * Functionality: kunit test
>>    * Test category: functionality test
>> - *
>> - * SUBTEST: xe_bo
>> - * Description:
>> - *	Kernel dynamic selftests to check if GPU buffer objects are
>> - *	being handled properly.
>> - * Functionality: bo
>> - *
>> - * SUBTEST: xe_dma_buf
>> - * Description: Kernel dynamic selftests for dmabuf functionality.
>> - * Functionality: dmabuf test
>> - *
>> - * SUBTEST: xe_migrate
>> - * Description:
>> - *	Kernel dynamic selftests to check if page table migrations
>> - *	are working properly.
>> - * Functionality: migrate
>> - *
>> - * SUBTEST: xe_mocs
>> - * Description:
>> - *	Kernel dynamic selftests to check mocs configuration.
>> - * Functionality: mocs configuration
>>    */
>>   
> Does that mean we will not have any subtests here? The point
> into moving subtests names into documentation was to have a testlist
> generated during compilation, so it will not depend on running
> test with --list option. Adding also Katarzyna to cc.
> Cc: Katarzyna Piecielska <katarzyna.piecielska@intel.com>
I'm not seeing how else this can be done. Either the test list is hard 
coded in the test or it is dynamically generated from the kernel module 
at run time. And if it is dynamically generated then it can only be 
queried by root not a regular user. So including it as auto-generation 
at compile time is broken. You could move the dynamically generated list 
into a sub-test level and have just one top level test that statically 
created and called 'all_kunit_tests' or something. No idea how to go 
about re-working all the kunit library code to create sub-tests instead 
of top level tests, though.

John.

>
> Regards,
> Kamil
>
>> -static const char *live_tests[] = {
>> -	"xe_bo",
>> -	"xe_dma_buf",
>> -	"xe_migrate",
>> -	"xe_mocs",
>> -};
>> -
>>   igt_main
>>   {
>> -	int i;
>> +	IGT_LIST_HEAD(names);
>> +	IGT_LIST_HEAD(done);
>> +	struct igt_kunit_names *name, *cmp;
>> +
>> +	igt_kunit_get_test_names("xe_live_test", NULL, &names);
>> +
>> +	while (!igt_list_empty(&names)) {
>> +		bool found = false;
>> +
>> +		name = igt_list_first_entry(&names, name, link);
>> +		igt_list_del(&name->link);
>> +
>> +		/*
>> +		 * Retuned list is sub-tests.
>> +		 * So, need filter out duplicated top level names.
>> +		 */
>> +		igt_list_for_each_entry(cmp, &done, link) {
>> +			if (strcmp(name->suite, cmp->suite) != 0)
>> +				continue;
>> +
>> +			found = true;
>> +		}
>> +
>> +		if (found)
>> +			continue;
>> +
>> +		igt_list_add(&name->link, &done);
>>   
>> -	for (i = 0; i < ARRAY_SIZE(live_tests); i++)
>> -		igt_kunit("xe_live_test", live_tests[i], NULL);
>> +		igt_kunit("xe_live_test", name->suite, NULL);
>> +	}
>>   }
>> -- 
>> 2.46.0
>>

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[John Harrison]

On 8/26/2024 06:07, Janusz Krzysztofik wrote:
> Hi John, Kamil,
>
> On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
>> Hi John.C.Harrison,
>> On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
>>> From: johnharr <johnharr@invalid-email.com>
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> Again invalid e-mail here.
>>
>>> The list of supported kunit tests is currently hard coded.
> That pattern originates from i915_selftest, where there are 3 stable subtests,
> "mock", "live" and "perf", each of them executing a (possibly long) list of
> dynamic sub-subtests actually provided by the module for each category.  Also,
> IIRC there was a separate module for each Xe test suite before, each mapped to
> a separate IGT test, later merged into one module with multiple suites and one
> test with multiple corresponding subtests.
Not sure if you are just explaining the history of the test or making a 
suggestion as to how it should evolve next?

>
>>> Which means
>>> that adding a new test to the kernel also requires patches to IGT as
>>> well.
>>>
>>> The list of available kunit tests is already exported by the kernel.
>>> So there is no need to bake a list into the IGT source code. So, add
>>> support for querying the test list dynamically.
>>>
>>> NB: Currently, the test list can only be queried by root but the IGT
>>> build system queries all tests at compile time. In theory this should
>>> not be a problem. However, the kunit helper code is all written to run
>>> inside a test and not inside the prep code, which means that when it
>>> fails to open the root only interfaces, it calls 'skip'. And skip is
>>> not allowed outside of running a test. Hence the build fails with:
>>>    skipping is allowed only in fixtures, subtests or igt_simple_main
>> Looks like we should fix it, move out skips from kunit libs.
> I suggest you consider a different approach: for a module, call igt_kunit()
> only once, with NULL suite argument.  As a result, you'll get results from one
> IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-subtests
> called "<test_suite>-<test_case>", one for each test case provided by the
> module.
I'm not following. This is what my patch does, isn't it? There is a 
single call to igt_kunit_get_test_names() which returns a list of 
'suite/case' name pairs having already registered those as dynamic 
sub-tests. Then it loops through the list running the suites (with any 
command line filtering being automatically applied to filter out sub-tests).


>
>>> And of course, putting all the query code inside an igt_fixture block
>>> means it doesn't get run as part of --show-testlist or --list-subtests.
>>>
>>> Reworking the kunit code to fail cleanly rather than skipping
>>> everywhere seems much more invasive. It is also not clear why the
>>> kunit code is considered 'library' when it is execlusively used from
>>> the kunit test alone!?
> Current users:
> 	igt$ grep -rl igt_kunit tests/
> 	tests/intel/xe_live_ktest.c
> 	tests/kms_selftest.c
> 	tests/drm_mm.c
> 	tests/drm_buddy.c
>
> The other 3 were first converted from i915_selftest standard to KUnit still
> before Xe KUnit IGT tests started appearing.

Argh, I must have been inside the 'intel' directory when I did my 
search! Oops.

The KMS one has another hard coded test list. The drm_mm one has a 
commented hard coded test list but actually just runs everything with no 
compiled in lists and the drm_buddy test has no test lists at all. Not 
sure what tests/sub-tests they actually run because all three of those 
just skip for me. Not sure why. The xe_live_ktest one runs everything 
fine so I definitely have the ktest system compiled in to the kernel. 
And this is running with a stock IGT tree, not with my patch, so it's 
not that my patch is breaking it.

John.

>
> Thanks,
> Janusz
>
>>> So posting this as an RFC before going too far
>>> down the possible rabbit hole.
>>>
>> As I see this, it is a lib because some other vendor (like amdpgu)
>> could use it.
>>
>> +cc Janusz
>> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
>>
>> btw could you split this into two patches, one for lib/* and
>> one for tests/intel/xe_live_ktest ?
>>
>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>> ---
>>>   lib/igt_kmod.c              | 68 +++++++++++++++++++++++++++++++++++++
>>>   lib/igt_kmod.h              |  6 ++++
>>>   tests/intel/xe_live_ktest.c | 60 ++++++++++++++++----------------
>>>   3 files changed, 103 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>>> index 464c0dcf484a..69c1254b397c 100644
>>> --- a/lib/igt_kmod.c
>>> +++ b/lib/igt_kmod.c
>>> @@ -1414,6 +1414,74 @@ static void __igt_kunit(struct igt_ktest *tst,
>>>   	}
>>>   }
>>>   
>>> +/**
>>> + * igt_kunit:
>>> + * @module_name: the name of the module
>>> + * @suite: the name of test suite to be executed, also used as subtest name;
>>> + *	   if NULL then test cases from all test suites provided by the module
>>> + *	   are executed as dynamic sub-subtests of one IGT subtest, which name
>>> + *	   is derived from the module name by cutting off its optional trailing
>>> + *	   _test or _kunit suffix
>>> + * @opts: options to load the module
>>> + *
>>> + * Loads the test module, parses its (k)tap dmesg output, then unloads it
>>> + */
>>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names)
>>> +{
>>> +	char debugfs_path[PATH_MAX] = { '\0', };
>>> +	struct igt_ktest tst = { .kmsg = -1, };
>>> +	struct igt_ktap_results *ktap = NULL;
>>> +	const char *subtest;
>>> +	char *suite_name = NULL, *case_name = NULL;
>>> +	DIR *debugfs_dir = NULL;
>>> +	IGT_LIST_HEAD(tests);
>>> +
>>> +	subtest = strdup(module_name);
>>> +	if (!igt_debug_on(!subtest)) {
>>> +		char *suffix = strstr(subtest, "_test");
>>> +
>>> +		if (!suffix)
>>> +			suffix = strstr(subtest, "_kunit");
>>> +
>>> +		if (suffix)
>>> +			*suffix = '\0';
>>> +	}
>>> +
>>> +	igt_require(subtest);
>>> +
>>> +	igt_require(!igt_ktest_init(&tst, module_name));
>>> +	igt_require(!igt_ktest_begin(&tst));
>>> +
>>> +	igt_ignore_warn(igt_kmod_load("kunit", NULL));
>>> +
>>> +	kunit_debugfs_path(debugfs_path);
>>> +	igt_info("kunit path: %s\n", debugfs_path);
>>> +	if (igt_debug_on(!*debugfs_path) ||
>>> +	    !kunit_get_tests(&tests, &tst, NULL, opts, debugfs_path, &debugfs_dir, &ktap)) {
>>> +		igt_info("kunit - no tests found!?\n");
>>> +	} else {
>>> +		struct igt_ktap_result *t;
>>> +		igt_info("kunit tests:\n");
>>> +		igt_list_for_each_entry(t, &tests, link) {
>>> +			struct igt_kunit_names *name = malloc(sizeof(*name));
>>> +			name->suite = strdup(t->suite_name);
>>> +			name->sub = strdup(t->case_name);
>>> +			igt_info("  %s / %s\n", t->suite_name, t->case_name);
>>> +			igt_list_add(&name->link, names);
>>> +		}
>>> +	}
>>> +
>>> +	igt_ktap_free(&ktap);
>>> +
>>> +	kunit_results_free(&tests, &suite_name, &case_name);
>>> +
>>> +	if (debugfs_dir)
>>> +		closedir(debugfs_dir);
>>> +
>>> +	igt_ktest_end(&tst);
>>> +	igt_ktest_fini(&tst);
>>> +}
>>> +
>>>   /**
>>>    * igt_kunit:
>>>    * @module_name: the name of the module
>>> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
>>> index efb46da128d4..2f608eb69d48 100644
>>> --- a/lib/igt_kmod.h
>>> +++ b/lib/igt_kmod.h
>>> @@ -71,7 +71,13 @@ static inline int igt_xe_driver_unload(void)
>>>   int igt_amdgpu_driver_load(const char *opts);
>>>   int igt_amdgpu_driver_unload(void);
>>>   
>>> +struct igt_kunit_names {
>>> +	char *suite;
>>> +	char *sub;
>>> +	struct igt_list_head link;
>>> +};
>>>   void igt_kunit(const char *module_name, const char *name, const char *opts);
>>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names);
>>>   
>>>   void igt_kselftests(const char *module_name,
>>>   		    const char *module_options,
>>> diff --git a/tests/intel/xe_live_ktest.c b/tests/intel/xe_live_ktest.c
>>> index 4376d5df7751..13265e87ba9d 100644
>>> --- a/tests/intel/xe_live_ktest.c
>>> +++ b/tests/intel/xe_live_ktest.c
>>> @@ -9,40 +9,38 @@
>>>    * Sub-category: kunit
>>>    * Functionality: kunit test
>>>    * Test category: functionality test
>>> - *
>>> - * SUBTEST: xe_bo
>>> - * Description:
>>> - *	Kernel dynamic selftests to check if GPU buffer objects are
>>> - *	being handled properly.
>>> - * Functionality: bo
>>> - *
>>> - * SUBTEST: xe_dma_buf
>>> - * Description: Kernel dynamic selftests for dmabuf functionality.
>>> - * Functionality: dmabuf test
>>> - *
>>> - * SUBTEST: xe_migrate
>>> - * Description:
>>> - *	Kernel dynamic selftests to check if page table migrations
>>> - *	are working properly.
>>> - * Functionality: migrate
>>> - *
>>> - * SUBTEST: xe_mocs
>>> - * Description:
>>> - *	Kernel dynamic selftests to check mocs configuration.
>>> - * Functionality: mocs configuration
>>>    */
>>>   
>> Does that mean we will not have any subtests here? The point
>> into moving subtests names into documentation was to have a testlist
>> generated during compilation, so it will not depend on running
>> test with --list option. Adding also Katarzyna to cc.
>> Cc: Katarzyna Piecielska <katarzyna.piecielska@intel.com>
>>
>> Regards,
>> Kamil
>>
>>> -static const char *live_tests[] = {
>>> -	"xe_bo",
>>> -	"xe_dma_buf",
>>> -	"xe_migrate",
>>> -	"xe_mocs",
>>> -};
>>> -
>>>   igt_main
>>>   {
>>> -	int i;
>>> +	IGT_LIST_HEAD(names);
>>> +	IGT_LIST_HEAD(done);
>>> +	struct igt_kunit_names *name, *cmp;
>>> +
>>> +	igt_kunit_get_test_names("xe_live_test", NULL, &names);
>>> +
>>> +	while (!igt_list_empty(&names)) {
>>> +		bool found = false;
>>> +
>>> +		name = igt_list_first_entry(&names, name, link);
>>> +		igt_list_del(&name->link);
>>> +
>>> +		/*
>>> +		 * Retuned list is sub-tests.
>>> +		 * So, need filter out duplicated top level names.
>>> +		 */
>>> +		igt_list_for_each_entry(cmp, &done, link) {
>>> +			if (strcmp(name->suite, cmp->suite) != 0)
>>> +				continue;
>>> +
>>> +			found = true;
>>> +		}
>>> +
>>> +		if (found)
>>> +			continue;
>>> +
>>> +		igt_list_add(&name->link, &done);
>>>   
>>> -	for (i = 0; i < ARRAY_SIZE(live_tests); i++)
>>> -		igt_kunit("xe_live_test", live_tests[i], NULL);
>>> +		igt_kunit("xe_live_test", name->suite, NULL);
>>> +	}
>>>   }
>
>
>

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[Janusz Krzysztofik]

Hi John,

On Wednesday, 28 August 2024 20:04:06 GMT+2 John Harrison wrote:
> On 8/26/2024 06:07, Janusz Krzysztofik wrote:
> > Hi John, Kamil,
> >
> > On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
> >> Hi John.C.Harrison,
> >> On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
> >>> From: johnharr <johnharr@invalid-email.com>
> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >> Again invalid e-mail here.
> >>
> >>> The list of supported kunit tests is currently hard coded.
> > That pattern originates from i915_selftest, where there are 3 stable subtests,
> > "mock", "live" and "perf", each of them executing a (possibly long) list of
> > dynamic sub-subtests actually provided by the module for each category.  Also,
> > IIRC there was a separate module for each Xe test suite before, each mapped to
> > a separate IGT test, later merged into one module with multiple suites and one
> > test with multiple corresponding subtests.
> Not sure if you are just explaining the history of the test or making a 
> suggestion as to how it should evolve next?

Both, I think.  Maybe not the history, but origin of ideas standing behind the 
implementation, and how tests are expected to use it (and maybe evolve if now 
doing that in a different way).

> 
> >
> >>> Which means
> >>> that adding a new test to the kernel also requires patches to IGT as
> >>> well.
> >>>
> >>> The list of available kunit tests is already exported by the kernel.
> >>> So there is no need to bake a list into the IGT source code. So, add
> >>> support for querying the test list dynamically.
> >>>
> >>> NB: Currently, the test list can only be queried by root but the IGT
> >>> build system queries all tests at compile time. In theory this should
> >>> not be a problem. However, the kunit helper code is all written to run
> >>> inside a test and not inside the prep code, which means that when it
> >>> fails to open the root only interfaces, it calls 'skip'. And skip is
> >>> not allowed outside of running a test. Hence the build fails with:
> >>>    skipping is allowed only in fixtures, subtests or igt_simple_main
> >> Looks like we should fix it, move out skips from kunit libs.
> > I suggest you consider a different approach: for a module, call igt_kunit()
> > only once, with NULL suite argument.  As a result, you'll get results from one
> > IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-subtests
> > called "<test_suite>-<test_case>", one for each test case provided by the
> > module.
> I'm not following. This is what my patch does, isn't it? 

No, your patch introduces a runtime determined list of subtests -- something 
not existent in IGT.

An IGT test may consist of one or more statically defined subtests with pre-
defined names.  The term dynamic subtest is usually used in two meanings.  It 
may mean a subtest of type igt_subtest_with_dynamic, still with a pre-defined 
name, but with a runtime determined list of sub-subtests, sometimes called 
dynamic sub-subtests, but often also called just dynamic subtests.  Names of 
dynamic sub-subtests are determined at runtime.

My approach tries to address your need for a maintenance-free kunit IGT test 
source file in a different way.  I'm following the IGT standard of statically 
defined list of subtests with pre-defined names: one subtest of type 
igt_subtest_with_dynamic, named after the kunit test module name which you 
have to enter into the test code anyway, and providing all test cases from 
all test suites contained in that module reported as dynamic sub-subtests 
named <test_suite>-<test_case>.

> There is a 
> single call to igt_kunit_get_test_names() which returns a list of 
> 'suite/case' name pairs having already registered those as dynamic 
> sub-tests. Then it loops through the list running the suites (with any 
> command line filtering being automatically applied to filter out sub-tests).

That's what breaks the IGT rule of statically defined lists of subtests with 
pre-defined names.

> 
> 
> >
> >>> And of course, putting all the query code inside an igt_fixture block
> >>> means it doesn't get run as part of --show-testlist or --list-subtests.
> >>>
> >>> Reworking the kunit code to fail cleanly rather than skipping
> >>> everywhere seems much more invasive. It is also not clear why the
> >>> kunit code is considered 'library' when it is execlusively used from
> >>> the kunit test alone!?
> > Current users:
> > 	igt$ grep -rl igt_kunit tests/
> > 	tests/intel/xe_live_ktest.c
> > 	tests/kms_selftest.c
> > 	tests/drm_mm.c
> > 	tests/drm_buddy.c
> >
> > The other 3 were first converted from i915_selftest standard to KUnit still
> > before Xe KUnit IGT tests started appearing.
> 
> Argh, I must have been inside the 'intel' directory when I did my 
> search! Oops.
> 
> The KMS one has another hard coded test list. 

But that's not a list of test suites, that's has a hardcoded list of module 
names -- something not avoidable when kunit test cases you want to group in a 
single IGT test are split among several modules.  IIRC, each of those modules 
provides only one test suite.  Names of test suites may be the same as module 
names (with _kunit or _test suffix stripped) or may be different.  It was 
decided to use module names as subtest names, then we are always passing NULL 
as suite.  If module and suite names don't match then IGT dynamic sub-subtests 
are called <test_suite>-<test_case>, otherwise just <test_case>.

> The drm_mm one has a commented hard coded test list 

I don't like the idea of documenting dynamic sub-subtest names in IGT test 
sources -- for me that breaks current IGT standards in the opposite direction.  
But maybe an important business need stands behind.

> but actually just runs everything with no compiled in lists and the
> drm_buddy test has no test lists at all. 

Both have a module name hardcoded, also used as a name of IGT subtest.

> Not 
> sure what tests/sub-tests they actually run because all three of those 
> just skip for me. Not sure why. 

Have you got those drm kunit modules built and available to the running 
kernel?

> The xe_live_ktest one runs everything 
> fine so I definitely have the ktest system compiled in to the kernel. 
> And this is running with a stock IGT tree, not with my patch, so it's 
> not that my patch is breaking it.

Your patch can't break anything since it doesn't modify any existing 
functions, only introduces a new one.  As long as you don't force IGT tests to 
use it, nothing can break.

Thanks,
Janusz

> 
> John.
> 
> >
> > Thanks,
> > Janusz
> >
> >>> So posting this as an RFC before going too far
> >>> down the possible rabbit hole.
> >>>
> >> As I see this, it is a lib because some other vendor (like amdpgu)
> >> could use it.
> >>
> >> +cc Janusz
> >> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> >>
> >> btw could you split this into two patches, one for lib/* and
> >> one for tests/intel/xe_live_ktest ?
> >>
> >>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> >>> ---
> >>>   lib/igt_kmod.c              | 68 +++++++++++++++++++++++++++++++++++++
> >>>   lib/igt_kmod.h              |  6 ++++
> >>>   tests/intel/xe_live_ktest.c | 60 ++++++++++++++++----------------
> >>>   3 files changed, 103 insertions(+), 31 deletions(-)
> >>>
> >>> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> >>> index 464c0dcf484a..69c1254b397c 100644
> >>> --- a/lib/igt_kmod.c
> >>> +++ b/lib/igt_kmod.c
> >>> @@ -1414,6 +1414,74 @@ static void __igt_kunit(struct igt_ktest *tst,
> >>>   	}
> >>>   }
> >>>   
> >>> +/**
> >>> + * igt_kunit:
> >>> + * @module_name: the name of the module
> >>> + * @suite: the name of test suite to be executed, also used as subtest name;
> >>> + *	   if NULL then test cases from all test suites provided by the module
> >>> + *	   are executed as dynamic sub-subtests of one IGT subtest, which name
> >>> + *	   is derived from the module name by cutting off its optional trailing
> >>> + *	   _test or _kunit suffix
> >>> + * @opts: options to load the module
> >>> + *
> >>> + * Loads the test module, parses its (k)tap dmesg output, then unloads it
> >>> + */
> >>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names)
> >>> +{
> >>> +	char debugfs_path[PATH_MAX] = { '\0', };
> >>> +	struct igt_ktest tst = { .kmsg = -1, };
> >>> +	struct igt_ktap_results *ktap = NULL;
> >>> +	const char *subtest;
> >>> +	char *suite_name = NULL, *case_name = NULL;
> >>> +	DIR *debugfs_dir = NULL;
> >>> +	IGT_LIST_HEAD(tests);
> >>> +
> >>> +	subtest = strdup(module_name);
> >>> +	if (!igt_debug_on(!subtest)) {
> >>> +		char *suffix = strstr(subtest, "_test");
> >>> +
> >>> +		if (!suffix)
> >>> +			suffix = strstr(subtest, "_kunit");
> >>> +
> >>> +		if (suffix)
> >>> +			*suffix = '\0';
> >>> +	}
> >>> +
> >>> +	igt_require(subtest);
> >>> +
> >>> +	igt_require(!igt_ktest_init(&tst, module_name));
> >>> +	igt_require(!igt_ktest_begin(&tst));
> >>> +
> >>> +	igt_ignore_warn(igt_kmod_load("kunit", NULL));
> >>> +
> >>> +	kunit_debugfs_path(debugfs_path);
> >>> +	igt_info("kunit path: %s\n", debugfs_path);
> >>> +	if (igt_debug_on(!*debugfs_path) ||
> >>> +	    !kunit_get_tests(&tests, &tst, NULL, opts, debugfs_path, &debugfs_dir, &ktap)) {
> >>> +		igt_info("kunit - no tests found!?\n");
> >>> +	} else {
> >>> +		struct igt_ktap_result *t;
> >>> +		igt_info("kunit tests:\n");
> >>> +		igt_list_for_each_entry(t, &tests, link) {
> >>> +			struct igt_kunit_names *name = malloc(sizeof(*name));
> >>> +			name->suite = strdup(t->suite_name);
> >>> +			name->sub = strdup(t->case_name);
> >>> +			igt_info("  %s / %s\n", t->suite_name, t->case_name);
> >>> +			igt_list_add(&name->link, names);
> >>> +		}
> >>> +	}
> >>> +
> >>> +	igt_ktap_free(&ktap);
> >>> +
> >>> +	kunit_results_free(&tests, &suite_name, &case_name);
> >>> +
> >>> +	if (debugfs_dir)
> >>> +		closedir(debugfs_dir);
> >>> +
> >>> +	igt_ktest_end(&tst);
> >>> +	igt_ktest_fini(&tst);
> >>> +}
> >>> +
> >>>   /**
> >>>    * igt_kunit:
> >>>    * @module_name: the name of the module
> >>> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> >>> index efb46da128d4..2f608eb69d48 100644
> >>> --- a/lib/igt_kmod.h
> >>> +++ b/lib/igt_kmod.h
> >>> @@ -71,7 +71,13 @@ static inline int igt_xe_driver_unload(void)
> >>>   int igt_amdgpu_driver_load(const char *opts);
> >>>   int igt_amdgpu_driver_unload(void);
> >>>   
> >>> +struct igt_kunit_names {
> >>> +	char *suite;
> >>> +	char *sub;
> >>> +	struct igt_list_head link;
> >>> +};
> >>>   void igt_kunit(const char *module_name, const char *name, const char *opts);
> >>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names);
> >>>   
> >>>   void igt_kselftests(const char *module_name,
> >>>   		    const char *module_options,
> >>> diff --git a/tests/intel/xe_live_ktest.c b/tests/intel/xe_live_ktest.c
> >>> index 4376d5df7751..13265e87ba9d 100644
> >>> --- a/tests/intel/xe_live_ktest.c
> >>> +++ b/tests/intel/xe_live_ktest.c
> >>> @@ -9,40 +9,38 @@
> >>>    * Sub-category: kunit
> >>>    * Functionality: kunit test
> >>>    * Test category: functionality test
> >>> - *
> >>> - * SUBTEST: xe_bo
> >>> - * Description:
> >>> - *	Kernel dynamic selftests to check if GPU buffer objects are
> >>> - *	being handled properly.
> >>> - * Functionality: bo
> >>> - *
> >>> - * SUBTEST: xe_dma_buf
> >>> - * Description: Kernel dynamic selftests for dmabuf functionality.
> >>> - * Functionality: dmabuf test
> >>> - *
> >>> - * SUBTEST: xe_migrate
> >>> - * Description:
> >>> - *	Kernel dynamic selftests to check if page table migrations
> >>> - *	are working properly.
> >>> - * Functionality: migrate
> >>> - *
> >>> - * SUBTEST: xe_mocs
> >>> - * Description:
> >>> - *	Kernel dynamic selftests to check mocs configuration.
> >>> - * Functionality: mocs configuration
> >>>    */
> >>>   
> >> Does that mean we will not have any subtests here? The point
> >> into moving subtests names into documentation was to have a testlist
> >> generated during compilation, so it will not depend on running
> >> test with --list option. Adding also Katarzyna to cc.
> >> Cc: Katarzyna Piecielska <katarzyna.piecielska@intel.com>
> >>
> >> Regards,
> >> Kamil
> >>
> >>> -static const char *live_tests[] = {
> >>> -	"xe_bo",
> >>> -	"xe_dma_buf",
> >>> -	"xe_migrate",
> >>> -	"xe_mocs",
> >>> -};
> >>> -
> >>>   igt_main
> >>>   {
> >>> -	int i;
> >>> +	IGT_LIST_HEAD(names);
> >>> +	IGT_LIST_HEAD(done);
> >>> +	struct igt_kunit_names *name, *cmp;
> >>> +
> >>> +	igt_kunit_get_test_names("xe_live_test", NULL, &names);
> >>> +
> >>> +	while (!igt_list_empty(&names)) {
> >>> +		bool found = false;
> >>> +
> >>> +		name = igt_list_first_entry(&names, name, link);
> >>> +		igt_list_del(&name->link);
> >>> +
> >>> +		/*
> >>> +		 * Retuned list is sub-tests.
> >>> +		 * So, need filter out duplicated top level names.
> >>> +		 */
> >>> +		igt_list_for_each_entry(cmp, &done, link) {
> >>> +			if (strcmp(name->suite, cmp->suite) != 0)
> >>> +				continue;
> >>> +
> >>> +			found = true;
> >>> +		}
> >>> +
> >>> +		if (found)
> >>> +			continue;
> >>> +
> >>> +		igt_list_add(&name->link, &done);
> >>>   
> >>> -	for (i = 0; i < ARRAY_SIZE(live_tests); i++)
> >>> -		igt_kunit("xe_live_test", live_tests[i], NULL);
> >>> +		igt_kunit("xe_live_test", name->suite, NULL);
> >>> +	}
> >>>   }
> >
> >
> >
> 
> 

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[Janusz Krzysztofik]

Auto-correction.

On Thursday, 29 August 2024 11:57:00 GMT+2 Janusz Krzysztofik wrote:
> Hi John,
> 
> On Wednesday, 28 August 2024 20:04:06 GMT+2 John Harrison wrote:
> > On 8/26/2024 06:07, Janusz Krzysztofik wrote:
> > > Hi John, Kamil,
> > >
> > > On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
> > >> Hi John.C.Harrison,
> > >> On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
> > >>> From: johnharr <johnharr@invalid-email.com>
> > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >> Again invalid e-mail here.
> > >>
> > >>> The list of supported kunit tests is currently hard coded.
> > > That pattern originates from i915_selftest, where there are 3 stable subtests,
> > > "mock", "live" and "perf", each of them executing a (possibly long) list of
> > > dynamic sub-subtests actually provided by the module for each category.  Also,
> > > IIRC there was a separate module for each Xe test suite before, each mapped to
> > > a separate IGT test, later merged into one module with multiple suites and one
> > > test with multiple corresponding subtests.
> > Not sure if you are just explaining the history of the test or making a 
> > suggestion as to how it should evolve next?
> 
> Both, I think.  Maybe not the history, but origin of ideas standing behind the 
> implementation, and how tests are expected to use it (and maybe evolve if now 
> doing that in a different way).
> 
> > 
> > >
> > >>> Which means
> > >>> that adding a new test to the kernel also requires patches to IGT as
> > >>> well.
> > >>>
> > >>> The list of available kunit tests is already exported by the kernel.
> > >>> So there is no need to bake a list into the IGT source code. So, add
> > >>> support for querying the test list dynamically.
> > >>>
> > >>> NB: Currently, the test list can only be queried by root but the IGT
> > >>> build system queries all tests at compile time. In theory this should
> > >>> not be a problem. However, the kunit helper code is all written to run
> > >>> inside a test and not inside the prep code, which means that when it
> > >>> fails to open the root only interfaces, it calls 'skip'. And skip is
> > >>> not allowed outside of running a test. Hence the build fails with:
> > >>>    skipping is allowed only in fixtures, subtests or igt_simple_main
> > >> Looks like we should fix it, move out skips from kunit libs.
> > > I suggest you consider a different approach: for a module, call igt_kunit()
> > > only once, with NULL suite argument.  As a result, you'll get results from one
> > > IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-subtests
> > > called "<test_suite>-<test_case>", one for each test case provided by the
> > > module.
> > I'm not following. This is what my patch does, isn't it? 
> 
> No, your patch introduces a runtime determined list of subtests -- something 
> not existent in IGT.
> 
> An IGT test may consist of one or more statically defined subtests with pre-
> defined names.  The term dynamic subtest is usually used in two meanings.  It 
> may mean a subtest of type igt_subtest_with_dynamic, still with a pre-defined 
> name, but with a runtime determined list of sub-subtests, sometimes called 
> dynamic sub-subtests, but often also called just dynamic subtests.  Names of 
> dynamic sub-subtests are determined at runtime.
> 
> My approach tries to address your need for a maintenance-free kunit IGT test 
> source file in a different way.  I'm following the IGT standard of statically 
> defined list of subtests with pre-defined names: one subtest of type 
> igt_subtest_with_dynamic, named after the kunit test module name which you 
> have to enter into the test code anyway, and providing all test cases from 
> all test suites contained in that module reported as dynamic sub-subtests 
> named <test_suite>-<test_case>.
> 
> > There is a 
> > single call to igt_kunit_get_test_names() which returns a list of 
> > 'suite/case' name pairs having already registered those as dynamic 
> > sub-tests. Then it loops through the list running the suites (with any 
> > command line filtering being automatically applied to filter out sub-tests).
> 
> That's what breaks the IGT rule of statically defined lists of subtests with 
> pre-defined names.
> 
> > 
> > 
> > >
> > >>> And of course, putting all the query code inside an igt_fixture block
> > >>> means it doesn't get run as part of --show-testlist or --list-subtests.
> > >>>
> > >>> Reworking the kunit code to fail cleanly rather than skipping
> > >>> everywhere seems much more invasive. It is also not clear why the
> > >>> kunit code is considered 'library' when it is execlusively used from
> > >>> the kunit test alone!?
> > > Current users:
> > > 	igt$ grep -rl igt_kunit tests/
> > > 	tests/intel/xe_live_ktest.c
> > > 	tests/kms_selftest.c
> > > 	tests/drm_mm.c
> > > 	tests/drm_buddy.c
> > >
> > > The other 3 were first converted from i915_selftest standard to KUnit still
> > > before Xe KUnit IGT tests started appearing.
> > 
> > Argh, I must have been inside the 'intel' directory when I did my 
> > search! Oops.
> > 
> > The KMS one has another hard coded test list. 
> 
> But that's not a list of test suites, that's has a hardcoded list of module 
> names -- something not avoidable when kunit test cases you want to group in a 
> single IGT test are split among several modules.  IIRC, each of those modules 
> provides only one test suite.  Names of test suites may be the same as module 
> names (with _kunit or _test suffix stripped) or may be different.  It was 
> decided to use module names as subtest names, then we are always passing NULL 
> as suite.  If module and suite names don't match then IGT dynamic sub-subtests 
> are called <test_suite>-<test_case>, otherwise just <test_case>.
> 
> > The drm_mm one has a commented hard coded test list 
> 
> I don't like the idea of documenting dynamic sub-subtest names in IGT test 
> sources -- for me that breaks current IGT standards in the opposite direction.  
> But maybe an important business need stands behind.
> 
> > but actually just runs everything with no compiled in lists and the
> > drm_buddy test has no test lists at all. 
> 
> Both have a module name hardcoded, also used as a name of IGT subtest.

True for drm_mm.  In case of drm_buddy, suite name was apparently different 
from module name, that's why it was decided to pass its name to igt_kunit() as 
suite for use as IGT subtest name and stripping meaningless <test_suite>- 
prefixes from names of IGT dynamic sub-subtest.

Thanks,
Janusz

> 
> > Not 
> > sure what tests/sub-tests they actually run because all three of those 
> > just skip for me. Not sure why. 
> 
> Have you got those drm kunit modules built and available to the running 
> kernel?
> 
> > The xe_live_ktest one runs everything 
> > fine so I definitely have the ktest system compiled in to the kernel. 
> > And this is running with a stock IGT tree, not with my patch, so it's 
> > not that my patch is breaking it.
> 
> Your patch can't break anything since it doesn't modify any existing 
> functions, only introduces a new one.  As long as you don't force IGT tests to 
> use it, nothing can break.
> 
> Thanks,
> Janusz
> 
> > 
> > John.
> > 
> > >
> > > Thanks,
> > > Janusz
> > >
> > >>> So posting this as an RFC before going too far
> > >>> down the possible rabbit hole.
> > >>>
> > >> As I see this, it is a lib because some other vendor (like amdpgu)
> > >> could use it.
> > >>
> > >> +cc Janusz
> > >> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > >>
> > >> btw could you split this into two patches, one for lib/* and
> > >> one for tests/intel/xe_live_ktest ?
> > >>
> > >>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > >>> ---
> > >>>   lib/igt_kmod.c              | 68 +++++++++++++++++++++++++++++++++++++
> > >>>   lib/igt_kmod.h              |  6 ++++
> > >>>   tests/intel/xe_live_ktest.c | 60 ++++++++++++++++----------------
> > >>>   3 files changed, 103 insertions(+), 31 deletions(-)
> > >>>
> > >>> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > >>> index 464c0dcf484a..69c1254b397c 100644
> > >>> --- a/lib/igt_kmod.c
> > >>> +++ b/lib/igt_kmod.c
> > >>> @@ -1414,6 +1414,74 @@ static void __igt_kunit(struct igt_ktest *tst,
> > >>>   	}
> > >>>   }
> > >>>   
> > >>> +/**
> > >>> + * igt_kunit:
> > >>> + * @module_name: the name of the module
> > >>> + * @suite: the name of test suite to be executed, also used as subtest name;
> > >>> + *	   if NULL then test cases from all test suites provided by the module
> > >>> + *	   are executed as dynamic sub-subtests of one IGT subtest, which name
> > >>> + *	   is derived from the module name by cutting off its optional trailing
> > >>> + *	   _test or _kunit suffix
> > >>> + * @opts: options to load the module
> > >>> + *
> > >>> + * Loads the test module, parses its (k)tap dmesg output, then unloads it
> > >>> + */
> > >>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names)
> > >>> +{
> > >>> +	char debugfs_path[PATH_MAX] = { '\0', };
> > >>> +	struct igt_ktest tst = { .kmsg = -1, };
> > >>> +	struct igt_ktap_results *ktap = NULL;
> > >>> +	const char *subtest;
> > >>> +	char *suite_name = NULL, *case_name = NULL;
> > >>> +	DIR *debugfs_dir = NULL;
> > >>> +	IGT_LIST_HEAD(tests);
> > >>> +
> > >>> +	subtest = strdup(module_name);
> > >>> +	if (!igt_debug_on(!subtest)) {
> > >>> +		char *suffix = strstr(subtest, "_test");
> > >>> +
> > >>> +		if (!suffix)
> > >>> +			suffix = strstr(subtest, "_kunit");
> > >>> +
> > >>> +		if (suffix)
> > >>> +			*suffix = '\0';
> > >>> +	}
> > >>> +
> > >>> +	igt_require(subtest);
> > >>> +
> > >>> +	igt_require(!igt_ktest_init(&tst, module_name));
> > >>> +	igt_require(!igt_ktest_begin(&tst));
> > >>> +
> > >>> +	igt_ignore_warn(igt_kmod_load("kunit", NULL));
> > >>> +
> > >>> +	kunit_debugfs_path(debugfs_path);
> > >>> +	igt_info("kunit path: %s\n", debugfs_path);
> > >>> +	if (igt_debug_on(!*debugfs_path) ||
> > >>> +	    !kunit_get_tests(&tests, &tst, NULL, opts, debugfs_path, &debugfs_dir, &ktap)) {
> > >>> +		igt_info("kunit - no tests found!?\n");
> > >>> +	} else {
> > >>> +		struct igt_ktap_result *t;
> > >>> +		igt_info("kunit tests:\n");
> > >>> +		igt_list_for_each_entry(t, &tests, link) {
> > >>> +			struct igt_kunit_names *name = malloc(sizeof(*name));
> > >>> +			name->suite = strdup(t->suite_name);
> > >>> +			name->sub = strdup(t->case_name);
> > >>> +			igt_info("  %s / %s\n", t->suite_name, t->case_name);
> > >>> +			igt_list_add(&name->link, names);
> > >>> +		}
> > >>> +	}
> > >>> +
> > >>> +	igt_ktap_free(&ktap);
> > >>> +
> > >>> +	kunit_results_free(&tests, &suite_name, &case_name);
> > >>> +
> > >>> +	if (debugfs_dir)
> > >>> +		closedir(debugfs_dir);
> > >>> +
> > >>> +	igt_ktest_end(&tst);
> > >>> +	igt_ktest_fini(&tst);
> > >>> +}
> > >>> +
> > >>>   /**
> > >>>    * igt_kunit:
> > >>>    * @module_name: the name of the module
> > >>> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> > >>> index efb46da128d4..2f608eb69d48 100644
> > >>> --- a/lib/igt_kmod.h
> > >>> +++ b/lib/igt_kmod.h
> > >>> @@ -71,7 +71,13 @@ static inline int igt_xe_driver_unload(void)
> > >>>   int igt_amdgpu_driver_load(const char *opts);
> > >>>   int igt_amdgpu_driver_unload(void);
> > >>>   
> > >>> +struct igt_kunit_names {
> > >>> +	char *suite;
> > >>> +	char *sub;
> > >>> +	struct igt_list_head link;
> > >>> +};
> > >>>   void igt_kunit(const char *module_name, const char *name, const char *opts);
> > >>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names);
> > >>>   
> > >>>   void igt_kselftests(const char *module_name,
> > >>>   		    const char *module_options,
> > >>> diff --git a/tests/intel/xe_live_ktest.c b/tests/intel/xe_live_ktest.c
> > >>> index 4376d5df7751..13265e87ba9d 100644
> > >>> --- a/tests/intel/xe_live_ktest.c
> > >>> +++ b/tests/intel/xe_live_ktest.c
> > >>> @@ -9,40 +9,38 @@
> > >>>    * Sub-category: kunit
> > >>>    * Functionality: kunit test
> > >>>    * Test category: functionality test
> > >>> - *
> > >>> - * SUBTEST: xe_bo
> > >>> - * Description:
> > >>> - *	Kernel dynamic selftests to check if GPU buffer objects are
> > >>> - *	being handled properly.
> > >>> - * Functionality: bo
> > >>> - *
> > >>> - * SUBTEST: xe_dma_buf
> > >>> - * Description: Kernel dynamic selftests for dmabuf functionality.
> > >>> - * Functionality: dmabuf test
> > >>> - *
> > >>> - * SUBTEST: xe_migrate
> > >>> - * Description:
> > >>> - *	Kernel dynamic selftests to check if page table migrations
> > >>> - *	are working properly.
> > >>> - * Functionality: migrate
> > >>> - *
> > >>> - * SUBTEST: xe_mocs
> > >>> - * Description:
> > >>> - *	Kernel dynamic selftests to check mocs configuration.
> > >>> - * Functionality: mocs configuration
> > >>>    */
> > >>>   
> > >> Does that mean we will not have any subtests here? The point
> > >> into moving subtests names into documentation was to have a testlist
> > >> generated during compilation, so it will not depend on running
> > >> test with --list option. Adding also Katarzyna to cc.
> > >> Cc: Katarzyna Piecielska <katarzyna.piecielska@intel.com>
> > >>
> > >> Regards,
> > >> Kamil
> > >>
> > >>> -static const char *live_tests[] = {
> > >>> -	"xe_bo",
> > >>> -	"xe_dma_buf",
> > >>> -	"xe_migrate",
> > >>> -	"xe_mocs",
> > >>> -};
> > >>> -
> > >>>   igt_main
> > >>>   {
> > >>> -	int i;
> > >>> +	IGT_LIST_HEAD(names);
> > >>> +	IGT_LIST_HEAD(done);
> > >>> +	struct igt_kunit_names *name, *cmp;
> > >>> +
> > >>> +	igt_kunit_get_test_names("xe_live_test", NULL, &names);
> > >>> +
> > >>> +	while (!igt_list_empty(&names)) {
> > >>> +		bool found = false;
> > >>> +
> > >>> +		name = igt_list_first_entry(&names, name, link);
> > >>> +		igt_list_del(&name->link);
> > >>> +
> > >>> +		/*
> > >>> +		 * Retuned list is sub-tests.
> > >>> +		 * So, need filter out duplicated top level names.
> > >>> +		 */
> > >>> +		igt_list_for_each_entry(cmp, &done, link) {
> > >>> +			if (strcmp(name->suite, cmp->suite) != 0)
> > >>> +				continue;
> > >>> +
> > >>> +			found = true;
> > >>> +		}
> > >>> +
> > >>> +		if (found)
> > >>> +			continue;
> > >>> +
> > >>> +		igt_list_add(&name->link, &done);
> > >>>   
> > >>> -	for (i = 0; i < ARRAY_SIZE(live_tests); i++)
> > >>> -		igt_kunit("xe_live_test", live_tests[i], NULL);
> > >>> +		igt_kunit("xe_live_test", name->suite, NULL);
> > >>> +	}
> > >>>   }
> > >
> > >
> > >
> > 
> > 
> 
> 
> 
> 
> 

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[Janusz Krzysztofik]

On Thursday, 29 August 2024 12:40:31 GMT+2 Janusz Krzysztofik wrote:
> Auto-correction.
> 
> On Thursday, 29 August 2024 11:57:00 GMT+2 Janusz Krzysztofik wrote:
> > Hi John,
> > 
> > On Wednesday, 28 August 2024 20:04:06 GMT+2 John Harrison wrote:
> > > On 8/26/2024 06:07, Janusz Krzysztofik wrote:
> > > > Hi John, Kamil,
> > > >
> > > > On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
> > > >> Hi John.C.Harrison,
> > > >> On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
> > > >>> From: johnharr <johnharr@invalid-email.com>
> > > >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >> Again invalid e-mail here.
> > > >>
> > > >>> The list of supported kunit tests is currently hard coded.
> > > > That pattern originates from i915_selftest, where there are 3 stable subtests,
> > > > "mock", "live" and "perf", each of them executing a (possibly long) list of
> > > > dynamic sub-subtests actually provided by the module for each category.  Also,
> > > > IIRC there was a separate module for each Xe test suite before, each mapped to
> > > > a separate IGT test, later merged into one module with multiple suites and one
> > > > test with multiple corresponding subtests.
> > > Not sure if you are just explaining the history of the test or making a 
> > > suggestion as to how it should evolve next?
> > 
> > Both, I think.  Maybe not the history, but origin of ideas standing behind the 
> > implementation, and how tests are expected to use it (and maybe evolve if now 
> > doing that in a different way).
> > 
> > > 
> > > >
> > > >>> Which means
> > > >>> that adding a new test to the kernel also requires patches to IGT as
> > > >>> well.
> > > >>>
> > > >>> The list of available kunit tests is already exported by the kernel.
> > > >>> So there is no need to bake a list into the IGT source code. So, add
> > > >>> support for querying the test list dynamically.
> > > >>>
> > > >>> NB: Currently, the test list can only be queried by root but the IGT
> > > >>> build system queries all tests at compile time. In theory this should
> > > >>> not be a problem. However, the kunit helper code is all written to run
> > > >>> inside a test and not inside the prep code, which means that when it
> > > >>> fails to open the root only interfaces, it calls 'skip'. And skip is
> > > >>> not allowed outside of running a test. Hence the build fails with:
> > > >>>    skipping is allowed only in fixtures, subtests or igt_simple_main
> > > >> Looks like we should fix it, move out skips from kunit libs.
> > > > I suggest you consider a different approach: for a module, call igt_kunit()
> > > > only once, with NULL suite argument.  As a result, you'll get results from one
> > > > IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-subtests
> > > > called "<test_suite>-<test_case>", one for each test case provided by the
> > > > module.
> > > I'm not following. This is what my patch does, isn't it? 
> > 
> > No, your patch introduces a runtime determined list of subtests -- something 
> > not existent in IGT.
> > 
> > An IGT test may consist of one or more statically defined subtests with pre-
> > defined names.  The term dynamic subtest is usually used in two meanings.  It 
> > may mean a subtest of type igt_subtest_with_dynamic, still with a pre-defined 
> > name, but with a runtime determined list of sub-subtests, sometimes called 
> > dynamic sub-subtests, but often also called just dynamic subtests.  Names of 
> > dynamic sub-subtests are determined at runtime.
> > 
> > My approach tries to address your need for a maintenance-free kunit IGT test 
> > source file in a different way.  I'm following the IGT standard of statically 
> > defined list of subtests with pre-defined names: one subtest of type 
> > igt_subtest_with_dynamic, named after the kunit test module name which you 
> > have to enter into the test code anyway, and providing all test cases from 
> > all test suites contained in that module reported as dynamic sub-subtests 
> > named <test_suite>-<test_case>.
> > 
> > > There is a 
> > > single call to igt_kunit_get_test_names() which returns a list of 
> > > 'suite/case' name pairs having already registered those as dynamic 
> > > sub-tests. Then it loops through the list running the suites (with any 
> > > command line filtering being automatically applied to filter out sub-tests).
> > 
> > That's what breaks the IGT rule of statically defined lists of subtests with 
> > pre-defined names.
> > 
> > > 
> > > 
> > > >
> > > >>> And of course, putting all the query code inside an igt_fixture block
> > > >>> means it doesn't get run as part of --show-testlist or --list-subtests.
> > > >>>
> > > >>> Reworking the kunit code to fail cleanly rather than skipping
> > > >>> everywhere seems much more invasive. It is also not clear why the
> > > >>> kunit code is considered 'library' when it is execlusively used from
> > > >>> the kunit test alone!?
> > > > Current users:
> > > > 	igt$ grep -rl igt_kunit tests/
> > > > 	tests/intel/xe_live_ktest.c
> > > > 	tests/kms_selftest.c
> > > > 	tests/drm_mm.c
> > > > 	tests/drm_buddy.c
> > > >
> > > > The other 3 were first converted from i915_selftest standard to KUnit still
> > > > before Xe KUnit IGT tests started appearing.
> > > 
> > > Argh, I must have been inside the 'intel' directory when I did my 
> > > search! Oops.
> > > 
> > > The KMS one has another hard coded test list. 
> > 
> > But that's not a list of test suites, that's has a hardcoded list of module 
> > names -- something not avoidable when kunit test cases you want to group in a 
> > single IGT test are split among several modules.  IIRC, each of those modules 
> > provides only one test suite.  Names of test suites may be the same as module 
> > names (with _kunit or _test suffix stripped) or may be different.  It was 
> > decided to use module names as subtest names, then we are always passing NULL 
> > as suite.  If module and suite names don't match then IGT dynamic sub-subtests 
> > are called <test_suite>-<test_case>, otherwise just <test_case>.
> > 
> > > The drm_mm one has a commented hard coded test list 
> > 
> > I don't like the idea of documenting dynamic sub-subtest names in IGT test 
> > sources -- for me that breaks current IGT standards in the opposite direction.  
> > But maybe an important business need stands behind.
> > 
> > > but actually just runs everything with no compiled in lists and the
> > > drm_buddy test has no test lists at all. 
> > 
> > Both have a module name hardcoded, also used as a name of IGT subtest.
> 
> True for drm_mm.  In case of drm_buddy, suite name was apparently different 
> from module name, that's why it was decided to pass its name to igt_kunit() as 
> suite for use as IGT subtest name and stripping meaningless <test_suite>- 
> prefixes from names of IGT dynamic sub-subtest.

Sorry, I was wrong, no suite name is passed to igt_kunit() in drm_buddy (that 
was only a left over from my testing of invalid suite name handling).

Thanks,
Janusz

> 
> Thanks,
> Janusz
> 
> > 
> > > Not 
> > > sure what tests/sub-tests they actually run because all three of those 
> > > just skip for me. Not sure why. 
> > 
> > Have you got those drm kunit modules built and available to the running 
> > kernel?
> > 
> > > The xe_live_ktest one runs everything 
> > > fine so I definitely have the ktest system compiled in to the kernel. 
> > > And this is running with a stock IGT tree, not with my patch, so it's 
> > > not that my patch is breaking it.
> > 
> > Your patch can't break anything since it doesn't modify any existing 
> > functions, only introduces a new one.  As long as you don't force IGT tests to 
> > use it, nothing can break.
> > 
> > Thanks,
> > Janusz
> > 
> > > 
> > > John.
> > > 
> > > >
> > > > Thanks,
> > > > Janusz
> > > >
> > > >>> So posting this as an RFC before going too far
> > > >>> down the possible rabbit hole.
> > > >>>
> > > >> As I see this, it is a lib because some other vendor (like amdpgu)
> > > >> could use it.
> > > >>
> > > >> +cc Janusz
> > > >> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> > > >>
> > > >> btw could you split this into two patches, one for lib/* and
> > > >> one for tests/intel/xe_live_ktest ?
> > > >>
> > > >>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > > >>> ---
> > > >>>   lib/igt_kmod.c              | 68 +++++++++++++++++++++++++++++++++++++
> > > >>>   lib/igt_kmod.h              |  6 ++++
> > > >>>   tests/intel/xe_live_ktest.c | 60 ++++++++++++++++----------------
> > > >>>   3 files changed, 103 insertions(+), 31 deletions(-)
> > > >>>
> > > >>> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> > > >>> index 464c0dcf484a..69c1254b397c 100644
> > > >>> --- a/lib/igt_kmod.c
> > > >>> +++ b/lib/igt_kmod.c
> > > >>> @@ -1414,6 +1414,74 @@ static void __igt_kunit(struct igt_ktest *tst,
> > > >>>   	}
> > > >>>   }
> > > >>>   
> > > >>> +/**
> > > >>> + * igt_kunit:
> > > >>> + * @module_name: the name of the module
> > > >>> + * @suite: the name of test suite to be executed, also used as subtest name;
> > > >>> + *	   if NULL then test cases from all test suites provided by the module
> > > >>> + *	   are executed as dynamic sub-subtests of one IGT subtest, which name
> > > >>> + *	   is derived from the module name by cutting off its optional trailing
> > > >>> + *	   _test or _kunit suffix
> > > >>> + * @opts: options to load the module
> > > >>> + *
> > > >>> + * Loads the test module, parses its (k)tap dmesg output, then unloads it
> > > >>> + */
> > > >>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names)
> > > >>> +{
> > > >>> +	char debugfs_path[PATH_MAX] = { '\0', };
> > > >>> +	struct igt_ktest tst = { .kmsg = -1, };
> > > >>> +	struct igt_ktap_results *ktap = NULL;
> > > >>> +	const char *subtest;
> > > >>> +	char *suite_name = NULL, *case_name = NULL;
> > > >>> +	DIR *debugfs_dir = NULL;
> > > >>> +	IGT_LIST_HEAD(tests);
> > > >>> +
> > > >>> +	subtest = strdup(module_name);
> > > >>> +	if (!igt_debug_on(!subtest)) {
> > > >>> +		char *suffix = strstr(subtest, "_test");
> > > >>> +
> > > >>> +		if (!suffix)
> > > >>> +			suffix = strstr(subtest, "_kunit");
> > > >>> +
> > > >>> +		if (suffix)
> > > >>> +			*suffix = '\0';
> > > >>> +	}
> > > >>> +
> > > >>> +	igt_require(subtest);
> > > >>> +
> > > >>> +	igt_require(!igt_ktest_init(&tst, module_name));
> > > >>> +	igt_require(!igt_ktest_begin(&tst));
> > > >>> +
> > > >>> +	igt_ignore_warn(igt_kmod_load("kunit", NULL));
> > > >>> +
> > > >>> +	kunit_debugfs_path(debugfs_path);
> > > >>> +	igt_info("kunit path: %s\n", debugfs_path);
> > > >>> +	if (igt_debug_on(!*debugfs_path) ||
> > > >>> +	    !kunit_get_tests(&tests, &tst, NULL, opts, debugfs_path, &debugfs_dir, &ktap)) {
> > > >>> +		igt_info("kunit - no tests found!?\n");
> > > >>> +	} else {
> > > >>> +		struct igt_ktap_result *t;
> > > >>> +		igt_info("kunit tests:\n");
> > > >>> +		igt_list_for_each_entry(t, &tests, link) {
> > > >>> +			struct igt_kunit_names *name = malloc(sizeof(*name));
> > > >>> +			name->suite = strdup(t->suite_name);
> > > >>> +			name->sub = strdup(t->case_name);
> > > >>> +			igt_info("  %s / %s\n", t->suite_name, t->case_name);
> > > >>> +			igt_list_add(&name->link, names);
> > > >>> +		}
> > > >>> +	}
> > > >>> +
> > > >>> +	igt_ktap_free(&ktap);
> > > >>> +
> > > >>> +	kunit_results_free(&tests, &suite_name, &case_name);
> > > >>> +
> > > >>> +	if (debugfs_dir)
> > > >>> +		closedir(debugfs_dir);
> > > >>> +
> > > >>> +	igt_ktest_end(&tst);
> > > >>> +	igt_ktest_fini(&tst);
> > > >>> +}
> > > >>> +
> > > >>>   /**
> > > >>>    * igt_kunit:
> > > >>>    * @module_name: the name of the module
> > > >>> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> > > >>> index efb46da128d4..2f608eb69d48 100644
> > > >>> --- a/lib/igt_kmod.h
> > > >>> +++ b/lib/igt_kmod.h
> > > >>> @@ -71,7 +71,13 @@ static inline int igt_xe_driver_unload(void)
> > > >>>   int igt_amdgpu_driver_load(const char *opts);
> > > >>>   int igt_amdgpu_driver_unload(void);
> > > >>>   
> > > >>> +struct igt_kunit_names {
> > > >>> +	char *suite;
> > > >>> +	char *sub;
> > > >>> +	struct igt_list_head link;
> > > >>> +};
> > > >>>   void igt_kunit(const char *module_name, const char *name, const char *opts);
> > > >>> +void igt_kunit_get_test_names(const char *module_name, const char *opts, struct igt_list_head *names);
> > > >>>   
> > > >>>   void igt_kselftests(const char *module_name,
> > > >>>   		    const char *module_options,
> > > >>> diff --git a/tests/intel/xe_live_ktest.c b/tests/intel/xe_live_ktest.c
> > > >>> index 4376d5df7751..13265e87ba9d 100644
> > > >>> --- a/tests/intel/xe_live_ktest.c
> > > >>> +++ b/tests/intel/xe_live_ktest.c
> > > >>> @@ -9,40 +9,38 @@
> > > >>>    * Sub-category: kunit
> > > >>>    * Functionality: kunit test
> > > >>>    * Test category: functionality test
> > > >>> - *
> > > >>> - * SUBTEST: xe_bo
> > > >>> - * Description:
> > > >>> - *	Kernel dynamic selftests to check if GPU buffer objects are
> > > >>> - *	being handled properly.
> > > >>> - * Functionality: bo
> > > >>> - *
> > > >>> - * SUBTEST: xe_dma_buf
> > > >>> - * Description: Kernel dynamic selftests for dmabuf functionality.
> > > >>> - * Functionality: dmabuf test
> > > >>> - *
> > > >>> - * SUBTEST: xe_migrate
> > > >>> - * Description:
> > > >>> - *	Kernel dynamic selftests to check if page table migrations
> > > >>> - *	are working properly.
> > > >>> - * Functionality: migrate
> > > >>> - *
> > > >>> - * SUBTEST: xe_mocs
> > > >>> - * Description:
> > > >>> - *	Kernel dynamic selftests to check mocs configuration.
> > > >>> - * Functionality: mocs configuration
> > > >>>    */
> > > >>>   
> > > >> Does that mean we will not have any subtests here? The point
> > > >> into moving subtests names into documentation was to have a testlist
> > > >> generated during compilation, so it will not depend on running
> > > >> test with --list option. Adding also Katarzyna to cc.
> > > >> Cc: Katarzyna Piecielska <katarzyna.piecielska@intel.com>
> > > >>
> > > >> Regards,
> > > >> Kamil
> > > >>
> > > >>> -static const char *live_tests[] = {
> > > >>> -	"xe_bo",
> > > >>> -	"xe_dma_buf",
> > > >>> -	"xe_migrate",
> > > >>> -	"xe_mocs",
> > > >>> -};
> > > >>> -
> > > >>>   igt_main
> > > >>>   {
> > > >>> -	int i;
> > > >>> +	IGT_LIST_HEAD(names);
> > > >>> +	IGT_LIST_HEAD(done);
> > > >>> +	struct igt_kunit_names *name, *cmp;
> > > >>> +
> > > >>> +	igt_kunit_get_test_names("xe_live_test", NULL, &names);
> > > >>> +
> > > >>> +	while (!igt_list_empty(&names)) {
> > > >>> +		bool found = false;
> > > >>> +
> > > >>> +		name = igt_list_first_entry(&names, name, link);
> > > >>> +		igt_list_del(&name->link);
> > > >>> +
> > > >>> +		/*
> > > >>> +		 * Retuned list is sub-tests.
> > > >>> +		 * So, need filter out duplicated top level names.
> > > >>> +		 */
> > > >>> +		igt_list_for_each_entry(cmp, &done, link) {
> > > >>> +			if (strcmp(name->suite, cmp->suite) != 0)
> > > >>> +				continue;
> > > >>> +
> > > >>> +			found = true;
> > > >>> +		}
> > > >>> +
> > > >>> +		if (found)
> > > >>> +			continue;
> > > >>> +
> > > >>> +		igt_list_add(&name->link, &done);
> > > >>>   
> > > >>> -	for (i = 0; i < ARRAY_SIZE(live_tests); i++)
> > > >>> -		igt_kunit("xe_live_test", live_tests[i], NULL);
> > > >>> +		igt_kunit("xe_live_test", name->suite, NULL);
> > > >>> +	}
> > > >>>   }
> > > >
> > > >
> > > >
> > > 
> > > 
> > 
> > 
> > 
> > 
> > 
> 
> 
> 
> 
> 

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[John Harrison]

On 8/29/2024 02:57, Janusz Krzysztofik wrote:
> Hi John,
>
> On Wednesday, 28 August 2024 20:04:06 GMT+2 John Harrison wrote:
>> On 8/26/2024 06:07, Janusz Krzysztofik wrote:
>>> Hi John, Kamil,
>>>
>>> On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
>>>> Hi John.C.Harrison,
>>>> On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
>>>>> From: johnharr <johnharr@invalid-email.com>
>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>> Again invalid e-mail here.
>>>>
>>>>> The list of supported kunit tests is currently hard coded.
>>> That pattern originates from i915_selftest, where there are 3 stable subtests,
>>> "mock", "live" and "perf", each of them executing a (possibly long) list of
>>> dynamic sub-subtests actually provided by the module for each category.  Also,
>>> IIRC there was a separate module for each Xe test suite before, each mapped to
>>> a separate IGT test, later merged into one module with multiple suites and one
>>> test with multiple corresponding subtests.
>> Not sure if you are just explaining the history of the test or making a
>> suggestion as to how it should evolve next?
> Both, I think.  Maybe not the history, but origin of ideas standing behind the
> implementation, and how tests are expected to use it (and maybe evolve if now
> doing that in a different way).
>
>>>>> Which means
>>>>> that adding a new test to the kernel also requires patches to IGT as
>>>>> well.
>>>>>
>>>>> The list of available kunit tests is already exported by the kernel.
>>>>> So there is no need to bake a list into the IGT source code. So, add
>>>>> support for querying the test list dynamically.
>>>>>
>>>>> NB: Currently, the test list can only be queried by root but the IGT
>>>>> build system queries all tests at compile time. In theory this should
>>>>> not be a problem. However, the kunit helper code is all written to run
>>>>> inside a test and not inside the prep code, which means that when it
>>>>> fails to open the root only interfaces, it calls 'skip'. And skip is
>>>>> not allowed outside of running a test. Hence the build fails with:
>>>>>     skipping is allowed only in fixtures, subtests or igt_simple_main
>>>> Looks like we should fix it, move out skips from kunit libs.
>>> I suggest you consider a different approach: for a module, call igt_kunit()
>>> only once, with NULL suite argument.  As a result, you'll get results from one
>>> IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-subtests
>>> called "<test_suite>-<test_case>", one for each test case provided by the
>>> module.
>> I'm not following. This is what my patch does, isn't it?
> No, your patch introduces a runtime determined list of subtests -- something
> not existent in IGT.
>
> An IGT test may consist of one or more statically defined subtests with pre-
> defined names.  The term dynamic subtest is usually used in two meanings.  It
> may mean a subtest of type igt_subtest_with_dynamic, still with a pre-defined
> name, but with a runtime determined list of sub-subtests, sometimes called
> dynamic sub-subtests, but often also called just dynamic subtests.  Names of
> dynamic sub-subtests are determined at runtime.
>
> My approach tries to address your need for a maintenance-free kunit IGT test
> source file in a different way.  I'm following the IGT standard of statically
> defined list of subtests with pre-defined names: one subtest of type
> igt_subtest_with_dynamic, named after the kunit test module name which you
> have to enter into the test code anyway, and providing all test cases from
> all test suites contained in that module reported as dynamic sub-subtests
> named <test_suite>-<test_case>.
Can you please prototype how to do this?  I can read your words but I 
don't really get your meaning and I have no clue how to implement what 
you are saying.

John.

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[Janusz Krzysztofik]

Hi John,

On Saturday, 14 September 2024 02:20:32 GMT+2 John Harrison wrote:
> On 8/29/2024 02:57, Janusz Krzysztofik wrote:
> > Hi John,
> >
> > On Wednesday, 28 August 2024 20:04:06 GMT+2 John Harrison wrote:
> >> On 8/26/2024 06:07, Janusz Krzysztofik wrote:
> >>> Hi John, Kamil,
> >>>
> >>> On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
> >>>> Hi John.C.Harrison,
> >>>> On 2024-08-23 at 11:24:18 -0700, John.C.Harrison@Intel.com wrote:
> >>>>> From: johnharr <johnharr@invalid-email.com>
> >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>>> Again invalid e-mail here.
> >>>>
> >>>>> The list of supported kunit tests is currently hard coded.
> >>> That pattern originates from i915_selftest, where there are 3 stable 
subtests,
> >>> "mock", "live" and "perf", each of them executing a (possibly long) list 
of
> >>> dynamic sub-subtests actually provided by the module for each category.  
Also,
> >>> IIRC there was a separate module for each Xe test suite before, each 
mapped to
> >>> a separate IGT test, later merged into one module with multiple suites 
and one
> >>> test with multiple corresponding subtests.
> >> Not sure if you are just explaining the history of the test or making a
> >> suggestion as to how it should evolve next?
> > Both, I think.  Maybe not the history, but origin of ideas standing behind 
the
> > implementation, and how tests are expected to use it (and maybe evolve if 
now
> > doing that in a different way).
> >
> >>>>> Which means
> >>>>> that adding a new test to the kernel also requires patches to IGT as
> >>>>> well.
> >>>>>
> >>>>> The list of available kunit tests is already exported by the kernel.
> >>>>> So there is no need to bake a list into the IGT source code. So, add
> >>>>> support for querying the test list dynamically.
> >>>>>
> >>>>> NB: Currently, the test list can only be queried by root but the IGT
> >>>>> build system queries all tests at compile time. In theory this should
> >>>>> not be a problem. However, the kunit helper code is all written to run
> >>>>> inside a test and not inside the prep code, which means that when it
> >>>>> fails to open the root only interfaces, it calls 'skip'. And skip is
> >>>>> not allowed outside of running a test. Hence the build fails with:
> >>>>>     skipping is allowed only in fixtures, subtests or igt_simple_main
> >>>> Looks like we should fix it, move out skips from kunit libs.
> >>> I suggest you consider a different approach: for a module, call 
igt_kunit()
> >>> only once, with NULL suite argument.  As a result, you'll get results 
from one
> >>> IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-
subtests
> >>> called "<test_suite>-<test_case>", one for each test case provided by 
the
> >>> module.
> >> I'm not following. This is what my patch does, isn't it?
> > No, your patch introduces a runtime determined list of subtests -- 
something
> > not existent in IGT.
> >
> > An IGT test may consist of one or more statically defined subtests with 
pre-
> > defined names.  The term dynamic subtest is usually used in two meanings.  
It
> > may mean a subtest of type igt_subtest_with_dynamic, still with a pre-
defined
> > name, but with a runtime determined list of sub-subtests, sometimes called
> > dynamic sub-subtests, but often also called just dynamic subtests.  Names 
of
> > dynamic sub-subtests are determined at runtime.
> >
> > My approach tries to address your need for a maintenance-free kunit IGT 
test
> > source file in a different way.  I'm following the IGT standard of 
statically
> > defined list of subtests with pre-defined names: one subtest of type
> > igt_subtest_with_dynamic, named after the kunit test module name which you
> > have to enter into the test code anyway, and providing all test cases from
> > all test suites contained in that module reported as dynamic sub-subtests
> > named <test_suite>-<test_case>.
> Can you please prototype how to do this?  I can read your words but I 
> don't really get your meaning and I have no clue how to implement what 
> you are saying.

The test code may look as simple as:

igt_main
{
        igt_kunit("xe_live_test", NULL, NULL);
}

Then, igt_kunit() will use xe_live_test module and should report results from 
all its KUnit test cases as results from a set of IGT dynamic sub-subtests 
"<test_suite>-<test_case>" under a single IGT subtest "xe_live".

Janusz

> 
> John.
> 
> 

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[John Harrison]

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

On 9/16/2024 01:17, Janusz Krzysztofik wrote:
> Hi John,
>
> On Saturday, 14 September 2024 02:20:32 GMT+2 John Harrison wrote:
>> On 8/29/2024 02:57, Janusz Krzysztofik wrote:
>>> Hi John,
>>>
>>> On Wednesday, 28 August 2024 20:04:06 GMT+2 John Harrison wrote:
>>>> On 8/26/2024 06:07, Janusz Krzysztofik wrote:
>>>>> Hi John, Kamil,
>>>>>
>>>>> On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
>>>>>> Hi John.C.Harrison,
>>>>>> On 2024-08-23 at 11:24:18 -0700,John.C.Harrison@Intel.com  wrote:
>>>>>>> From: johnharr<johnharr@invalid-email.com>
>>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>>> Again invalid e-mail here.
>>>>>>
>>>>>>> The list of supported kunit tests is currently hard coded.
>>>>> That pattern originates from i915_selftest, where there are 3 stable
> subtests,
>>>>> "mock", "live" and "perf", each of them executing a (possibly long) list
> of
>>>>> dynamic sub-subtests actually provided by the module for each category.
> Also,
>>>>> IIRC there was a separate module for each Xe test suite before, each
> mapped to
>>>>> a separate IGT test, later merged into one module with multiple suites
> and one
>>>>> test with multiple corresponding subtests.
>>>> Not sure if you are just explaining the history of the test or making a
>>>> suggestion as to how it should evolve next?
>>> Both, I think.  Maybe not the history, but origin of ideas standing behind
> the
>>> implementation, and how tests are expected to use it (and maybe evolve if
> now
>>> doing that in a different way).
>>>
>>>>>>> Which means
>>>>>>> that adding a new test to the kernel also requires patches to IGT as
>>>>>>> well.
>>>>>>>
>>>>>>> The list of available kunit tests is already exported by the kernel.
>>>>>>> So there is no need to bake a list into the IGT source code. So, add
>>>>>>> support for querying the test list dynamically.
>>>>>>>
>>>>>>> NB: Currently, the test list can only be queried by root but the IGT
>>>>>>> build system queries all tests at compile time. In theory this should
>>>>>>> not be a problem. However, the kunit helper code is all written to run
>>>>>>> inside a test and not inside the prep code, which means that when it
>>>>>>> fails to open the root only interfaces, it calls 'skip'. And skip is
>>>>>>> not allowed outside of running a test. Hence the build fails with:
>>>>>>>      skipping is allowed only in fixtures, subtests or igt_simple_main
>>>>>> Looks like we should fix it, move out skips from kunit libs.
>>>>> I suggest you consider a different approach: for a module, call
> igt_kunit()
>>>>> only once, with NULL suite argument.  As a result, you'll get results
> from one
>>>>> IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-
> subtests
>>>>> called "<test_suite>-<test_case>", one for each test case provided by
> the
>>>>> module.
>>>> I'm not following. This is what my patch does, isn't it?
>>> No, your patch introduces a runtime determined list of subtests --
> something
>>> not existent in IGT.
>>>
>>> An IGT test may consist of one or more statically defined subtests with
> pre-
>>> defined names.  The term dynamic subtest is usually used in two meanings.
> It
>>> may mean a subtest of type igt_subtest_with_dynamic, still with a pre-
> defined
>>> name, but with a runtime determined list of sub-subtests, sometimes called
>>> dynamic sub-subtests, but often also called just dynamic subtests.  Names
> of
>>> dynamic sub-subtests are determined at runtime.
>>>
>>> My approach tries to address your need for a maintenance-free kunit IGT
> test
>>> source file in a different way.  I'm following the IGT standard of
> statically
>>> defined list of subtests with pre-defined names: one subtest of type
>>> igt_subtest_with_dynamic, named after the kunit test module name which you
>>> have to enter into the test code anyway, and providing all test cases from
>>> all test suites contained in that module reported as dynamic sub-subtests
>>> named <test_suite>-<test_case>.
>> Can you please prototype how to do this?  I can read your words but I
>> don't really get your meaning and I have no clue how to implement what
>> you are saying.
> The test code may look as simple as:
>
> igt_main
> {
>          igt_kunit("xe_live_test", NULL, NULL);
> }
>
> Then, igt_kunit() will use xe_live_test module and should report results from
> all its KUnit test cases as results from a set of IGT dynamic sub-subtests
> "<test_suite>-<test_case>" under a single IGT subtest "xe_live".
>
> Janusz
>
Doing that solves the problem of not generating a test list at compile 
time. But it creates the problem of not being able to generate a test 
list at all:

    0:28> sudo ./build/tests/xe_live_ktest --show-testlist
    igt@xe_live_ktest@xe_live
    0:29> sudo ./build/tests/xe_live_ktest --list-subtests
    xe_live
    0:30> sudo ./build/tests/xe_live_ktest --describe
    SUB xe_live ../lib/igt_kmod.c:1475:
       NO DOCUMENTATION!


And that last is clearly wrong because if I don't have a comment 
description of xe_live then it gives a build time error.

It is possible to run a specific sub-test, but you have to know the name 
already. E.g.:

    0:25> sudo ./build/tests/xe_live_ktest --dynamic-subtest xe_bo-xe_bo_evict_kunit
    IGT-Version: 1.28-g8df80ef3edc8 (x86_64) (Linux: 6.11.0-rc5-g2g x86_64)
    Using IGT_SRANDOM=1726523447 for randomisation
    Starting subtest: xe_live
    Starting dynamic subtest: xe_bo-xe_bo_evict_kunit
    ...


That seems less than ideal.

John.

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

Re: [RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest

[Janusz Krzysztofik]

On Tuesday, 17 September 2024 00:10:42 GMT+2 John Harrison wrote:
> On 9/16/2024 01:17, Janusz Krzysztofik wrote:
> > Hi John,
> >
> > On Saturday, 14 September 2024 02:20:32 GMT+2 John Harrison wrote:
> >> On 8/29/2024 02:57, Janusz Krzysztofik wrote:
> >>> Hi John,
> >>>
> >>> On Wednesday, 28 August 2024 20:04:06 GMT+2 John Harrison wrote:
> >>>> On 8/26/2024 06:07, Janusz Krzysztofik wrote:
> >>>>> Hi John, Kamil,
> >>>>>
> >>>>> On Monday, 26 August 2024 13:46:45 GMT+2 Kamil Konieczny wrote:
> >>>>>> Hi John.C.Harrison,
> >>>>>> On 2024-08-23 at 11:24:18 -0700,John.C.Harrison@Intel.com  wrote:
> >>>>>>> From: johnharr<johnharr@invalid-email.com>
> >>>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >>>>>> Again invalid e-mail here.
> >>>>>>
> >>>>>>> The list of supported kunit tests is currently hard coded.
> >>>>> That pattern originates from i915_selftest, where there are 3 stable
> > subtests,
> >>>>> "mock", "live" and "perf", each of them executing a (possibly long) list
> > of
> >>>>> dynamic sub-subtests actually provided by the module for each category.
> > Also,
> >>>>> IIRC there was a separate module for each Xe test suite before, each
> > mapped to
> >>>>> a separate IGT test, later merged into one module with multiple suites
> > and one
> >>>>> test with multiple corresponding subtests.
> >>>> Not sure if you are just explaining the history of the test or making a
> >>>> suggestion as to how it should evolve next?
> >>> Both, I think.  Maybe not the history, but origin of ideas standing behind
> > the
> >>> implementation, and how tests are expected to use it (and maybe evolve if
> > now
> >>> doing that in a different way).
> >>>
> >>>>>>> Which means
> >>>>>>> that adding a new test to the kernel also requires patches to IGT as
> >>>>>>> well.
> >>>>>>>
> >>>>>>> The list of available kunit tests is already exported by the kernel.
> >>>>>>> So there is no need to bake a list into the IGT source code. So, add
> >>>>>>> support for querying the test list dynamically.
> >>>>>>>
> >>>>>>> NB: Currently, the test list can only be queried by root but the IGT
> >>>>>>> build system queries all tests at compile time. In theory this should
> >>>>>>> not be a problem. However, the kunit helper code is all written to run
> >>>>>>> inside a test and not inside the prep code, which means that when it
> >>>>>>> fails to open the root only interfaces, it calls 'skip'. And skip is
> >>>>>>> not allowed outside of running a test. Hence the build fails with:
> >>>>>>>      skipping is allowed only in fixtures, subtests or igt_simple_main
> >>>>>> Looks like we should fix it, move out skips from kunit libs.
> >>>>> I suggest you consider a different approach: for a module, call
> > igt_kunit()
> >>>>> only once, with NULL suite argument.  As a result, you'll get results
> > from one
> >>>>> IGT subtest called "<module_name>" with a bunch of IGT dynamic sub-
> > subtests
> >>>>> called "<test_suite>-<test_case>", one for each test case provided by
> > the
> >>>>> module.
> >>>> I'm not following. This is what my patch does, isn't it?
> >>> No, your patch introduces a runtime determined list of subtests --
> > something
> >>> not existent in IGT.
> >>>
> >>> An IGT test may consist of one or more statically defined subtests with
> > pre-
> >>> defined names.  The term dynamic subtest is usually used in two meanings.
> > It
> >>> may mean a subtest of type igt_subtest_with_dynamic, still with a pre-
> > defined
> >>> name, but with a runtime determined list of sub-subtests, sometimes called
> >>> dynamic sub-subtests, but often also called just dynamic subtests.  Names
> > of
> >>> dynamic sub-subtests are determined at runtime.
> >>>
> >>> My approach tries to address your need for a maintenance-free kunit IGT
> > test
> >>> source file in a different way.  I'm following the IGT standard of
> > statically
> >>> defined list of subtests with pre-defined names: one subtest of type
> >>> igt_subtest_with_dynamic, named after the kunit test module name which you
> >>> have to enter into the test code anyway, and providing all test cases from
> >>> all test suites contained in that module reported as dynamic sub-subtests
> >>> named <test_suite>-<test_case>.
> >> Can you please prototype how to do this?  I can read your words but I
> >> don't really get your meaning and I have no clue how to implement what
> >> you are saying.
> > The test code may look as simple as:
> >
> > igt_main
> > {
> >          igt_kunit("xe_live_test", NULL, NULL);
> > }
> >
> > Then, igt_kunit() will use xe_live_test module and should report results from
> > all its KUnit test cases as results from a set of IGT dynamic sub-subtests
> > "<test_suite>-<test_case>" under a single IGT subtest "xe_live".
> >
> > Janusz
> >
> Doing that solves the problem of not generating a test list at compile 
> time. But it creates the problem of not being able to generate a test 
> list at all:
> 
>     0:28> sudo ./build/tests/xe_live_ktest --show-testlist
>     igt@xe_live_ktest@xe_live
>     0:29> sudo ./build/tests/xe_live_ktest --list-subtests
>     xe_live
>     0:30> sudo ./build/tests/xe_live_ktest --describe
>     SUB xe_live ../lib/igt_kmod.c:1475:
>        NO DOCUMENTATION!
> 
> 
> And that last is clearly wrong because if I don't have a comment 
> description of xe_live then it gives a build time error.

I suggested only how the code could look like, leaving necessary documentation 
changes to an implementer.  Please have a look at tests/intel/i915_selftest.c 
to see how subtest documentation is managed there.  There are 3 subtests: 
mock, live and perf, each of them having a bunch of dynamic sub-subtests 
also documented.

> 
> It is possible to run a specific sub-test, but you have to know the name 
> already. E.g.:
> 
>     0:25> sudo ./build/tests/xe_live_ktest --dynamic-subtest xe_bo-xe_bo_evict_kunit
>     IGT-Version: 1.28-g8df80ef3edc8 (x86_64) (Linux: 6.11.0-rc5-g2g x86_64)
>     Using IGT_SRANDOM=1726523447 for randomisation
>     Starting subtest: xe_live
>     Starting dynamic subtest: xe_bo-xe_bo_evict_kunit
>     ...
> 
> 
> That seems less than ideal.

AFAICU, CI now depends on availability of subtest documentation in IGT source 
files, from where a list of IGT subtests (not including dynamic sub-subtests, 
I believe) is derived at compile time.  Then, some of your options are:

1. Keep adding new subtests with documentation to xe_live_ktest IGT test when 
   new test suites are added to xe_live_test module, otherwise new test cases 
   added to the module won't be executed.
2. Use one subtest, as I suggested, correctly documented, and optionally 
   update the xe_live_ktest IGT test documentation with description of new 
   dynamic sub-subtests corresponding to new test cases added to the 
   xe_live_test module,
3. Develop and implement a new (hardware independent) way of providing CI with 
   a complete list of IGT subtests, including those corresponding to all KUnit 
   test suites provided by KUnit modules used in IGT tests, at compile time.

I'm not against the third option, I only tried to show you a ready-to-use even 
if not ideal option 2.  With that approach, if you add a new test suite to the 
xe_live_test module without touching the xe_live_ktest IGT test, those new 
test cases, unlike now, will be automatically executed by in CI shards runs 
and their results reported, only their (optional) documentation will be 
missing if you don't add it to the xe_live_ktest IGT test source file.

Thanks,
Janusz

> 
> John.
>