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@xxxxxxxxxx> 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@xxxxxxxxxx/ 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
