dri-devel.lists.freedesktop.org archive mirror
 help / color / mirror / Atom feed
From: David Gow <davidgow@google.com>
To: "Maíra Canal" <maira.canal@usp.br>
Cc: "Harrison Chiu" <harrison.chiu@amd.com>,
	"Daniel Latypov" <dlatypov@google.com>,
	"Brendan Higgins" <brendanhiggins@google.com>,
	"Isabella Basso" <isabbasso@riseup.net>,
	andrealmeid@riseup.net, "Jun Lei" <jun.lei@amd.com>,
	"Rodrigo Siqueira" <Rodrigo.Siqueira@amd.com>,
	"Javier Martinez Canillas" <javierm@redhat.com>,
	amd-gfx@lists.freedesktop.org, "Leo Li" <sunpeng.li@amd.com>,
	mwen@igalia.com, "Sean Paul" <seanpaul@chromium.org>,
	"KUnit Development" <kunit-dev@googlegroups.com>,
	"Mark Yacoub" <markyacoub@chromium.org>,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>,
	"Dmytro Laktyushkin" <Dmytro.Laktyushkin@amd.com>,
	"Nicholas Choi" <Nicholas.Choi@amd.com>,
	"Alex Deucher" <alexander.deucher@amd.com>,
	"Christian König" <christian.koenig@amd.com>
Subject: Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
Date: Sat, 18 Jun 2022 17:08:49 +0800	[thread overview]
Message-ID: <CABVgOSk8grWzD2AR3KLOK_CioDu=vy_gOgpofTvp+8PvZSaoRg@mail.gmail.com> (raw)
In-Reply-To: <44745ed7-18ad-ad7c-fef5-2f0f71d289d1@usp.br>

[-- Attachment #1: Type: text/plain, Size: 6962 bytes --]

On Sat, Jun 18, 2022 at 4:24 AM Maíra Canal <maira.canal@usp.br> wrote:
> On 6/17/22 04:55, David Gow wrote:
> > On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
> >>
> >> 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@usp.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.
> >
> > As you point out, there are really two separate problems with
> > splitting the tests out totally:
> > - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
> > everywhere.
> > - It's impossible to have multiple init_module() "calls" in the same module.
> >
> > The first of these is, I think, the harder to solve generally. (There
> > are some ways to mitigate the namespace pollution part of it by either
> > hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
> > similar, or by using symbol namespaces:
> > https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
> > -- or both -- but they don't solve the issue entirely.)
> >
> > That being said, it's as much a matter of taste as anything, so if
> > keeping things in the amdgpu module works well, don't let me stop you.
> > Either way should work, and have their own advantages and
> > disadvantages.
> >
> > The latter is just a quirk of the current KUnit implementation of
> > kunit_test_suites(). This multiple-definition issue will go away in
> > the not-too-distant future.
> >
> > So my suggestion here would be to make sure any changes you make to
> > work around the issue with multiple init_module definitions are easy
> > to remove. I suspect you could probably significantly simplify the
> > whole dml_test.{c,h} bit to just directly export the kunit_suites and
> > maybe throw them all in one array to pass to
> > __kunit_test_suites_init(). Then, when the improved modules work
> > lands, they could be deleted entirely and replaced with one or more
> > calls to kunit_test_suite().
> >
> >>>
> >>> 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?
> >>
> >
> > Mostly just time and some other priorities. We've taken another look
> > at it over the last couple of days, and will try to accelerate getting
> > it in within the next couple of kernel releases. (Hopefully sooner
> > rather than later.)
> Is there anything we can do to make this move faster? As it is our great
> interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
> can start work on this feature. We don´t have much knowledge of the
> inner workings of KUnit, but if you point out a path, we can try to work
> on this task.
> Maybe, could we work in the same way as Jeremy?

Daniel and I have quickly tidied up and finished the various
in-progress bits of this and sent it out here:

You should be able to apply that series and then just use
kunit_test_suites(), which will no-longer conflict with module_init

The most useful thing you could do is to test and/or review it --
there's almost certainly something I'll have missed.

-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

  reply	other threads:[~2022-06-18  9:09 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-08  1:07 [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library Maíra Canal
2022-06-08  1:07 ` [RFC 1/3] drm/amd/display: Introduce KUnit to DML Maíra Canal
2022-06-08  2:36   ` Daniel Latypov
2022-06-15 15:05     ` Maíra Canal
2022-06-08  1:07 ` [RFC 2/3] drm/amd/display: Move bw_fixed macros to header file Maíra Canal
2022-06-08  1:07 ` [RFC 3/3] drm/amd/display: Introduce KUnit tests to the bw_fixed library Maíra Canal
2022-06-16 14:39 ` [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library David Gow
2022-06-16 22:41   ` Maíra Canal
2022-06-17  7:55     ` David Gow
2022-06-17 20:24       ` Maíra Canal
2022-06-18  9:08         ` David Gow [this message]
2022-06-22 22:55           ` Rodrigo Siqueira Jordao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABVgOSk8grWzD2AR3KLOK_CioDu=vy_gOgpofTvp+8PvZSaoRg@mail.gmail.com' \
    --to=davidgow@google.com \
    --cc=Dmytro.Laktyushkin@amd.com \
    --cc=Nicholas.Choi@amd.com \
    --cc=Rodrigo.Siqueira@amd.com \
    --cc=alexander.deucher@amd.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=andrealmeid@riseup.net \
    --cc=brendanhiggins@google.com \
    --cc=christian.koenig@amd.com \
    --cc=dlatypov@google.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=harrison.chiu@amd.com \
    --cc=isabbasso@riseup.net \
    --cc=javierm@redhat.com \
    --cc=jun.lei@amd.com \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=magalilemes00@gmail.com \
    --cc=maira.canal@usp.br \
    --cc=markyacoub@chromium.org \
    --cc=mwen@igalia.com \
    --cc=seanpaul@chromium.org \
    --cc=sunpeng.li@amd.com \
    --cc=tales.aparecida@gmail.com \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).