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

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

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);
> +
> +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