[igt-dev] [PATCH i-g-t 4/8] tests: DRM selftests: switch to KUnit
Mauro Carvalho Chehab
mauro.chehab at linux.intel.com
Thu Jun 8 07:56:34 UTC 2023
On Wed, 07 Jun 2023 19:40:39 +0200
Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com> wrote:
> On Wednesday, 7 June 2023 17:59:20 CEST Mauro Carvalho Chehab wrote:
> > On Wed, 07 Jun 2023 16:39:23 +0200
> > Janusz Krzysztofik <janusz.krzysztofik at 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 at linux.intel.com> wrote:
> > > > >
> > > > > > On Monday, 5 June 2023 12:47:12 CEST Dominik Karol Piatkowski wrote:
> > > > > > > From: Isabella Basso <isabbasso at 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 at 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 at intel.com>
> > > > > > > Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
> > > > > > > Cc: Mauro Carvalho Chehab <mauro.chehab at 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.
>
> Each instance of igt_subtest_dynamic()? If there are two, one for kunit, one
> for i915-like sleftest, and the second one is called conditionally, only if
> the first one fails, then what?
> The first one never fails with --list-subtests option provided.
I don't see any reason to have a fallback. For DRM tests, we should
simply remove selftests, as, afaikt, upstream doesn't support it
anymore. The same applies for Xe and KMS, as we won't have support
for selftests at the Xe driver.
Selftests should only be kept for i915 selftests, which this series
don't touch.
So, what I meant to say is that KUnit tests - after their conversion
from selftest - should use igt_subtest_dynamic(). This is needed in order
for IGT to list KUnit tests as dynamic subtests when using --list parameter.
>
> >
> > > >
> > > > >
> > > > > 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?
>
> I'm not sure what you mean. Would you like me to reply with a patch that
> fixes the issues I've pointed out?
Yes, a patch on the top of this series, doing the changes you think
are still needed to get this series merged.
> Or would you like me to respond to another
> patch of this series that's more related to my points than this one?
>
> If the former then I don't have a solution ready out of top of my head since
> I'm still not sure how we want to have coexistence of backword compatible
> subtests with new kunit ones designed.
>
> Thanks,
> Janusz
>
>
> >
> > Regards,
> > Mauro
> >
> >
> >
> >
>
>
>
>
More information about the igt-dev
mailing list