[PATCH v1 2/6] drm-uapi/xe: add exec_queue_id member to drm_xe_wait_user_fence structure
Francois Dugast
francois.dugast at intel.com
Thu Dec 14 09:50:38 UTC 2023
On Wed, Dec 13, 2023 at 10:16:17AM -0800, Welty, Brian wrote:
>
> 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 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 ?
> >
>
> Looks fine to me. No need to resend. Thanks.
Alright, I will make update before pushing.
Francois
>
>
> > 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]
More information about the igt-dev
mailing list