Re: [PATCH v2 2/4] drm/xe/rtp: Ensure gt_was doesn't evaluate rules with engine types

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

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

> Violet Monti <violet.monti@intel.com> writes:
>
>> 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 XEP_RTP_ENGINE_CLASS() rules. If a workaround does have one of
>> these rules, the workaround is then checked for the "FOREACH_ENGINE" flag,
>> which ensures the workaround is implemented properly.
>>
>> The result of this test is an expectation failure if a workaround has an
>> improper XE_RTP_ENGINE_CLASS() rule setup, and aims to prevent future
>> issues of gt_was workarounds being applied without proper contexts.
>>
>> v2:
>>     - Moved contents of xe_rtp_tables_test.h to .c and removed file
>>     - Renamed macro RTP_KUNIT_ARRAY_PARAM to RTP_TABLE_PARAM
>>     - Removed unnecessary functions and iterative components from
>>       generated _gen_params functions and implemented usage of table
>>       name and WA number as entry name
>>     - Condensed xe_rtp_table_gt_test() to use KUNIT_EXPECT_TRUE with no
>>       message statement
>>     - Removed xe_rtp_table_test_init() and xe_rtp_table_test_exit() as
>>       fake device initialization is not necessary
>>
>> 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 | 55 +++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_wa.c                    |  3 +-
>>  drivers/gpu/drm/xe/xe_wa.h                    |  5 ++
>>  4 files changed, 63 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
>>
>> 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..01d2bc6e8aad
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
>> @@ -0,0 +1,55 @@
>> +// 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_wa.h"
>> +
>> +/**
>
> So, the macro is used only internally and simple enough that I
> personally think we don't need the comment.  But, if you think that will
> be useful, I'm fine with it too (but it does not need to be a
> kernel-doc). 
>
>> + * RTP_TABLE_PARAM() - Define test parameter generator from a RTP table.
>> + * @table: array of test parameters which includes a n_entries value
>
> The input is the RTP table, no? And then the function will generate the
> test parameters from the table.
>
>> + *
>> + * Define function @table_gen_params which uses @table to generate parameters.
>
> If this were to be processed by the kernel documentation generator, it
> would not find @table_gen_params.  Not sure about what would be the
> alternative here.  Maybe just spell out the CPP syntax here? I.e. Define
> function table##_gen_params().
>
>> + */
>> +#define RTP_TABLE_PARAM(table)						\
>> +	static const void *table##_gen_params(struct kunit *test,				\
>> +					     const void *prev, char *desc)			\
>> +	{											\
>> +		typeof((table.entries)[0]) *__next = prev ?					\
>> +			((typeof(__next))prev) + 1 : (table.entries);				\
>> +		if (__next - table.entries < table.n_entries) {					\
>> +			scnprintf(desc, KUNIT_PARAM_DESC_SIZE, #table "/%s", __next->name);	\
>> +			return __next;								\
>> +		}										\
>> +		return NULL;									\
>> +	}
>> +
>> +static void xe_rtp_table_gt_test(struct kunit *test)
>> +{
>> +	const struct xe_rtp_entry_sr *entry = test->param_value;
>> +
>> +	for (int i = 0; i < entry->n_rules; i++)
>> +		KUNIT_EXPECT_TRUE(test,
>> +				  (entry->rules[i].match_type != XE_RTP_MATCH_ENGINE_CLASS &&
>> +				   entry->rules[i].match_type != XE_RTP_MATCH_NOT_ENGINE_CLASS) ||
>> +				  entry->flags & XE_RTP_ENTRY_FLAG_FOREACH_ENGINE);
>> +}
>> +
>> +RTP_TABLE_PARAM(gt_was);

Ah, another thing: it occurred to me that gt_tunings is also applicable
for xe_rtp_table_gt_test, so we could also add
RTP_TABLE_PARAM(gt_tunnings) here and add a new KUNIT_CASE_PARAM()
below.

I think it would be fine to add this to this same patch and reword it a
bit.  But if you prefer to send it as a separate patch, that would be
fine as well.

--
Gustavo Sousa

>> +
>> +static struct kunit_case xe_rtp_table_tests[] = {
>> +	KUNIT_CASE_PARAM(xe_rtp_table_gt_test, gt_was_gen_params),
>> +	{}
>> +};
>> +
>> +static struct kunit_suite xe_rtp_tables_test_suite = {
>> +	.name = "xe_rtp_tables_test",
>> +	.test_cases = xe_rtp_table_tests,
>> +};
>> +
>> +kunit_test_suite(xe_rtp_tables_test_suite);
>> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
>> index b9d9fe0801aa..1a1e04215f21 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"),
>> @@ -307,6 +307,7 @@ static const struct xe_rtp_table_sr gt_was = XE_RTP_TABLE_SR(
>>  	  XE_RTP_ACTIONS(SET(GUC_INTR_CHICKEN, DISABLE_SIGNALING_ENGINES))
>>  	},
>>  );
>> +EXPORT_SYMBOL_IF_KUNIT(gt_was);
>>  
>>  static const struct xe_rtp_table_sr engine_was = XE_RTP_TABLE_SR(
>>  	/* Workarounds applying over a range of IPs */
>> 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;
>
> The VISIBLE_IF_KUNIT here is unnecessary and doesn't make much sense:
> that macro is about adding or not the "static" keyword in the
> declaration and should only be part of the variable definition in the .c
> file.
>
>
> --
> Gustavo Sousa
>
>> +#endif
>> +
>>  /**
>>   * XE_GT_WA - Out-of-band GT workarounds, to be queried and called as needed.
>>   * @gt__: gt instance
>> -- 
>> 2.43.0