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

[Mauro Carvalho Chehab <mauro.chehab@linux.intel.com>]

On Wed, 07 Jun 2023 16:39:23 +0200
Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:

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

It always display the name that igt_subtest_dynamic() uses. Such
behavior should be preserved after adding support for KUnit.





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

Yes, I missed the point. Could you please reply with a patch showing
what kind of changes are you talking about?

Regards,
Mauro