Re: [PATCH i-g-t 1/4] tests/intel/kms_frontbuffer_tracking: update the outdated fbc status checks

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

On Fri, 2026-03-27 at 00:45 +0200, Ville Syrjälä wrote:
> On Thu, Mar 26, 2026 at 11:27:47AM +0200, Jani Nikula wrote:
> > On Thu, 26 Mar 2026, Vinod Govindapillai
> > <vinod.govindapillai@intel.com> wrote:
> > > Replace the fbc status check for "mode too large for compression"
> > > wchich
> > > is no longer being set by the driver with "plane size too big"
> > > and
> > > "surface size too big" fbc status checks.
> > > 
> > > Signed-off-by: Vinod Govindapillai
> > > <vinod.govindapillai@intel.com>
> > > ---
> > >  tests/intel/kms_frontbuffer_tracking.c | 17 +++++++++++++----
> > >  1 file changed, 13 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/tests/intel/kms_frontbuffer_tracking.c
> > > b/tests/intel/kms_frontbuffer_tracking.c
> > > index c16f63199..e764c9ce1 100644
> > > --- a/tests/intel/kms_frontbuffer_tracking.c
> > > +++ b/tests/intel/kms_frontbuffer_tracking.c
> > > @@ -1664,15 +1664,23 @@ static bool
> > > fbc_stride_not_supported(void)
> > >  	char buf[128];
> > >  
> > >  	debugfs_read_crtc("i915_fbc_status", buf);
> > > -	return strstr(buf, "FBC disabled: framebuffer stride not
> > > supported\n");
> > > +	return strstr(buf, "FBC disabled: stride not
> > > supported\n");
> > >  }
> > >  
> > > -static bool fbc_mode_too_large(void)
> > > +static bool fbc_plane_size_too_big(void)
> > >  {
> > >  	char buf[128];
> > >  
> > >  	debugfs_read_crtc("i915_fbc_status", buf);
> > > -	return strstr(buf, "FBC disabled: mode too large for
> > > compression\n");
> > > +	return strstr(buf, "FBC disabled: plane size too
> > > big\n");
> > > +}
> > > +
> > > +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");
> > >  }
> > >  
> > >  static bool fbc_psr_not_possible(void)
> > > @@ -2356,7 +2364,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_mode_too_large());
> > > +		igt_require(!fbc_plane_size_too_big());
> > > +		igt_require(!fbc_surface_size_too_big());
> > 
> > I think the problem with the current implementation and this series
> > is
> > that it still keeps all those i915_fbc_status reads in place,
> > instead of
> > reading it *once* and then figuring it out.
> > 
> > You could e.g. have a function that reads i915_fbc_status, parses
> > it
> > into, I don't know, an enum on the igt side, with some handling of
> > unknown "FBC disabled: xxx" messages, and returns that.
> > 
> > And then you get that status once, and check that, instead of
> > reading
> > the file N times.
> 
> The other big problem is that the buffer size used for this is too 
> small. A while ago I noticed that it was getting chopped off so I
> have a patch in some branch that doubles it. But dunno if even that
> is sufficient on the latest platforms with more and more FBC capable
> planes. Someone should probably check that...
> 

Just to clarify, I had considered this when I was reviewing the IGT
patch for enabling FBC on planes 0, 1 and 2. The fbc statuc check for
planes do use a bigger buffer. I did not asked for a change to these
above FBC disabled status-es, as the driver is printing the disabled
status on the first line itself and existing buffer length should
cover. 

But, anyway, I will change this as well to reflect the bigger buffer
size and probably use one single declaration on lib/tests/intel_fbc.h

BR
Vinod