Re: [PATCH 00/24] drm: Introduce Kunit Tests to VC4

[Maxime Ripard <maxime@cerno.tech>]

Hi David,

On Thu, Nov 24, 2022 at 04:31:14PM +0800, David Gow wrote:
> On Wed, Nov 23, 2022 at 11:28 PM Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > Hi,
> >
> > This series introduce Kunit tests to the vc4 KMS driver, but unlike what we
> > have been doing so far in KMS, it actually tests the atomic modesetting code.
> >
> > In order to do so, I've had to improve a fair bit on the Kunit helpers already
> > found in the tree in order to register a full blown and somewhat functional KMS
> > driver.
> >
> > It's of course relying on a mock so that we can test it anywhere. The mocking
> > approach created a number of issues, the main one being that we need to create
> > a decent mock in the first place, see patch 22. The basic idea is that I
> > created some structures to provide a decent approximation of the actual
> > hardware, and that would support both major architectures supported by vc4.
> >
> > This is of course meant to evolve over time and support more tests, but I've
> > focused on testing the HVS FIFO assignment code which is fairly tricky (and the
> > tests have actually revealed one more bug with our current implementation). I
> > used to have a userspace implementation of those tests, where I would copy and
> > paste the kernel code and run the tests on a regular basis. It's was obviously
> > fairly suboptimal, so it seemed like the perfect testbed for that series.
>
> Thanks very much for this! I'm really excited to see these sorts of
> tests being written.
> 
> I was able to successfully run these under qemu with:
> ./tools/testing/kunit/kunit.py run --kunitconfig
> drivers/gpu/drm/vc4/tests --arch arm64
> --cross_compile=aarch64-linux-gnu-
> (and also with clang, using --make_options LLVM=1 instead of the
> --cross_compile flag)
> 
> On the other hand, they don't compile as a module:
> ERROR: modpost: missing MODULE_LICENSE() in drivers/gpu/drm/vc4/tests/vc4_mock.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/vc4/tests/vc4_mock_crtc.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/vc4/tests/vc4_mock_output.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/vc4/tests/vc4_mock_plane.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.o
> ERROR: modpost: missing MODULE_LICENSE() in
> drivers/gpu/drm/tests/drm_managed_test.o
> ERROR: modpost: "vc4_drm_driver"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc5_drm_driver"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "drm_kunit_helper_alloc_device"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "__drm_kunit_helper_alloc_drm_device_with_driver"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "__vc4_hvs_alloc"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc4_dummy_plane"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc4_mock_pv" [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc4_dummy_output"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> ERROR: modpost: "vc4_kms_load" [drivers/gpu/drm/vc4/tests/vc4_mock.ko]
> undefined!
> ERROR: modpost: "vc4_txp_crtc_data"
> [drivers/gpu/drm/vc4/tests/vc4_mock.ko] undefined!
> WARNING: modpost: suppressed 17 unresolved symbol warnings because
> there were too many)

Thanks I'll fix it

> Most of those are just the need to export some symbols. There's some
> work underway to support conditionally exporting symbols only if KUnit
> is enabled, which may help:
> https://lore.kernel.org/linux-kselftest/20221102175959.2921063-1-rmoar@google.com/

That's awesome :)

The current solution to include the test implementation is not ideal, so
it's great to see a nicer solution being worked on.

> Otherwise, I suspect the better short-term solution would just be to
> require that the tests are built-in (or at least compiled into
> whatever of the drm/vc4 modules makes most sense).
> 
> The only other thing which has me a little confused is the naming of
> some of the functions, specifically with the __ prefix. Is it just for
> internal functions (many of them aren't static, but maybe they could
> use the VISIBLE_IF_KUNIT macro if that makes sense), or for versions
> of functions which accept extra arguments?

It was for internal functions that would definitely benefit from
VISIBLE_IF_KUNIT indeed

> Not a big deal (and maybe it's a DRM naming convention I'm ignorant
> of), but I couldn't quite find a pattern on my first read through.
> 
> But on the whole, these look good from a KUnit point-of-view. It's
> really to see some solid mocking and driver testing, too. There would
> be ways to avoid passing the 'struct kunit' around in more places (or
> to store extra data as a kunit_resource), but I think it's better
> overall to pass it around like you have in this case -- it's certainly
> more compatible with things which might span threads (e.g. the
> workqueues).

One thing I'm really unsure about and would like your input on is
basically the entire device instantiation code in drm_kunit_helpers.c

It's a little fishy since it will allocate a platform_device while the
driver might expect some other bus device. And the code to bind the
driver based around probe and workqueues seems like a hack.

This is something that would benefit from having proper functions in
kunit to allocate a proper device for a given test. This is already
something that other unit test suites seems to get wrong, and I'm sure
there's some bugs somewhere in the helpers I did for DRM. What do you
think?

Maxime