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

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Oct 26, 2026 at 02:24:16AM +0000, sashiko-bot@kernel.org wrote:
> 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)

I traced this through and I believe this feedback is correct - upon
drm_sched_job_add_dependency returning an error,
dma_fence_put(iter.chain) should be called. We probably could rework
dma_fence_unwrap_for_each to be guard based so this happens
automatically when the scope is exitted but we can look at that in a
follow up.

Everything else in this patch LGTM.

Thanks - Matt

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