Re: [PATCH v2 4/4] drm/xe/rtp: Ensure device_oob_was only evaluates correct rules

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

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 device OOB workaround list. As well
> as checking for XE_RTP_ENGINE_CLASS(), this test also checks for rules
> involving XE_RTP_GRAPHICS() and XE_RTP_MEDIA(), as well as their
> derivatives.
>
> This test will raise expectation fails for any workarounds in the
> device_oob_was list that has an invalid rule type, preventing evaluation
> or inclusion of rules that could be applied in the wrong context.
>
> v2:
>     - Changed xe_rtp_table_dev_oob_test() to follow format of
>       xe_rtp_table_gt_test
>     - Changed device_oob_was generated params to follow format of
>       gt_was
>
> Signed-off-by: Violet Monti <violet.monti@intel.com>
> ---
>  drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c | 21 +++++++++++++++++++
>  drivers/gpu/drm/xe/xe_wa.c                    |  3 ++-
>  drivers/gpu/drm/xe/xe_wa.h                    |  1 +
>  3 files changed, 24 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 eb1b3ee075c9..669163e52ebd 100644
> --- a/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
> +++ b/drivers/gpu/drm/xe/tests/xe_rtp_tables_test.c
> @@ -54,9 +54,30 @@ static void xe_rtp_table_oob_test(struct kunit *test)
>  
>  RTP_TABLE_PARAM(oob_was);
>  
> +static void xe_rtp_table_dev_oob_test(struct kunit *test)
> +{
> +	const struct xe_rtp_entry *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->rules[i].match_type != XE_RTP_MATCH_GRAPHICS_VERSION &&
> +				  entry->rules[i].match_type != XE_RTP_MATCH_GRAPHICS_VERSION_RANGE &&
> +				  entry->rules[i].match_type != XE_RTP_MATCH_GRAPHICS_VERSION_ANY_GT &&
> +				  entry->rules[i].match_type != XE_RTP_MATCH_GRAPHICS_STEP &&
> +				  entry->rules[i].match_type != XE_RTP_MATCH_MEDIA_VERSION &&
> +				  entry->rules[i].match_type != XE_RTP_MATCH_MEDIA_VERSION_RANGE &&
> +				  entry->rules[i].match_type != XE_RTP_MATCH_MEDIA_VERSION_ANY_GT &&
> +				  entry->rules[i].match_type != XE_RTP_MATCH_MEDIA_STEP);

Hm... The checks themselves look correct, so, with VISIBLE_IF_KUNIT
dropped from the extern variable declaration,

   Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>

That said, here is an optional feedback: I guess it would be easier for
the developer to spot exactly what the offending match type is if we
used one expectation macro for each? I.e.:

  enum xe_rtp_match_type match_type = entry->rules[i].match_type;
  
  KUNIT_EXPECT_NEQ(test, match_type, XE_RTP_MATCH_ENGINE_CLASS);
  KUNIT_EXPECT_NEQ(test, match_type, XE_RTP_MATCH_NOT_ENGINE_CLASS);
  ...
  KUNIT_EXPECT_NEQ(test, match_type, XE_RTP_MATCH_MEDIA_STEP);

This would also apply to the previous patches.

--
Gustavo Sousa

> +}
> +
> +RTP_TABLE_PARAM(device_oob_was);
> +
>  static struct kunit_case xe_rtp_table_tests[] = {
>  	KUNIT_CASE_PARAM(xe_rtp_table_gt_test, gt_was_gen_params),
>  	KUNIT_CASE_PARAM(xe_rtp_table_oob_test, oob_was_gen_params),
> +	KUNIT_CASE_PARAM(xe_rtp_table_dev_oob_test, device_oob_was_gen_params),
>  	{}
>  };
>  
> diff --git a/drivers/gpu/drm/xe/xe_wa.c b/drivers/gpu/drm/xe/xe_wa.c
> index 410099545f4e..635d5461f712 100644
> --- a/drivers/gpu/drm/xe/xe_wa.c
> +++ b/drivers/gpu/drm/xe/xe_wa.c
> @@ -815,10 +815,11 @@ static const struct xe_rtp_entry device_oob_was_entries[] = {
>  
>  static_assert(ARRAY_SIZE(device_oob_was_entries) == _XE_DEVICE_WA_OOB_COUNT);
>  
> -static __maybe_unused const struct xe_rtp_table device_oob_was = {
> +VISIBLE_IF_KUNIT __maybe_unused const struct xe_rtp_table device_oob_was = {
>  	.entries = device_oob_was_entries,
>  	.n_entries = ARRAY_SIZE(device_oob_was_entries),
>  };
> +EXPORT_SYMBOL_IF_KUNIT(device_oob_was);
>  
>  __diag_pop();
>  
> diff --git a/drivers/gpu/drm/xe/xe_wa.h b/drivers/gpu/drm/xe/xe_wa.h
> index 7ac6d365b65f..568e05e49bf9 100644
> --- a/drivers/gpu/drm/xe/xe_wa.h
> +++ b/drivers/gpu/drm/xe/xe_wa.h
> @@ -28,6 +28,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;
> +extern VISIBLE_IF_KUNIT __maybe_unused const struct xe_rtp_table device_oob_was;
>  #endif
>  
>  /**
> -- 
> 2.43.0