[Intel-xe] [PATCH 1/7] drm/xe: Add basic unit tests for rtp

Sun Apr 2 01:24:47 UTC 2023

On Sat, Apr 01, 2023 at 08:54:18PM +0200, Michał Winiarski wrote:
>On Sat, Apr 01, 2023 at 01:59:32AM -0700, Lucas De Marchi wrote:
>> On Fri, Mar 31, 2023 at 07:43:34AM -0600, Lucas De Marchi wrote:
>> > On Mon, Mar 27, 2023 at 07:56:01PM +0200, Michał Winiarski wrote:
>> > > On Tue, Mar 21, 2023 at 03:05:21PM -0700, Lucas De Marchi wrote:
>> > > > Add some basic unit tests for rtp. This is intended to prove the
>> > > > functionality of the rtp itself, like coalescing entries, rejecting
>> > > > non-disjoint values, etc.
>> > > >
>> > > > Contrary to the other tests in xe, this is a unit test to test the
>> > > > sw-side only, so it can be executed on any machine - it doesn't interact
>> > > > with the real hardware. For that a mock was provided with the minimum
>> > > > initialization needed. Running it produces the following output:
>> > > >
>> > > > 	$ ./tools/testing/kunit/kunit.py run --raw_output  \
>> > > > 		--kunitconfig drivers/gpu/drm/xe/.kunitconfig xe_rtp
>> > > > 	...
>> > > > 	[13:46:14] Starting KUnit Kernel (1/1)...
>> > > > 	KTAP version 1
>> > > > 	1..1
>> > > > 	    KTAP version 1
>> > > > 	    # Subtest: xe_rtp
>> > > > 	    1..3
>> > > > 	    ok 1 xe_rtp_process_basic
>> > > > 	[drm:xe_reg_sr_add] *ERROR* Discarding save-restore reg 0001 (clear: 00000001, set: 00000001, masked: no): ret=-22
>> > > > 	[drm:xe_reg_sr_add] *ERROR* Discarding save-restore reg 0001 (clear: 00000003, set: 00000003, masked: no): ret=-22
>> > > > 	    ok 2 xe_rtp_process_dup
>> > > > 	[drm:xe_reg_sr_add] *ERROR* Discarding save-restore reg 0001 (clear: 00000001, set: 00000001, masked: no): ret=-22
>> > > > 	[drm:xe_reg_sr_add] *ERROR* Discarding save-restore reg 0001 (clear: 00000003, set: 00000003, masked: no): ret=-22
>> > > > 	    ok 3 xe_rtp_process_incompat_types
>> > > > 	# xe_rtp: pass:3 fail:0 skip:0 total:3
>> > > > 	# Totals: pass:3 fail:0 skip:0 total:3
>> > > > 	ok 1 xe_rtp
>> > > > 	[13:46:14] Elapsed time: 6.800s total, 0.002s configuring, 6.682s building, 0.073s running
>> > > >
>> > > > Note that the ERRORs in the kernel log are expected since it's testing
>> > > > non-disjoint or incompatible entries.
>> > > >
>> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> > > > Reviewed-by: Maarten Lankhorst<maarten.lankhorst at linux.intel.com>
>> > > > ---
>> > > > drivers/gpu/drm/xe/Kconfig.debug       |   1 +
>> > > > drivers/gpu/drm/xe/tests/Makefile      |   7 +-
>> > > > drivers/gpu/drm/xe/tests/xe_rtp.c      | 201 +++++++++++++++++++++++++
>> > > > drivers/gpu/drm/xe/tests/xe_rtp_test.c |  64 ++++++++
>> > > > drivers/gpu/drm/xe/tests/xe_rtp_test.h |  15 ++
>> > > > drivers/gpu/drm/xe/xe_rtp.c            |   4 +
>> > > > 6 files changed, 290 insertions(+), 2 deletions(-)
>> > > > create mode 100644 drivers/gpu/drm/xe/tests/xe_rtp.c
>> > > > create mode 100644 drivers/gpu/drm/xe/tests/xe_rtp_test.c
>> > > > create mode 100644 drivers/gpu/drm/xe/tests/xe_rtp_test.h
>> > > >
>> > > > diff --git a/drivers/gpu/drm/xe/Kconfig.debug b/drivers/gpu/drm/xe/Kconfig.debug
>> > > > index 93b284cdd0a2..11bb13c73e7b 100644
>> > > > --- a/drivers/gpu/drm/xe/Kconfig.debug
>> > > > +++ b/drivers/gpu/drm/xe/Kconfig.debug
>> > > > @@ -66,6 +66,7 @@ config DRM_XE_KUNIT_TEST
>> > > > 	depends on DRM_XE && KUNIT && DEBUG_FS
>> > > > 	default KUNIT_ALL_TESTS
>> > > > 	select DRM_EXPORT_FOR_TESTS if m
>> > > > +	select DRM_KUNIT_TEST_HELPERS
>> > > > 	help
>> > > > 	  Choose this option to allow the driver to perform selftests under
>> > > > 	  the kunit framework
>> > >
>> > > But now we're mixing proper isolated tests with HW-interacting ones.
>> > > Can we move the HW-interacting ones to a separate Kconfig which is not
>> > > included in .kunitconfig and doesn't set its default value using
>> > > KUNIT_ALL_TESTS?
>> >
>> > I'm not sure moving them to a separate kconfig is good, because that
>> > would mean people building without one or the other. The only case it
>> > makes sense to build without the hw-interacting ones would be for when
>> > we are building for UML.
>> >
>> > I want to make sure the kconfig from CI includes both and people can
>> > run under !UML both of them.
>> >
>> > On the positive side for your approach, we'd be moving the
>> > *HW-interacting* ones to a separate config. This means we can create the
>> > split between the true unit tests and the ones just using the kunit
>> > infrastructure. I will give this a try and see how it looks in the end.
>> >
>> > I've been advocating for a split on a name-basis, calling these unit
>> > tests as sw_* and the others as hw_*. Then running just the sw ones is
>> > simply a matter of passing it as argument to kunit.py
>>
>> I will leave this for later as I don't want to stall the tests on
>> something that can be converted later and... this is not making the
>> situation any worse: actually running this in CI will provide an output
>> a little more useful than it actually is giving: a skip on all tests.
>>
>> I'm almost convinced we can make it work with separate configs as you
>> suggested, but we will need to sync with CI to make sure it fits
>> everyody.
>
>Sure, just something to keep in mind for the future.
>
>And it's not really about architecture (UML or not).
>.kunitconfig and KUNIT_ALL_TESTS are only relevant for kunit.py users,
>and so we want to have config for non-HW-interacting tests in
>.kunitconfig with a default value based on KUNIT_ALL_TESTS. Adding any
>architecture dependencies here would break kunit.py --arch=X usecase.
>
>For HW-interacting tests, we want a completely separate config, that's
>not part of .kunitconfig and does not have anything to do with
>KUNIT_ALL_TESTS.
>
>This way - kunit.py users won't even notice the skips (unless they build
>custom config), and for HW, I expect CI to be using a custom kernel
>config anyway (KUnit one is very unlikely to boot on real hardware), so

that's why this will need to be synchronized with a change in CI config
as with the current config we would stop having the hw tests.

>.kunitconfig and KUNIT_ALL_TESTS don't really matter for HW-interacting
>tests.

I agree about .kunitconfig, but if we create a
CONFIG_DRM_XE_HW_KUNIT_TEST and default it to N instead of
KUNIT_ALL_TESTS as you propose, CI would stop having those tests in its
config. Here it's not about .kunitconfig, but the .config CI is using:
https://gitlab.freedesktop.org/drm/xe/ci/-/blob/main/kernel/kconfig

Options:
1) CONFIG_DRM_XE_HW_KUNIT_TEST 
2) CONFIG_DRM_XE_LIVE_TEST
3) CONFIG_DRM_XE_SELFTEST

The above would be my order of preference, but I don't have a strong
opinion.

thanks
Lucas De Marchi