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

Sun Apr 2 10:51:17 UTC 2023

On 02.04.2023 03:24, Lucas De Marchi wrote:
> 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

likely we can workaround that by using

	default Y if DRM_XE_DEBUG

or just explicitly update the CI config to the desired value

> 
> 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.

before choosing the name for new config for HW tests from the above
list, we should discuss that in connection with existing config name, as
we plan to use KUNIT in both scenarios, and maybe worth to change both
to clearly indicate our intentions/use case.

here is sample of possible pairs:

a) CONFIG_DRM_XE_KUNIT_TEST
   CONFIG_DRM_XE_HW_KUNIT_TEST

b) CONFIG_DRM_XE_KUNIT_TEST
   CONFIG_DRM_XE_LIVE_TEST

c) CONFIG_DRM_XE_KUNIT_TEST
   CONFIG_DRM_XE_SELFTEST

d) CONFIG_DRM_XE_KUNIT_SELFTESTS
   CONFIG_DRM_XE_KUNIT_LIVETESTS

e) CONFIG_DRM_XE_SELFTESTS
   CONFIG_DRM_XE_LIVETESTS

f) CONFIG_DRM_XE_SW_SELFTESTS
   CONFIG_DRM_XE_HW_SELFTESTS

g) CONFIG_DRM_XE_SW_UNIT_TESTS
   CONFIG_DRM_XE_HW_LIVE_TESTS

...

my vote would go for e) as a good balance

thanks,
Michal

> 
> thanks
> Lucas De Marchi