Re: [PATCH 2/4] drm/xe/rtp: Add GT WAs rule checking

[Gustavo Sousa <gustavo.sousa@intel.com>]

Adding some things I forgot in my previous reply...

Gustavo Sousa <gustavo.sousa@intel.com> writes:

> Violet Monti <violet.monti@intel.com> writes:
>

I think the commit subject is a bit vague. We could try to be more
specific with something like:

  drm/xe/rtp: Ensure gt_was doesn't check for engine types

>> It is currently possible for a RTP rule, and subsequently a workaround,
>> to expect contexts that may not be present when the workaround is
>> applied. For example, the workarounds in the engine_was[] in drm/xe/xe_wa.c
>> expect an engine entity to be active. Conversely, the gt_was[] is not
>> depending on an engine entity to implement its workarounds. This kunit
>> test addition checks the gt_was[] workaround list for any workarounds
>> with specifically an engine_class rule. If a workaround does have an
>
> I think "an engine_class rule" can be a bit vague. I would spell out
> ENGINE_CLASS() or XE_RTP_ENGINE_CLASS().
>
>> engine_class rule, the workaround is then checked for the
>> "FOREACH_ENGINE" flag, which ensures the workaround is not implemented
>> in an improper context.
>>
>> The result of this test is an expectation failure if a workaround has an
>> improper engine_class rule, and aims to prevent future issues of
>> gt_was being applied without proper contexts.
>>
>> Signed-off-by: Violet Monti <violet.monti@intel.com>
>> ---
>>  drivers/gpu/drm/xe/tests/Makefile             |  1 +
>>  drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c | 76 +++++++++++++++++++
>>  drivers/gpu/drm/xe/tests/xe_rtp_tables_test.h | 24 ++++++
>>  drivers/gpu/drm/xe/xe_wa.c                    |  2 +-
>>  drivers/gpu/drm/xe/xe_wa.h                    |  5 ++
>>  5 files changed, 107 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
>>  create mode 100644 drivers/gpu/drm/xe/tests/xe_rtp_tables_test.h
>>
>> diff --git a/drivers/gpu/drm/xe/tests/Makefile b/drivers/gpu/drm/xe/tests/Makefile
>> index 0e3408f4952c..f7aa47f11a36 100644
>> --- a/drivers/gpu/drm/xe/tests/Makefile
>> +++ b/drivers/gpu/drm/xe/tests/Makefile
>> @@ -9,5 +9,6 @@ obj-$(CONFIG_DRM_XE_KUNIT_TEST) += xe_test.o
>>  xe_test-y = xe_test_mod.o \
>>  	xe_args_test.o \
>>  	xe_pci_test.o \
>> +	xe_rtp_tables_test.o \
>>  	xe_rtp_test.o \
>>  	xe_wa_test.o
>> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c b/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
>> new file mode 100644
>> index 000000000000..100cac379b08
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
>> @@ -0,0 +1,76 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +#include <drm/drm_kunit_helpers.h>
>> +
>> +#include "xe_kunit_helpers.h"
>> +#include "xe_pci_test.h"
>> +#include "xe_rtp.h"
>> +#include "xe_rtp_tables_test.h"
>> +#include "xe_wa.h"
>> +
>> +static void xe_rtp_table_gt_test(struct kunit *test)
>> +{
>> +	const struct xe_rtp_entry_sr *param = test->param_value;
>
> I think it makes sense to call the variable "entry" here, like done in
> other places for instances of xe_rtp_entry_sr.
>
>> +
>> +	for (int i = 0; i < param->n_rules; i++) {
>> +		if (param->rules[i].match_type == XE_RTP_MATCH_ENGINE_CLASS)
>
> We would need to check XE_RTP_MATCH_NOT_ENGINE_CLASS as well.
>
>> +			KUNIT_EXPECT_EQ_MSG(test, param->flags,
>> +					    XE_RTP_ENTRY_FLAG_FOREACH_ENGINE,
>
> If the entry has other flags set along with
> XE_RTP_ENTRY_FLAG_FOREACH_ENGINE, this check would fail.
>
>> +					    "GT WA %s has an invalid engine class rule setup",
>> +					    param->name);
>
> I think it would be simpler to replace this with something like
>
>   KUNIT_EXPECT_TRUE(param->rules[i].match_type != XE_RTP_MATCH_ENGINE_CLASS ||
>                     param->flags & XE_RTP_ENTRY_FLAG_FOREACH_ENGINE)
>
> And another one like that for XE_RTP_MATCH_NOT_ENGINE_CLASS.
>
> Since we are using entries as parameters of the test, the RTP entry name
> will be displayed in the kunit output, so I think we can skip the
> explanatory string.
>
>> +	}
>> +}
>> +
>> +static int xe_rtp_table_test_init(struct kunit *test)
>> +{
>> +	struct xe_device *xe;
>> +	struct device *dev;
>> +	int ret;
>> +
>> +	dev = drm_kunit_helper_alloc_device(test);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, dev);
>> +
>> +	xe = xe_kunit_helper_alloc_xe_device(test, dev);
>> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xe);
>> +
>> +	/* Initialize an empty device */
>> +	test->priv = NULL;
>> +
>> +	ret = xe_pci_fake_device_init(xe);
>> +	KUNIT_ASSERT_EQ(test, ret, 0);
>> +	xe->drm.dev = dev;
>> +	test->priv = xe;
>> +
>> +	return 0;
>> +}
>> +
>> +static void xe_rtp_table_test_exit(struct kunit *test)
>> +{
>> +	struct xe_device *xe = test->priv;
>> +
>> +	drm_kunit_helper_free_device(test, xe->drm.dev);
>> +}
>> +
>> +static void rtp_table_gt_test_desc(const struct xe_rtp_entry_sr *t, char *desc)
>> +{
>> +	strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
>> +}
>> +
>> +RTP_KUNIT_ARRAY_PARAM(rtp_table_gt, gt_was, rtp_table_gt_test_desc);
>> +
>> +static struct kunit_case xe_rtp_table_tests[] = {
>> +	KUNIT_CASE_PARAM(xe_rtp_table_gt_test, rtp_table_gt_gen_params),
>> +	{}
>> +};
>> +
>> +static struct kunit_suite xe_rtp_tables_test_suite = {
>> +	.name = "xe_rtp_tables_test",
>> +	.init = xe_rtp_table_test_init,
>> +	.exit = xe_rtp_table_test_exit,
>
> We do not need fake device initialization for the current test being
> done here.  We can simply drop those functions and the .init and .exit
> initializations here.
>
>> +	.test_cases = xe_rtp_table_tests,
>> +};
>> +
>> +kunit_test_suite(xe_rtp_tables_test_suite);
>> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.h b/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.h
>> new file mode 100644
>> index 000000000000..3e91dd08f854
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: GPL-2.0 AND MIT */
>> +/*
>> + * Copyright © 2026 Intel Corporation
>> + */
>> +
>> +// The "gen_params" function functionality does not work to dynamically collect the size
>> +// of the WA array, even with the new n_entries pointer. This addition builds on that framework
>> +// to allow dynamic sizing
>> +#define RTP_KUNIT_ARRAY_PARAM(name, array, get_desc)						\
>
> The only user of this macro is xe_rtp_tables_test.c, I don't think we
> need to expose it via a header.
>
> I also think RTP_TABLE_PARAM() would be a good name for the macro.
>
>> +	static const void *name##_gen_params(struct kunit *test,				\
>> +					     const void *prev, char *desc)			\
>> +	{											\
>> +		typeof((array.entries)[0]) *__next = prev ?					\
>> +			((typeof(__next))prev) + 1 : (array.entries);				\
>> +		if (!prev)									\
>> +			kunit_register_params_array(test, array.entries, array.n_entries, NULL);\
>
> Do we really need to call kunit_register_params_array()?  Looking at the
> implementation of kunit, it appears kunit_register_params_array() is
> meant to be used together with kunit_array_gen_params() as the parameter
> generator.  Here we are using our own custom generator.
>
> By the way, I wonder why kunit itself is doing that for
> KUNIT_ARRAY_PARAM() and KUNIT_ARRAY_PARAM_DESC().  Commit b820b9077b7f
> ("kunit: Enable direct registration of parameter arrays to a KUnit
> test") is the one that added kunit_register_params_array() and says the
> following:
>
>     "The arrays passed to KUNIT_ARRAY_PARAM(,DESC) will also be registered to
>     the parameterized test context for consistency as well as for higher
>     availability of the parameter count that will be used for outputting a KTAP
>     test plan for a parameterized test."
>
> , however, the registered data is only used if the generator is
> kunit_array_gen_params() anyway, so the registration in those macros
> seems useless.
>
> Bottom line: I think we can simply drop this call.
>
>> +		if (__next - array.entries < array.n_entries) {					\
>> +			void (*__get_desc)(typeof(__next), char *) = get_desc;			\
>> +			if (__get_desc)								\
>> +				__get_desc(__next, desc);					\
>
> Since we are writing this macro specifically for RTP entries, I don't
> think we need a "get description" function: we can use __next->name
> directly here.
>
> Also, for the description, I think we could also include the name of the
> variable, for completeness.  Today we are only adding gt_was, but in the
> future we could add others.  Using the variable name in the parameter
> name here should make it easy to correctly spot the offending RTP entry
> in a expectation failure.
>
> Hence, here is what I would suggest for such a macro (tested locally):
>
>     #define RTP_TABLE_PARAM(array)								\

Maybe we should use "table" for the parameter here, since we are
expecting xe_rtp_table or xe_rtp_table_sr instances?

--
Gustavo Sousa

>     	static const void *array##_gen_params(struct kunit *test,				\
>     					     const void *prev, char *desc)			\
>     	{											\
>     		typeof((array.entries)[0]) *__next = prev ?					\
>     			((typeof(__next))prev) + 1 : (array.entries);				\
>     		if (__next - array.entries < array.n_entries) {					\
>     			scnprintf(desc, KUNIT_PARAM_DESC_SIZE, #array "/%s", __next->name);	\
>     			return __next;								\
>     		}										\
>     		return NULL;									\
>     	}
>     
>
>
> --
> Gustavo Sousa
>
>> +			return __next;								\
>> +		}										\
>> +		return NULL;									\
>> +	}
>> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
>> index 39c507edb8ce..285072fe0a47 100644
>> --- a/drivers/gpu/drm/xe/xe_wa.c
>> +++ b/drivers/gpu/drm/xe/xe_wa.c
>> @@ -130,7 +130,7 @@
>>  __diag_push();
>>  __diag_ignore_all("-Woverride-init", "Allow field overrides in table");
>>  
>> -static const struct xe_rtp_table_sr gt_was = XE_RTP_TABLE_SR(
>> +VISIBLE_IF_KUNIT const struct xe_rtp_table_sr gt_was = XE_RTP_TABLE_SR(
>>  	/* Workarounds applying over a range of IPs */
>>  
>>  	{ XE_RTP_NAME("14011060649"),
>> diff --git a/drivers/gpu/drm/xe/xe_wa.h b/drivers/gpu/drm/xe/xe_wa.h
>> index a5f7d33c1b32..17dff615e507 100644
>> --- a/drivers/gpu/drm/xe/xe_wa.h
>> +++ b/drivers/gpu/drm/xe/xe_wa.h
>> @@ -6,6 +6,7 @@
>>  #ifndef _XE_WA_H_
>>  #define _XE_WA_H_
>>  
>> +#include <kunit/visibility.h>
>>  #include "xe_assert.h"
>>  
>>  struct drm_printer;
>> @@ -24,6 +25,10 @@ void xe_wa_apply_tile_workarounds(struct xe_tile *tile);
>>  void xe_wa_device_dump(struct xe_device *xe, struct drm_printer *p);
>>  int xe_wa_gt_dump(struct xe_gt *gt, struct drm_printer *p);
>>  
>> +#if IS_ENABLED(CONFIG_DRM_XE_KUNIT_TEST)
>> +extern VISIBLE_IF_KUNIT const struct xe_rtp_table_sr gt_was;
>> +#endif
>> +
>>  /**
>>   * XE_GT_WA - Out-of-band GT workarounds, to be queried and called as needed.
>>   * @gt__: gt instance
>> -- 
>> 2.43.0