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

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

On 2.4.2019 21.22, Souza, Jose wrote:
> On Tue, 2019-04-02 at 17:42 +0300, Juha-Pekka Heikkila wrote:
>> Before start testing try out how many planes kernel will allow
>> simultaneously to be used.
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>> ---
>>   tests/kms_plane_multiple.c | 32 ++++++++++++++++++++++++++------
>>   1 file changed, 26 insertions(+), 6 deletions(-)
>>
>> diff --git a/tests/kms_plane_multiple.c b/tests/kms_plane_multiple.c
>> index bfaeede..5107d9d 100644
>> --- a/tests/kms_plane_multiple.c
>> +++ b/tests/kms_plane_multiple.c
>> @@ -260,7 +260,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,17 +276,35 @@ 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++;
>> +		prepare_planes(data, pipe, &blue, tiling, c, output);
>> +		err = igt_display_try_commit2(&data->display,
>> COMMIT_ATOMIC);
>> +	} while (!err && c < n_planes);
>> +
>> +	if(err)
>> +		c--;
> 
> "do {} while()" does the job but "for()" would be better suitable:
> 
> for (planes_allowed = 0; planes_allowed < n_planes; planes_allowed++) {
> 	int err;
> 
> 	prepare_planes(data, pipe, &blue, tiling, planes_allowed,
> output);
> 	err = igt_display_try_commit2(&data->display, COMMIT_ATOMIC);
> 	if (err)
> 		break;
> }

I don't have strong preference on this issue. For such loops I normally 
use do{}..while() to make it certain loop is ran at least once in any 
situation and underline it for 'the next guy'.

Anyway your for loop wouldn't do the trick. I think it will become 
somewhat more complex and need different handling for 'all planes did 
work' and 'not all planes did work'. Loop cannot start with zero planes, 
then if n_planes is just one it will never be tested unless specially 
taken care of.

> 
> 
> Also you are leaking a bunch o memory here, each call to
> prepare_planes() will leak max_planes framebuffers, this could cause
> test to fail due no GPU memory available.
> 
> Actually this test is already leaking a bunch as test_fini() is only
> freeing the first framebuffer, there is other leaks here too like:
> data->fb, prepare_planes().x, prepare_planes().y and
> prepare_planes().size.
> 
> Can you take care of that too?
> 

yea, I did see those in Valgrind already while writing this patch. 
Reason why I didn't go after them yet is below 'clean up failed state' 
is something I shouldn't need to do. Last commit in my loop can fail and 
IGT has mechanism to do cleaning, for some reason IGT mechanism misses 
it here. I want to find out how IGT lost the cleaning information and 
this test can show it to me.

> 
>> +
>> +	/*
>> +	 * clean up failed state.
>> +	 */
>> +	for_each_plane_on_pipe(&data->display, pipe, plane)
>> +		igt_plane_set_fb(plane, NULL);
>> +
>> +	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);
>> +	while ((i < iterations || loop_forever) && c > 0) {
> 
> We actually want to fail the test if it can not even test one plane, so
> add this before this while()
> 
> igt_assert_lt(0, planes_allowed);

On after thought checking here for zero is useless, I'll throw it away. 
If there was not a single plane this test would've asserted already 
during setup where it get reference crc.

> 
>> +		prepare_planes(data, pipe, &blue, tiling, c, output);
>>   
>>   		igt_display_commit2(&data->display, COMMIT_ATOMIC);
>>   

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