[igt-dev] [PATCH i-g-t v4 2/2] tests/kms_addfb_basic: add vendor-specific errno assertions

[Lyude Paul]

Reviewed-by: Lyude Paul <lyude at redhat.com>

I'll go ahead and push this, thanks!

On Wed, 2020-11-18 at 10:23 -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.
> 
> In situations where Nouveau and Intel return codes diverge, split out
> the errno assertions into device-specific blocks.
> 
> Reviewed-by: Petri Latvala <petri.latvala at intel.com>
> Signed-off-by: Jeremy Cline <jcline at redhat.com>
> ---
> Changes since v3:
>   - Debug statement in the loop testing too-high framebuffers.
> 
>  tests/kms_addfb_basic.c | 46 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> index 0ec583fe..090abe7f 100644
> --- a/tests/kms_addfb_basic.c
> +++ b/tests/kms_addfb_basic.c
> @@ -281,8 +281,19 @@ static void pitch_tests(int fd)
>         for (i = 0; i < ARRAY_SIZE(bad_pitches); i++) {
>                 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);
> +                       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 (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;
>                 }
>         }
>  
> @@ -367,6 +378,8 @@ static void size_tests(int fd)
>         struct drm_mode_fb_cmd2 f = {};
>         struct drm_mode_fb_cmd2 f_16 = {};
>         struct drm_mode_fb_cmd2 f_8 = {};
> +       struct drm_mode_fb_cmd2 *framebuffers[] = {&f, &f_16, &f_8};
> +       int i;
>  
>         f.width = 1024;
>         f.height = 1024;
> @@ -426,18 +439,31 @@ static void size_tests(int fd)
>         f_16.height++;
>         f_8.height++;
>         igt_subtest("too-high") {
> -               igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f) == -1 &&
> -                          errno == EINVAL);
> -               igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f_16) == -1 &&
> -                          errno == EINVAL);
> -               igt_assert(drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f_8) == -1 &&
> -                          errno == EINVAL);
> +               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))
> +                               igt_assert_eq(errno, ERANGE);
> +                       else
> +                               igt_info("Unknown vendor; errno unchecked
> (returned %i)", errno);
> +                       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);
> +               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))
> +                       igt_assert_eq(errno, ERANGE);
> +               else
> +                       igt_info("Unknown vendor; errno unchecked (returned
> %i)", errno);
> +               errno = 0;
>         }
>  
>         /* Just to check that the parameters would work. */

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!