Re: [PATCH v2 26/33] drm/xe: Add tracepoint for xe_gem_create_ioctl

[sashiko-bot@kernel.org]

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 <matthew.brost@intel.com>

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/20260710215442.2444235-1-matthew.brost@intel.com?part=26