Date:   Thu, 16 Jun 2022 19:41:05 -0300
From:   Maíra Canal <maira.canal@....br>
To:     David Gow <davidgow@...gle.com>
Cc:     Harry Wentland <harry.wentland@....com>,
        Leo Li <sunpeng.li@....com>,
        Rodrigo Siqueira <Rodrigo.Siqueira@....com>,
        Alex Deucher <alexander.deucher@....com>,
        Christian König <christian.koenig@....com>,
        Dmytro Laktyushkin <Dmytro.Laktyushkin@....com>,
        Jun Lei <jun.lei@....com>,
        Nicholas Choi <Nicholas.Choi@....com>,
        Harrison Chiu <harrison.chiu@....com>,
        Mark Yacoub <markyacoub@...omium.org>,
        Sean Paul <seanpaul@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>,
        Javier Martinez Canillas <javierm@...hat.com>,
        Isabella Basso <isabbasso@...eup.net>, magalilemes00@...il.com,
        tales.aparecida@...il.com, mwen@...lia.com, andrealmeid@...eup.net,
        Daniel Latypov <dlatypov@...gle.com>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KUnit Development <kunit-dev@...glegroups.com>
Subject: Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode
 Library

Hi David,

Thank you for your feedback!

On 6/16/22 11:39, David Gow wrote:
> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@....br> wrote:

>>
>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>> be able to compile the tests as modules and, therefore, won't be able to use
>> IGT to run the tests. This problem with kunit_test_suites() was already
>> discussed in the KUnit mailing list, as can be seen in [7].
> 
> I'm not sure I fully understand why these tests need to be part of the
> amdgpu module, though admittedly I've not played with IGT much. Would
> it be possible to compile these tests as separate modules, which could
> depend on amdgpu (or maybe include the DML stuff directly), and
> therefore not have this conflict? I definitely was able to get these
> tests working under kunit_tool (albeit as built-ins) by using
> kunit_test_suites(). If each suite were built as a separate module (or
> indeed, even if all the tests were in one module, with one list of
> suites), then it should be possible to avoid the init_module()
> conflict. That'd also make it possible to run these tests without
> actually needing the driver to initialise, which seems like it might
> require actual hardware(?)

Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().

At this point, we thought about a couple of options to resolve this problem:
- Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
- Take the Thunderbolt path and add the tests to the driver stack.

We end up taking the Thunderbolt path as it would be more maintainable.

Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.

If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.

> 
> There are two other reasons the 'thunderbolt'-style technique is one
> we want to avoid:
> 1. It makes it much more difficult to run tests using kunit_tool and
> KUnit-based CI tools: these tests would not run automatically, and if
> they were built-in as-is, they'd need to be
> 2. We're planning to improve module support to replace the
> init_module()-based implementation of kunit_test_suites() with one
> which won't have these conflicts, so the need for this should be
> short-lived.
> 
> If you're curious, an early version of the improved module support can
> be found here, though it's out-of-date enough it won't apply or work
> as-is:
> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
> 
> Now, that's unlikely to be ready very soon, but I'd be hesitant to
> implement too extensive a system for avoiding kunit_test_suites()
> given at some point it should work and we'll need to migrate back to
> it.

We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.

Could you explain more about what is missing to make this improved module support come upstream?

> 
> At the very least, having the dependency on KUNIT=m is a very bad
> idea: it should be possible to have tests built as modules, even if
> KUnit itself isn't, and ideally (even if this sort-of implementation
> is required), it _should_ be possible to have these tests be built-in
> if all their dependencies (KUnit, amdgpu) are, which would make it
> possible to run the tests without a userland.
> 

Thank you for the suggestion! We will change the KUNIT dependency.

- Maíra Canal

Powered by blists - more mailing lists