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

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

On Thu, Sep 20, 2018 at 04:36:13PM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-07-19 às 18:03 +0300, Ville Syrjala escreveu:
> > From: Ville Syrjälä <ville.syrjala@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@gmail.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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.

I shouldn't have included CCS here, for the same reason I didn't include
planar formats in formats[]. I guess I never ran this in skl+.

> 
> 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.

Yeah, I wrote this before that patch probably.

> 
> 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.

Should be fairly reasonable.

> 
> 
> > +	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.

I prefer to assert on the first error. Then you can easily run it
under gdb and examine the full state when the failure occurs.

> 
> 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);
> > +	}
> >  }

-- 
Ville Syrjälä
Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev