Re: [PATCH i-g-t 1/2] lib/igt_fb: Add IGT_FORMAT_MOD_PREFERRED helper modifier

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Apr 16, 2026 at 03:37:20PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Den 2026-04-13 kl. 20:06, skrev Ville Syrjälä:
> > On Mon, Apr 13, 2026 at 10:20:05PM +0530, Jeevan B wrote:
> >> Add a new IGT‑internal framebuffer modifier,
> >> IGT_FORMAT_MOD_PREFERRED, allowing tests to request the preferred
> >> non‑linear tiling for a platform without hardcoding
> >> driver‑specific modifiers.
> >>
> >> The modifier is resolved in igt_fb_resolve_modifier() using simple
> >> platform rules (Intel, VC4, fallback to linear) and is never passed to
> >> the kernel. It is defined outside the valid DRM modifier range and used
> >> only as an internal sentinel.
> >>
> >> This provides a driver‑agnostic way for tests to pick an optimal
> >> modifier, avoiding per‑test hacks.
> >>
> >> Signed-off-by: Jeevan B <jeevan.b@intel.com>
> >> ---
> >>  lib/igt_fb.c | 27 ++++++++++++++++++++++++++-
> >>  lib/igt_fb.h | 21 +++++++++++++++++++++
> >>  2 files changed, 47 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> >> index fa9220953..9c3d0338b 100644
> >> --- a/lib/igt_fb.c
> >> +++ b/lib/igt_fb.c
> >> @@ -790,6 +790,29 @@ static int fb_num_planes(const struct igt_fb *fb)
> >>  	return num_planes;
> >>  }
> >>  
> >> +static uint64_t igt_fb_resolve_modifier(int fd, uint64_t modifier)
> >> +{
> >> +	if (modifier != IGT_FORMAT_MOD_PREFERRED)
> >> +		return modifier;
> >> +
> >> +	if (is_xe_device(fd)) {
> >> +		return I915_FORMAT_MOD_4_TILED;
> >> +	} else if (is_i915_device(fd)) {
> > 
> > That is wrong for tgl/adl. I think i915 and xe need to use
> > the same logic here.
> > 
> >> +		int ver = intel_display_ver(intel_get_drm_devid(fd));
> >> +
> >> +		if (ver >= 13)
> >> +			return I915_FORMAT_MOD_4_TILED;
> > 
> > Still borked for adl.
> > 
> >> +		else if (ver >= 9)
> >> +			return I915_FORMAT_MOD_Y_TILED;
> > 
> > I don't think we want Y tile, at least on skl class hardware.
> > It has much higher watermark requirements and could therefore
> > prevent some tests from working.
> > 
> >> +		else
> >> +			return I915_FORMAT_MOD_X_TILED;
> > 
> > I'm thinking we may want to prefer X tile on all the hardware
> > that still has it. Though I suppose anything that has tile4 also
> > has pretty large DDBs, so tile4 might be fine. Shrug.
> > 
> > 
> > Anyways, this should perhaps just be something simple like
> > 
> > if (display_has_format_mod(XRGB8888, X_TILE))
> > 	return X_TILE;
> > if (display_has_format_mod(XRGB8888, 4_TILE))
> > 	return 4_TILE;
> > return LINEAR;
> > 
> > Doesn't even need any driver checks.
> > 
> > 
> > Hmm. We might still prefer fenced GGTT access for X tile, so
> > it might actually end up slowing down some things. We may
> > need to change the policy to prefer a different method for
> > tiled<->linear conversion even for X tile. It would be good
> > get actual numbers on the overall execution time for whatever
> > we choose here vs. LINEAR...
> > 
> >> +	} else if (is_vc4_device(fd))
> >> +		return DRM_FORMAT_MOD_BROADCOM_VC4_T_TILED;
> >> +
> >> +	/* For AMD, nouveau, and any other driver, fall back to linear. */
> >> +	return DRM_FORMAT_MOD_LINEAR;
> >> +}
> >> +
> >>  void igt_init_fb(struct igt_fb *fb, int fd, int width, int height,
> >>  		 uint32_t drm_format, uint64_t modifier,
> >>  		 enum igt_color_encoding color_encoding,
> >> @@ -803,7 +826,7 @@ void igt_init_fb(struct igt_fb *fb, int fd, int width, int height,
> >>  
> >>  	fb->width = width;
> >>  	fb->height = height;
> >> -	fb->modifier = modifier;
> >> +	fb->modifier = igt_fb_resolve_modifier(fd, modifier);
> > 
> > I think it should have been resolved long before that.
> > 
> > AFAICS you could just add a display->preferred_modifier, figure out
> > what that is during display init, and then switch tests to use that.
> > No need for this magic IGT_FORMAT_MOD_PREFERRED at all then I think.
> > 
> > And I'm thinking we might also want to print it out in the debug logs.
> > 
> > And for some extra fun we could add a debug knob (maybe env variable,
> > or a standard igt command line parameter) to force a specific modifier.
> > This would allow running the test with a specific modifier, even if
> > the test doesn't have explicit support for it.
> > 
> > The downside of using !LINEAR is that we will now depend even
> > more on all the magic stuff to convert between linear and tiled
> > for software rendering. So we may end up running more code under
> > the hood in most tests. I suppose we just have to hope that code
> > doesn't have too many bugs.
> 
> The reason I chose IGT_FORMAT_MOD_PREFERRED is that is's a simple change
> showing intention in the test, you don't care what the modifier is, just
> that it's reasonably fast
> 
> On some drivers like nouveau, and maybe AMD the preferred modifier may
> change depending on width/height/format, so there's no such thing as a
> single display->preferred_modifier.
> 
> See #DRM_FORMAT_MOD_NVIDIA_BLOCK_LINEAR_2D.
> 
> The only place to resolve it, is during FB creation, hence the extra parameter.

display->preferred_modifier wouldn't prevent that approach.

> 
> >>  	fb->drm_format = drm_format;
> >>  	fb->fd = fd;
> >>  	fb->num_planes = fb_num_planes(fb);
> >> @@ -5266,6 +5289,8 @@ void igt_format_array_fill(uint32_t **formats_array, unsigned int *count,
> >>  const char *igt_fb_modifier_name(uint64_t modifier)
> >>  {
> >>  	switch (modifier) {
> >> +	case IGT_FORMAT_MOD_PREFERRED:
> >> +		return "preferred";
> >>  	case DRM_FORMAT_MOD_LINEAR:
> >>  		return "linear";
> >>  	case I915_FORMAT_MOD_X_TILED:
> >> diff --git a/lib/igt_fb.h b/lib/igt_fb.h
> >> index 8e5907dab..5380b3dfa 100644
> >> --- a/lib/igt_fb.h
> >> +++ b/lib/igt_fb.h
> >> @@ -50,6 +50,27 @@ typedef struct _igt_crc igt_crc_t;
> >>   */
> >>  #define IGT_FORMAT_FLOAT fourcc_code('I', 'G', 'F', 'x')
> >>  
> >> +/**
> >> + * IGT_FORMAT_MOD_PREFERRED:
> >> + *
> >> + * Modifier used by tests to request the best tiling format for the
> >> + * current device. The actual modifier is selected inside igt_init_fb(),
> >> + * so tests don’t need to handle driver-specific details.
> >> + *
> >> + * Selection rules:
> >> + *  - Intel Xe:              4_TILED
> >> + *  - Intel i915 (gen ≥13):  4_TILED
> >> + *  - Intel i915 (gen 9–12): Y_TILED
> >> + *  - Intel i915 (gen <9):   X_TILED
> >> + *  - Broadcom VC4:          T_TILED
> >> + *  - Others:                LINEAR (fallback)
> >> + *
> >> + * Note: This is a special placeholder value and must never be sent to
> >> + * the kernel.
> >> + */
> >> +#define IGT_FORMAT_MOD_PREFERRED  UINT64_C(0xfffffffffffffffe)
> >> +
> >> +
> >>  #define IGT_FORMAT_FMT "%c%c%c%c(0x%08x)"
> >>  #define IGT_FORMAT_ARGS(f) ((f) >> 0) & 0xff, ((f) >> 8) & 0xff, \
> >>  		((f) >> 16) & 0xff, ((f) >> 24) & 0xff, (f)
> >> -- 
> >> 2.43.0
> > 
> 
> Kind regards,
> ~Maarten Lankhorst

-- 
Ville Syrjälä
Intel