Re: [PATCH 01/15] dma-buf: cleanup dma_fence_describe

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

On Tue, Oct 14, 2025 at 03:37:03PM +0100, Tvrtko Ursulin wrote:
> 
> On 13/10/2025 14:48, Christian König wrote:
> > The driver and timeline name are meaningless for signaled fences.
> > 
> > Drop them and also print the context number.
> > 
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > ---
> >   drivers/dma-buf/dma-fence.c | 11 ++++++-----
> >   1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 3f78c56b58dc..f0539c73ed57 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -1001,17 +1001,18 @@ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq)
> >   {
> >   	const char __rcu *timeline;
> >   	const char __rcu *driver;
> > +	const char *signaled = "un";
> >   	rcu_read_lock();
> >   	timeline = dma_fence_timeline_name(fence);
> >   	driver = dma_fence_driver_name(fence);
> > -	seq_printf(seq, "%s %s seq %llu %ssignalled\n",
> > -		   rcu_dereference(driver),
> > -		   rcu_dereference(timeline),
> > -		   fence->seqno,
> > -		   dma_fence_is_signaled(fence) ? "" : "un");
> > +	if (dma_fence_is_signaled(fence))
> > +		timeline = driver = signaled = "";
> 
> FWIW you could avoid calling dma_fence_timeline_name() and
> dma_fence_driver_name() since you added the signaled check.
> 

+1 to avoid calling dma_fence_timeline_name / dma_fence_driver_name on
signaled fences.

Matt

> May end up slightly nicer than to override strings returned from helpers
> with a chained assignment.
> 
> Or even store the signaled status in a local bool and branch off two
> seq_printfs based on it.
> 
> > +
> > +	seq_printf(seq, "%llu %s %s seq %llu %ssignalled\n", fence->context,
> > +		   timeline, driver, fence->seqno, signaled);
> 
> I was initially worried if this string ends up anywhere which could be
> considered ABI but it seems debugfs only so changing the formatting is fine.
> 
> How about making dma_fence_describe() conditional on CONFIG_DEBUG_FS to set
> this in stone? (And dma_resv_describe..)
> 
> And maybe unify the %llu:%llu context:fence as the tracepoints use?
> 
> Altogether something like:
> 
> rcu_read_lock();
> 
> signaled = dma_fence_is_signaled(fence);
> 
> if (signaled)
> 	seq_printf("%llu:%llu %s %s signalled",
> 		   fence->context,
> 		   fence->seqno,
> 		   dma_fence_timeline_name(fence),
> 		   dma_fence_timeline_name(fence);
> else
> 	seq_print("%llu:%llu unsignalled",
> 		  fence->seqno, fence->context);
> 
> Maybe more readable but up to you.
> 
> Regards,
> 
> Tvrtko
> >   	rcu_read_unlock();
> >   }
>