[PATCH v2 6/7] drm/xe/kunit: Test rtp with no actions
Lucas De Marchi
lucas.demarchi at intel.com
Fri Jul 26 16:59:08 UTC 2024
On Fri, Jul 26, 2024 at 01:37:17PM GMT, Gustavo Sousa wrote:
>Quoting Lucas De Marchi (2024-07-26 03:43:36-03:00)
>>The OOB WAs use xe_rtp_process(), without passing an sr to save result
>>of the actions since there are none. They are also executed in a gt-only
>>context, making it harder to share the implementation. Thus, introduce a
>>new set of tests to check these RTP entries. The only check that can be
>>done is if the entry was marked as active.
>>
>>Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>---
>> drivers/gpu/drm/xe/tests/xe_rtp_test.c | 163 +++++++++++++++++++++++++
>> drivers/gpu/drm/xe/xe_rtp.c | 1 +
>> 2 files changed, 164 insertions(+)
>>
>>diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_test.c b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>>index 73e97f8d2b5b..5b425a335706 100644
>>--- a/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>>+++ b/drivers/gpu/drm/xe/tests/xe_rtp_test.c
>>@@ -42,6 +42,12 @@ struct rtp_to_sr_test_case {
>> const struct xe_rtp_entry_sr *entries;
>> };
>>
>>+struct rtp_test_case {
>>+ const char *name;
>>+ unsigned long expected_active;
>>+ const struct xe_rtp_entry *entries;
>>+};
>>+
>> static bool match_yes(const struct xe_gt *gt, const struct xe_hw_engine *hwe)
>> {
>> return true;
>>@@ -339,6 +345,155 @@ static void xe_rtp_process_to_sr_tests(struct kunit *test)
>> KUNIT_EXPECT_EQ(test, reg_sr->errors, param->expected_sr_errors);
>> }
>>
>>+/*
>>+ * Entries below follow the logic used with xe_wa_oob.rules:
>>+ * 1) Entries with empty name are OR'ed: all entries marked active since the
>>+ * last entry with a name
>>+ * 2) There are no action associated with rules
>>+ */
>>+static const struct rtp_test_case rtp_cases[] = {
>>+ {
>>+ .name = "active1",
>>+ .expected_active = BIT(0),
>>+ .entries = (const struct xe_rtp_entry[]) {
>>+ { XE_RTP_NAME("r1"),
>>+ XE_RTP_RULES(FUNC(match_yes)),
>>+ },
>>+ {}
>>+ },
>>+ },
>>+ {
>>+ .name = "active2",
>>+ .expected_active = BIT(0) | BIT(1),
>>+ .entries = (const struct xe_rtp_entry[]) {
>>+ { XE_RTP_NAME("r1"),
>>+ XE_RTP_RULES(FUNC(match_yes)),
>>+ },
>>+ { XE_RTP_NAME("r2"),
>>+ XE_RTP_RULES(FUNC(match_yes)),
>>+ },
>>+ {}
>>+ },
>>+ },
>>+ {
>>+ .name = "active-inactive",
>>+ .expected_active = BIT(0),
>>+ .entries = (const struct xe_rtp_entry[]) {
>>+ { XE_RTP_NAME("r1"),
>>+ XE_RTP_RULES(FUNC(match_yes)),
>>+ },
>>+ { XE_RTP_NAME("r2"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ {}
>>+ },
>>+ },
>>+ {
>>+ .name = "inactive-active",
>>+ .expected_active = BIT(1),
>>+ .entries = (const struct xe_rtp_entry[]) {
>>+ { XE_RTP_NAME("r1"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ { XE_RTP_NAME("r2"),
>>+ XE_RTP_RULES(FUNC(match_yes)),
>>+ },
>>+ {}
>>+ },
>>+ },
>>+ {
>>+ .name = "inactive-1st_or_active-inactive",
>>+ .expected_active = BIT(1) | BIT(2) | BIT(3),
>>+ .entries = (const struct xe_rtp_entry[]) {
>>+ { XE_RTP_NAME("r1"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ { XE_RTP_NAME("r2_or_conditions"),
>>+ XE_RTP_RULES(FUNC(match_yes)),
>>+ },
>>+ { XE_RTP_RULES(FUNC(match_no)) },
>>+ { XE_RTP_RULES(FUNC(match_no)) },
>>+ { XE_RTP_NAME("r3"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ {}
>>+ },
>>+ },
>>+ {
>>+ .name = "inactive-2nd_or_active-inactive",
>>+ .expected_active = BIT(1) | BIT(2) | BIT(3),
>>+ .entries = (const struct xe_rtp_entry[]) {
>>+ { XE_RTP_NAME("r1"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ { XE_RTP_NAME("r2_or_conditions"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ { XE_RTP_RULES(FUNC(match_yes)) },
>>+ { XE_RTP_RULES(FUNC(match_no)) },
>>+ { XE_RTP_NAME("r3"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ {}
>>+ },
>>+ },
>>+ {
>>+ .name = "inactive-last_or_active-inactive",
>>+ .expected_active = BIT(1) | BIT(2) | BIT(3),
>>+ .entries = (const struct xe_rtp_entry[]) {
>>+ { XE_RTP_NAME("r1"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ { XE_RTP_NAME("r2_or_conditions"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ { XE_RTP_RULES(FUNC(match_no)) },
>>+ { XE_RTP_RULES(FUNC(match_yes)) },
>>+ { XE_RTP_NAME("r3"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ {}
>>+ },
>>+ },
>>+ {
>>+ .name = "inactive-no_or_active-inactive",
>>+ .expected_active = 0,
>>+ .entries = (const struct xe_rtp_entry[]) {
>>+ { XE_RTP_NAME("r1"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ { XE_RTP_NAME("r2_or_conditions"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ { XE_RTP_RULES(FUNC(match_no)) },
>>+ { XE_RTP_RULES(FUNC(match_no)) },
>>+ { XE_RTP_NAME("r3"),
>>+ XE_RTP_RULES(FUNC(match_no)),
>>+ },
>>+ {}
>>+ },
>>+ },
>>+};
>>+
>>+static void xe_rtp_process_tests(struct kunit *test)
>>+{
>>+ const struct rtp_test_case *param = test->param_value;
>>+ struct xe_device *xe = test->priv;
>>+ struct xe_gt *gt = xe_device_get_root_tile(xe)->primary_gt;
>>+ struct xe_rtp_process_ctx ctx = XE_RTP_PROCESS_CTX_INITIALIZER(gt);
>>+ unsigned long count_rtp_entries = 0, active = 0;
>>+ const struct xe_rtp_entry *rtp_entry;
>>+
>>+ for (rtp_entry = param->entries; rtp_entry && rtp_entry->rules;
>>+ rtp_entry++, count_rtp_entries++)
>>+ ;
>
>Same comment as for "drm/xe/kunit: Test active rtp entries", otherwise,
>looks good to me.
>
>Reviewed-by: Gustavo Sousa <gustavo.sousa at intel.com>
>
>Since we are on the subject, I wonder if this "unnamed entry to
>represent an OR" could be now replaced with a proper OR rule, so we
>would have a single implementation for the OR logic.
yes... I was looking at going one step further and merge even the
process function. One drawback of merging just the OR is that we'd have
to raise the max number of rules/actions for not much benefit. Hhumn..
however this would only change the preprocessor with no change on the
size of the tables, so maybe it's worth it. It would also simplify this
kunit tests as then it's simply checking 1 entry rather than a mask.
I will think about that when sending a v2 - I will merge the bug fix
right away, so I can mention the commit hash too.
Thanks for spotting the bug and the reviews.
Lucas De Marchi
>
>Since xe_wa_oob.rules currently has a nice formatting, we could have
>xe_gen_wa_oob doing the conversion.
>
>--
>Gustavo Sousa
>
>>+
>>+ xe_rtp_process_ctx_enable_active_tracking(&ctx, &active, count_rtp_entries);
>>+ xe_rtp_process(&ctx, param->entries);
>>+
>>+ KUNIT_EXPECT_EQ(test, active, param->expected_active);
>>+}
>>+
>> static void rtp_to_sr_desc(const struct rtp_to_sr_test_case *t, char *desc)
>> {
>> strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
>>@@ -346,6 +501,13 @@ static void rtp_to_sr_desc(const struct rtp_to_sr_test_case *t, char *desc)
>>
>> KUNIT_ARRAY_PARAM(rtp_to_sr, rtp_to_sr_cases, rtp_to_sr_desc);
>>
>>+static void rtp_desc(const struct rtp_test_case *t, char *desc)
>>+{
>>+ strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
>>+}
>>+
>>+KUNIT_ARRAY_PARAM(rtp, rtp_cases, rtp_desc);
>>+
>> static int xe_rtp_test_init(struct kunit *test)
>> {
>> struct xe_device *xe;
>>@@ -378,6 +540,7 @@ static void xe_rtp_test_exit(struct kunit *test)
>>
>> static struct kunit_case xe_rtp_tests[] = {
>> KUNIT_CASE_PARAM(xe_rtp_process_to_sr_tests, rtp_to_sr_gen_params),
>>+ KUNIT_CASE_PARAM(xe_rtp_process_tests, rtp_gen_params),
>> {}
>> };
>>
>>diff --git a/drivers/gpu/drm/xe/xe_rtp.c b/drivers/gpu/drm/xe/xe_rtp.c
>>index f054ac9cf06d..1c641cc0f5a1 100644
>>--- a/drivers/gpu/drm/xe/xe_rtp.c
>>+++ b/drivers/gpu/drm/xe/xe_rtp.c
>>@@ -327,6 +327,7 @@ void xe_rtp_process(struct xe_rtp_process_ctx *ctx,
>> entry - entries);
>> }
>> }
>>+EXPORT_SYMBOL_IF_KUNIT(xe_rtp_process);
>>
>> bool xe_rtp_match_even_instance(const struct xe_gt *gt,
>> const struct xe_hw_engine *hwe)
>>--
>>2.43.0
>>
More information about the Intel-xe
mailing list