Re: [igt-dev] [PATCH i-g-t] tests/kms_plane_multiple: before going testing check how many planes are allowed

["Souza, Jose" <jose.souza@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9107 bytes --]

On Tue, 2019-04-09 at 13:47 +0100, Lisovskiy, Stanislav wrote:
> On Mon, 2019-04-08 at 18:29 -0700, Souza, Jose wrote:
> > On Fri, 2019-04-05 at 17:55 +0300, Juha-Pekka Heikkila wrote:
> > > before start testing try out how many planes kernel will
> > > allow simultaneously to be used.
> > > 
> > > v2 (Jose Souza, Daniel Vetter): Fix resource leaks. Randomize
> > > used planes.
> > > 
> > > Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
> > > ---
> > >  tests/kms_plane_multiple.c | 88
> > > +++++++++++++++++++++++++++++++++++++---------
> > >  1 file changed, 71 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/tests/kms_plane_multiple.c
> > > b/tests/kms_plane_multiple.c
> > > index bfaeede..f2707dc 100644
> > > --- a/tests/kms_plane_multiple.c
> > > +++ b/tests/kms_plane_multiple.c
> > > @@ -80,16 +80,6 @@ static void test_fini(data_t *data,
> > > igt_output_t
> > > *output, int n_planes)
> > >  {
> > >  	igt_pipe_crc_stop(data->pipe_crc);
> > >  
> > > -	for (int i = 0; i < n_planes; i++) {
> > > -		igt_plane_t *plane = data->plane[i];
> > > -		if (!plane)
> > > -			continue;
> > > -		if (plane->type == DRM_PLANE_TYPE_PRIMARY)
> > > -			continue;
> > > -		igt_plane_set_fb(plane, NULL);
> > > -		data->plane[i] = NULL;
> > > -	}
> > > -
> > >  	/* reset the constraint on the pipe */
> > >  	igt_output_set_pipe(output, PIPE_ANY);
> > >  
> > > @@ -99,7 +89,8 @@ static void test_fini(data_t *data,
> > > igt_output_t
> > > *output, int n_planes)
> > >  	free(data->plane);
> > >  	data->plane = NULL;
> > >  
> > > -	igt_remove_fb(data->drm_fd, data->fb);
> > > +	free(data->fb);
> > > +	data->fb = NULL;
> > >  
> > >  	igt_display_reset(&data->display);
> > >  }
> > > @@ -195,6 +186,7 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > >  	int *y;
> > >  	int *size;
> > >  	int i;
> > > +	int* suffle;
> > >  
> > >  	igt_output_set_pipe(output, pipe_id);
> > >  	primary = igt_output_get_plane_type(output,
> > > DRM_PLANE_TYPE_PRIMARY);
> > > @@ -206,6 +198,34 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > >  	igt_assert_f(y, "Failed to allocate %ld bytes for variable
> > > y\n", (long int) (pipe->n_planes * sizeof(*y)));
> > >  	size = malloc(pipe->n_planes * sizeof(*size));
> > >  	igt_assert_f(size, "Failed to allocate %ld bytes for variable
> > > size\n", (long int) (pipe->n_planes * sizeof(*size)));
> > > +	suffle = malloc(pipe->n_planes * sizeof(*suffle));
> > > +	igt_assert_f(suffle, "Failed to allocate %ld bytes for variable
> > > size\n", (long int) (pipe->n_planes * sizeof(*suffle)));
> > > +
> > > +	for (i = 0; i < pipe->n_planes; i++)
> > > +		suffle[i] = i;
> > > +
> > > +	/*
> > > +	 * suffle table for planes. using rand() should keep it
> > > +	 * 'randomized in expected way'
> > > +	 */
> > > +	for (i = 0; i < 256; i++) {
> > 
> > Not beautiful at all
> > 
> > > +		int n, m;
> > > +		int a, b;
> > > +
> > > +		n = rand() % (pipe->n_planes-1);
> > > +		m = rand() % (pipe->n_planes-1);
> > > +
> > > +		/*
> > > +		 * keep primary plane at its place for test's sake.
> > > +		 */
> > > +		if(n == primary->index || m == primary->index)
> > > +			continue;
> > > +
> > > +		a = suffle[n];
> > > +		b = suffle[m];
> > > +		suffle[n] = b;
> > > +		suffle[m] = a;
> > > +	}
> > >  
> > >  	mode = igt_output_get_mode(output);
> > >  
> > > @@ -213,7 +233,11 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > >  	x[primary->index] = 0;
> > >  	y[primary->index] = 0;
> > >  	for (i = 0; i < max_planes; i++) {
> > > -		igt_plane_t *plane = igt_output_get_plane(output, i);
> > > +		/*
> > > +		 * Here is made assumption primary plane will have
> > > +		 * index zero.
> > > +		 */
> > > +		igt_plane_t *plane = igt_output_get_plane(output,
> > > suffle[i]);
> > >  		uint32_t plane_format;
> > >  		uint64_t plane_tiling;
> > >  
> > > @@ -251,6 +275,10 @@ prepare_planes(data_t *data, enum pipe
> > > pipe_id,
> > > color_t *color,
> > >  	create_fb_for_mode_position(data, output, mode, color, x, y,
> > >  				    size, size, tiling, max_planes);
> > >  	igt_plane_set_fb(data->plane[primary->index], &data-
> > > > fb[primary->index]);
> > > 
> > > +	free((void*)x);
> > > +	free((void*)y);
> > > +	free((void*)size);
> > > +	free((void*)suffle);
> > >  }
> > >  
> > >  static void
> > > @@ -260,7 +288,9 @@ test_plane_position_with_output(data_t *data,
> > > enum pipe pipe,
> > >  {
> > >  	color_t blue  = { 0.0f, 0.0f, 1.0f };
> > >  	igt_crc_t crc;
> > > +	igt_plane_t *plane;
> > >  	int i;
> > > +	int err, c = 0;
> > >  	int iterations = opt.iterations < 1 ? 1 : opt.iterations;
> > >  	bool loop_forever;
> > >  	char info[256];
> > > @@ -274,22 +304,46 @@ test_plane_position_with_output(data_t
> > > *data,
> > > enum pipe pipe,
> > >  			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);
> > >  
> > > +	/*
> > > +	 * Find out how many planes are allowed simultaneously
> > > +	 */
> > > +	do {
> > > +		c++;
> > 
> > Adding one plane at time is a waste of CI time in platforms without
> > a
> > high number of planes(anything besides ICL), better start with max
> > and
> > decreasing until commit is accepted.
> 
> It is actually opposite. Platforms with high amount of planes would
> be slower obviously, as you will have to increment more and faster 
> for systems with less amount of planes. Consider 0->7 and 0-
> > 3(platforms with less amount of planes) - so where is more waste
> of time?

Platforms with 3 planes will do 3 iterations with current approach
while it could be done with just one and ICL will will do 5
iterations(my ICL setup support 5 planes at the same time) while it
could be done in 3 iterations.

Now takes this additional iterations * 4 tests per pipe * number of
pipes and you have a considerable waste of CI time.

> 
> > > +		prepare_planes(data, pipe, &blue, tiling, c, output);
> > > +		err = igt_display_try_commit2(&data->display,
> > > COMMIT_ATOMIC);
> > 
> > No need to do a real commit here, you can only test with:
> 
> This is igt_display_TRY_commit2.

The TRY here means that the test will not fail in case the commit is
not accepted/valid, it will not call drmModeAtomicCommit() with
DRM_MODE_ATOMIC_TEST_ONLY, this will save some time when the commit is
valid/accepted.

> 
> > err = igt_display_try_commit_atomic(&data->display,
> > DRM_MODE_ATOMIC_TEST_ONLY | DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > 
> > 
> > Also if a err different than -EINVAL is returned you should handle
> > like
> > a error.
> > 
> > > +
> > > +		for_each_plane_on_pipe(&data->display, pipe, plane)
> > > +			igt_plane_set_fb(plane, NULL);
> > > +
> > > +		for (int x = 0; x < c; x++)
> > > +			igt_remove_fb(data->drm_fd, &data->fb[x]);
> > 
> > This is still leaking, example:
> > 
> > c = 3
> > plane 0, plane 1 and plane 3 enabled
> 
> There should be no leak here. He uses suffle[i] 
> for index indirection in prepare_planes like igt_plane_t *plane =
> igt_output_get_plane(output, suffle[i]), so that suffle[0]=0,
> suffle[1]=1, suffle[2]=3, meanwhile correspondent data->fb[i]
> remains contiguous, so that plane 0 corresponds to data->fb[0],
> plane 1 -> data->fb[1] and plane 3 -> data->fb[2]. 
> So all the igt_remove_fb calls target correct data->fb[] through
> suffle[i] used as indirection layer.

Okay

> 
> > Other leak, the framebuffer created in test_grab_crc()
> > 
> > 
> > > +	} while (!err && c < n_planes);
> > > +
> > > +	if(err)
> > > +		c--;
> > > +
> > > +	igt_info("Testing connector %s using pipe %s with %d planes %s
> > > with seed %d\n",
> > > +		 igt_output_name(output), kmstest_pipe_name(pipe), c,
> > > +		 info, opt.seed);
> > > +
> > >  	i = 0;
> > >  	while (i < iterations || loop_forever) {
> > > -		prepare_planes(data, pipe, &blue, tiling, n_planes,
> > > output);
> > > +		prepare_planes(data, pipe, &blue, tiling, c, output);
> > >  
> > >  		igt_display_commit2(&data->display, COMMIT_ATOMIC);
> > >  
> > >  		igt_pipe_crc_get_current(data->display.drm_fd, data-
> > > > pipe_crc, &crc);
> > > 
> > >  
> > > +		for_each_plane_on_pipe(&data->display, pipe, plane)
> > > +			igt_plane_set_fb(plane, NULL);
> > > +
> > > +		for (int x = 0; x < c; x++)
> > > +			igt_remove_fb(data->drm_fd, &data->fb[x]);
> > 
> > move this cleanup to function and call it here and where you test
> > the
> > number of planes that can be enabled
> > 
> > > +
> > >  		igt_assert_crc_equal(&data->ref_crc, &crc);
> > >  
> > >  		i++;

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 153 bytes --]

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