[PATCH i-g-t] tests/intel/kms_frontbuffer_tracking: Refine FBC test conditions for BMG variants

[Govindapillai, Vinod]

Hi 

Some comments.

I would split this into two patches,

1. Enable the tests support variations based on the WAs

2. Add the FBC specific WA check

On Wed, 2025-05-07 at 13:00 +0530, Mohammed Thasleem wrote:
> This update stops skipping fbc-* tests based on the BMG platform.
> Instead, tests are excluded only for specific unsupported BMG variants.
> 
> Signed-off-by: Mohammed Thasleem <mohammed.thasleem at intel.com>
> ---
>  lib/igt_kms.c                          | 30 ++++++++++++++++++++++++++
>  lib/igt_kms.h                          |  1 +
>  tests/intel/kms_frontbuffer_tracking.c |  7 +++---
>  3 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> index f3bc481f2..668da5915 100644
> --- a/lib/igt_kms.c
> +++ b/lib/igt_kms.c
> @@ -61,6 +61,7 @@
>  #include "igt_device.h"
>  #include "igt_sysfs.h"
>  #include "sw_sync.h"
> +#include "xe/xe_query.h"
>  #ifdef HAVE_CHAMELIUM
>  #include "igt_chamelium.h"
>  #endif
> @@ -7442,3 +7443,32 @@ int igt_backlight_write(int value, const char *fname,
> igt_backlight_context_t *c
>  
>  	return 0;
>  }
> +
> +/**
> + * igt_read_debugfs_wa:
> + * @drm_fd:	A drm file descriptor
> + * @check_wa:	Wa to be checked
> + */
> +bool igt_read_debugfs_wa(int drm_fd, const char *check_wa)
> +{
> +	char buf[4096];

What if there are more WAs than it can fit into 4096 characters?
IMO, you might have to dump the whole wa sysfs file and then search for the wa. Ref: igt_sysfs_get()

> +	bool found = false;
> +	char name[PATH_MAX];
> +	unsigned int xe, debugfs_fd;
> +
> +	debugfs_fd = igt_debugfs_dir(drm_fd);
> +
> +	xe_for_each_gt(drm_fd, xe) {
> +		sprintf(name, "gt%d/workarounds", xe);
> +		igt_require(igt_debugfs_exists(drm_fd, name, O_RDONLY));
> +
> +		if (igt_debugfs_simple_read(debugfs_fd, name, buf, sizeof(buf)) > 0) {
> +			if (strstr(buf, check_wa)) {
> +				found = true;

Also you may return directly from here if the wa is found and then you can avoid "found" and also
this loop might look clean

> +				break;
> +			}
> +		}
> +	}
> +
> +	return found;
> +}
> diff --git a/lib/igt_kms.h b/lib/igt_kms.h
> index 0381c82ad..cd83a56e7 100644
> --- a/lib/igt_kms.h
> +++ b/lib/igt_kms.h
> @@ -1279,5 +1279,6 @@ void igt_set_link_params(int drm_fd, igt_output_t *output,
>  			   char *link_rate, char *lane_count);
>  int igt_backlight_read(int *result, const char *fname, igt_backlight_context_t *context);
>  int igt_backlight_write(int value, const char *fname, igt_backlight_context_t *context);
> +bool igt_read_debugfs_wa(int drm_fd, const char *check_wa);
>  
>  #endif /* __IGT_KMS_H__ */
> diff --git a/tests/intel/kms_frontbuffer_tracking.c b/tests/intel/kms_frontbuffer_tracking.c
> index 0809352cb..56d6c6f74 100644
> --- a/tests/intel/kms_frontbuffer_tracking.c
> +++ b/tests/intel/kms_frontbuffer_tracking.c
> @@ -781,6 +781,7 @@
>   */
>  
>  #define TIME SLOW_QUICK(1000, 10000)
> +#define CHECK_WA "16023588340"

May be a better name for this? WA_FBC_DISABLED or something like that?

>  
>  IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and "
>  		     "its related features: FBC, PSR and DRRS");
> @@ -2610,7 +2611,7 @@ static void prepare_subtest_data(const struct test_mode *t,
>  static void prepare_subtest_screens(const struct test_mode *t)
>  {
>  	/* FBC disabled: Wa_16023588340 */
> -	igt_skip_on_f((IS_BATTLEMAGE(drm.devid) && t->feature == FEATURE_FBC),
> +	igt_skip_on_f((igt_read_debugfs_wa(drm.fd, CHECK_WA) && t->feature == FEATURE_FBC),
>  		      "FBC isn't supported on BMG\n");
>  
>  	if (t->pipes == PIPE_DUAL)
> @@ -2658,7 +2659,7 @@ static void prepare_subtest(const struct test_mode *t,
>  static void rte_subtest(const struct test_mode *t)
>  {
>  	/* FBC disabled: Wa_16023588340 */
> -	igt_skip_on_f((IS_BATTLEMAGE(drm.devid) && t->feature == FEATURE_FBC),
> +	igt_skip_on_f((igt_read_debugfs_wa(drm.fd, CHECK_WA) && t->feature == FEATURE_FBC),
>  		      "FBC isn't supported on BMG\n");
May be align the description here to the WA?

>  
>  	prepare_subtest_data(t, NULL);
> @@ -4157,7 +4158,7 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
>  			t.tiling = opt.tiling;
>  
>  			/* FBC disabled: Wa_16023588340 */
> -			igt_skip_on_f((IS_BATTLEMAGE(drm.devid) && t.feature == FEATURE_FBC),
> +			igt_skip_on_f((igt_read_debugfs_wa(drm.fd, CHECK_WA) && t.feature ==
> FEATURE_FBC),
>  				      "FBC isn't supported on BMG\n");
Same..

BR
Vinod
>  
>  			for_each_pipe(&drm.display, pipe) {