Re: [PATCH i-g-t 1/6] i-g-t: kms_plane_scaling: Fix basic scaling test

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

Op 13-12-17 om 10:50 schreef Vidya Srinivas:
> From: Mahesh Kumar <mahesh1.kumar@intel.com>
>
> PIPEC doesnt have 3rd plane in GEN9. So, we skip the
> 3rd plane related scaling test where 2nd OVERLAY
> plane is not available.
>
> Restricting downscaling to (9/10)x original size of the
> image to avoid "Max pixel rate limitation" of the hardware.
>
> Later patches in this series will cover corner cases of
> scaling.
>
> Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> Signed-off-by: Jyoti Yadav <jyoti.r.yadav@intel.com>
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> ---
>  tests/kms_plane_scaling.c | 234 +++++++++++++++++++++++++---------------------
>  1 file changed, 125 insertions(+), 109 deletions(-)
>
> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
> index 403df47..a827cb3 100644
> --- a/tests/kms_plane_scaling.c
> +++ b/tests/kms_plane_scaling.c
> @@ -163,144 +163,159 @@ static void iterate_plane_scaling(data_t *d, drmModeModeInfo *mode)
>  	}
>  }

The scaler tests should really require display->is_atomic. Even if there are
theoretically more limits on the amount of scalers, having a YUV mode or interlaced
mode may take a scaler, causing this test to fail.

With try_commit_atomic, it would be possible to test if enough scalers are available.

On top of that, I would really like the test to be more readable, and having
test_plane_scaling_on_pipe split up in smaller pieces would accomplish that.

I would do some split ups first before doing any fixes, it's hard to read what's
changed through the whitespace changes, even if the code is correct.

Also this test needs to be split up into subtests, this is a good start for it.

Replace igt_simple_main with igt_main, and add subtests for each pipe and various
tests being performed. See for example kms_color, this test is already trying to do too
much in one go. :)

~Maarten

>  
> -static void test_plane_scaling(data_t *d)
> +static void
> +test_plane_scaling_on_pipe(data_t *d, enum pipe pipe, igt_output_t *output)
>  {
>  	igt_display_t *display = &d->display;
> -	igt_output_t *output;
> -	enum pipe pipe;
> -	int valid_tests = 0;
> +	drmModeModeInfo *mode;
>  	int primary_plane_scaling = 0; /* For now */
>  
> -	igt_require(d->num_scalers);
> +	igt_output_set_pipe(output, pipe);
> +	mode = igt_output_get_mode(output);
> +
> +	/* allocate fb2 with image size */
> +	d->fb_id2 = igt_create_image_fb(d->drm_fd, 0, 0,
> +			DRM_FORMAT_XRGB8888,
> +			LOCAL_I915_FORMAT_MOD_X_TILED, /* tiled */
> +			FILE_NAME, &d->fb2);
> +	igt_assert(d->fb_id2);
> +
> +	d->fb_id3 = igt_create_pattern_fb(d->drm_fd,
> +			mode->hdisplay, mode->vdisplay,
> +			DRM_FORMAT_XRGB8888,
> +			LOCAL_I915_FORMAT_MOD_X_TILED, /* tiled */
> +			&d->fb3);
> +	igt_assert(d->fb_id3);
> +
> +	/* Set up display with plane 1 */
> +	d->plane1 = igt_output_get_plane(output, 0);
> +	prepare_crtc(d, output, pipe, d->plane1, mode, COMMIT_UNIVERSAL);
> +
> +	if (primary_plane_scaling) {
> +		/* Primary plane upscaling */
> +		igt_fb_set_position(&d->fb1, d->plane1, 100, 100);
> +		igt_fb_set_size(&d->fb1, d->plane1, 500, 500);
> +		igt_plane_set_position(d->plane1, 0, 0);
> +		igt_plane_set_size(d->plane1, mode->hdisplay, mode->vdisplay);
> +		igt_display_commit2(display, COMMIT_UNIVERSAL);
>  
> -	for_each_pipe_with_valid_output(display, pipe, output) {
> -		drmModeModeInfo *mode;
> -
> -		igt_output_set_pipe(output, pipe);
> -
> -		mode = igt_output_get_mode(output);
> -
> -		/* allocate fb2 with image size */
> -		d->fb_id2 = igt_create_image_fb(d->drm_fd, 0, 0,
> -						DRM_FORMAT_XRGB8888,
> -						LOCAL_I915_FORMAT_MOD_X_TILED, /* tiled */
> -						FILE_NAME, &d->fb2);
> -		igt_assert(d->fb_id2);
> -
> -		d->fb_id3 = igt_create_pattern_fb(d->drm_fd,
> -						  mode->hdisplay, mode->vdisplay,
> -						  DRM_FORMAT_XRGB8888,
> -						  LOCAL_I915_FORMAT_MOD_X_TILED, /* tiled */
> -						  &d->fb3);
> -		igt_assert(d->fb_id3);
> -
> -		/* Set up display with plane 1 */
> -		d->plane1 = igt_output_get_plane(output, 0);
> -		prepare_crtc(d, output, pipe, d->plane1, mode, COMMIT_UNIVERSAL);
> -
> -		if (primary_plane_scaling) {
> -			/* Primary plane upscaling */
> -			igt_fb_set_position(&d->fb1, d->plane1, 100, 100);
> -			igt_fb_set_size(&d->fb1, d->plane1, 500, 500);
> -			igt_plane_set_position(d->plane1, 0, 0);
> -			igt_plane_set_size(d->plane1, mode->hdisplay, mode->vdisplay);
> -			igt_display_commit2(display, COMMIT_UNIVERSAL);
> +		/* Primary plane 1:1 no scaling */
> +		igt_fb_set_position(&d->fb1, d->plane1, 0, 0);
> +		igt_fb_set_size(&d->fb1, d->plane1, d->fb1.width, d->fb1.height);
> +		igt_plane_set_position(d->plane1, 0, 0);
> +		igt_plane_set_size(d->plane1, mode->hdisplay, mode->vdisplay);
> +		igt_display_commit2(display, COMMIT_UNIVERSAL);
> +	}
>  
> -			/* Primary plane 1:1 no scaling */
> -			igt_fb_set_position(&d->fb1, d->plane1, 0, 0);
> -			igt_fb_set_size(&d->fb1, d->plane1, d->fb1.width, d->fb1.height);
> -			igt_plane_set_position(d->plane1, 0, 0);
> -			igt_plane_set_size(d->plane1, mode->hdisplay, mode->vdisplay);
> -			igt_display_commit2(display, COMMIT_UNIVERSAL);
> -		}
> +	/* Set up fb2->plane2 mapping. */
> +	d->plane2 = igt_output_get_plane(output, 1);
> +	igt_plane_set_fb(d->plane2, &d->fb2);
>  
> -		/* Set up fb2->plane2 mapping. */
> -		d->plane2 = igt_output_get_plane(output, 1);
> -		igt_plane_set_fb(d->plane2, &d->fb2);
> +	/* 2nd plane windowed */
> +	igt_fb_set_position(&d->fb2, d->plane2, 100, 100);
> +	igt_fb_set_size(&d->fb2, d->plane2, d->fb2.width-200, d->fb2.height-200);
> +	igt_plane_set_position(d->plane2, 100, 100);
> +	igt_plane_set_size(d->plane2, mode->hdisplay-200, mode->vdisplay-200);
> +	igt_display_commit2(display, COMMIT_UNIVERSAL);
>  
> -		/* 2nd plane windowed */
> -		igt_fb_set_position(&d->fb2, d->plane2, 100, 100);
> -		igt_fb_set_size(&d->fb2, d->plane2, d->fb2.width-200, d->fb2.height-200);
> -		igt_plane_set_position(d->plane2, 100, 100);
> -		igt_plane_set_size(d->plane2, mode->hdisplay-200, mode->vdisplay-200);
> -		igt_display_commit2(display, COMMIT_UNIVERSAL);
> +	iterate_plane_scaling(d, mode);
>  
> -		iterate_plane_scaling(d, mode);
> +	/* 2nd plane up scaling */
> +	igt_fb_set_position(&d->fb2, d->plane2, 100, 100);
> +	igt_fb_set_size(&d->fb2, d->plane2, 500, 500);
> +	igt_plane_set_position(d->plane2, 10, 10);
> +	igt_plane_set_size(d->plane2, mode->hdisplay-20, mode->vdisplay-20);
> +	igt_display_commit2(display, COMMIT_UNIVERSAL);
>  
> -		/* 2nd plane up scaling */
> -		igt_fb_set_position(&d->fb2, d->plane2, 100, 100);
> -		igt_fb_set_size(&d->fb2, d->plane2, 500, 500);
> -		igt_plane_set_position(d->plane2, 10, 10);
> -		igt_plane_set_size(d->plane2, mode->hdisplay-20, mode->vdisplay-20);
> -		igt_display_commit2(display, COMMIT_UNIVERSAL);
> +	/* 2nd plane downscaling */
> +	igt_fb_set_position(&d->fb2, d->plane2, 0, 0);
> +	igt_fb_set_size(&d->fb2, d->plane2, d->fb2.width, d->fb2.height);
> +	igt_plane_set_position(d->plane2, 10, 10);
>  
> -		/* 2nd plane downscaling */
> -		igt_fb_set_position(&d->fb2, d->plane2, 0, 0);
> -		igt_fb_set_size(&d->fb2, d->plane2, d->fb2.width, d->fb2.height);
> -		igt_plane_set_position(d->plane2, 10, 10);
> -		igt_plane_set_size(d->plane2, 500, 500 * d->fb2.height/d->fb2.width);
> +	/* Downscale (10/9)x of original image */
> +	igt_plane_set_size(d->plane2, (d->fb2.width * 10)/9, (d->fb2.height * 10)/9);
> +	igt_display_commit2(display, COMMIT_UNIVERSAL);
> +
> +	if (primary_plane_scaling) {
> +		/* Primary plane up scaling */
> +		igt_fb_set_position(&d->fb1, d->plane1, 100, 100);
> +		igt_fb_set_size(&d->fb1, d->plane1, 500, 500);
> +		igt_plane_set_position(d->plane1, 0, 0);
> +		igt_plane_set_size(d->plane1, mode->hdisplay, mode->vdisplay);
>  		igt_display_commit2(display, COMMIT_UNIVERSAL);
> +	}
>  
> -		if (primary_plane_scaling) {
> -			/* Primary plane up scaling */
> -			igt_fb_set_position(&d->fb1, d->plane1, 100, 100);
> -			igt_fb_set_size(&d->fb1, d->plane1, 500, 500);
> -			igt_plane_set_position(d->plane1, 0, 0);
> -			igt_plane_set_size(d->plane1, mode->hdisplay, mode->vdisplay);
> -			igt_display_commit2(display, COMMIT_UNIVERSAL);
> -		}
> +	/* Set up fb3->plane3 mapping. */
> +	d->plane3 = igt_output_get_plane(output, 2);
> +	igt_plane_set_fb(d->plane3, &d->fb3);
>  
> -		/* Set up fb3->plane3 mapping. */
> -		d->plane3 = igt_output_get_plane(output, 2);
> -		igt_plane_set_fb(d->plane3, &d->fb3);
> +	if(d->plane3->type == DRM_PLANE_TYPE_CURSOR) {
> +		igt_info("Plane-3 doesnt exist on pipe %s\n", kmstest_pipe_name(pipe));
> +		goto cleanup;
> +	}
>  
> -		/* 3rd plane windowed - no scaling */
> -		igt_fb_set_position(&d->fb3, d->plane3, 100, 100);
> -		igt_fb_set_size(&d->fb3, d->plane3, d->fb3.width-300, d->fb3.height-300);
> -		igt_plane_set_position(d->plane3, 100, 100);
> -		igt_plane_set_size(d->plane3, mode->hdisplay-300, mode->vdisplay-300);
> -		igt_display_commit2(display, COMMIT_UNIVERSAL);
> +	/* 3rd plane windowed - no scaling */
> +	igt_fb_set_position(&d->fb3, d->plane3, 100, 100);
> +	igt_fb_set_size(&d->fb3, d->plane3, d->fb3.width-300, d->fb3.height-300);
> +	igt_plane_set_position(d->plane3, 100, 100);
> +	igt_plane_set_size(d->plane3, mode->hdisplay-300, mode->vdisplay-300);
> +	igt_display_commit2(display, COMMIT_UNIVERSAL);
> +
> +	/* Switch scaler from plane 2 to plane 3 */
> +	igt_fb_set_position(&d->fb2, d->plane2, 100, 100);
> +	igt_fb_set_size(&d->fb2, d->plane2, d->fb2.width-200, d->fb2.height-200);
> +	igt_plane_set_position(d->plane2, 100, 100);
> +	igt_plane_set_size(d->plane2, d->fb2.width-200, d->fb2.height-200);
> +
> +	igt_fb_set_position(&d->fb3, d->plane3, 100, 100);
> +	igt_fb_set_size(&d->fb3, d->plane3, d->fb3.width-400, d->fb3.height-400);
> +	igt_plane_set_position(d->plane3, 10, 10);
> +	igt_plane_set_size(d->plane3, mode->hdisplay-300, mode->vdisplay-300);
> +	igt_display_commit2(display, COMMIT_UNIVERSAL);
> +
> +	if (primary_plane_scaling) {
> +		/* Switch scaler from plane 1 to plane 2 */
> +		igt_fb_set_position(&d->fb1, d->plane1, 0, 0);
> +		igt_fb_set_size(&d->fb1, d->plane1, d->fb1.width, d->fb1.height);
> +		igt_plane_set_position(d->plane1, 0, 0);
> +		igt_plane_set_size(d->plane1, mode->hdisplay, mode->vdisplay);
>  
> -		/* Switch scaler from plane 2 to plane 3 */
>  		igt_fb_set_position(&d->fb2, d->plane2, 100, 100);
> -		igt_fb_set_size(&d->fb2, d->plane2, d->fb2.width-200, d->fb2.height-200);
> +		igt_fb_set_size(&d->fb2, d->plane2, d->fb2.width-500,d->fb2.height-500);
>  		igt_plane_set_position(d->plane2, 100, 100);
> -		igt_plane_set_size(d->plane2, d->fb2.width-200, d->fb2.height-200);
> -
> -		igt_fb_set_position(&d->fb3, d->plane3, 100, 100);
> -		igt_fb_set_size(&d->fb3, d->plane3, d->fb3.width-400, d->fb3.height-400);
> -		igt_plane_set_position(d->plane3, 10, 10);
> -		igt_plane_set_size(d->plane3, mode->hdisplay-300, mode->vdisplay-300);
> +		igt_plane_set_size(d->plane2, mode->hdisplay-200, mode->vdisplay-200);
>  		igt_display_commit2(display, COMMIT_UNIVERSAL);
> +	}
>  
> -		if (primary_plane_scaling) {
> -			/* Switch scaler from plane 1 to plane 2 */
> -			igt_fb_set_position(&d->fb1, d->plane1, 0, 0);
> -			igt_fb_set_size(&d->fb1, d->plane1, d->fb1.width, d->fb1.height);
> -			igt_plane_set_position(d->plane1, 0, 0);
> -			igt_plane_set_size(d->plane1, mode->hdisplay, mode->vdisplay);
> -
> -			igt_fb_set_position(&d->fb2, d->plane2, 100, 100);
> -			igt_fb_set_size(&d->fb2, d->plane2, d->fb2.width-500,d->fb2.height-500);
> -			igt_plane_set_position(d->plane2, 100, 100);
> -			igt_plane_set_size(d->plane2, mode->hdisplay-200, mode->vdisplay-200);
> -			igt_display_commit2(display, COMMIT_UNIVERSAL);
> -		}
> +cleanup:
> +	/* back to single plane mode */
> +	igt_plane_set_fb(d->plane2, NULL);
> +	igt_plane_set_fb(d->plane3, NULL);
> +	igt_display_commit2(display, COMMIT_UNIVERSAL);
>  
> -		/* back to single plane mode */
> -		igt_plane_set_fb(d->plane2, NULL);
> -		igt_plane_set_fb(d->plane3, NULL);
> -		igt_display_commit2(display, COMMIT_UNIVERSAL);
> +	cleanup_crtc(d, output, d->plane1);
> +}
> +
> +static void test_plane_scaling(data_t *d, enum pipe pipe)
> +{
> +	igt_output_t *output;
> +	int valid_tests = 0;
> +
> +	igt_require(d->num_scalers);
>  
> +	for_each_valid_output_on_pipe(&d->display, pipe, output) {
> +		test_plane_scaling_on_pipe(d, pipe, output);
> +		igt_output_set_pipe(output, PIPE_ANY);
>  		valid_tests++;
> -		cleanup_crtc(d, output, d->plane1);
>  	}
> +
>  	igt_require_f(valid_tests, "no valid crtc/connector combinations found\n");
>  }
>  
>  igt_simple_main
>  {
>  	data_t data = {};
> +	enum pipe pipe;
>  
>  	igt_skip_on_simulation();
>  
> @@ -312,7 +327,8 @@ igt_simple_main
>  
>  	data.num_scalers = intel_gen(data.devid) >= 9 ? 2 : 0;
>  
> -	test_plane_scaling(&data);
> +	for_each_pipe_static(pipe)
> +		test_plane_scaling(&data, pipe);
>  
>  	igt_display_fini(&data.display);
>  }


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx