[igt-dev] [PATCH i-g-t 04/25] tests/kms_addfb_basic: Check that addfb2 accepts/rejects the expected formats

[Paulo Zanoni]

Em Qui, 2018-07-19 às 18:03 +0300, Ville Syrjala escreveu:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Try to create a bunch of fbs with different formats/modfifiers and
> make sure sure addfb2 accepts/rejects them in accordance with what
> the plane IN_FORMATS blobifiers are advertizing.
> 
> We only check "easy" formats (ie. RGB/C8, no YUV/planar etc.). We
> also assume that one can always create a 64x64 fb with stride of
> 64*8 bytes.
> 
> v2: Skip when blobifiers aren't supported

Some trivial rebasing required.

More below.

> 
> Cc: Ulrich Hecht <ulrich.hecht+renesas at gmail.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  tests/kms_addfb_basic.c | 107
> ++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 104 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> index 7d8852f02003..9e33d00346e0 100644
> --- a/tests/kms_addfb_basic.c
> +++ b/tests/kms_addfb_basic.c
> @@ -41,6 +41,97 @@
>  uint32_t gem_bo;
>  uint32_t gem_bo_small;
>  
> +static const uint64_t modifiers[] = {
> +	DRM_FORMAT_MOD_LINEAR,
> +	I915_FORMAT_MOD_X_TILED,
> +	I915_FORMAT_MOD_Y_TILED,
> +	I915_FORMAT_MOD_Yf_TILED,
> +	I915_FORMAT_MOD_Y_TILED_CCS,
> +	I915_FORMAT_MOD_Yf_TILED_CCS,

For the buffers that support CCS, I see the test failing with the
Kernel complaining that we're not passing 2 planes (since .num_planes=2
in ccs_formats from intel_display.c). It looks like igt needs to craft
the aux buffer too.

More below.

> +};
> +
> +static const uint32_t formats[] = {
> +	DRM_FORMAT_C8,
> +
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_BGR565,
> +
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_BGR888,
> +
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_RGBX8888,
> +	DRM_FORMAT_BGRX8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_RGBA8888,
> +	DRM_FORMAT_BGRA8888,
> +
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_RGBX1010102,
> +	DRM_FORMAT_BGRX1010102,
> +	DRM_FORMAT_ARGB2101010,
> +	DRM_FORMAT_ABGR2101010,
> +	DRM_FORMAT_RGBA1010102,
> +	DRM_FORMAT_BGRA1010102,
> +};
> +
> +#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)
> +
> +/*
> + * make sure addfb2 accepts/rejects every format/modifier
> + * in accordance with the plane IN_FORMATS properties.
> + */
> +static void expected_formats(igt_display_t *display)
> +{
> +	int fd = display->drm_fd;
> +	uint32_t handle;
> +
> +	igt_require(display->format_mod_count);

After patch 01, this is really not skipping a lot.

Perhaps it would be better to igt_require(plane->modifiers) if you
implement my suggestion from patch 01? :)

> +
> +	handle = gem_create(fd, 64*8*64);

I'm assuming you have checked that this is sane.


> +	igt_assert(handle);
> +
> +	for (int i = 0; i < ARRAY_SIZE(formats); i++) {
> +		uint32_t format = formats[i];
> +
> +		for (int j = 0; j < ARRAY_SIZE(modifiers); j++) {
> +			uint64_t modifier = modifiers[j];
> +			struct drm_mode_fb_cmd2 f = {
> +				.width = 64,
> +				.height = 64,
> +				.pixel_format = format,
> +				.handles[0] = handle,
> +				.pitches[0] = 64 * 8,
> +				.modifier[0] = modifier,
> +				.flags = DRM_MODE_FB_MODIFIERS,
> +			};
> +			int ret;
> +
> +			igt_info("Testing format " IGT_FORMAT_FMT "
> / modifier 0x%" PRIx64 "\n",
> +				 IGT_FORMAT_ARGS(format), modifier);
> +
> +			ret = drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2,
> &f);
> +
> +			if (igt_display_has_format_mod(display,
> format, modifier)) {
> +				igt_assert_eq(ret, 0);
> +				igt_assert_neq(f.fb_id, 0);
> +			} else {
> +				igt_assert_neq(ret, 0);
> +				igt_assert_eq(f.fb_id, 0);
> +			}
> +

I was fine with this until I ran the test and it failed. My problem
with this style is that once you hit one of these assertions you stop
the test, so you don't get an idea of how bad your Kernel is. You could
simply igt_info() the failures, failures++ (or false_positives++ and
false_negatives++) and then igt_assert(failures == 0) outside the loop.
This way we can see all the bad combinations in one run, without having
to fix the first problem in order to uncover the next.

Everything else looks good.

> +			drmIoctl(fd, DRM_IOCTL_MODE_RMFB, &f.fb_id);
> +		}
> +	}
> +
> +	gem_close(fd, handle);
> +}
> +
>  static void invalid_tests(int fd)
>  {
>  	struct local_drm_mode_fb_cmd2 f = {};
> @@ -539,13 +630,17 @@ static void prop_tests(int fd)
>  
>  }
>  
> -int fd;
> +static igt_display_t display;
> +static int fd;
>  
>  igt_main
>  {
> -	igt_fixture
> +	igt_fixture {
>  		fd = drm_open_driver_master(DRIVER_ANY);
>  
> +		igt_display_init(&display, fd);
> +	}
> +
>  	invalid_tests(fd);
>  
>  	pitch_tests(fd);
> @@ -560,6 +655,12 @@ igt_main
>  
>  	prop_tests(fd);
>  
> -	igt_fixture
> +	igt_subtest("expected-formats")
> +		expected_formats(&display);
> +
> +	igt_fixture {
> +		igt_display_fini(&display);
> +
>  		close(fd);
> +	}
>  }