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

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

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

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.

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

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

>  }
>  
>  static void blit_plane_cairo(data_t *data, cairo_surface_t *result,
> @@ -914,20 +997,6 @@ static void blit_plane_cairo(data_t *data, cairo_surface_t *result,
>  	cairo_destroy(cr);
>  }
>  
> -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);
> -
> -	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 prepare_randomized_plane(data_t *data,
>  				     drmModeModeInfo *mode,
>  				     igt_plane_t *plane,
> @@ -947,24 +1016,12 @@ static void prepare_randomized_plane(data_t *data,
>  	size_t stride;
>  	bool tiled;
>  	int fb_id;
> +	uint32_t tile_w, tile_h;
>  
> -	randomize_plane_dimensions(mode, &overlay_fb_w, &overlay_fb_h,
> -				   &overlay_src_w, &overlay_src_h,
> -				   &overlay_src_x, &overlay_src_y,
> -				   &overlay_crtc_w, &overlay_crtc_h,
> -				   &overlay_crtc_x, &overlay_crtc_y,
> -				   allow_scaling);
> +	randomize_plane_dimensions(mode, &overlay_fb_w, &overlay_fb_h);
>  
>  	igt_debug("Plane %d: framebuffer size %dx%d\n", index,
>  		  overlay_fb_w, overlay_fb_h);
> -	igt_debug("Plane %d: on-crtc size %dx%d\n", index,
> -		  overlay_crtc_w, overlay_crtc_h);
> -	igt_debug("Plane %d: on-crtc position %dx%d\n", index,
> -		  overlay_crtc_x, overlay_crtc_y);
> -	igt_debug("Plane %d: in-framebuffer size %dx%d\n", index,
> -		  overlay_src_w, overlay_src_h);
> -	igt_debug("Plane %d: in-framebuffer position %dx%d\n", index,
> -		  overlay_src_x, overlay_src_y);
>  
>  	/* Get a pattern framebuffer for the overlay plane. */
>  	fb_id = chamelium_get_pattern_fb(data, overlay_fb_w, overlay_fb_h,
> @@ -976,23 +1033,42 @@ static void prepare_randomized_plane(data_t *data,
>  
>  	tiled = (modifier != LOCAL_DRM_FORMAT_MOD_NONE);
>  
> +	igt_get_fb_tile_size(data->display.drm_fd, modifier,
> +			     igt_drm_format_to_bpp(format),
> +			     &tile_w, &tile_h);
> +
> +	/* Make sure stride is a multiple of tile size */
> +	stride = DIV_ROUND_UP(stride, tile_w) * tile_w;
> +
>  	igt_debug("Plane %d: %s format (%s) with stride %ld\n", index,
>  		  igt_format_str(format), tiled ? "tiled" : "linear", stride);
>  
> +
>  	fb_id = igt_fb_convert_with_stride(overlay_fb, &pattern_fb, format,
>  					   modifier, stride);
>  	igt_assert(fb_id > 0);
>  
> +	randomize_plane_coordinates(data, plane, mode, overlay_fb,
> +				    &overlay_src_w, &overlay_src_h,
> +				    &overlay_src_x, &overlay_src_y,
> +				    &overlay_crtc_w, &overlay_crtc_h,
> +				    &overlay_crtc_x, &overlay_crtc_y,
> +				    allow_scaling);
> +
> +	igt_debug("Plane %d: in-framebuffer size %dx%d\n", index,
> +		  overlay_src_w, overlay_src_h);
> +	igt_debug("Plane %d: in-framebuffer position %dx%d\n", index,
> +		  overlay_src_x, overlay_src_y);
> +	igt_debug("Plane %d: on-crtc size %dx%d\n", index,
> +		  overlay_crtc_w, overlay_crtc_h);
> +	igt_debug("Plane %d: on-crtc position %dx%d\n", index,
> +		  overlay_crtc_x, overlay_crtc_y);
> +
>  	blit_plane_cairo(data, result_surface, overlay_src_w, overlay_src_h,
>  			 overlay_src_x, overlay_src_y,
>  			 overlay_crtc_w, overlay_crtc_h,
>  			 overlay_crtc_x, overlay_crtc_y, &pattern_fb);
>  
> -	configure_plane(plane, overlay_src_w, overlay_src_h,
> -			overlay_src_x, overlay_src_y,
> -			overlay_crtc_w, overlay_crtc_h,
> -			overlay_crtc_x, overlay_crtc_y, overlay_fb);
> -
>  	/* Remove the original pattern framebuffer. */
>  	igt_remove_fb(data->drm_fd, &pattern_fb);
>  }
> @@ -1068,7 +1144,7 @@ static void test_display_planes_random(data_t *data,
>  		igt_output_count_plane_type(output, DRM_PLANE_TYPE_OVERLAY);
>  
>  	/* Limit the number of planes to a reasonable scene. */
> -	overlay_planes_max = max(overlay_planes_max, 4);
> +	overlay_planes_max = min(overlay_planes_max, 4);
>  
>  	overlay_planes_count = (rand() % overlay_planes_max) + 1;
>  	igt_debug("Using %d overlay planes\n", overlay_planes_count);
> @@ -1121,17 +1197,8 @@ static void test_display_planes_random(data_t *data,
>  		chamelium_destroy_frame_dump(dump);
>  	}
>  
> -	for (i = 0; i < overlay_planes_count; i++) {
> -		struct igt_fb *overlay_fb = &overlay_fbs[i];
> -		igt_plane_t *plane;
> -
> -		plane = igt_output_get_plane_type_index(output,
> -							DRM_PLANE_TYPE_OVERLAY,
> -							i);
> -		igt_assert(plane);
> -
> -		igt_remove_fb(data->drm_fd, overlay_fb);
> -	}
> +	for (i = 0; i < overlay_planes_count; i++)
> +		igt_remove_fb(data->drm_fd, &overlay_fbs[i]);
>  
>  	free(overlay_fbs);
>  
-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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