[igt-dev] [PATCH i-g-t v2] tests/kms_plane_scaling: add invalid parameter tests
Luca Coelho
luca at coelho.fi
Tue Jan 17 21:13:57 UTC 2023
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.
More information about the igt-dev
mailing list