On Wed, Nov 23, 2022 at 11:28 PM Maxime Ripard <[email protected]> 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.
>
> Let me know what you think,
> Maxime
>
> To: David Airlie <[email protected]>
> To: Daniel Vetter <[email protected]>
> To: Maarten Lankhorst <[email protected]>
> To: Maxime Ripard <[email protected]>
> To: Thomas Zimmermann <[email protected]>
> Cc: Dave Stevenson <[email protected]>
> Cc: Javier Martinez Canillas <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Maíra Canal <[email protected]>
> Cc: Brendan Higgins <[email protected]>
> Cc: David Gow <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Maxime Ripard <[email protected]>
>
> ---
Hi Maxime,
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)
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/[email protected]/
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? 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).
Thanks a bunch,
-- David
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 <[email protected]> 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/[email protected]/
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