Re: [PATCH v3 1/3] tests/xe_waitfence: Rename invalid_engine to invalid-exec_queue

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jan 11, 2024 at 11:55:16AM +0530, Bommu Krishnaiah wrote:
> Rename invalid_engine to invalid-exec_queue subtest and changed return value
> as per kernel implementation.
> 
> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  tests/intel/xe_waitfence.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/tests/intel/xe_waitfence.c b/tests/intel/xe_waitfence.c
> index 7ba20764c..fd515a151 100644
> --- a/tests/intel/xe_waitfence.c
> +++ b/tests/intel/xe_waitfence.c
> @@ -151,8 +151,8 @@ waitfence(int fd, enum waittype wt)
>   * SUBTEST: invalid-ops
>   * Description: Check query with invalid ops returns expected error code
>   *
> - * SUBTEST: invalid-engine
> - * Description: Check query with invalid engine info returns expected error code
> + * SUBTEST: invalid-exec_queue
> + * Description: Check query with invalid exec_queue info returns expected error code
>   *
>   * SUBTEST: exec_queue-reset-wait
>   * Description: Don’t wait till timeout on user fence when exec_queue reset is detected and return return proper error
> @@ -207,7 +207,7 @@ invalid_ops(int fd)
>  }
>  
>  static void
> -invalid_engine(int fd)
> +invalid_exec_queue(int fd)
>  {
>  	uint32_t bo;
>  
> @@ -218,7 +218,7 @@ invalid_engine(int fd)
>  		.value = 1,
>  		.mask = DRM_XE_UFENCE_WAIT_MASK_U64,
>  		.timeout = -1,
> -		.exec_queue_id = 0,
> +		.exec_queue_id = -1,

The exec_queue_id shouldn't be here. It is totally ignored in the
exec_queue_create ioctl that is what is getting tested here.

>  	};
>  
>  	uint32_t vm = xe_vm_create(fd, 0, 0);
> @@ -227,7 +227,7 @@ invalid_engine(int fd)
>  
>  	do_bind(fd, vm, bo, 0, 0x200000, 0x40000, 1);
>  
> -	do_ioctl_err(fd, DRM_IOCTL_XE_WAIT_USER_FENCE, &wait, EFAULT);
> +	do_ioctl_err(fd, DRM_IOCTL_XE_WAIT_USER_FENCE, &wait, ENOENT);

I'm afraid that this is wrong as well. ENOENT is not returned
in the exec_queue_create.

It is EFAULT that is returned when the wrong 'engine' (hw_engine) is
selected for this exec_queue.

Also, with this in mind, I believe this patch is entirely going to
the wrong direction. We are stop validating the invalid_engine and
trying to validate some other thing.

If you need to validate the exec_queue_id you likely need a new
test, instead of replacing this one like this.

>  }
>  
>  static void
> @@ -329,8 +329,8 @@ igt_main
>  	igt_subtest("invalid-ops")
>  		invalid_ops(fd);
>  
> -	igt_subtest("invalid-engine")
> -		invalid_engine(fd);
> +	igt_subtest("invalid-exec_queue")
> +		invalid_exec_queue(fd);
>  
>  	igt_subtest("exec_queue-reset-wait")
>  		exec_queue_reset_wait(fd);
> -- 
> 2.25.1
>