On 2/11/26 10:50, Philipp Stanner wrote:
> On Tue, 2026-02-10 at 11:01 +0100, Christian König wrote:
...
>> Using a per-fence spinlock allows completely decoupling spinlock producer
>> and consumer life times, simplifying the handling in most use cases.
>
> That's a good commit message btw, detailing what the motivation is.
> Would be great to see messages like that more frequently :]
Yeah, but they are not so easy to write.
>> trace_dma_fence_init(fence);
>> @@ -1091,7 +1094,7 @@ __dma_fence_init(struct dma_fence *fence, const struct
>> dma_fence_ops *ops,
>> * dma_fence_init - Initialize a custom fence.
>> * @fence: the fence to initialize
>> * @ops: the dma_fence_ops for operations on this fence
>> - * @lock: the irqsafe spinlock to use for locking this fence
>> + * @lock: optional irqsafe spinlock to use for locking this fence
>> * @context: the execution context this fence is run on
>> * @seqno: a linear increasing sequence number for this context
>> *
>> @@ -1101,6 +1104,10 @@ __dma_fence_init(struct dma_fence *fence, const
>> struct dma_fence_ops *ops,
>> *
>> * context and seqno are used for easy comparison between fences, allowing
>> * to check which fence is later by simply using dma_fence_later().
>> + *
>> + * It is strongly discouraged to provide an external lock. This is only
>> allowed
>
> "strongly discouraged […] because this does not decouple lock and fence
> life times." ?
Good point, added some more text.
>> + * for legacy use cases when multiple fences need to be prevented from
>> + * signaling out of order.
>
> I think our previous discussions revealed that the external lock does
> not even help with that, does it?
Well only when you provide a ->signaled() callback in the dma_fence_ops.
The reason we have so much different approaches in the dma_fence handling is
because it is basically the unification multiple different driver
implementations which all targeted more or less different use cases.
>> + * for legacy use cases when multiple fences need to be prevented from
>> + * signaling out of order.
>> */
>> void
>> dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
>> diff --git a/drivers/dma-buf/sync_debug.h b/drivers/dma-buf/sync_debug.h
>> index 02af347293d0..c49324505b20 100644
>> --- a/drivers/dma-buf/sync_debug.h
>> +++ b/drivers/dma-buf/sync_debug.h
>> @@ -47,7 +47,7 @@ struct sync_timeline {
>>
>> 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);
>
> You're sure that this will never have to check for the flag?
Yes, the code would have crashed before if anything than a sync_pt created by
sync_pt_create was encountered here.
We could drop the wrapper, move the cast to the only place where it matters and
document the why and what with a code comment.... but this is all dead code
which breaks some of the fundamental dma-fence rules and it is only left here
because we can't break the UAPI.
>> static const char *xe_hw_fence_get_driver_name(struct dma_fence *dma_fence)
>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>> index 88c842fc35d5..6eabbb1c471c 100644
>> --- a/include/linux/dma-fence.h
>> +++ b/include/linux/dma-fence.h
>> @@ -34,7 +34,8 @@ struct seq_file;
>> * @ops: dma_fence_ops associated with this fence
>> * @rcu: used for releasing fence with kfree_rcu
>> * @cb_list: list of all callbacks to call
>> - * @lock: spin_lock_irqsave used for locking
>> + * @extern_lock: external spin_lock_irqsave used for locking
>
> Add a "(deprecated)" ?
Done.
>
>> + * @inline_lock: alternative internal spin_lock_irqsave used for locking
>> * @context: execution context this fence belongs to, returned by
>> * dma_fence_context_alloc()
>> * @seqno: the sequence number of this fence inside the execution context,
>> @@ -49,6 +50,7 @@ struct seq_file;
>> * of the time.
>> *
>> * DMA_FENCE_FLAG_INITIALIZED_BIT - fence was initialized
>> + * 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
>> @@ -66,7 +68,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;
>> + };
>> const struct dma_fence_ops __rcu *ops;
>> /*
>> * We clear the callback list on kref_put so that by the time we
>> @@ -100,6 +105,7 @@ struct dma_fence {
>>
>> enum dma_fence_flag_bits {
>> DMA_FENCE_FLAG_INITIALIZED_BIT,
>> + DMA_FENCE_FLAG_INLINE_LOCK_BIT,
>
> Just asking about a nit: what's the order here, always alphabetically?
In which the flags are used in the code flow.
>> DMA_FENCE_FLAG_SEQNO64_BIT,
>> DMA_FENCE_FLAG_SIGNALED_BIT,
>> DMA_FENCE_FLAG_TIMESTAMP_BIT,
>> @@ -381,11 +387,12 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep)
>> * dma_fence_spinlock - return pointer to the spinlock protecting the fence
>> * @fence: the fence to get the lock from
>> *
>> - * Return the pointer to the extern lock.
>> + * Return either the pointer to the embedded or the external spin lock.
>> */
>> static inline spinlock_t *dma_fence_spinlock(struct dma_fence *fence)
>> {
>> - return fence->lock;
>> + return test_bit(DMA_FENCE_FLAG_INLINE_LOCK_BIT, &fence->flags) ?
>> + &fence->inline_lock : fence->extern_lock;
>
> I personally am not a fan of using '?' for anything longer than 1 line
> and think that
>
> if (condition)
> return a;
>
> return b;
>
> is much better readable.
Mhm, I disagree in this particular case. Especially that you have both
possibilities side by side makes it more readable I think.
Regards,
Christian.
