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

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Wed, 2019-04-10 at 12:55 -0700, Souza, Jose wrote:
> 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.

That is exactly what I said. "Platforms with high amount of planes 
would be slower obviously". However in reality I'm currently testing
it with bandwidth patches: the number of allowed planes can be quite
different, based on tiling and other resources,
so it is really hard to predict from which plane it is beneficial to 
start. For example: on my ICL setup it can sometimes tolerate 8(yeah)
planes on one pipe, however if second pipe is used simultaneously, it
is allowed to have only 1(!) plane. 

> 
> > 
> > > > +		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.

I agree here, naming confused me - there is a try commit2 function with
quite similar name..

> 
> > 
> > > 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++;
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev