[PATCH i-g-t v5 4/7] tests/kms_addfb_basic: fix x-tiled tests for case when there is no x-tile

[Cavitt, Jonathan]

-----Original Message-----
From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Clint Taylor
Sent: Wednesday, December 18, 2024 1:37 PM
To: igt-dev at lists.freedesktop.org
Subject: [PATCH i-g-t v5 4/7] tests/kms_addfb_basic: fix x-tiled tests for case when there is no x-tile
> 
> From: Juha-pekka Heikkila <juha-pekka.heikkila at intel.com>
> 
> On Xe3 display no more support x-tile and will disable such framebuffers
> 
> Signed-off-by: Juha-pekka Heikkila <juha-pekka.heikkila at intel.com>
> Signed-off-by: Clint Taylor <Clinton.A.Taylor at intel.com>
> ---
>  tests/kms_addfb_basic.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> index b22818592..3ba87117b 100644
> --- a/tests/kms_addfb_basic.c
> +++ b/tests/kms_addfb_basic.c
> @@ -217,6 +217,12 @@ static int legacy_addfb(int fd, struct drm_mode_fb_cmd *arg)
>  	return err;
>  }
>  
> +static int addfb_expected_ret(igt_display_t *disp, struct drm_mode_fb_cmd2 *f)
> +{
> +	return igt_display_has_format_mod(disp, f->pixel_format,
> +					  f->modifier[0]) ? 0 : -1;
> +}
> +
>  static void invalid_tests(int fd)
>  {
>  	struct drm_mode_fb_cmd2 f = {};
> @@ -735,8 +741,10 @@ static void addfb25_tests(int fd)
>  		igt_describe("Check if addfb2 call works for x-tiling");
>  		igt_subtest("addfb25-x-tiled-legacy") {
>  			f.modifier[0] = I915_FORMAT_MOD_X_TILED;
> -			do_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &f);
> -			do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id);
> +			igt_assert_eq(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f),
> +				      addfb_expected_ret(&display, &f));
> +			if (!addfb_expected_ret(&display, &f))
> +				do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id);

If I'm understanding correctly, what we were doing before was we were performing
a DRM_IOCTL_MODE_ADDFB2, then subsequently clearing the action with a
DRM_IOCTL_MODE_RMFB.  However, now that we occasionally expect the first call
to report being unsupported, we no longer want to perform the RMFB unconditionally
as it is supposed to mirror the ADDFB2 call prior, and in the unsupported case, the
latter does not need RMFB to clear it because it did not run.

It might read better if we did this slightly differently.  Maybe something like:
"""
		igt_subtest("addfb25-x-tiled-legacy") {
			f.modifier[0] = I915_FORMAT_MOD_X_TILED;
			if (addfb_expected_ret(&display, &f)) { /* x-tile unsupported */
				igt_asser_eq(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f), -1);
			} else {
				do_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &fd);
				do_ioctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id);
			}
			...
"""
Though this is just a suggestion.  I won't block on it.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
-Jonathan Cavitt

>  			f.fb_id = 0;
>  		}
>  
> @@ -756,12 +764,6 @@ static void addfb25_tests(int fd)
>  		gem_close(fd, gem_bo);
>  }
>  
> -static int addfb_expected_ret(igt_display_t *disp, struct drm_mode_fb_cmd2 *f)
> -{
> -	return igt_display_has_format_mod(disp, f->pixel_format,
> -					  f->modifier[0]) ? 0 : -1;
> -}
> -
>  static void addfb25_ytile(int fd)
>  {
>  	struct drm_mode_fb_cmd2 f = {};
> @@ -1012,8 +1014,6 @@ igt_main
>  
>  	master_tests(fd);
>  
> -	addfb25_tests(fd);
> -
>  	tiling_tests(fd);
>  
>  	igt_subtest_group {
> @@ -1025,6 +1025,8 @@ igt_main
>  		igt_fixture
>  			igt_require_intel(fd);
>  
> +		addfb25_tests(fd);
> +
>  		addfb25_ytile(fd);
>  
>  		addfb25_4tile(fd);
> -- 
> 2.25.1
> 
>