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

[Petri Latvala]

On Tue, Nov 17, 2020 at 03:02:07PM -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.
> 
> Signed-off-by: Jeremy Cline <jcline at redhat.com>
> ---

Looks good. Just a minor nit below:

> Changes since v2:
>   - Remove vendor-specific subtests.
> 
>  tests/kms_addfb_basic.c | 45 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c
> index 0ec583fe..0e5e3aeb 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,30 @@ 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++) {

If any of the asserts trigger in this for-loop, the failure will just
say "assertion failed, errno xyz instead of EINVAL" etc, losing the
information which fb it was we failed on. Add a igt_debug line right
here:

igt_debug("Checking framebuffer %i\n", i);

With that added,
Reviewed-by: Petri Latvala <petri.latvala at intel.com>



> +			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. */
> -- 
> 2.28.0
>