Re: [PATCH v2] drm/xe/xe_sync: Add debug printing to check_ufence

[Nirmoy Das <nirmoy.das@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 3696 bytes --]


On 12/11/2024 4:24 PM, Cavitt, Jonathan wrote:
>
>  
>
>  
>
> *From:*Nirmoy Das <nirmoy.das@linux.intel.com>
> *Sent:* Friday, December 6, 2024 11:28 PM
> *To:* Cavitt, Jonathan <jonathan.cavitt@intel.com>; intel-xe@lists.freedesktop.org
> *Cc:* Gupta, saurabhg <saurabhg.gupta@intel.com>; Zuo, Alex <alex.zuo@intel.com>; Vivi, Rodrigo <rodrigo.vivi@intel.com>
> *Subject:* Re: [PATCH v2] drm/xe/xe_sync: Add debug printing to check_ufence
>
>  
>
>  
>
> On 12/6/2024 7:11 PM, Jonathan Cavitt wrote:
>
>     The xe_sync helper function check_ufence can occasionally report EBUSY
>
>     if the ufence has not been signalled yet.  EBUSY is a non-fatal error
>
>     value for the function, so it is not desireable to warn in cases where
>
>     EBUSY is reported because it is up to the user to decide if EBUSY is a
>
>     fatal error in their use cases.  However, we can and should report EBUSY
>
>     to the debug logs for diagnostic purposes.
>
>      
>
>     v2: Use vm_dbg instead of XE_IOCTL_DBG (Rodrigo)
>
>      
>
>     Signed-off-by: Jonathan Cavitt <jonathan.cavitt@intel.com> <mailto:jonathan.cavitt@intel.com>
>
>     CC: Rodrigo Vivi <rodrigo.vivi@intel.com> <mailto:rodrigo.vivi@intel.com>
>
>     ---
>
>      drivers/gpu/drm/xe/xe_vm.c | 10 ++++++++--
>
>      1 file changed, 8 insertions(+), 2 deletions(-)
>
>      
>
>     diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
>
>     index 74d684708b00..8c770d1b916c 100644
>
>     --- a/drivers/gpu/drm/xe/xe_vm.c
>
>     +++ b/drivers/gpu/drm/xe/xe_vm.c
>
>     @@ -2402,8 +2402,11 @@ static int op_lock_and_prep(struct drm_exec *exec, struct xe_vm *vm,
>
>                break;
>
>        case DRM_GPUVA_OP_REMAP:
>
>                err = check_ufence(gpuva_to_vma(op->base.remap.unmap->va));
>
>     -          if (err)
>
>     +          if (err) {
>
>     +                  vm_dbg(&vm->xe->drm,
>
>     +                         "REMAP: vma check ufence status = %i\n", err);
>
> Move that to check_ufence() instead so there there is only one copy of logging ?
>
> IMO I think there’s value in knowing which operation is failing, and that information would
>
> be lost if we moved the logging into check_ufence.
>
igt stack strace[1] dumps the operation info and any UMD should be able know the failed operation but not against as it is now:

Reviewed-by: Nirmoy Das <nirmoy.das@intel.com>


[1] https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_12052/shard-bmg-8/igt@xe_exec_compute_mode@many-rebind.html

> -Jonathan Cavitt
>
> Regards,
>
> Nirmoy
>
>
>
>      
>
>                        break;
>
>     +          }
>
>      
>
>                err = vma_lock_and_validate(exec,
>
>                                           gpuva_to_vma(op->base.remap.unmap->va),
>
>     @@ -2415,8 +2418,11 @@ static int op_lock_and_prep(struct drm_exec *exec, struct xe_vm *vm,
>
>                break;
>
>        case DRM_GPUVA_OP_UNMAP:
>
>                err = check_ufence(gpuva_to_vma(op->base.unmap.va));
>
>     -          if (err)
>
>     +          if (err) {
>
>     +                  vm_dbg(&vm->xe->drm,
>
>     +                         "UNMAP: vma check ufence status = %i\n", err);
>
>                        break;
>
>     +          }
>
>      
>
>                err = vma_lock_and_validate(exec,
>
>                                           gpuva_to_vma(op->base.unmap.va),
>

[-- Attachment #2: Type: text/html, Size: 10606 bytes --]