Re: [igt-dev] [PATCH i-g-t 10/10] KUnit: replace abort with graceful skip

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

On Wednesday, 14 June 2023 09:58:34 CEST Mauro Carvalho Chehab wrote:
> On Tue, 13 Jun 2023 16:25:01 +0200
> Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com> wrote:
> 
> > > No. Since 2022 - specifically, since those commits:
> > > 
> > > 	- 932da861956a ("drm: selftest: convert drm_buddy selftest to KUnit")
> > > 	- fc8d29e298cf ("drm: selftest: convert drm_mm selftest to KUnit")
> > > 
> > > those tests don't work anymore, as upstream removed support for selftest,
> > > in favor of KUnit. So, those tests currently do nothing.  
> > 
> > In my opinion -- no, those tests currently skip, which is different from doing 
> > nothing.
> > 
> > > 
> > > We are changing because those tests are currently broken on IGT.   
> > 
> > In my opinion -- no, they are not broken, they are outdated.
> 
> IGT tests are broken, as IGT is not able anymore to execute drm_buddy and
> drm_mm unit tests. This series fix it.

Semantics.

> 
> > 
> > > That
> > > is the reason why we need KUnit support for such tests.
> > >   
> > > > CI expects that behavior, users 
> > > > are used to it.  We shouldn't change that only because we switch from an old 
> > > > underlying kernel selftest format to a new shiny one, I believe, unless we are 
> > > > able to prove that there was something wrong with that former approach, or we 
> > > > can explain why it no longer fits into the kunit variant.  
> > > 
> > > Yes, there's something wrong with the former approach:   
> > 
> > Are you saying that error handling in igt_ksleftest() is broken?  In my 
> > opinion, i915 selftests successfully executing on CI now and again, as well as 
> > the outdated DRM selftests successfully skipping, prove something different.
> 
> No, I'm saying that drm core tests are broken on IGT, because drm core doesn't
> support the legacy selftest infrastructure since 2022, and IGT is currently
> missing kUnit support.
> 
> On other words, IGT won't run such tests *even* if the Kernel is build with
> support for DRM core tests.
> 
> > I believe that igt_kunit() that follows as closely as possible the pattern of 
> > how igt_kselftest() is handling unmet conditions and errors, with full respect 
> > to real differences between kunit and i915-like selftests, would be the best 
> > solution.
> 
> I'm not saying anthing different, but KUnit and selftest executions are
> different:
> 
> - with selftest, the module loads asynchronously. After the module is
>   loaded with success, the probe() method will be called and the tests
>   will run.
> 
>   It means that it is possible to first load the driver and then run
>   igt dynamic subtest logic, parsing the results from dmesg.

Please have a look at drivers/gpu/drm/i915/i915_module.c, see how 
i915_mock_selftests() is called, then reevaluate your opinion on kunit doing 
that in a different way.

Thanks,
Janusz


> 
> - with KUnit, the unit tests will run synchronously at modprobe time.
>   So, when modprobe() finishes execution, all KUnit tests were already
>   executed - and if the Kernel crashes on a KUnit test - results will be
>   lost.
>   
>   It means that all steps needed to execute KUnit should be handled
>   before modprobing, including starting a thread to capture dmesg
>   results during modprobe time. So, igt dynamic subtests should be created 
>   before calling modprobe() function.
> 
> due to such difference, igt_subtest_with_dynamic() should be called
> before modprobe(), and, if something goes wrong on that time - for instance
> if the module doesn't exist and returns EENOENT - IGT needs to use
> igt_skip() or igt_fail().
> 
> Regards,
> Mauro
>