[igt-dev] [PATCH i-g-t] tests/kms_big_joiner: Convert test to dynamic

Modem, Bhanuprakash bhanuprakash.modem at intel.com
Wed May 18 08:21:08 UTC 2022


On Wed-27-04-2022 01:52 pm, Karthik B S wrote:
> Covert the tests to dynamic subtests at pipe level.
> 
> Signed-off-by: Karthik B S <karthik.b.s at intel.com>
> ---
>   tests/i915/kms_big_joiner.c | 218 +++++++++++++++++++-----------------
>   1 file changed, 118 insertions(+), 100 deletions(-)
> 
> diff --git a/tests/i915/kms_big_joiner.c b/tests/i915/kms_big_joiner.c
> index d7fa2e53..9922e43c 100644
> --- a/tests/i915/kms_big_joiner.c
> +++ b/tests/i915/kms_big_joiner.c
> @@ -35,6 +35,8 @@ typedef struct {
>   	igt_display_t display;
>   	struct igt_fb fb;
>   	int n_pipes;
> +	enum pipe pipe1;
> +	enum pipe pipe2;
>   	struct output_data {
>   		uint32_t id;
>   		int mode_number;
> @@ -46,10 +48,12 @@ static void test_invalid_modeset(data_t *data)
>   	drmModeModeInfo *mode;
>   	igt_display_t *display = &data->display;
>   	igt_output_t *output, *big_joiner_output = NULL, *second_output = NULL;
> -	int i, ret;
> +	int ret;
>   	igt_pipe_t *pipe;
>   	igt_plane_t *plane;
>   
> +	igt_display_reset(display);
> +
>   	for_each_connected_output(display, output) {
>   		mode = &output->config.connector->modes[0];
>   
> @@ -60,100 +64,92 @@ static void test_invalid_modeset(data_t *data)
>   		}
>   	}
>   
> -	for_each_pipe(display, i) {
> -		if (i < (data->n_pipes - 1)) {
> -			igt_output_set_pipe(big_joiner_output, i);
> +	igt_output_set_pipe(big_joiner_output, data->pipe1);
>   
> -			mode = &big_joiner_output->config.connector->modes[data->big_joiner_output[0].mode_number];
> -			igt_output_override_mode(big_joiner_output, mode);
> +	mode = &big_joiner_output->config.connector->modes[data->big_joiner_output[0].mode_number];
> +	igt_output_override_mode(big_joiner_output, mode);
>   
> -			pipe = &display->pipes[i];
> -			plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> +	pipe = &display->pipes[data->pipe1];
> +	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
> -			igt_plane_set_fb(plane, &data->fb);
> -			igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> -			igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_fb(plane, &data->fb);
> +	igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>   
> -			igt_display_commit2(display, COMMIT_ATOMIC);
> +	igt_display_commit2(display, COMMIT_ATOMIC);

As we are using atomic path, do we really need this commit?

>   
> -			igt_output_set_pipe(second_output, i + 1);
> +	igt_output_set_pipe(second_output, data->pipe2);
>   
> -			mode = igt_output_get_mode(second_output);
> +	mode = igt_output_get_mode(second_output);
>   
> -			pipe = &display->pipes[i + 1];
> -			plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> +	pipe = &display->pipes[data->pipe2];
> +	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
> -			igt_plane_set_fb(plane, &data->fb);
> -			igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> -			igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_fb(plane, &data->fb);
> +	igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>   
> -			/* This commit is expectd to fail as this pipe is being used for big joiner */
> -			ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
> -							    DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> -			igt_assert_lt(ret, 0);
> +	/* This commit is expectd to fail as this pipe is being used for big joiner */
> +	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
> +					    DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +	igt_assert_lt(ret, 0);
>   
> -			igt_output_set_pipe(big_joiner_output, PIPE_NONE);
> -			igt_output_set_pipe(second_output, PIPE_NONE);
> +	igt_output_set_pipe(big_joiner_output, PIPE_NONE);
> +	igt_output_set_pipe(second_output, PIPE_NONE);
>   
> -			pipe = &display->pipes[i];
> -			plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> +	pipe = &display->pipes[data->pipe1];
> +	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
> -			/*
> -			 * Do not explicitly set the plane of the second output to NULL,
> -			 * as it is the adjacent pipe to the big joiner output and
> -			 * setting the big joiner plane to NULL will take care of this.
> -			 */
> -			igt_plane_set_fb(plane, NULL);
> -			igt_display_commit2(display, COMMIT_ATOMIC);
> -			igt_output_override_mode(big_joiner_output, NULL);
> -		}
> -	}
> +	/*
> +	 * Do not explicitly set the plane of the second output to NULL,
> +	 * as it is the adjacent pipe to the big joiner output and
> +	 * setting the big joiner plane to NULL will take care of this.
> +	 */
> +	igt_plane_set_fb(plane, NULL);
> +	igt_display_commit2(display, COMMIT_ATOMIC);
> +	igt_output_override_mode(big_joiner_output, NULL);
>   
> -	for_each_pipe(display, i) {
> -		if (i < (data->n_pipes - 1)) {
> -			igt_output_set_pipe(second_output, i + 1);
> +	igt_output_set_pipe(second_output, data->pipe2);
>   
> -			mode = igt_output_get_mode(second_output);
> +	mode = igt_output_get_mode(second_output);
>   
> -			pipe = &display->pipes[i + 1];
> -			plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> +	pipe = &display->pipes[data->pipe2];
> +	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
> -			igt_plane_set_fb(plane, &data->fb);
> -			igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> -			igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_fb(plane, &data->fb);
> +	igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>   
> -			igt_display_commit2(display, COMMIT_ATOMIC);
> +	igt_display_commit2(display, COMMIT_ATOMIC);

Same here.

>   
> -			igt_output_set_pipe(big_joiner_output, i);
> +	igt_output_set_pipe(big_joiner_output, data->pipe1);
>   
> -			mode = &big_joiner_output->config.connector->modes[data->big_joiner_output[0].mode_number];
> -			igt_output_override_mode(big_joiner_output, mode);
> +	mode = &big_joiner_output->config.connector->modes[data->big_joiner_output[0].mode_number];
> +	igt_output_override_mode(big_joiner_output, mode);
>   
> -			pipe = &display->pipes[i];
> -			plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> +	pipe = &display->pipes[data->pipe1];
> +	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
> -			igt_plane_set_fb(plane, &data->fb);
> -			igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> -			igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_fb(plane, &data->fb);
> +	igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>   
> -			/* This commit is expected to fail as the adjacent pipe is already in use*/
> -			ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
> -							    DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> -			igt_assert_lt(ret, 0);
> +	/* This commit is expected to fail as the adjacent pipe is already in use*/
> +	ret = igt_display_try_commit_atomic(display, DRM_MODE_ATOMIC_TEST_ONLY |
> +					    DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> +	igt_assert_lt(ret, 0);
>   
> -			igt_output_set_pipe(big_joiner_output, PIPE_NONE);
> -			igt_output_set_pipe(second_output, PIPE_NONE);
> -			igt_plane_set_fb(plane, NULL);
> +	igt_output_set_pipe(big_joiner_output, PIPE_NONE);
> +	igt_output_set_pipe(second_output, PIPE_NONE);
> +	igt_plane_set_fb(plane, NULL);
>   
> -			pipe = &display->pipes[i + 1];
> -			plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> -			igt_plane_set_fb(plane, NULL);
> +	pipe = &display->pipes[data->pipe2];
> +	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> +	igt_plane_set_fb(plane, NULL);
>   
> -			igt_display_commit2(display, COMMIT_ATOMIC);
> +	igt_display_commit2(display, COMMIT_ATOMIC);
>   
> -			igt_output_override_mode(big_joiner_output, NULL);
> -		}
> -	}
> +	igt_output_override_mode(big_joiner_output, NULL);
>   }
>   
>   static void test_basic_modeset(data_t *data)
> @@ -161,10 +157,11 @@ static void test_basic_modeset(data_t *data)
>   	drmModeModeInfo *mode;
>   	igt_output_t *output, *big_joiner_output = NULL;
>   	igt_display_t *display = &data->display;
> -	int i;
>   	igt_pipe_t *pipe;
>   	igt_plane_t *plane;
>   
> +	igt_display_reset(display);
> +
>   	for_each_connected_output(display, output) {
>   		if (data->big_joiner_output[0].id == output->id) {
>   			big_joiner_output = output;
> @@ -172,27 +169,23 @@ static void test_basic_modeset(data_t *data)
>   		}
>   	}
>   
> -	for_each_pipe(display, i) {
> -		if (i < (data->n_pipes - 1)) {
> -			igt_output_set_pipe(big_joiner_output, i);
> +	igt_output_set_pipe(big_joiner_output, data->pipe1);
>   
> -			mode = &big_joiner_output->config.connector->modes[data->big_joiner_output[0].mode_number];
> -			igt_output_override_mode(big_joiner_output, mode);
> +	mode = &big_joiner_output->config.connector->modes[data->big_joiner_output[0].mode_number];
> +	igt_output_override_mode(big_joiner_output, mode);
>   
> -			pipe = &display->pipes[i];
> -			plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> +	pipe = &display->pipes[data->pipe1];
> +	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
> -			igt_plane_set_fb(plane, &data->fb);
> -			igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> -			igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_fb(plane, &data->fb);
> +	igt_fb_set_size(&data->fb, plane, mode->hdisplay, mode->vdisplay);
> +	igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
>   
> -			igt_display_commit2(display, COMMIT_ATOMIC);
> +	igt_display_commit2(display, COMMIT_ATOMIC);
>   
> -			igt_output_set_pipe(big_joiner_output, PIPE_NONE);
> -			igt_plane_set_fb(plane, NULL);
> -			igt_display_commit2(display, COMMIT_ATOMIC);
> -		}
> -	}
> +	igt_output_set_pipe(big_joiner_output, PIPE_NONE);
> +	igt_plane_set_fb(plane, NULL);
> +	igt_display_commit2(display, COMMIT_ATOMIC);
>   }
>   
>   static void test_dual_display(data_t *data)
> @@ -204,6 +197,8 @@ static void test_dual_display(data_t *data)
>   	igt_plane_t *plane;
>   	int count = 0;
>   
> +	igt_display_reset(display);
> +
>   	for_each_connected_output(display, output) {
>   		if (data->big_joiner_output[count].id == output->id) {
>   			big_joiner_output[count] = output;
> @@ -214,14 +209,14 @@ static void test_dual_display(data_t *data)
>   			break;
>   	}
>   
> -	igt_output_set_pipe(big_joiner_output[0], PIPE_A);
> -	igt_output_set_pipe(big_joiner_output[1], PIPE_C);
> +	igt_output_set_pipe(big_joiner_output[0], data->pipe1);
> +	igt_output_set_pipe(big_joiner_output[1], data->pipe2);
>   
>   	/* Set up first big joiner output on Pipe A*/
>   	mode = &big_joiner_output[0]->config.connector->modes[data->big_joiner_output[0].mode_number];
>   	igt_output_override_mode(big_joiner_output[0], mode);
>   
> -	pipe = &display->pipes[PIPE_A];
> +	pipe = &display->pipes[data->pipe1];
>   	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
>   	igt_plane_set_fb(plane, &data->fb);
> @@ -232,7 +227,7 @@ static void test_dual_display(data_t *data)
>   	mode = &big_joiner_output[1]->config.connector->modes[data->big_joiner_output[1].mode_number];
>   	igt_output_override_mode(big_joiner_output[1], mode);
>   
> -	pipe = &display->pipes[PIPE_C];
> +	pipe = &display->pipes[data->pipe2];
>   	plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
>   
>   	igt_plane_set_fb(plane, &data->fb);
> @@ -244,8 +239,8 @@ static void test_dual_display(data_t *data)
>   	/* Clean up */
>   	igt_output_set_pipe(big_joiner_output[0], PIPE_NONE);
>   	igt_output_set_pipe(big_joiner_output[1], PIPE_NONE);
> -	igt_plane_set_fb(igt_pipe_get_plane_type(&display->pipes[PIPE_A], DRM_PLANE_TYPE_PRIMARY), NULL);
> -	igt_plane_set_fb(igt_pipe_get_plane_type(&display->pipes[PIPE_C], DRM_PLANE_TYPE_PRIMARY), NULL);
> +	igt_plane_set_fb(igt_pipe_get_plane_type(&display->pipes[data->pipe1], DRM_PLANE_TYPE_PRIMARY), NULL);
> +	igt_plane_set_fb(igt_pipe_get_plane_type(&display->pipes[data->pipe2], DRM_PLANE_TYPE_PRIMARY), NULL);

As we are preserving the primary plane in variable plane, I think we can 
have one more variable plane2 and use as below:

igt_plane_set_fb(plane, NULL);
igt_plane_set_fb(plane2, NULL);

>   	igt_display_commit2(display, COMMIT_ATOMIC);
>   }
>   
> @@ -254,8 +249,9 @@ igt_main
>   	data_t data;
>   	igt_output_t *output;
>   	drmModeModeInfo *mode;
> -	int valid_output = 0, i, count = 0;
> +	int valid_output = 0, i, count = 0, j = 0;
>   	uint16_t width = 0, height = 0;
> +	enum pipe pipe_seq[IGT_MAX_PIPES];
>   
>   	igt_fixture {
>   		data.drm_fd = drm_open_driver_master(DRIVER_INTEL);

Please add below items to igt_fixture.

igt_display_require_output();
igt_require(display->atomic);

> @@ -282,8 +278,11 @@ igt_main
>   		}
>   
>   		data.n_pipes = 0;
> -		for_each_pipe(&data.display, i)
> +		for_each_pipe(&data.display, i) {
>   			data.n_pipes++;
> +			pipe_seq[j] = i;
> +			j++;
> +		}
>   
>   		igt_require_f(count > 0, "No output with 5k+ mode found\n");
>   
> @@ -292,21 +291,40 @@ igt_main
>   	}
>   
>   	igt_describe("Verify the basic modeset on big joiner mode on all pipes");
> -	igt_subtest("basic")
> -		test_basic_modeset(&data);
> +	igt_subtest_with_dynamic("basic") {
> +		for (i = 0; i < data.n_pipes - 1; i++) {
> +			if (i < (data.n_pipes - 1)) {

This check is redundant.

> +				data.pipe1 = pipe_seq[i];
> +				igt_dynamic_f("pipe-%s", kmstest_pipe_name(pipe_seq[i]))
> +					test_basic_modeset(&data);
> +			}
> +		}
> +	}
>   
>   	igt_describe("Verify if the modeset on the adjoining pipe is rejected "
>   		     "when the pipe is active with a big joiner modeset");
> -	igt_subtest("invalid-modeset") {
> +	igt_subtest_with_dynamic("invalid-modeset") {
>   		igt_require_f(valid_output > 1, "No valid Second output found\n");
> -		test_invalid_modeset(&data);
> +		for (i = 0; i < data.n_pipes - 1; i++) {
> +			if (i < (data.n_pipes - 1)) {

Same here.

> +				data.pipe1 = pipe_seq[i];
> +				data.pipe2 = pipe_seq[i + 1];
> +				igt_dynamic_f("pipe-%s", kmstest_pipe_name(pipe_seq[i]))

Can we add second pipe name to the subtest?

> +					test_invalid_modeset(&data);
> +			}
> +		}
>   	}
>   
>   	igt_describe("Verify simultaneous modeset on 2 big joiner outputs");
> -	igt_subtest("2x-modeset") {
> +	igt_subtest_with_dynamic("2x-modeset") {
>   		igt_require_f(count > 1, "2 outputs with big joiner modes are required\n");
>   		igt_require_f(data.n_pipes > 3, "Minumum of 4 pipes are required\n");
> -		test_dual_display(&data);
> +		for (i = 0; (i + 2) < data.n_pipes - 1; i++) {

consider adl_p, which is having 4 active pipes (A, B, C, D)

With this logic, we can run this test with Pipe-A-C only but Pipe-C-A is 
also a valid combo.

Are we not loosing the coverage?

> +			data.pipe1 = pipe_seq[i];
> +			data.pipe2 = pipe_seq[i + 2];
> +			igt_dynamic_f("pipe-%s%s", kmstest_pipe_name(pipe_seq[i]), kmstest_pipe_name(pipe_seq[i + 2]))

Nit: s/"pipe-%s%s"/"pipe-%s-%s"/

> +				test_dual_display(&data);
> +		}
>   	}
>   
>   	igt_fixture {



More information about the igt-dev mailing list