Re: [PATCH 3/4] drm/xe/rtp: Add OOB WAs rule checking

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

Same comments from previous patch about commit title and body applies
here.

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

> This commit builds on the implementation of the GT WA testing, increasing
> the scope of testing to include the OOB workaround list. The added test
> checks for workarounds with an engine_class rule, and raises an expectation
> failure if any are found. Unlike the GT workarounds, there are no flags
> within this workaround list, so all engine_class rules will fail.
>
> This test prevents future implementation of rules that would apply an
> engine_class rule without an active engine in context, preventing rule
> errors or mishaps.
>
> Signed-off-by: Violet Monti <violet.monti@intel.com>
> ---
>  drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c | 20 +++++++++++++++++++
>  drivers/gpu/drm/xe/xe_wa.c                    |  2 +-
>  drivers/gpu/drm/xe/xe_wa.h                    |  1 +
>  3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c b/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
> index 100cac379b08..eae3262775bb 100644
> --- a/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
> @@ -24,6 +24,18 @@ static void xe_rtp_table_gt_test(struct kunit *test)
>  	}
>  }
>  
> +static void xe_rtp_table_oob_test(struct kunit *test)
> +{
> +	const struct xe_rtp_entry *param = test->param_value;
> +
> +	for (int i = 0; i < param->n_rules; i++) {
> +		KUNIT_EXPECT_NE_MSG(test, param->rules[i].match_type,
> +				    XE_RTP_MATCH_ENGINE_CLASS,
> +				    "OOB_WA %s has an engine class rule",
> +				    param->name);

Like with the previous patch, I think we could simplify by using
something like

  KUNIT_EXPECT_NE(test, param->rules[i].match_type,
                  XE_RTP_MATCH_ENGINE_CLASS)

> +	}
> +}
> +
>  static int xe_rtp_table_test_init(struct kunit *test)
>  {
>  	struct xe_device *xe;
> @@ -61,8 +73,16 @@ static void rtp_table_gt_test_desc(const struct xe_rtp_entry_sr *t, char *desc)
>  
>  RTP_KUNIT_ARRAY_PARAM(rtp_table_gt, gt_was, rtp_table_gt_test_desc);
>  
> +static void rtp_table_oob_test_desc(const struct xe_rtp_entry *t, char *desc)
> +{
> +	strscpy(desc, t->name, KUNIT_PARAM_DESC_SIZE);
> +}
> +
> +RTP_KUNIT_ARRAY_PARAM(rtp_table_oob, oob_was, rtp_table_oob_test_desc);
> +
>  static struct kunit_case xe_rtp_table_tests[] = {
>  	KUNIT_CASE_PARAM(xe_rtp_table_gt_test, rtp_table_gt_gen_params),
> +	KUNIT_CASE_PARAM(xe_rtp_table_oob_test, rtp_table_oob_gen_params),

Yeah.  It is unfortunate that we need 2 different functions here.  I think
in the future we could try to have a single way of expressing RTP entries
instead of having two different struct types (xe_rtp_entry vs
xe_rtp_entry_sr).  That would allow us to unify code specific for
processing rules in those entries.

--
Gustavo Sousa

>  	{}
>  };
>  
> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
> index 285072fe0a47..5744bd3f8be9 100644
> --- a/drivers/gpu/drm/xe/xe_wa.c
> +++ b/drivers/gpu/drm/xe/xe_wa.c
> @@ -802,7 +802,7 @@ static const struct xe_rtp_entry oob_was_entries[] = {
>  
>  static_assert(ARRAY_SIZE(oob_was_entries)  == _XE_WA_OOB_COUNT);
>  
> -static __maybe_unused const struct xe_rtp_table oob_was = {
> +VISIBLE_IF_KUNIT __maybe_unused const struct xe_rtp_table oob_was = {
>  	.entries = oob_was_entries,
>  	.n_entries = ARRAY_SIZE(oob_was_entries),
>  };
> diff --git a/drivers/gpu/drm/xe/xe_wa.h b/drivers/gpu/drm/xe/xe_wa.h
> index 17dff615e507..7ac6d365b65f 100644
> --- a/drivers/gpu/drm/xe/xe_wa.h
> +++ b/drivers/gpu/drm/xe/xe_wa.h
> @@ -27,6 +27,7 @@ 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;
> +extern VISIBLE_IF_KUNIT __maybe_unused const struct xe_rtp_table oob_was;
>  #endif
>  
>  /**
> -- 
> 2.43.0