Re: [PATCH v1 2/6] drm-uapi/xe: add exec_queue_id member to drm_xe_wait_user_fence structure

["Welty, Brian" <brian.welty@intel.com>]

On 12/13/2023 9:07 AM, Bommu, Krishnaiah wrote:
> 
> 
>> -----Original Message-----
>> From: Bommu, Krishnaiah
>> Sent: Wednesday, December 13, 2023 8:41 AM
>> To: Welty, Brian <brian.welty@intel.com>; Dugast, Francois
>> <Francois.Dugast@intel.com>; igt-dev@lists.freedesktop.org
>> Cc: Vivi, Rodrigo <rodrigo.vivi@intel.com>
>> Subject: RE: [PATCH v1 2/6] drm-uapi/xe: add exec_queue_id member to
>> drm_xe_wait_user_fence structure
>>
>>
>>
>>> -----Original Message-----
>>> From: Welty, Brian <brian.welty@intel.com>
>>> Sent: Wednesday, December 13, 2023 3:01 AM
>>> To: Dugast, Francois <francois.dugast@intel.com>; igt-
>>> dev@lists.freedesktop.org
>>> Cc: Bommu, Krishnaiah <krishnaiah.bommu@intel.com>; Vivi, Rodrigo
>>> <rodrigo.vivi@intel.com>
>>> Subject: Re: [PATCH v1 2/6] drm-uapi/xe: add exec_queue_id member to
>>> drm_xe_wait_user_fence structure
>>>
>>>
>>> On 12/12/2023 9:30 AM, Francois Dugast wrote:
>>>> From: Bommu Krishnaiah <krishnaiah.bommu@intel.com>
>>>>
>>>> remove the num_engines/instances members from
>>> drm_xe_wait_user_fence
>>>> structure and add a exec_queue_id member
>>>>
>>>> Right now this is only checking if the engine list is sane and
>>>> nothing else. In the end every operation with this IOCTL is a soft check.
>>>> So, let's formalize that and only use this IOCTL to wait on the fence.
>>>>
>>>> exec_queue_id member will help to user space to get proper error
>>>> code from kernel while in exec_queue reset
>>>>
>>>> v2: Also fix test invalid_flag (Francois Dugast)
>>>>
>>>> Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Francois Dugast <francois.dugast@intel.com>
>>>> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> ---
>>>>    include/drm-uapi/xe_drm.h          | 17 +++++-----------
>>>>    lib/xe/xe_ioctl.c                  | 29 ++++++++++++---------------
>>>>    lib/xe/xe_ioctl.h                  | 11 ++++------
>>>>    tests/intel/xe_evict.c             |  4 ++--
>>>>    tests/intel/xe_exec_balancer.c     | 15 +++++++-------
>>>>    tests/intel/xe_exec_compute_mode.c | 18 ++++++++---------
>>>>    tests/intel/xe_exec_fault_mode.c   | 21 +++++++++++---------
>>>>    tests/intel/xe_exec_reset.c        |  6 +++---
>>>>    tests/intel/xe_exec_threads.c      | 15 +++++++-------
>>>>    tests/intel/xe_waitfence.c         | 32 ++++++++++++++----------------
>>>>    10 files changed, 79 insertions(+), 89 deletions(-)
>>>>
>>> [snip]
>>>> diff --git a/tests/intel/xe_exec_reset.c
>>>> b/tests/intel/xe_exec_reset.c index 094b34896..8e6c2e2e4 100644
>>>> --- a/tests/intel/xe_exec_reset.c
>>>> +++ b/tests/intel/xe_exec_reset.c
>>>> @@ -564,7 +564,7 @@ test_compute_mode(int fd, struct
>>> drm_xe_engine_class_instance *eci,
>>>>    	xe_vm_bind_async(fd, vm, 0, bo, 0, addr, bo_size, sync, 1);
>>>>
>>>>    #define THREE_SEC	MS_TO_NS(3000)
>>>> -	xe_wait_ufence(fd, &data[0].vm_sync, USER_FENCE_VALUE, NULL,
>>> THREE_SEC);
>>>> +	xe_wait_ufence(fd, &data[0].vm_sync, USER_FENCE_VALUE, 0,
>>>> +THREE_SEC);
>>>>    	data[0].vm_sync = 0;
>>>>
>>>>    	for (i = 0; i < n_execs; i++) {
>>>> @@ -621,7 +621,7 @@ test_compute_mode(int fd, struct
>>> drm_xe_engine_class_instance *eci,
>>>>    		int err;
>>>>
>>>>    		err = __xe_wait_ufence(fd, &data[i].exec_sync,
>>> USER_FENCE_VALUE,
>>>> -				       NULL, &timeout);
>>>> +				       exec_queues[i % n_exec_queues],
>>> &timeout);
>>>>    		if (flags & GT_RESET)
>>>>    			/* exec races with reset: may timeout or complete */
>>>>    			igt_assert(err == -ETIME || !err);
>>>
>>> I believe driver now returns EIO instead of ETIME, if I understand the
>>> driver changes from Krishnaiah.
>>> So seems igt_assert above needs to replace -ETIME with -EIO.
>>>
>>> -Brian
>>
>> Yes I agree, we need to change -ETIME to -EIO.
>> Thank you for pointing out
>>
>> Regards,
>> Krishna.
> 
> @Dugast, Francois please add below code while merging the changes
> diff --git a/tests/intel/xe_exec_reset.c b/tests/intel/xe_exec_reset.c
> index 8e6c2e2e4..9d0c7658b 100644
> --- a/tests/intel/xe_exec_reset.c
> +++ b/tests/intel/xe_exec_reset.c
> @@ -623,8 +623,8 @@ test_compute_mode(int fd, struct drm_xe_engine_class_instance *eci,
>                  err = __xe_wait_ufence(fd, &data[i].exec_sync, USER_FENCE_VALUE,
>                                         exec_queues[i % n_exec_queues], &timeout);
>                  if (flags & GT_RESET)
> -                       /* exec races with reset: may timeout or complete */
> -                       igt_assert(err == -ETIME || !err);
> +                       /* exec races with reset: may return -EIO or complete */
> +                       igt_assert(err == -EIO || !err);
>                  else
>                          igt_assert_eq(err, 0);
>          }
> root@kbommu-desk:/home/kbommu/xe_public/igt-gpu-tools#
> 
> @Welty, Brian this is fine or do you wants to send new patch ?
> 

Looks fine to me.  No need to resend.  Thanks.


> Regards,
> Krishna.
>>>
>>>
>>>> @@ -631,7 +631,7 @@ test_compute_mode(int fd, struct
>>>> drm_xe_engine_class_instance *eci,
>>>>
>>>>    	sync[0].addr = to_user_pointer(&data[0].vm_sync);
>>>>    	xe_vm_unbind_async(fd, vm, 0, 0, addr, bo_size, sync, 1);
>>>> -	xe_wait_ufence(fd, &data[0].vm_sync, USER_FENCE_VALUE, NULL,
>>> THREE_SEC);
>>>> +	xe_wait_ufence(fd, &data[0].vm_sync, USER_FENCE_VALUE, 0,
>>>> +THREE_SEC);
>>>>
>>>>    	if (!(flags & GT_RESET)) {
>>>>    		for (i = 1; i < n_execs; i++)
>>> [snip]