[Intel-xe] [PATCH 1/7] drm/xe: Add basic unit tests for rtp
Lucas De Marchi
lucas.demarchi at intel.com
Fri Mar 31 13:43:34 UTC 2023
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
>> diff --git a/drivers/gpu/drm/xe/tests/Makefile b/drivers/gpu/drm/xe/tests/Makefile
>> index 47056b6459e3..c5c2f108d017 100644
>> --- a/drivers/gpu/drm/xe/tests/Makefile
>> +++ b/drivers/gpu/drm/xe/tests/Makefile
>> @@ -1,4 +1,7 @@
>> # SPDX-License-Identifier: GPL-2.0
>>
>> -obj-$(CONFIG_DRM_XE_KUNIT_TEST) += xe_bo_test.o xe_dma_buf_test.o \
>> - xe_migrate_test.o
>> +obj-$(CONFIG_DRM_XE_KUNIT_TEST) += \
>> + xe_bo_test.o \
>> + xe_dma_buf_test.o \
>> + xe_migrate_test.o \
>> + xe_rtp_test.o
>> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp.c b/drivers/gpu/drm/xe/tests/xe_rtp.c
>> new file mode 100644
>> index 000000000000..92e2c2b8b44d
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/tests/xe_rtp.c
>> @@ -0,0 +1,201 @@
>> +// SPDX-License-Identifier: GPL-2.0 AND MIT
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include "tests/xe_rtp_test.h"
>> +
>> +#include <linux/string.h>
>> +#include <linux/xarray.h>
>
>This test is not using xarray.
>
>> +
>> +#include <kunit/test.h>
>> +
>> +#include "regs/xe_gt_regs.h"
>> +#include "regs/xe_reg_defs.h"
>> +#include "xe_rtp.h"
>> +
>> +#undef _MMIO
>> +#undef MCR_REG
>> +#define _MMIO(x) _XE_RTP_REG(x)
>> +#define MCR_REG(x) _XE_RTP_MCR_REG(x)
>> +
>> +#define REGULAR_REG1 _MMIO(1)
>> +#define REGULAR_REG2 _MMIO(2)
>> +#define REGULAR_REG3 _MMIO(3)
>> +#define MCR_REG1 MCR_REG(1)
>> +#define MCR_REG2 MCR_REG(2)
>> +#define MCR_REG3 MCR_REG(3)
>> +
>> +static bool match_yes(const struct xe_gt *gt, const struct xe_hw_engine *hwe)
>> +{
>> + return true;
>> +}
>> +
>> +static bool match_no(const struct xe_gt *gt, const struct xe_hw_engine *hwe)
>> +{
>> + return false;
>> +}
>> +
>> +/* Basic entries: same register, only one action, disjoint values */
>> +static const struct xe_rtp_entry basic_entries[] = {
>> + { XE_RTP_NAME("basic-yes-1"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(0)))
>> + },
>> + { XE_RTP_NAME("basic-yes-2"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(1)))
>> + },
>> + { XE_RTP_NAME("basic-no-3"),
>> + XE_RTP_RULES(FUNC(match_no)),
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(2)))
>> + },
>> + { XE_RTP_NAME("basic-no-4"),
>> + XE_RTP_RULES(FUNC(match_yes), FUNC(match_no)),
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(3)))
>> + },
>> + { XE_RTP_NAME("basic-yes-clr-1"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + XE_RTP_ACTIONS(CLR(REGULAR_REG1, REG_BIT(4)))
>> + },
>> + { XE_RTP_NAME("basic-yes-clr-2"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + XE_RTP_ACTIONS(CLR(REGULAR_REG1, REG_BIT(5)))
>> + },
>> + {}
>> +};
>> +
>> +void xe_rtp_process_basic(struct kunit *test)
>> +{
>> + struct xe_device *xe = test->priv;
>> + struct xe_reg_sr *reg_sr = &xe->gt[0].reg_sr;
>> + const struct xe_rtp_entry *rtp_entry;
>> + const struct xe_reg_sr_entry *sre, *sr_entry = NULL;
>> + unsigned long idx, count = 0, set_bits = 0, clr_bits = 0;
>> +
>> + xe_reg_sr_init(reg_sr, "basic", xe);
>> + xe_rtp_process(basic_entries, reg_sr, &xe->gt[0], NULL);
>> +
>> + xa_for_each(®_sr->xa, idx, sre) {
>> + count++;
>> + sr_entry = sre;
>> + }
>> +
>> + /*
>> + * Same REGULAR_REG1 register being used: all the entries should
>> + * coalesce into a single entry
>> + */
>> + KUNIT_ASSERT_EQ(test, count, 1);
>> +
>> + /* Only bits in the entries named *-yes* are set */
>> + for (rtp_entry = basic_entries; rtp_entry && rtp_entry->name; rtp_entry++) {
>> + if (!strstr(rtp_entry->name, "-yes"))
>> + continue;
>> +
>> + clr_bits |= rtp_entry->actions[0].clr_bits;
>> + set_bits |= rtp_entry->actions[0].set_bits;
>> + }
>
>To keep the test simple, we should hardcode the expected value rather
>than compute it inside the test.
>
>> +
>> + /* implementation detail of RTP - SET() also clears */
>> + clr_bits |= set_bits;
>> +
>> + KUNIT_EXPECT_EQ(test, clr_bits, sr_entry->clr_bits);
>> + KUNIT_EXPECT_EQ(test, set_bits, sr_entry->set_bits);
>
>All 3 tests follow very similar logic.
>I would suggest adding a name and expected values to entries, something
>like:
>
>struct xe_rtp_test {
> name;
> expected_set_bits;
> expected_clr_bits;
> expected_count;
> *entries;
>};
>
>static const struct xe_rtp_test xe_rtp_tests[] {
> { .name = "process basic",
> .expected_count = 1,
> (...)
> .entries = (const struct xe_rtp_entry[]) {
> { XE_RTP_NAME("incompat-types-yes-1"),
> (...)
> }
> },
> { .name = "process dup",
> (...)
> },
> (...)
>};
>
>And then we could handle all 3 test cases as a single parameterized test
>with KUNIT_CASE_PARAM
At the time I wrote these tests, it didn't seem worth it as I don't see
us extending or adapting much of the rtp tests. For the WAs I followed
the param approach as we will keep changing them.
Anyway, I will give this a try.
thanks
Lucas De Marchi
>
>> +}
>> +EXPORT_SYMBOL(xe_rtp_process_basic);
>> +
>> +/*
>> + * Duplicate entries - same register register, only one action,
>> + * with actions trying to set/clear bits already changed in previous entries
>> + */
>> +static const struct xe_rtp_entry dup_entries[] = {
>> + { XE_RTP_NAME("dup-entries-yes-1"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(0)))
>> + },
>> + { XE_RTP_NAME("dup-entries-no-2"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + /* should be dropped as it's the same as a previous */
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(0)))
>> + },
>> + { XE_RTP_NAME("dup-entries-no-3"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + /* should be dropped as not disjoint */
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_GENMASK(1, 0)))
>> + },
>> + {}
>> +};
>> +
>> +void xe_rtp_process_dup(struct kunit *test)
>> +{
>> + struct xe_device *xe = test->priv;
>> + struct xe_reg_sr *reg_sr = &xe->gt[0].reg_sr;
>> + const struct xe_reg_sr_entry *sre, *sr_entry = NULL;
>> + unsigned long idx, count = 0;
>> +
>> + xe_reg_sr_init(reg_sr, "duplicate values", xe);
>> + xe_rtp_process(dup_entries, reg_sr, &xe->gt[0], NULL);
>> +
>> + xa_for_each(®_sr->xa, idx, sre) {
>> + count++;
>> + sr_entry = sre;
>> + }
>> +
>> + /*
>> + * Same REGULAR_REG1 register being used: all the entries should
>> + * coalesce into a single entry
>> + */
>> + KUNIT_EXPECT_EQ(test, count, 1);
>> + KUNIT_EXPECT_EQ(test, sr_entry->set_bits, REG_BIT(0));
>> +}
>> +EXPORT_SYMBOL(xe_rtp_process_dup);
>> +
>> +/*
>> + * Incompatible types: same register register, but entries trying to set the
>> + * same register passing incompatible types (masked / mcr)
>> + */
>> +static const struct xe_rtp_entry incompat_types_entries[] = {
>> + { XE_RTP_NAME("incompat-types-yes-1"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(0)))
>> + },
>> + { XE_RTP_NAME("incompat-types-no-2"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + /* dropped since has incompatible type with previous entry */
>> + XE_RTP_ACTIONS(SET(MCR_REG1, REG_BIT(1)))
>> + },
>> + { XE_RTP_NAME("incompat-types-no-3"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + /* dropped since has incompatible type with previous entry */
>> + XE_RTP_ACTIONS(SET(REGULAR_REG1, REG_BIT(2),
>> + XE_RTP_ACTION_FLAG(MASKED_REG)))
>> + },
>> + { XE_RTP_NAME("incompat-types-no-3"),
>> + XE_RTP_RULES(FUNC(match_yes)),
>> + /* dropped since has incompatible type with previous entry */
>> + XE_RTP_ACTIONS(SET(MCR_REG1, REG_BIT(3),
>> + XE_RTP_ACTION_FLAG(MASKED_REG)))
>> + },
>> + {}
>> +};
>> +
>> +void xe_rtp_process_incompat_types(struct kunit *test)
>> +{
>> + struct xe_device *xe = test->priv;
>> + struct xe_reg_sr *reg_sr = &xe->gt[0].reg_sr;
>> + const struct xe_reg_sr_entry *sre, *sr_entry = NULL;
>> + unsigned long idx, count = 0;
>> +
>> + xe_reg_sr_init(reg_sr, "incompatible types", xe);
>> + xe_rtp_process(incompat_types_entries, reg_sr, &xe->gt[0], NULL);
>> +
>> + xa_for_each(®_sr->xa, idx, sre) {
>> + count++;
>> + sr_entry = sre;
>> + }
>> +
>> + /* Just the first entry is added, the rest are incompatible */
>> + KUNIT_EXPECT_EQ(test, count, 1);
>> + KUNIT_EXPECT_EQ(test, sr_entry->set_bits, REG_BIT(0));
>> +}
>> +EXPORT_SYMBOL(xe_rtp_process_incompat_types);
>> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_test.c b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>> new file mode 100644
>> index 000000000000..4a12aad3f759
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>
>There's no need to have this as a separate file.
>We can move the contents of tests/xe_rtp.c into tests/xe_rtp_test.c and
>include tests/xe_rtp_test.c directly in xe_rtp.c
>
>-Michał
>
>> @@ -0,0 +1,64 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#include "xe_rtp_test.h"
>> +
>> +#include <drm/drm_drv.h>
>> +#include <drm/drm_kunit_helpers.h>
>> +
>> +#include <kunit/test.h>
>> +
>> +#include "xe_device_types.h"
>> +
>> +static void xe_fake_device_init(struct xe_device *xe)
>> +{
>> + xe->gt[0].xe = xe;
>> +}
>> +
>> +static int xe_rtp_test_init(struct kunit *test)
>> +{
>> + struct xe_device *xe;
>> + struct device *dev;
>> +
>> + dev = drm_kunit_helper_alloc_device(test);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
>> +
>> + xe = drm_kunit_helper_alloc_drm_device(test, dev,
>> + struct xe_device,
>> + drm, DRIVER_GEM);
>> + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xe);
>> +
>> + xe_fake_device_init(xe);
>> + xe->drm.dev = dev;
>> + test->priv = xe;
>> +
>> + return 0;
>> +}
>> +
>> +static void xe_rtp_test_exit(struct kunit *test)
>> +{
>> + struct xe_device *xe = test->priv;
>> +
>> + drm_kunit_helper_free_device(test, xe->drm.dev);
>> +}
>> +
>> +static struct kunit_case xe_rtp_tests[] = {
>> + KUNIT_CASE(xe_rtp_process_basic),
>> + KUNIT_CASE(xe_rtp_process_dup),
>> + KUNIT_CASE(xe_rtp_process_incompat_types),
>> + {}
>> +};
>> +
>> +static struct kunit_suite xe_rtp_test_suite = {
>> + .name = "xe_rtp",
>> + .init = xe_rtp_test_init,
>> + .exit = xe_rtp_test_exit,
>> + .test_cases = xe_rtp_tests,
>> +};
>> +
>> +kunit_test_suite(xe_rtp_test_suite);
>> +
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_test.h b/drivers/gpu/drm/xe/tests/xe_rtp_test.h
>> new file mode 100644
>> index 000000000000..a2f1437d6299
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_test.h
>> @@ -0,0 +1,15 @@
>> +/* SPDX-License-Identifier: GPL-2.0 AND MIT */
>> +/*
>> + * Copyright © 2023 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_RTP_TEST_H_
>> +#define _XE_RTP_TEST_H_
>> +
>> +struct kunit;
>> +
>> +void xe_rtp_process_basic(struct kunit *test);
>> +void xe_rtp_process_dup(struct kunit *test);
>> +void xe_rtp_process_incompat_types(struct kunit *test);
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
>> index cb9dd894547d..f634558b6e50 100644
>> --- a/drivers/gpu/drm/xe/xe_rtp.c
>> +++ b/drivers/gpu/drm/xe/xe_rtp.c
>> @@ -186,3 +186,7 @@ bool xe_rtp_match_first_gslice_fused_off(const struct xe_gt *gt,
>>
>> return dss >= dss_per_gslice;
>> }
>> +
>> +#if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST)
>> +#include "tests/xe_rtp.c"
>> +#endif
>> --
>> 2.39.0
>>
More information about the Intel-xe
mailing list