Re: [igt-dev] [PATCH i-g-t v5 2/7] tests/kms_frontbuffer_tracking: Improve tiling test coverage

[Petri Latvala <petri.latvala@intel.com>]

On Mon, Mar 23, 2020 at 01:46:16PM -0700, José Roberto de Souza wrote:
> Lets add some more tiling tests, if tiling is supported by FBC draw
> some rectangles and compare the CRC against benchmark if not
> supported run the test to guarantee that FBC is disabled.
> 
> This is a preparation for when kernel will allow FBC to be enabled
> without fences, so we can better test linear and Y tiled
> frontbuffers.
> 
> v2:
> - renamed tile to tiling (Ville)
> - removed *-tiling-x test (Ville)
> - improved tiling_disable_fbc_subtest documentation (Ville)
> 
> v3:
> - added new tests description
> - fixed failure in prepare_subtest()
> 
> v5:
> - not adding tiling Y tests to GENs older than 9 because it is not
> supported
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  tests/kms_frontbuffer_tracking.c | 79 +++++++++++++++++++++++++-------
>  1 file changed, 62 insertions(+), 17 deletions(-)
> 
> diff --git a/tests/kms_frontbuffer_tracking.c b/tests/kms_frontbuffer_tracking.c
> index c4deb72b..267aa631 100644
> --- a/tests/kms_frontbuffer_tracking.c
> +++ b/tests/kms_frontbuffer_tracking.c
> @@ -2170,6 +2170,23 @@ static void format_draw_subtest(const struct test_mode *t)
>  		badformat_subtest(t);
>  }
>  
> +static bool tiling_is_valid(int feature_flags, enum tiling_type tiling)
> +{
> +	if (!(feature_flags & FEATURE_FBC))
> +		return true;
> +
> +	switch (tiling) {
> +	case TILING_LINEAR:
> +		return false;
> +	case TILING_X:
> +	case TILING_Y:
> +		return true;
> +	default:
> +		igt_assert(false);
> +		return false;
> +	}
> +}
> +
>  /*
>   * slow_draw - sleep a little bit between drawing operations
>   *
> @@ -2975,45 +2992,47 @@ static void stridechange_subtest(const struct test_mode *t)
>  }
>  
>  /**
> - * tilingchange - alternate between tiled and untiled in multiple ways
> + * tiling_disable_fbc_subtest - Check if tiling is unsupported by FBC
>   *
>   * METHOD
> - *   This test alternates between tiled and untiled frontbuffers of the same
> - *   size and format through multiple different APIs: the page flip IOCTL,
> - *   normal modesets and the plane APIs.
> + *   This test alternates between a FBC supported and non-supported tiled
> + *   frontbuffers of the same size and format through multiple different
> + *   APIs: the page flip IOCTL, normal modesets and the plane APIs.
>   *
>   * EXPECTED RESULTS
> - *   FBC gets properly disabled for the untiled FB and reenabled for the
> - *   tiled FB.
> + *   FBC gets properly disabled for the non-supported tiling and reenabled for
> + *   the supported tiling.
>   *
>   * FAILURES
>   *   Bad Kernels may somehow leave FBC enabled, which can cause FIFO underruns
>   *   that lead to CRC assertion failures.
>   */
> -static void tilingchange_subtest(const struct test_mode *t)
> +static void tiling_disable_fbc_subtest(const struct test_mode *t)
>  {
> -	struct igt_fb new_fb, *old_fb;
> +	struct igt_fb new_fb, *supported_fb;
>  	struct modeset_params *params = pick_params(t);
>  	enum flip_type flip_type;
> +	struct test_mode supported_mode;
>  
> -	prepare_subtest(t, NULL);
> +	memcpy(&supported_mode, t, sizeof(*t));
> +	supported_mode.tiling = TILING_X;
> +	prepare_subtest(&supported_mode, NULL);
> +	supported_fb = params->primary.fb;
>  
> -	old_fb = params->primary.fb;
> -
> -	create_fb(t->format, params->primary.fb->width, params->primary.fb->height,
> -		  LOCAL_DRM_FORMAT_MOD_NONE, t->plane, &new_fb);
> +	create_fb(t->format, params->primary.fb->width,
> +		  params->primary.fb->height, t->tiling, t->plane, &new_fb);
>  	fill_fb(&new_fb, COLOR_PRIM_BG);
>  
>  	for (flip_type = 0; flip_type < FLIP_COUNT; flip_type++) {
>  		igt_debug("Flip type: %d\n", flip_type);
>  
> -		/* Set a buffer with no tiling. */
> +		/* Set a buffer with new tiling. */
>  		params->primary.fb = &new_fb;
>  		page_flip_for_params(params, flip_type);
>  		do_assertions(ASSERT_FBC_DISABLED);
>  
>  		/* Put FBC back in a working state. */
> -		params->primary.fb = old_fb;
> +		params->primary.fb = supported_fb;
>  		page_flip_for_params(params, flip_type);
>  		do_assertions(0);
>  	}
> @@ -3513,8 +3532,34 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
>  			igt_subtest_f("%s-stridechange", feature_str(t.feature))
>  				stridechange_subtest(&t);
>  
> -			igt_subtest_f("%s-tilingchange", feature_str(t.feature))
> -				tilingchange_subtest(&t);
> +			for (t.tiling = TILING_LINEAR; t.tiling < TILING_COUNT;
> +			     t.tiling++) {
> +				int devid = intel_get_drm_devid(drm.fd);

You can't access device information without being inside a fixture or a subtest.

> +
> +				if (t.tiling == TILING_X)
> +					continue;
> +
> +				/* Tiling Y is only supported on GEN9+ */
> +				if (!AT_LEAST_GEN(devid, 9) &&
> +				    t.tiling == TILING_Y)
> +					continue;

Don't attempt to do this. All subtests need to be statically
enumerated regardless of what devices are available and what they're
capable of.

You can check those conditions in an igt_fixture with a subtest group
to skip only the subtest(s) for that tiling mode:


igt_subtest_group {
igt_fixture {
int devid = intel_get_drm_devid(drm.fd);
igt_skip_on_f(t.tiling == TILING_X, "Can't use X tiling because of reasons\n");
igt_skip_on_f(!AT_LEAST_GEN(devid, 9) && t.tiling == TILING_Y, "Can't use Y tiling before gen9\n");
}

igt_describe(...);
igt_subtest_f(....);

...
}



-- 
Petri Latvala
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev