[igt-dev] [PATCH] tests/kms_addfb_basic: Remove handling for Unknown Vendor.

[Mark Yacoub]

1) I looked at the IOCTL implementation. the IOCTL function makes its
way to drm_internal_framebuffer_create() [1] and framebuffer_check()
[2] where the return values are -EINVAL everywhere except for 2 cases
[3] where they return as handled by Nouveau.  Given that it's in DRM,
I think I can assume it would be a consistent behavior across drivers.

2) yeah, within the tests I ran, it was consistently an EINVAL except
for nouveau, which is still a fair return value.

3) I checked Chrome and it just checks that it's 0, otherwise, it
returns false to the operation.

[1] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_framebuffer.c#L287
[2] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_framebuffer.c#L172
[3] https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_framebuffer.c#L214

On Mon, Sep 27, 2021 at 8:57 AM Petri Latvala <petri.latvala at intel.com> wrote:
>
> On Fri, Sep 24, 2021 at 02:55:10PM -0400, Mark Yacoub wrote:
> > From: Mark Yacoub <markyacoub at google.com>
> >
> > [Why]
> > Looking at multiple drivers such as i915, amdgpu, and msm, EINVAL is the
> > standard errno when failing some fb errors.
> > Unless it's an exception like nouveau, every driver should act in a
> > standard manner and return EINVAL just like every other driver. Hence,
> > no special handling for unknown vendor is needed.
> >
> > [How]
> > Remove asserts for specific drivers except for a known exception.
> >
> > Tested on ChromeOS Trogdor(msm).
> >
> > Signed-off-by: Mark Yacoub <markyacoub at chromium.org>
> > ---
> >  tests/kms_addfb_basic.c | 26 +++++++-------------------
> >  1 file changed, 7 insertions(+), 19 deletions(-)
> >
> > diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> > index 082f10ee..35c2ae2c 100644
> > --- a/tests/kms_addfb_basic.c
> > +++ b/tests/kms_addfb_basic.c
> > @@ -307,17 +307,10 @@ static void pitch_tests(int fd)
> >               igt_subtest_f("bad-pitch-%i", bad_pitches[i]) {
> >                       f.pitches[0] = bad_pitches[i];
> >                       igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &f), -1);
> > -                     igt_assert(errno != 0);
> > -                     if (is_i915_device(fd) || is_amdgpu_device(fd) || is_msm_device(fd)) {
> > +                     if (is_nouveau_device(fd) && bad_pitches[i] > 4 * 1024)
> > +                             igt_assert_eq(errno, ERANGE);
> > +                     else
> >                               igt_assert_eq(errno, EINVAL);
> > -                     } else if (is_nouveau_device(fd)) {
> > -                             if (bad_pitches[i] > 4 * 1024)
> > -                                     igt_assert_eq(errno, ERANGE);
> > -                             else
> > -                                     igt_assert_eq(errno, EINVAL);
> > -                     } else {
> > -                             igt_info("Unknown vendor; errno unchecked (returned %i)", errno);
> > -                     }
> >                       errno = 0;
> >               }
> >       }
> > @@ -479,13 +472,10 @@ static void size_tests(int fd)
> >               for (i = 0; i < ARRAY_SIZE(framebuffers); i++) {
> >                       igt_debug("Checking framebuffer %i\n", i);
> >                       igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, framebuffers[i]), -1);
> > -                     igt_assert(errno != 0);
> > -                     if (is_i915_device(fd))
> > -                             igt_assert_eq(errno, EINVAL);
> > -                     else if (is_nouveau_device(fd))
> > +                     if (is_nouveau_device(fd))
> >                               igt_assert_eq(errno, ERANGE);
> >                       else
> > -                             igt_info("Unknown vendor; errno unchecked (returned %i)", errno);
> > +                             igt_assert_eq(errno, EINVAL);
> >                       errno = 0;
> >               }
> >       }
> > @@ -495,12 +485,10 @@ static void size_tests(int fd)
> >       igt_subtest("bo-too-small") {
> >               igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_MODE_ADDFB2, &f), -1);
> >               igt_assert(errno != 0);
> > -             if (is_i915_device(fd))
> > -                     igt_assert_eq(errno, EINVAL);
> > -             else if (is_nouveau_device(fd))
> > +             if (is_nouveau_device(fd))
> >                       igt_assert_eq(errno, ERANGE);
> >               else
> > -                     igt_info("Unknown vendor; errno unchecked (returned %i)", errno);
> > +                     igt_assert_eq(errno, EINVAL);
> >               errno = 0;
> >       }
>
> Tentatively,
> Reviewed-by: Petri Latvala <petri.latvala at intel.com>
>
> How does this map to the holy trinity of
>
> 1) documentation for the ioctl
> 2) actual behaviour of the ioctl across drivers (this one is already answered in the commit message though)
> 3) how non-IGT userspace handles the ioctl
>
>
> --
> Petri Latvala