From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id F22F510E2A3 for ; Wed, 13 Dec 2023 18:16:22 +0000 (UTC) Message-ID: Date: Wed, 13 Dec 2023 10:16:17 -0800 Subject: Re: [PATCH v1 2/6] drm-uapi/xe: add exec_queue_id member to drm_xe_wait_user_fence structure To: "Bommu, Krishnaiah" , "Dugast, Francois" , "igt-dev@lists.freedesktop.org" References: <20231212173059.7-1-francois.dugast@intel.com> <20231212173059.7-3-francois.dugast@intel.com> <0f65d2d6-9f6e-4fbb-bb01-6121cf77c879@intel.com> Content-Language: en-US From: "Welty, Brian" In-Reply-To: Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit MIME-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "Vivi, Rodrigo" Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: 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 ; Dugast, Francois >> ; igt-dev@lists.freedesktop.org >> Cc: Vivi, Rodrigo >> 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 >>> Sent: Wednesday, December 13, 2023 3:01 AM >>> To: Dugast, Francois ; igt- >>> dev@lists.freedesktop.org >>> Cc: Bommu, Krishnaiah ; Vivi, Rodrigo >>> >>> 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 >>>> >>>> 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 >>>> Cc: Rodrigo Vivi >>>> Cc: Francois Dugast >>>> Reviewed-by: Rodrigo Vivi >>>> --- >>>> 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]