Re: [PATCH 5/9] dma-buf: inline spinlock for fence protection v4

["Christian König" <christian.koenig@amd.com>]

On 1/20/26 12:41, Tvrtko Ursulin wrote:
> 
> On 20/01/2026 10:54, Christian König wrote:
>> Implement per-fence spinlocks, allowing implementations to not give an
>> external spinlock to protect the fence internal statei. Instead a spinlock
>> embedded into the fence structure itself is used in this case.
>>
>> Shared spinlocks have the problem that implementations need to guarantee
>> that the lock live at least as long all fences referencing them.
>>
>> Using a per-fence spinlock allows completely decoupling spinlock producer
>> and consumer life times, simplifying the handling in most use cases.
>>
>> v2: improve naming, coverage and function documentation
>> v3: fix one additional locking in the selftests
>> v4: separate out some changes to make the patch smaller,
>>      fix one amdgpu crash found by CI systems
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/dma-buf/dma-fence.c             | 25 +++++++++++++++++-------
>>   drivers/dma-buf/sync_debug.h            |  2 +-
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |  2 +-
>>   drivers/gpu/drm/drm_crtc.c              |  2 +-
>>   drivers/gpu/drm/drm_writeback.c         |  2 +-
>>   drivers/gpu/drm/nouveau/nouveau_fence.c |  3 ++-
>>   drivers/gpu/drm/qxl/qxl_release.c       |  3 ++-
>>   drivers/gpu/drm/vmwgfx/vmwgfx_fence.c   |  3 ++-
>>   drivers/gpu/drm/xe/xe_hw_fence.c        |  3 ++-
> 
> i915 needed changes too, based on the kbuild report.

Going to take a look now.
 
> Have you seen my note about the RCU sparse warning as well?

Nope, I must have missed that mail.

...
>>   +/**
>> + * dma_fence_spinlock - return pointer to the spinlock protecting the fence
>> + * @fence: the fence to get the lock from
>> + *
>> + * Return either the pointer to the embedded or the external spin lock.
>> + */
>> +static inline spinlock_t *dma_fence_spinlock(struct dma_fence *fence)
>> +{
>> +    return test_bit(DMA_FENCE_FLAG_INLINE_LOCK_BIT, &fence->flags) ?
>> +        &fence->inline_lock : fence->extern_lock;
>> +}
> 
> You did not want to move this helper into "dma-buf: abstract fence locking" ?

I was avoiding that to keep the pre-requisite patch smaller, cause this change here seemed independent to that.

But thinking about it I could make a third patch which introduces dma_fence_spinlock() and changes all the container_of uses.

> I think that would have been better to keep everything mechanical in one patch, and then this patch which changes behaviour does not touch any drivers but only dma-fence core.
> 
> Also, what about adding something like dma_fence_container_of() in that patch as well?

I would rather like to avoid that. Using the spinlock pointer with container_of seemed to be a bit of a hack to me in the first place and I don't want to encourage people to do that in new code as well.

Regards,
Christian.

> 
> Regards,
> 
> Tvrtko
> 
>> +
>>   /**
>>    * dma_fence_lock_irqsave - irqsave lock the fence
>>    * @fence: the fence to lock
>> @@ -385,7 +403,7 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
>>    * Lock the fence, preventing it from changing to the signaled state.
>>    */
>>   #define dma_fence_lock_irqsave(fence, flags)    \
>> -    spin_lock_irqsave(fence->lock, flags)
>> +    spin_lock_irqsave(dma_fence_spinlock(fence), flags)
>>     /**
>>    * dma_fence_unlock_irqrestore - unlock the fence and irqrestore
>> @@ -395,7 +413,7 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
>>    * Unlock the fence, allowing it to change it's state to signaled again.
>>    */
>>   #define dma_fence_unlock_irqrestore(fence, flags)    \
>> -    spin_unlock_irqrestore(fence->lock, flags)
>> +    spin_unlock_irqrestore(dma_fence_spinlock(fence), flags)
>>     #ifdef CONFIG_LOCKDEP
>>   bool dma_fence_begin_signalling(void);
>