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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@bootlin.com>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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 

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

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

~Maarten

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