[igt-dev] [PATCH i-g-t 2/2] tests/kms_addfb_basic: Relax errno assertions

[Jeremy Cline]

On Fri, Nov 13, 2020 at 01:59:55PM -0500, Lyude Paul wrote:
> On Fri, 2020-11-13 at 11:49 -0500, Jeremy Cline wrote:
> > This adjusts the check on failed calls to DRM_IOCTL_MODE_ADDFB2 in
> > various scenarios. According to the kernel's
> > drm_mode_config_funcs.fb_create documentation, a negative return code is
> > all that is required to signal failure, and different drivers return
> > different errors in these scenarios.
> > 
> > Particularly, Nouveau returns -ERANGE rather than -EINVAL in scenarios
> > where the buffer is too small for the proposed configuration.
> > 
> > Signed-off-by: Jeremy Cline <jcline at redhat.com>
> > ---
> >  tests/kms_addfb_basic.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> > index 0ec583fe..b284bfc1 100644
> > --- a/tests/kms_addfb_basic.c
> > +++ b/tests/kms_addfb_basic.c
> > @@ -282,7 +282,7 @@ static void pitch_tests(int fd)
> >                 igt_subtest_f("bad-pitch-%i", bad_pitches[i]) {
> >                         f.pitches[0] = bad_pitches[i];
> >                         igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) ==
> > -1 &&
> > -                                  errno == EINVAL);
> > +                                  errno != 0);
> >                 }
> >         }
> >  
> > @@ -413,11 +413,11 @@ static void size_tests(int fd)
> >         f_8.width++;
> >         igt_subtest("too-wide") {
> >                 igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
> > -                          errno == EINVAL);
> > +                          errno != 0);
> >                 igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f_16) == -1 &&
> > -                          errno == EINVAL);
> > +                          errno != 0);
> >                 igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f_8) == -1 &&
> > -                          errno == EINVAL);
> > +                          errno != 0);
> >         }
> >         f.width--;
> >         f_16.width--;
> > @@ -427,17 +427,17 @@ static void size_tests(int fd)
> >         f_8.height++;
> >         igt_subtest("too-high") {
> >                 igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
> > -                          errno == EINVAL);
> > +                          errno != 0);
> >                 igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f_16) == -1 &&
> > -                          errno == EINVAL);
> > +                          errno != 0);
> >                 igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f_8) == -1 &&
> > -                          errno == EINVAL);
> > +                          errno != 0);
> >         }
> >  
> >         f.handles[0] = gem_bo_small;
> >         igt_subtest("bo-too-small") {
> >                 igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
> > -                          errno == EINVAL);
> > +                          errno != 0);
> 
> Probably should be do_ioctl_err() instead. JFYI too, there's an old outdated
> cocci file for converting things like this automatically that I meant to update.
> I made some progress, but had to put it on the backburner because of time
> constraints.

Ah, neat, thanks for the pointer. It looks like the macro wants one to
provide the exact error. It seems like breaking this out into
vendor-specific tests where it checks for EINVAL for Intel and ERANGE
for Nouveau would be better in any case. Assuming I do that, would it be
good to leave this here for generic "at least don't succeed" testing for
any additional vendors?

- Jeremy