[PATCH v5 9/9] drm: selftest: convert drm_mm selftest to KUnit

[Michał Winiarski]

On Sun, Aug 21, 2022 at 07:22:30PM -0300, Isabella Basso wrote:
> Hi Michał,
> 
> While I totally understand your point, we have talked about this in our GSoC
> meetings with mentors, and have found a few reasons as to why a KUnit runner
> integrated to IGT might be really useful. 
> 
> > Am 22/07/2022 um 1:25 PM schrieb Michał Winiarski <michal at hardline.pl>:
> > 
> > On Fri, Jul 22, 2022 at 08:04:51AM -0300, Maíra Canal wrote:
> >> On 7/22/22 07:35, Matthew Auld wrote:
> >>> On Fri, 8 Jul 2022 at 21:32, Maíra Canal <maira.canal at usp.br> wrote:
> >>>> 
> >>>> From: Arthur Grillo <arthur.grillo at usp.br>
> >>>> 
> >>>> Considering the current adoption of the KUnit framework, convert the
> >>>> DRM mm selftest to the KUnit API.
> >>> 
> >>> Is there a plan to convert the corresponding selftest IGT that was
> >>> responsible for running this (also drm_buddy) to somehow work with
> >>> kunit? Previously these IGTs were always triggered as part of
> >>> intel-gfx CI, but it looks like they are no longer run[1].
> >>> 
> >>> [1] https://gitlab.freedesktop.org/drm/intel/-/issues/6433
> >> 
> >> Hi Matthew,
> >> 
> >> Isabella sent a while ago a patch to IGT adding KUnit compatibility to
> >> IGT [1], but there wasn't any feedback on the patch. I believe that soon
> >> she will resend the series in order to make all KUnit DRM tests run on IGT.
> >> 
> >> Any feedback on the patch is welcomed so that we can fix this issue as
> >> soon as possible.
> >> 
> >> [1] https://patchwork.freedesktop.org/patch/489985/
> >> 
> >> Best Regards,
> >> - Maíra Canal
> > 
> > Hi.
> > 
> > Instead of going back to using IGT for *unit* tests, it would be a better idea
> > to adjust the CI to just run the tests once at "build" time (just like e.g.
> > checkpatch).
> 
> First, I’d like to point out that there would be some inherent overhead in
> doing so, which might actually not be worth it, as KUnit tool would need to
> compile HEAD in the UML arch, then we’d have to re-compile everything to a real
> machine’s architecture, like x86_64 (in the least), having in mind still that
> arch-dependent issues would not show up when we run tests in UML, so there’s
> still a downside to it even if it’s quick enough.
> 
> Even if we don’t run them as UML and instead use a VM, there’s a VM being run
> just for a couple of tests, which might be slower than adding a step to a
> dedicated machine that’s (probably) already available, plus the setup and
> hardware needed to run a VM inside of a CI runner are overheads in themselves,
> needing dedicated, modern machines.

No - we don't need a dedicated machine for running kunit - the machine that we
just used to compile the code is perfectly fine.
Builders used in CI systems usually have beefy server-grade CPUs - pretty good
candidates for running unit tests (even with virtualization overhead).
Plus - if the unit tests fail, we can consider skipping the deployment and
not run any regular tests (just like the case where build has failed).
Meanwhile, one of the "dedicated machines" (ones that are used to run the tests)
can actually be a low-power device (think tablet). And if the test ends up
crashing the kernel, it needs to be rebooted. VMs are much easier to work with,
especially with kunit.py abstracting away all of the qemu interactions.

> 
> > We would then stop executing the same test multiple times on different machines
> > (note that both DRM selftests and i915 "mock" selftests are pure unit tests - in
> > other words, they don't need the hardware to be present), which would save some
> > (small) amount of machine-time that can be utilized to do something that
> > actually needs the hardware.
> 
> I totally agree with your solution in regards to arch-independent tests, though.

There are no arch-specific kunit tests in DRM-core. There shouldn't be any
arch-specific code in DRM-core. Same thing for drivers (at least for the purpose
of COMPILE_TEST and by extension, running kunit).
All of DRM kunit tests should pass on all architectures supported by kunit.

> 
> > Plus there's no need to maintain the kunit-runner in IGT.
> > Note - we're currently going to lose "DMESG-WARN" detection if we go this route,
> > but this is something that can be improved on the kunit-side.
> > 
> > -Michał
> 
> There’s also a point to be made on maintaining such a runner if we think about
> companies like AMD, as they rely heavily on IGT, so they have lots of tests
> written in there, and it'd be difficult for them to accommodate one more
> non-trivial thing to their CI. Plus I think this might be a good starting point
> for them to transition their CI to a KUnit-centered approach without stressing
> engineers unnecessarily.

I agree with the IGT-compatibility angle, however, that would only apply to test
content that gets converted from selftests to kunit (just like DRM selftests),
not newly introduced test content (as is the case with amdgpu).
I also wouldn't call interpreting exit code of "kunit.py run (...)" something
that's difficult to be added to various CI pipelines.
Also - do we really want to transition to KUnit-centered approach?
Regular IGTs are actually about exercising the HW through driver uAPI from
userspace, not about isolated unit testing (which is what KUnit is about).
Then we have selftests, which are implemented on the kernel side, and are about
internal implementation. Selftests may or may not require HW to operate (if HW
is needed, we're usually doing more of a functional/integration testing, if not
- it's most likely going to be a pure unit test).
I view regular IGTs and KUnit (and kselftests that are not isolated, and need
the HW to be present) as complementary mechanisms, not something to be replaced
(in other words - we only want to transition unit tests to KUnit).

When it comes to transition, I'm just worried that once the IGT KTAP parser is
adopted, the transition to kunit.py @ build time will never happen, and we'll
end up maintaining custom DRM-specific solution instead of participating in
wider kernel community.

-Michał

> 
> Cheers,
> —
> Isabella
>