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

[Bommu, Krishnaiah]

> -----Original Message-----
> From: Bommu, Krishnaiah
> Sent: Wednesday, December 13, 2023 8:41 AM
> To: Welty, Brian <brian.welty at intel.com>; Dugast, Francois
> <Francois.Dugast at intel.com>; igt-dev at lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi at 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 at intel.com>
> > Sent: Wednesday, December 13, 2023 3:01 AM
> > To: Dugast, Francois <francois.dugast at intel.com>; igt-
> > dev at lists.freedesktop.org
> > Cc: Bommu, Krishnaiah <krishnaiah.bommu at intel.com>; Vivi, Rodrigo
> > <rodrigo.vivi at 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 at 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 at intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Francois Dugast <francois.dugast at intel.com>
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi at 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 at kbommu-desk:/home/kbommu/xe_public/igt-gpu-tools#

@Welty, Brian this is fine or do you wants to send new patch ?

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]