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

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

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

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

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

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

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

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

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

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

Thanks,
Janusz

> 
> Regards,
> Mauro
>