| Message ID | 20221123-rpi-kunit-tests-v1-0-051a0bb60a16@cerno.tech (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: Introduce Kunit Tests to VC4 | expand |
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. > > Let me know what you think, > Maxime > > To: David Airlie <airlied@gmail.com> > To: Daniel Vetter <daniel@ffwll.ch> > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > To: Maxime Ripard <mripard@kernel.org> > To: Thomas Zimmermann <tzimmermann@suse.de> > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> > Cc: Javier Martinez Canillas <javierm@redhat.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Maíra Canal <mairacanal@riseup.net> > Cc: Brendan Higgins <brendan.higgins@linux.dev> > Cc: David Gow <davidgow@google.com> > Cc: linux-kselftest@vger.kernel.org > Cc: kunit-dev@googlegroups.com > Cc: dri-devel@lists.freedesktop.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-media@vger.kernel.org > Cc: linaro-mm-sig@lists.linaro.org > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- 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/20221102175959.2921063-1-rmoar@google.com/ 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 <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
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 <airlied@gmail.com> To: Daniel Vetter <daniel@ffwll.ch> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> To: Maxime Ripard <mripard@kernel.org> To: Thomas Zimmermann <tzimmermann@suse.de> Cc: Dave Stevenson <dave.stevenson@raspberrypi.com> Cc: Javier Martinez Canillas <javierm@redhat.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Maíra Canal <mairacanal@riseup.net> Cc: Brendan Higgins <brendan.higgins@linux.dev> Cc: David Gow <davidgow@google.com> Cc: linux-kselftest@vger.kernel.org Cc: kunit-dev@googlegroups.com Cc: dri-devel@lists.freedesktop.org Cc: linux-kernel@vger.kernel.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- Maxime Ripard (24): drm/tests: helpers: Rename the device init helper drm/tests: helpers: Remove the name parameter drm/tests: helpers: Create the device in another function drm/tests: helpers: Switch to a platform_device drm/tests: helpers: Make sure the device is bound drm/tests: kunit: Allow for a custom device struct to be allocated drm/tests: helpers: Allow to pass a custom drm_driver drm/tests: Add a test for DRM managed actions drm/atomic: Constify the old/new state accessors drm/vc4: kms: Sort the CRTCs by output before assigning them drm/vc4: Constify container_of wrappers drm/vc4: Move HVS state to main header drm/vc4: kms: Constify the HVS old/new state helpers drm/vc4: txp: Reorder the variable assignments drm/vc4: Add TXP encoder type drm/vc4: txp: Initialise the CRTC before the encoder and connector drm/vc4: crtc: Pass the device and data in vc4_crtc_init drm/vc4: crtc: Introduce a lower-level crtc init helper drm/vc4: crtc: Make encoder lookup helper public drm/vc4: crtc: Provide a CRTC name drm/vc4: hvs: Provide a function to initialize the HVS structure drm/vc4: tests: Introduce a mocking infrastructure drm/vc4: tests: Fail the current test if we access a register drm/vc4: tests: Add unit test suite for the PV muxing drivers/gpu/drm/drm_atomic.c | 12 +- drivers/gpu/drm/tests/Makefile | 1 + drivers/gpu/drm/tests/drm_client_modeset_test.c | 16 +- drivers/gpu/drm/tests/drm_kunit_helpers.c | 116 +++-- drivers/gpu/drm/tests/drm_kunit_helpers.h | 39 +- drivers/gpu/drm/tests/drm_managed_test.c | 68 +++ drivers/gpu/drm/vc4/Kconfig | 15 + drivers/gpu/drm/vc4/Makefile | 1 + drivers/gpu/drm/vc4/tests/.kunitconfig | 14 + drivers/gpu/drm/vc4/tests/Makefile | 8 + drivers/gpu/drm/vc4/tests/vc4_mock.c | 174 +++++++ drivers/gpu/drm/vc4/tests/vc4_mock.h | 58 +++ drivers/gpu/drm/vc4/tests/vc4_mock_crtc.c | 39 ++ drivers/gpu/drm/vc4/tests/vc4_mock_output.c | 97 ++++ drivers/gpu/drm/vc4/tests/vc4_mock_plane.c | 45 ++ drivers/gpu/drm/vc4/tests/vc4_test_pv_muxing.c | 624 ++++++++++++++++++++++++ drivers/gpu/drm/vc4/vc4_crtc.c | 119 +++-- drivers/gpu/drm/vc4/vc4_dpi.c | 13 +- drivers/gpu/drm/vc4/vc4_drv.c | 4 +- drivers/gpu/drm/vc4/vc4_drv.h | 113 ++++- drivers/gpu/drm/vc4/vc4_dsi.c | 9 +- drivers/gpu/drm/vc4/vc4_hdmi_regs.h | 4 + drivers/gpu/drm/vc4/vc4_hvs.c | 81 +-- drivers/gpu/drm/vc4/vc4_kms.c | 138 +++--- drivers/gpu/drm/vc4/vc4_txp.c | 66 ++- drivers/gpu/drm/vc4/vc4_vec.c | 13 +- include/drm/drm_atomic.h | 32 +- 27 files changed, 1678 insertions(+), 241 deletions(-) --- base-commit: 35c3a2d02f0dc153a5f2f304ba33e1436b6a8d8f change-id: 20221123-rpi-kunit-tests-87a388492a73 Best regards,