[PATCH v2 2/2] tests/intel/kms_frontbuffer_tracking: Enable the tests support variations based on the WAs

[Govindapillai, Vinod]

Hi,

Some comments, 

On Mon, 2025-05-19 at 18:01 +0530, Mohammed Thasleem wrote:
> This update stops skipping fbc-* tests support variations based on the WAs
> 
> v2: Use WA_FBC_DISABLED instead CHECK_WA. (Vinod)
>     Update igt_skip_on_f discription. (Vinod)
> 
> Signed-off-by: Mohammed Thasleem <mohammed.thasleem at intel.com>
> ---
>  tests/intel/kms_frontbuffer_tracking.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/tests/intel/kms_frontbuffer_tracking.c b/tests/intel/kms_frontbuffer_tracking.c
> index 0809352cb..aebee7048 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 WA_FBC_DISABLED "16023588340"
>  
>  IGT_TEST_DESCRIPTION("Test the Kernel's frontbuffer tracking mechanism and "
>  		     "its related features: FBC, PSR and DRRS");
> @@ -2248,6 +2249,20 @@ static void do_flush(const struct test_mode *t)
>  
>  #define ASSERT_NO_IDLE_GPU		(1 << 11)
>  
> +static void check_fbc_support(int devid, const struct test_mode *t)
> +{
> +	int wa;
> +
> +	if (IS_BATTLEMAGE(drm.devid) && t->feature == FEATURE_FBC) {
> +		wa = igt_has_intel_wa(drm.fd, WA_FBC_DISABLED);
> +
> +		if (wa == 1)
> +			igt_skip("WA has disabled FBC on BMG\n");
> +		else if (wa == -1)
> +			igt_assert_f(wa >= 0, "WA path not found on GTs\n");
> +	}
> +}
> +

I guess the main point of using the wa to check a feature is to avoid the explicit platform checks
and generally a  wa can impact on multiple platforms. IMO, we dont need to use a specific platform
check before call igt_has_intel_wa(). But then you would have to handle that this igt_has_intel_wa()
is called only for xe as "workarounds" debugfs exist in xe only. This is just a suggestion for a
generic implementation. 

Also please check comments from Kamil, "else if (wa == -1)" is not needed.

You need to add the same WA check in few other tests as well not just in the kms_frontbuffer.
kms_dirtyfb and kms_fbcon_fbt also skips FBC tests in a similar way. So you need to extend this for
those tests as well.

BR
vinod

>  static int adjust_assertion_flags(const struct test_mode *t, int flags)
>  {
>  	if (!(flags & DONT_ASSERT_FEATURE_STATUS)) {
> @@ -2610,8 +2625,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),
> -		      "FBC isn't supported on BMG\n");
> +	check_fbc_support(drm.devid, t);
>  
>  	if (t->pipes == PIPE_DUAL)
>  		enable_both_screens_and_wait(t);
> @@ -2658,8 +2672,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),
> -		      "FBC isn't supported on BMG\n");
> +	check_fbc_support(drm.devid, t);
>  
>  	prepare_subtest_data(t, NULL);
>  
> @@ -4157,8 +4170,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),
> -				      "FBC isn't supported on BMG\n");
> +			check_fbc_support(drm.devid, &t);
>  
>  			for_each_pipe(&drm.display, pipe) {
>  				if (pipe == default_pipe) {