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

[Luca Coelho]

On Tue, 2023-01-17 at 23:03 +0200, Juha-Pekka Heikkila wrote:
> 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.

Well, you're making them constant, so it really makes no difference in
practice, because it will end up in the same text_section in memory as
if you had defined them as global consts outside the function.

I was just thinking that it would be a bit easier to read if you had
them outside the function.  But if there is common precedent for doing
this, who am I to say otherwise? 😉


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

--
Cheers,
Luca.