[RFC i-g-t] kunit: Remove hard-coded test list from xe_live_ktest
John Harrison
john.c.harrison at intel.com
Wed Aug 28 18:03:15 UTC 2024
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 at Intel.com wrote:
>> From: johnharr <johnharr at 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 at 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 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>
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
>>
More information about the igt-dev
mailing list