[igt-dev] [PATCH i-g-t] 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.

[Arkadiusz Hiler]

On Thu, Jul 18, 2019 at 04:36:01PM +0530, Nidhi Gupta wrote:
> 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.

The original test makes sure that kernel handles commits updating planes'
geometry correctly. Due to the nature of the checks it also verifies
that blending happens correctly.

I assume that you want to make sure that this works with multiple pipes
being used at the same time (correct me if I am wrong), but I don't
think the changes here achieve that. Check the in-line comments for
details.


As of the title:

"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." is too
long.

Please read: https://chris.beams.io/posts/git-commit/ and browse through
the repo history for some inspiration.

My suggestion would be:

"tests/kms_plane_multiple: Add subtests that utilizes multiple pipes at the same time"

> ---
>  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 0df1053..f178c3a 100644
> --- a/tests/kms_plane_multiple.c
> +++ b/tests/kms_plane_multiple.c
> @@ -506,6 +506,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)
> +{

This function looks mostly like a copy and paste
taste_plane_position_with_output(). Extracting the code to common
helpers is generally the way to go.

> +	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");
> +	}
> +
> +	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);
> +
> +	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);

The commit here is done per pipe. You should construct a single request
setting all the state in a single go instead, and loop over that.

Also please make sure that atomic is supported.

> +
> +		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);

Remove unused code instead of just commenting it out. We are using C
style comments (/* ... */) instead of the C++ styled ones (// ...).

> +}
> +
> +
> +static void
>  test_plane_position(data_t *data, enum pipe pipe, uint64_t tiling)
>  {
>  	igt_output_t *output;
> @@ -528,6 +574,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;

Here you are assuming that we will succeed with all the planes enabled
on all the pipes without doing try_commits() (the non-multiple does the
checks). This is scary, especially taking into account the bandwith
issues we had.

https://patchwork.freedesktop.org/patch/306864/?series=60921&rev=2

> +
> +	output = igt_get_single_output_for_pipe(&data->display, pipe);

This does not guarantee that you will get different outputs for each
pipe you are enumerating, so you will end up working on just a single
output for each pipe, overwriting it configuration. You should use
different mechanism for selecting them and assuring that you have enough
outputs. See notes below.

> +	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 {
> @@ -612,6 +677,12 @@ igt_main_args("", long_options, help_str, opt_handler, NULL)
>  			run_tests_for_pipe(&data, pipe);
>  	}
>  
> +	igt_subtest_f("atomic-maximum-pipes-tiling-none") {

Missing igt_describe()

https://gitlab.freedesktop.org/drm/igt-gpu-tools/blob/master/CONTRIBUTING.md#L19

Also, this test should depend on having multiple output connected to
utilize multiple pipes. Check other subtests that have 2x, 3x, 4x, etc
in their names to see how they handle that.

Cheers,
Arek

> +	for_each_pipe_static(pipe)
> +		test_multiple_plane_position(&data, pipe, LOCAL_DRM_FORMAT_MOD_NONE);
> +	}
> +
> +
>  	igt_fixture {
>  		igt_display_fini(&data.display);
>  	}
> -- 
> 2.7.4
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev