Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] Tracepoint metric `lock_us` incorrectly reports system uptime on early error paths and includes lock-dropped backoff time during contention retries. --
--- Patch [26]: [PATCH v2 26/33] drm/xe: Add tracepoint for xe_gem_create_ioctl --- commit 245e602165eaf1749a023b1941ce834af1e88d5e Author: Matthew Brost <[email protected]> drm/xe: Add tracepoint for xe_gem_create_ioctl This commit adds a tracepoint to xe_gem_create_ioctl to profile GEM creation, particularly tracking the time spent under the dma-resv lock versus total IOCTL time. The asid is captured at VM lookup time since the VM reference is dropped before the tracepoint fires. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index d9226a3995fe4..c096f3dbd5289 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -4057,6 +4062,10 @@ int xe_gem_create_ioctl(struct drm_device *dev, void > *data, > if (err) > break; > } > + > + if (lock_start == ktime_set(0, 0)) > + lock_start = ktime_get(); [Severity: Medium] Will this include lock-dropped backoff time if there is lock contention? If a retry occurs and the validation loop restarts, lock_start is not reset to the actual lock acquisition time because it is no longer 0. > + > bo = xe_bo_create_user(xe, vm, args->size, args->cpu_caching, > bo_flags, have_prealloc ? &prealloc : > NULL, > &exec); > @@ -4067,6 +4076,7 @@ int xe_gem_create_ioctl(struct drm_device *dev, void > *data, > break; > } > } > + lock_ns = ktime_to_ns(ktime_sub(ktime_get(), lock_start)); [Severity: Medium] Does this calculate a duration based on system uptime on early error paths? If the validation loop breaks early (e.g. from an -EINTR in xe_vm_drm_exec_lock) before lock_start is set, lock_start will still be 0. > if (have_prealloc) > ttm_pool_prealloc_fini(&xe->ttm.pool, &prealloc); > if (err) -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=26
