Re: [PATCH 05/15] dma-buf: inline spinlock for fence protection

[Philipp Stanner <phasta@mailbox.org>]

On Thu, 2025-10-16 at 10:26 +0100, Tvrtko Ursulin wrote:
> 
> Hi Christian,
> 
> Only some preliminary comments while I am building a complete picture.
> 
> On 13/10/2025 14:48, Christian König wrote:
> > Allow implementations to not give a spinlock to protect the fence
> > internal state, instead a spinlock embedded into the fence structure
> > itself is used in this case.
> > 
> > Apart from simplifying the handling for containers and the stub fence
> > this has the advantage of allowing implementations to issue fences
> > without caring about theit spinlock lifetime.
> > 
> > That in turn is necessary for independent fences who outlive the module
> > who originally issued them.

I like the overall idea and think that separate locks will help me with
Rust dma_fence, where I also had begun to investigate the issue of
module unload vs backend_ops.

> > 
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > 

[…]

> >   
> >   static inline struct sync_timeline *dma_fence_parent(struct dma_fence *fence)
> >   {
> > -	return container_of(fence->lock, struct sync_timeline, lock);
> > +	return container_of(fence->extern_lock, struct sync_timeline, lock);
> 
> These container_ofs are a bit annoying. Maybe even a bit fragile if 
> someone switches to embedded lock and forgets to update them all.
> 
> Would prep patch to first replace them with some dma_fence_container_of 
> wrapper make sense? 
> 

+1, would be a nice change.

It's not related to the series, though, so should be done in an
independent patch (IMO).

> Then it could even have a (debug builds only) assert 
> added to check for correct usage.
> 
> >   }
> >   
> >   /**
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> > 
> > 

[…]

> >    *
> > + * DMA_FENCE_FLAG_INLINE_LOCK_BIT - use inline spinlock instead of external one
> >    * DMA_FENCE_FLAG_SIGNALED_BIT - fence is already signaled
> >    * DMA_FENCE_FLAG_TIMESTAMP_BIT - timestamp recorded for fence signaling
> >    * DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT - enable_signaling might have been called
> > @@ -65,7 +67,10 @@ struct seq_file;
> >    * been completed, or never called at all.
> >    */
> >   struct dma_fence {
> > -	spinlock_t *lock;
> > +	union {
> > +		spinlock_t *extern_lock;
> > +		spinlock_t inline_lock;
> 
> This will grow the struct on some architectures so I think, given the
> strong push back to struct past a 64B cacheline in the past, it should 
> be called out in the commit message.

+1

Although: Christian, you told me at XDC that you did some exact
measurements about the new vs old cache line size. Can you help out my
memory here, what were those sizes?


P.