Re: [PATCH i-g-t v2 2/8] tests/intel/kms_frontbuffer_tracking: avoid multiple fbc status queries

[Jani Nikula <jani.nikula@intel.com>]

On Tue, 07 Apr 2026, Vinod Govindapillai <vinod.govindapillai@intel.com> wrote:
> While looking for "no fbc reasons" and decide whether to skip any specific
> fbc related test, read the debugfs fbc status only once and look for any
> skip reasons.
>
> Suggested-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> ---
>  lib/i915/intel_fbc.h                   |  8 +++++
>  tests/intel/kms_frontbuffer_tracking.c | 50 +++++++-------------------
>  2 files changed, 20 insertions(+), 38 deletions(-)
>
> diff --git a/lib/i915/intel_fbc.h b/lib/i915/intel_fbc.h
> index 8e2662824..b4596cca5 100644
> --- a/lib/i915/intel_fbc.h
> +++ b/lib/i915/intel_fbc.h
> @@ -10,6 +10,14 @@
>  
>  enum psr_mode;
>  
> +const char *const no_fbc_reasons[] = {
> +	"FBC disabled: not enough stolen memory",
> +	"FBC disabled: stride not supported",
> +	"FBC disabled: plane size too big",
> +	"FBC disabled: surface size too big",
> +	"FBC disabled: PSR1 enabled (Wa_14016291713)"
> +};

This being in a header, it gets duplicated to every file that includes
it. I'm somewhat surprised it builds, actually. In fact adding another
include of it anywhere in lib does break the build.

Basically my suggestion was to have a helper function for this, and the
strings should be hidden there. There should be only one place in the
whole IGT that matches and maps the strings. You change the strings in
kernel, you change that one place, and nothing else.

BR,
Jani.




> +
>  void intel_fbc_enable(igt_display_t *display);
>  void intel_fbc_disable(igt_display_t *display);
>  bool intel_fbc_supported(igt_crtc_t *crtc);
> diff --git a/tests/intel/kms_frontbuffer_tracking.c b/tests/intel/kms_frontbuffer_tracking.c
> index e764c9ce1..427cf3262 100644
> --- a/tests/intel/kms_frontbuffer_tracking.c
> +++ b/tests/intel/kms_frontbuffer_tracking.c
> @@ -1651,44 +1651,21 @@ static bool fbc_wait_for_compression(void)
>  	return igt_wait(fbc_is_compressing(), 2000, 1);
>  }
>  
> -static bool fbc_not_enough_stolen(void)
> +static bool fbc_found_skip_reason(void)
>  {
> -	char buf[128];
> -
> -	debugfs_read_crtc("i915_fbc_status", buf);
> -	return strstr(buf, "FBC disabled: not enough stolen memory\n");
> -}
> -
> -static bool fbc_stride_not_supported(void)
> -{
> -	char buf[128];
> -
> -	debugfs_read_crtc("i915_fbc_status", buf);
> -	return strstr(buf, "FBC disabled: stride not supported\n");
> -}
> -
> -static bool fbc_plane_size_too_big(void)
> -{
> -	char buf[128];
> +	bool found_reason = false;
> +	char fbc_status[128];
> +	int i;
>  
> -	debugfs_read_crtc("i915_fbc_status", buf);
> -	return strstr(buf, "FBC disabled: plane size too big\n");
> -}
> +	debugfs_read_crtc("i915_fbc_status", fbc_status);
>  
> -static bool fbc_surface_size_too_big(void)
> -{
> -	char buf[128];
> -
> -	debugfs_read_crtc("i915_fbc_status", buf);
> -	return strstr(buf, "FBC disabled: surface size too big\n");
> -}
> +	if (strstr(fbc_status, "FBC Enabled\n"))
> +		return false;
>  
> -static bool fbc_psr_not_possible(void)
> -{
> -	char buf[128];
> +	for (i = 0; !found_reason && i < ARRAY_SIZE(no_fbc_reasons); i++)
> +		found_reason = strstr(fbc_status, no_fbc_reasons[i]);
>  
> -	debugfs_read_crtc("i915_fbc_status", buf);
> -	return strstr(buf, "FBC disabled: PSR1 enabled (Wa_14016291713)");
> +	return found_reason;
>  }
>  
>  static bool fbc_enable_per_plane(int plane_index, igt_crtc_t *crtc)
> @@ -2362,11 +2339,8 @@ static void do_status_assertions(int flags)
>  	}
>  
>  	if (flags & ASSERT_FBC_ENABLED) {
> -		igt_require(!fbc_not_enough_stolen());
> -		igt_require(!fbc_stride_not_supported());
> -		igt_require(!fbc_plane_size_too_big());
> -		igt_require(!fbc_surface_size_too_big());
> -		igt_require(!fbc_psr_not_possible());
> +		igt_require(!fbc_found_skip_reason());
> +
>  		if (!intel_fbc_wait_until_enabled(prim_mode_params.crtc)) {
>  			igt_assert_f(intel_fbc_is_enabled(prim_mode_params.crtc, IGT_LOG_WARN),
>  				     "FBC disabled\n");

-- 
Jani Nikula, Intel