Re: [PATCH] drm/xe: Unwrap sync fences before adding them as job dependencies

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] drm/xe: dma_fence leak in xe_sync_entry_add_deps() on error return
--

commit 7c1f1992f566aa87fc231ce7e528d8b1c35d0686
Author: Chen, Junjia <junjia.chen@intel.com>

drm/xe: Unwrap sync fences before adding them as job dependencies

This commit unwraps fence containers in xe_sync_entry_add_deps() using
dma_fence_unwrap_for_each() before adding unsignaled underlying fences as
job dependencies via drm_sched_job_add_dependency(). This allows the DRM
scheduler to recognize same-entity fences wrapped inside timeline syncobj
chain nodes instead of treating the container as a cross-queue dependency.

> diff --git a/drivers/gpu/drm/xe/xe_sync.c b/drivers/gpu/drm/xe/xe_sync.c
> index 06b1c913588ab..73fe7f05ba166 100644
> --- a/drivers/gpu/drm/xe/xe_sync.c
> +++ b/drivers/gpu/drm/xe/xe_sync.c

[ ... ]

> @@ -233,9 +234,30 @@ ALLOW_ERROR_INJECTION(xe_sync_entry_parse, ERRNO);
>  
>  int xe_sync_entry_add_deps(struct xe_sync_entry *sync, struct xe_sched_job *job)
>  {
> -	if (sync->fence)
> -		return  drm_sched_job_add_dependency(&job->drm,
> -						     dma_fence_get(sync->fence));
> +	struct dma_fence_unwrap iter;
> +	struct dma_fence *fence;
> +	int err;
> +
> +	if (!sync->fence)
> +		return 0;
> +
> +	/*
> +	 * Unwrap fence containers (e.g. dma_fence_chain from a timeline
> +	 * syncobj) so the scheduler sees the underlying fences. That lets it
> +	 * drop dependencies on jobs from the same entity and only wait for
> +	 * jobs on the same scheduler to be scheduled, rather than waiting for
> +	 * the container, which forces a full GPU completion wait and drains
> +	 * the queue.
> +	 */
> +	dma_fence_unwrap_for_each(fence, &iter, sync->fence) {
> +		if (dma_fence_is_signaled(fence))
> +			continue;
> +
> +		err = drm_sched_job_add_dependency(&job->drm,
> +						   dma_fence_get(fence));
> +		if (err)
> +			return err;

[Severity: Medium]
Does returning early from inside dma_fence_unwrap_for_each() leak the
dma_fence reference held in iter.chain when drm_sched_job_add_dependency()
fails?

When dma_fence_unwrap_for_each() starts, dma_fence_unwrap_first() takes a
reference on the head fence and stores it in cursor->chain:

drivers/dma-buf/dma-fence-unwrap.c:dma_fence_unwrap_first() {
	cursor->chain = dma_fence_get(head);
	return __dma_fence_unwrap_array(cursor);
}

That reference is only stepped and eventually dropped as
dma_fence_unwrap_next() walks cursor->chain to the end of the chain via
dma_fence_chain_walk():

drivers/dma-buf/dma-fence-unwrap.c:dma_fence_unwrap_next() {
	...
	cursor->chain = dma_fence_chain_walk(cursor->chain);
	return __dma_fence_unwrap_array(cursor);
}

If drm_sched_job_add_dependency() returns an error before the loop finishes
iterating, should xe_sync_entry_add_deps() call dma_fence_put(iter.chain)
before returning err?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026020721.973291-1-junjia.chen@intel.com?part=1