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

[Petri Latvala]

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