Re: [PATCH i-g-t v4 4/8] XE3: tests/kms_addfb_basic: fix x-tiled tests for case when there is no x-tile

[Matt Roper <matthew.d.roper@intel.com>]

On Fri, Dec 06, 2024 at 01:27:44PM -0800, Clint Taylor wrote:
> From: "Heikkila, Juha-pekka" <juha-pekka.heikkila@intel.com>
> 
> On Xe3 display no more support x-tile and will disable such framebuffers
> 
> Signed-off-by: Heikkila, Juha-pekka <juha-pekka.heikkila@intel.com>

I think you were going to fixup the name ordering here since JP gave an
okay for that on the last series, right?  "Last, First" causes problems
for git-send-email and such.


> Signed-off-by: Clint Taylor <Clinton.A.Taylor@intel.com>
> Signed-off-by: Juha-pekka Heikkila <juha-pekka.heikkila@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);
>  			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);
> +

Doesn't moving this here prevent this whole batch of tests from running
on non-Intel devices?  Some of the tests here shouldn't be
Intel-specific as far as I know.  Should we rather just be adding
igt_require_intel(fd) to the specific subtest you're modifying?


Matt

>  		addfb25_ytile(fd);
>  
>  		addfb25_4tile(fd);
> -- 
> 2.25.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation