[igt-dev] [PATCH i-g-t 2/2] tests/kms_chamelium: Fix *-cmp-random and *-crc-random tests.

[Paul Kocialkowski]

Hi,

On Thu, 2019-04-04 at 15:42 +0200, Maarten Lankhorst wrote:
> Op 04-04-2019 om 14:14 schreef Paul Kocialkowski:
> > Hi,
> > 
> > Le mardi 02 avril 2019 à 21:12 +0200, Maarten Lankhorst a écrit :
> > > The random tests allowed potentially invalid things:
> > > - 1x1 fb's to be created. Force a minimum of 32 so a scaled
> > >   plane will be at least 16x16. This will fix scaled/planar format
> > >   support in i915 and avoid a div by zero when calculating a value
> > >   modulo h/2 and w/2.
> > > - Downscaling to any amount, restrict it to 2x to make the test pass.
> > > - Some hw may not allow scaling, in those cases we should fallback
> > >   to no scaling at all.
> > > - Attempting to configure a minimum of 4 planes, instead of a maximum.
> > >   This fails with a null pointer deref if the hw doesn't have 4
> > >   configurable overlay planes.
> > Thanks for your work, it's nice to see improvements and fixes for the
> > shortcomings of my initial implementation. Most of it looks good to me,
> > but I have some reservations about how we should handle hw-specific
> > limits, see comments below.
> > 
> > > Cc: Paul Kocialkowski <paul.kocialkowski at bootlin.com>
> > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > ---
> > >  tests/kms_chamelium.c | 195 ++++++++++++++++++++++++++++--------------
> > >  1 file changed, 131 insertions(+), 64 deletions(-)
> > > 
> > > diff --git a/tests/kms_chamelium.c b/tests/kms_chamelium.c
> > > index 2dc1049d2dda..5e5c68ccf879 100644
> > > --- a/tests/kms_chamelium.c
> > > +++ b/tests/kms_chamelium.c
> > > @@ -792,26 +792,53 @@ static void randomize_plane_format_stride(igt_plane_t *plane,
> > >  }
> > >  
> > >  static void randomize_plane_dimensions(drmModeModeInfo *mode,
> > > -				       uint32_t *width, uint32_t *height,
> > > -				       uint32_t *src_w, uint32_t *src_h,
> > > -				       uint32_t *src_x, uint32_t *src_y,
> > > -				       uint32_t *crtc_w, uint32_t *crtc_h,
> > > -				       int32_t *crtc_x, int32_t *crtc_y,
> > > -				       bool allow_scaling)
> > > +				       uint32_t *width, uint32_t *height)
> > >  {
> > > -	double ratio;
> > > +	/*
> > > +	 * Randomize width and height in the mode dimensions range.
> > > +	 *
> > > +	 * Restrict to a min of 32, this way src_w/h are always at least 16,
> > > +	 * the minimum for scaled planar YUV on i915.
> > > +	 */
> > Mhh, I don't really like the idea of constraining the tests to avoid
> > hitting hardware-specific limitations. Since the test is not supposed
> > to be tied to a driver in particular, trying to handle these limits
> > explicitly feels like a slippery slope. I believe it should be up to
> > the kernel to report these limits, not for userspace to have them
> > hardcoded. (Although I would be lying if I said that I didn't tailor
> > the test explicitly for my use case (with VC4).)
> 
> Some of the limitations are not known initially, for example min size may
> change depending on what tiling mode is used, or whether scaling is active.
> All those constraints make it harder to 

Mhh I see, so doing this dynamically will probably over-complexify the
process if we need to take all that into.

> > So here is a proposal: let's have an initial function that probes what
> > the hardware can and can't do in terms of plane min/max size and
> > up/down scaling. I would expect that i915 rejects commits where the
> > plane dimensions and such are off-limits (maybe it's also in the props
> > ranges directly), so we can test just that early and constrain the test
> > based on that info later on, so we keep the code generic and avoid
> > comments about why driver/hardware x/y needs to do this or that.
> > 
> > What do you think?
> > 
> > > +	*width = max((rand() % mode->hdisplay) + 1, 32);
> > > +	*height = max((rand() % mode->vdisplay) + 1, 32);
> > Testing a small plane feels like a case we should consider if the
> > hardware is supposed to be able to do it.
> > 
> > I'm not opposed to having "reasonable" limits though (nobody needs a
> > 1x1 plane for anything), but maybe that should be e.g. 8x8 rather than
> > 32x32.
> 
> This could be 2x2 for !i915, if you want to display a 1x1 pixel onscreen. src_w = width - (src_x = rand() % (w / 2))
> 
> As long as i915 limits are a minimum of 32x32 for src w/h all tests will work.

Ah sorry, I didn't make myself clear: I don't think 1x1 is something we
want to test, but 8x8 is where I feel the sane limit should be.

> > > +}
> > > +
> > > +static void configure_plane(igt_plane_t *plane, uint32_t src_w, uint32_t src_h,
> > > +			    uint32_t src_x, uint32_t src_y, uint32_t crtc_w,
> > > +			    uint32_t crtc_h, int32_t crtc_x, int32_t crtc_y,
> > > +			    struct igt_fb *fb)
> > > +{
> > > +	igt_plane_set_fb(plane, fb);
> > >  
> > > -	/* Randomize width and height in the mode dimensions range. */
> > > -	*width = (rand() % mode->hdisplay) + 1;
> > > -	*height = (rand() % mode->vdisplay) + 1;
> > > +	igt_plane_set_position(plane, crtc_x, crtc_y);
> > > +	igt_plane_set_size(plane, crtc_w, crtc_h);
> > > +
> > > +	igt_fb_set_position(fb, plane, src_x, src_y);
> > > +	igt_fb_set_size(fb, plane, src_w, src_h);
> > > +}
> > > +
> > > +static void randomize_plane_coordinates(data_t *data, igt_plane_t *plane,
> > > +					drmModeModeInfo *mode,
> > > +					struct igt_fb *fb,
> > > +					uint32_t *src_w, uint32_t *src_h,
> > > +					uint32_t *src_x, uint32_t *src_y,
> > > +					uint32_t *crtc_w, uint32_t *crtc_h,
> > > +					int32_t *crtc_x, int32_t *crtc_y,
> > > +					bool allow_scaling)
> > > +{
> > > +	bool is_yuv = igt_format_is_yuv(fb->drm_format);
> > > +	uint32_t width = fb->width, height = fb->height;
> > > +	double ratio;
> > > +	int ret;
> > >  
> > >  	/* Randomize source offset in the first half of the original size. */
> > > -	*src_x = rand() % (*width / 2);
> > > -	*src_y = rand() % (*height / 2);
> > > +	*src_x = rand() % (width / 2);
> > > +	*src_y = rand() % (height / 2);
> > >  
> > >  	/* The source size only includes the active source area. */
> > > -	*src_w = *width - *src_x;
> > > -	*src_h = *height - *src_y;
> > > +	*src_w = width - *src_x;
> > > +	*src_h = height - *src_y;
> > >  
> > >  	if (allow_scaling) {
> > >  		*crtc_w = (rand() % mode->hdisplay) + 1;
> > > @@ -821,17 +848,22 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
> > >  		 * Don't bother with scaling if dimensions are quite close in
> > >  		 * order to get non-scaling cases more frequently. Also limit
> > >  		 * scaling to 3x to avoid agressive filtering that makes
> > > -		 * comparison less reliable.
> > > +		 * comparison less reliable, and don't go above 2x downsampling
> > > +		 * to avoid possible hw limitations.
> > >  		 */
> > Same comment as above about hw limits, although 2x downsampling does
> > feel like a reasonable general limit too.
> Yes, same reason we only allow up to 3x upscaling.
> > >  
> > >  		ratio = ((double) *crtc_w / *src_w);
> > > -		if (ratio > 0.8 && ratio < 1.2)
> > > +		if (ratio < 0.5)
> > > +			*src_w = *crtc_w * 2;
> > > +		else if (ratio > 0.8 && ratio < 1.2)
> > >  			*crtc_w = *src_w;
> > >  		else if (ratio > 3.0)
> > >  			*crtc_w = *src_w * 3;
> > >  
> > >  		ratio = ((double) *crtc_h / *src_h);
> > > -		if (ratio > 0.8 && ratio < 1.2)
> > > +		if (ratio < 0.5)
> > > +			*src_h = *crtc_h * 2;
> > > +		else if (ratio > 0.8 && ratio < 1.2)
> > >  			*crtc_h = *src_h;
> > >  		else if (ratio > 3.0)
> > >  			*crtc_h = *src_h * 3;
> > > @@ -846,8 +878,15 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
> > >  		 * scaled clipping may result in decimal dimensions, that most
> > >  		 * drivers don't support.
> > >  		 */
> > > -		*crtc_x = rand() % (mode->hdisplay - *crtc_w);
> > > -		*crtc_y = rand() % (mode->vdisplay - *crtc_h);
> > > +		if (*crtc_w < mode->hdisplay)
> > > +			*crtc_x = rand() % (mode->hdisplay - *crtc_w);
> > > +		else
> > > +			*crtc_x = 0;
> > > +
> > > +		if (*crtc_h < mode->vdisplay)
> > > +			*crtc_y = rand() % (mode->vdisplay - *crtc_h);
> > > +		else
> > > +			*crtc_y = 0;
> > >  	} else {
> > >  		/*
> > >  		 * Randomize the on-crtc position and allow the plane to go
> > > @@ -856,6 +895,50 @@ static void randomize_plane_dimensions(drmModeModeInfo *mode,
> > >  		*crtc_x = (rand() % mode->hdisplay) - *crtc_w / 2;
> > >  		*crtc_y = (rand() % mode->vdisplay) - *crtc_h / 2;
> > >  	}
> > > +
> > > +	configure_plane(plane, *src_w, *src_h, *src_x, *src_y,
> > > +			*crtc_w, *crtc_h, *crtc_x, *crtc_y, fb);
> > > +	ret = igt_display_try_commit_atomic(&data->display,
> > > +					    DRM_MODE_ATOMIC_TEST_ONLY |
> > > +					    DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > +					    NULL);
> > > +	if (!ret)
> > > +		return;
> > > +
> > > +	/* Coordinates are logged in the dumped debug log, so only report w/h on failure here */
> > Maybe add a trailing "." here.
> > 
> > > +	igt_assert_f(ret != -ENOSPC, "Failure in testcase, invalid coordinates on a %ux%u fb\n", width, height);
> > > +
> > > +	/* Make YUV coordinates a multiple of 2 and retry the math.. */
> > Maybe remove a trailing "." here.
> > 
> > > +	if (is_yuv) {
> > > +		*src_x &= ~1;
> > > +		*src_y &= ~1;
> > > +		*src_w &= ~1;
> > > +		*src_h &= ~1;
> > > +		/* To handle 1:1 scaling, clear crtc_w/h too */
> > > +		*crtc_w &= ~1;
> > > +		*crtc_h &= ~1;
> > > +		configure_plane(plane, *src_w, *src_h, *src_x, *src_y, *crtc_w,
> > > +				*crtc_h, *crtc_x, *crtc_y, fb);
> > > +		ret = igt_display_try_commit_atomic(&data->display,
> > > +						DRM_MODE_ATOMIC_TEST_ONLY |
> > > +						DRM_MODE_ATOMIC_ALLOW_MODESET,
> > > +						NULL);
> > > +		if (!ret)
> > > +			return;
> > > +	}
> > > +
> > > +	igt_assert(!ret || allow_scaling);
> > > +	igt_info("Scaling ratio %g / %g failed, trying without scaling.\n",
> > > +		  ((double) *crtc_w / *src_w), ((double) *crtc_h / *src_h));
> > > +
> > > +	*crtc_w = *src_w;
> > > +	*crtc_h = *src_h;
> > > +
> > > +	configure_plane(plane, *src_w, *src_h, *src_x, *src_y, *crtc_w,
> > > +			*crtc_h, *crtc_x, *crtc_y, fb);
> > > +	igt_display_commit_atomic(&data->display,
> > > +				  DRM_MODE_ATOMIC_TEST_ONLY |
> > > +				  DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > I don't understand the purpose of this trailing call.
> 
> We assume our configuration is valid now without scaling, the TEST_ONLY commit only makes sure. :)
> This way we fail as early as possible, but I can add a comment explaining it. 

Oh okay, I think I confused igt_display_commit_atomic (which asserts
ret) with igt_display_commit2 (which doesn't).

Sounds all right to me :)

Cheers,

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com