Re: [igt-dev] [PATCH i-g-t] tests/xe_create: Remove create-invalid-size subtest

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

Hi, Zbigniew, Maarten

On 10/12/23 09:01, Zbigniew Kempczyński wrote:
> Test exercises kernel returns EINVAL on bo create ioctl with unaligned
> size what was never merged and according to discussion with Thomas
> will never be. Lets remove this test as it is incorrect in this case.
>
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

I have been looking at an implementation over the weekend where the 
buffer object size alignment restrictions are pushed out to the ttm 
resources, so that a buffer object has a 4K-aligned size and any other 
size alignment restrictions are solved by allocating the TTM resource 
larger when needed.

It turns out that this requires changes with TTM, that accounts memory 
using the buffer object size, but also would, I figure, require 
non-standard dma-buf scatter-gather lists if attempting to bind p2p vram 
buffers if not using PCI p2p.

So given the relative simplicity of the -EINVAL solution for unaligned 
buffer objects and the fact that we could relax this in the future if we 
implement pushing out the alignment to TTM resources I can indeed see 
the benefit of using the -EINVAL solution for now.

That would mean keeping this test and adding the check for -EINVAL for 
unaligned size buffer objects.

/Thomas

> ---
>   tests/intel/xe_create.c | 52 -----------------------------------------
>   1 file changed, 52 deletions(-)
>
> diff --git a/tests/intel/xe_create.c b/tests/intel/xe_create.c
> index 8d845e5c86..541fc2b335 100644
> --- a/tests/intel/xe_create.c
> +++ b/tests/intel/xe_create.c
> @@ -39,54 +39,6 @@ static int __create_bo(int fd, uint32_t vm, uint64_t size, uint32_t flags,
>   	return ret;
>   }
>   
> -/**
> - * SUBTEST: create-invalid-size
> - * Functionality: ioctl
> - * Test category: negative test
> - * Description: Verifies xe bo create returns expected error code on invalid
> - *              buffer sizes.
> - */
> -static void create_invalid_size(int fd)
> -{
> -	struct drm_xe_query_mem_region *memregion;
> -	uint64_t memreg = all_memory_regions(fd), region;
> -	uint32_t vm;
> -	uint32_t handle;
> -	int ret;
> -
> -	vm = xe_vm_create(fd, DRM_XE_VM_CREATE_ASYNC_BIND_OPS, 0);
> -
> -	xe_for_each_mem_region(fd, memreg, region) {
> -		memregion = xe_mem_region(fd, region);
> -
> -		/* first try, use half of possible min page size */
> -		ret = __create_bo(fd, vm, memregion->min_page_size >> 1,
> -				  region, &handle);
> -		if (!ret) {
> -			gem_close(fd, handle);
> -			xe_vm_destroy(fd, vm);
> -		}
> -		igt_assert_eq(ret, -EINVAL);
> -
> -		/*
> -		 * second try, add page size to min page size if it is
> -		 * bigger than page size.
> -		 */
> -		if (memregion->min_page_size > PAGE_SIZE) {
> -			ret = __create_bo(fd, vm,
> -					  memregion->min_page_size + PAGE_SIZE,
> -					  region, &handle);
> -			if (!ret) {
> -				gem_close(fd, handle);
> -				xe_vm_destroy(fd, vm);
> -			}
> -			igt_assert_eq(ret, -EINVAL);
> -		}
> -	}
> -
> -	xe_vm_destroy(fd, vm);
> -}
> -
>   enum exec_queue_destroy {
>   	NOLEAK,
>   	LEAK
> @@ -214,10 +166,6 @@ igt_main
>   	igt_fixture
>   		xe = drm_open_driver(DRIVER_XE);
>   
> -	igt_subtest("create-invalid-size") {
> -		create_invalid_size(xe);
> -	}
> -
>   	igt_subtest("create-execqueues-noleak")
>   		create_execqueues(xe, NOLEAK);
>