Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple.c: Enhance the IGT test to enable multiple planes and multiple pipe.

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 24.4.2019 12.38, Nidhi Gupta wrote:
> In Existing kms_plane_multiple we are testing Multiple plane position on
> one pipe at a time, we do not have a testcase where all the pipes are
> enabled at the same time and checking for the plane positions.
> 
> Adding a subtest with all the pipes enabled and performing a test to
> check the multiple plane positions with all the pipes at the same time. Clearing
> of the resources are done after performing plane position test on all pipes.

^^
Clearing of resources I didn't see after the test

> 
> Signed-off-by: Nidhi Gupta <nidhi1.gupta@intel.com>
> ---
>   tests/kms_plane_multiple.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 71 insertions(+)
> 
> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
> index d2d02a5..c11de9d 100644
> --- a/tests/kms_plane_multiple.c
> +++ b/tests/kms_plane_multiple.c
> @@ -299,6 +299,52 @@ test_plane_position_with_output(data_t *data, enum pipe pipe,
>   }
>   
>   static void
> +test_multiple_plane_position_with_output(data_t *data, enum pipe pipe,
> +				igt_output_t *output, int n_planes,
> +				uint64_t tiling)
> +{
> +	color_t blue  = { 0.0f, 0.0f, 1.0f };
> +	igt_crc_t crc;
> +	int i;
> +	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> +	bool loop_forever;
> +	char info[256];
> +
> +	if (opt.iterations == LOOP_FOREVER) {
> +		loop_forever = true;
> +		sprintf(info, "forever");
> +	} else {
> +		loop_forever = false;
> +		sprintf(info, "for %d %s",
> +			iterations, iterations > 1 ? "iterations" : "iteration");
> +	}

This looping code I think is just harmful here, I don't know if it's of 
any use for your subtest and if you want to do possibility for looping 
for this test this is wrong place for that as this would just loop on 
one pipe.

> +
> +	igt_info("Testing connector %s using pipe %s with %d planes %s with seed %d\n",
> +		 igt_output_name(output), kmstest_pipe_name(pipe), n_planes,
> +		 info, opt.seed);

Does this printout make sense here?

> +
> +	test_init(data, pipe, n_planes);
> +
> +	test_grab_crc(data, output, pipe, &blue, tiling);
> +
> +	i = 0;
> +	while (i < iterations || loop_forever) {
> +		prepare_planes(data, pipe, &blue, tiling, n_planes, output);
> +
> +		igt_display_commit2(&data->display, COMMIT_ATOMIC);
> +
> +		igt_pipe_crc_get_current(data->display.drm_fd, data->pipe_crc, &crc);
> +
> +		igt_assert_crc_equal(&data->ref_crc, &crc);
> +
> +		i++;
> +	}
> +
> +	//test_fini(data, output, n_planes);

Don't leave these commented out things. :)

Here at minimum I think you'll need to stop crc collection which was 
started at test_init, otherwise you'll get errors in dmesg about 
userspace reading too slow if crc buffer becomes full.

For testing purpose it would be better to get crc's after you've enabled 
maximum amount of pipes, that's when you'd see if there are anomalies 
which crc could catch.


> +}
> +
> +
> +static void
>   test_plane_position(data_t *data, enum pipe pipe, uint64_t tiling)
>   {
>   	igt_output_t *output;
> @@ -317,6 +363,25 @@ test_plane_position(data_t *data, enum pipe pipe, uint64_t tiling)
>   }
>   
>   static void
> +test_multiple_plane_position(data_t *data, enum pipe pipe, uint64_t tiling)
> +{
> +	igt_output_t *output;
> +	int n_planes = data->display.pipes[pipe].n_planes;
> +
> +	output = igt_get_single_output_for_pipe(&data->display, pipe);
> +	igt_require(output);
> +
> +	if (!opt.user_seed)
> +		opt.seed = time(NULL);
> +
> +	srand(opt.seed);
> +
> +	test_multiple_plane_position_with_output(data, pipe, output,
> +					n_planes, tiling);
> +}
> +
> +
> +static void
>   run_tests_for_pipe(data_t *data, enum pipe pipe)
>   {
>   	igt_fixture {
> @@ -393,6 +458,12 @@ int main(int argc, char *argv[])
>   			run_tests_for_pipe(&data, pipe);
>   	}
>   
> +	igt_subtest_f("atomic-maximum-pipes-tiling-none") {
> +	for_each_pipe_static(pipe)
> +		test_multiple_plane_position(&data, pipe, LOCAL_DRM_FORMAT_MOD_NONE);

^^
intendation

You'll need to enumerate outputs here in some way. Now if you have three 
pipes and two displays you'll get something funny when third pipe will 
be shown somewhere which will mean one of earlier enabled pipes will be 
disabled.

> +	}
> +
> +
>   	igt_fixture {
>   		igt_display_fini(&data.display);
>   	}
> 

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