[igt-dev] [PATCH i-g-t v2] tests/kms_plane_scaling: add invalid parameter tests

[Juha-Pekka Heikkila]

Hi Luca, thanks for looking into this.

On 17.1.2023 14.29, Luca Coelho wrote:
> On Mon, 2023-01-16 at 22:39 +0200, Juha-Pekka Heikkila wrote:
>> Add skeleton for adding invalid parameter tests and add two tests which are
>> expected to return -EINVAL or -ERANGE
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
> 
> Looks good! Though I have a few nitpicks and/or questions below.
> 
> 
>>   tests/kms_plane_scaling.c | 73 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 73 insertions(+)
>>
>> diff --git a/tests/kms_plane_scaling.c b/tests/kms_plane_scaling.c
>> index 887a55e63..4ec660bc9 100644
>> --- a/tests/kms_plane_scaling.c
>> +++ b/tests/kms_plane_scaling.c
>> @@ -819,6 +819,77 @@ static void test_scaler_with_multi_pipe_plane(data_t *d)
>>   	igt_assert_eq(ret1 && ret2, 0);
>>   }
>>   
>> +static void invalid_parameter_tests(data_t *d) {
>> +	enum pipe pipe = PIPE_A;
>> +	igt_output_t *output;
>> +	igt_fb_t fb;
>> +	igt_plane_t* plane;
>> +	int32_t rval;
> 
> I'm not sure what the conventions are, but AFAICT
> igt_display_try_commit2() is defined as returning int, and all the
> other ret values in the code here are using "int ret".  Wouldn't it be
> better for consistency?

Yea, I guess it is better to call int as int.

> 
> 
>> +
>> +	const struct {
>> +		const char* testname;
>> +		uint32_t planesize[2];
>> +		struct {
>> +			enum igt_atomic_plane_properties prop;
>> +			uint32_t value;
>> +		} params[8];
>> +	} paramtests[] = {
>> +		{
>> +			.testname = "less-than-1-height-src",
>> +			.planesize = {256, 8},
>> +			.params = {{IGT_PLANE_SRC_H, IGT_FIXED(0, 30) }, {~0}}
>> +		},
>> +		{
>> +			.testname = "less-than-1-width-src",
>> +			.planesize = {8, 256},
>> +			.params = {{IGT_PLANE_SRC_W, IGT_FIXED(0, 30) }, {~0}}
>> +		},
>> +	};
> 
> Why are these defined as locals? Wouldn't it be better to make them
> global, if not for more readability, at least for consistency?

Not sure why should these be global? On igt tests you'll see similar 
structures both global and local, for example kms_cursor_crc or 
kms_rotation_crc has similar tables defined locally but on kms_ccs I 
moved test defining table from local to global because it was needed to 
be accessed globally. Usually I try to avoid putting things global when 
not needed.

> 
> 
>> +
>> +	igt_fixture {
>> +		output = igt_get_single_output_for_pipe(&d->display, pipe);
>> +		igt_require(output);
>> +
>> +		igt_output_set_pipe(output, pipe);
>> +		plane = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY);
>> +
>> +		igt_create_fb(d->drm_fd, 256, 256,
>> +			DRM_FORMAT_XRGB8888,
>> +			DRM_FORMAT_MOD_NONE,
>> +			&fb);
>> +	}
>> +
>> +	igt_subtest_group {
> 
> Why do you need a group with a single subtest?

This was forgotten here :) I earlier had those fixtures inside group but 
then reshaped and moved them around a bit.

> 
> 
>> +		igt_describe("test parameters which should not be accepted");
>> +		igt_subtest_with_dynamic("invalid-parameters") {
>> +			for (uint32_t i = 0; i < ARRAY_SIZE(paramtests); i++) {
>> +				igt_dynamic(paramtests[i].testname) {
>> +					igt_plane_set_position(plane, 0, 0);
>> +					igt_plane_set_fb(plane, &fb);
>> +					igt_plane_set_size(plane,
>> +							   paramtests[i].planesize[0],
>> +							   paramtests[i].planesize[1]);
>> +
>> +
>> +					for (uint32_t j = 0; paramtests[i].params[j].prop != ~0; j++)
>> +						igt_plane_set_prop_value(plane,
>> +									 paramtests[i].params[j].prop,
>> +									 paramtests[i].params[j].value);
>> +
>> +					rval = 	igt_display_try_commit2(&d->display, COMMIT_ATOMIC);
>> +
>> +					igt_assert(rval == -EINVAL || rval == -ERANGE);
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	igt_fixture {
>> +		igt_remove_fb(d->drm_fd, &fb);
>> +		igt_output_set_pipe(output, PIPE_NONE);
>> +	}
>> +}
>> +
>>   static int opt_handler(int opt, int opt_index, void *_data)
>>   {
>>   	data_t *data = _data;
>> @@ -982,6 +1053,8 @@ igt_main_args("", long_opts, help_str, opt_handler, &data)
>>   	igt_subtest_f("2x-scaler-multi-pipe")
>>   		test_scaler_with_multi_pipe_plane(&data);
>>   
>> +	invalid_parameter_tests(&data);
>> +
>>   	igt_fixture {
>>   		igt_display_fini(&data.display);
>>   		close(data.drm_fd);
> 
> --
> Cheers,
> Luca.