[igt-dev] [PATCH i-g-t 1/2] tests/intel/xe_create: sanity check all MBZ fields

[Kamil Konieczny]

Hi Matthew,
On 2023-11-30 at 11:17:34 +0000, Matthew Auld wrote:
> Explicitly check that all MBZ fields are rejected by the KMD.

Please add also info about testing extension, which is now unused.
Write also what MBZ means, for example:

Explicitly check that all MBZ (Must Be Zero) fields are rejected.

Btw no need to mension Xe driver.

> 
> Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> Cc: Pallavi Mishra <pallavi.mishra at intel.com>
> Cc: Francois Dugast <francois.dugast at intel.com>
> ---
>  tests/intel/xe_create.c | 59 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 54 insertions(+), 5 deletions(-)
> 
> diff --git a/tests/intel/xe_create.c b/tests/intel/xe_create.c
> index bb55a15e1..b0051f5d5 100644
> --- a/tests/intel/xe_create.c
> +++ b/tests/intel/xe_create.c
> @@ -18,6 +18,18 @@
>  
>  #define PAGE_SIZE 0x1000
>  
> +static int ___create_bo(int fd, struct drm_xe_gem_create *create)

Could you avoid adding yet enother underscore? I would suggest
something simple:

static int __ioctl_create(int fd, struct drm_xe_gem_create *create)

or just __create:

static int __create(int fd, struct drm_xe_gem_create *create)

> +{
> +	int ret = 0;
> +
> +	if (igt_ioctl(fd, DRM_IOCTL_XE_GEM_CREATE, create)) {
> +		ret = -errno;
> +		errno = 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static int __create_bo(int fd, uint32_t vm, uint64_t size, uint32_t flags,
>  		       uint32_t *handlep)
>  {
> @@ -27,14 +39,11 @@ static int __create_bo(int fd, uint32_t vm, uint64_t size, uint32_t flags,
>  		.flags = flags,
>  		.cpu_caching = __xe_default_cpu_caching_from_flags(fd, flags),
>  	};
> -	int ret = 0;
> +	int ret;
>  
>  	igt_assert(handlep);
>  
> -	if (igt_ioctl(fd, DRM_IOCTL_XE_GEM_CREATE, &create)) {
> -		ret = -errno;
> -		errno = 0;
> -	}
> +	ret = ___create_bo(fd, &create);

Please avoid mixing optimizations, here it is simple and may be
accepted but with longer patches would be cumbersome.

Regards,
Kamil

>  	*handlep = create.handle;
>  
>  	return ret;
> @@ -88,6 +97,43 @@ static void create_invalid_size(int fd)
>  	xe_vm_destroy(fd, vm);
>  }
>  
> +/**
> + * SUBTEST: create-invalid-mbz
> + * Functionality: ioctl
> + * Test category: negative test
> + * Description: Verifies xe bo create returns expected error code on all MBZ fields.
> + */
> +static void create_invalid_mbz(int fd)
> +{
> +	struct drm_xe_gem_create create = {
> +		.size = PAGE_SIZE,
> +		.flags = system_memory(fd),
> +		.cpu_caching = DRM_XE_GEM_CPU_CACHING_WB,
> +	};
> +	int i;
> +
> +	/* Make sure the baseline passes */
> +	igt_assert_eq(___create_bo(fd, &create), 0);
> +	gem_close(fd, create.handle);
> +	create.handle = 0;
> +
> +	/* No supported extensions yet */
> +	create.extensions = -1;
> +	igt_assert_eq(___create_bo(fd, &create), -EINVAL);
> +	create.extensions = 0;
> +
> +	/* Make sure KMD rejects non-zero padding/reserved fields */
> +	create.pad = -1;
> +	igt_assert_eq(___create_bo(fd, &create), -EINVAL);
> +	create.pad = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(create.reserved); i++) {
> +		create.reserved[i] = -1;
> +		igt_assert_eq(___create_bo(fd, &create), -EINVAL);
> +		create.reserved[i] = 0;
> +	}
> +}
> +
>  enum exec_queue_destroy {
>  	NOLEAK,
>  	LEAK
> @@ -222,6 +268,9 @@ igt_main
>  	igt_fixture
>  		xe = drm_open_driver(DRIVER_XE);
>  
> +	igt_subtest("create-invalid-mbz")
> +		create_invalid_mbz(xe);
> +
>  	igt_subtest("create-invalid-size") {
>  		create_invalid_size(xe);
>  	}
> -- 
> 2.43.0
>