[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 at Intel.com wrote:
>>> From: johnharr <johnharr at 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 at 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 at 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 at 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);
>>> +	}
>>>   }
>
>
>