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

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

Auto-correction, sorry.

On Wednesday, 7 June 2023 16:35:32 CEST Janusz Krzysztofik wrote:
> 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.  

Should read: option 3

> 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.  
 
Should read: option 1

> 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
> > 
> 
>