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.
linaro-mm-sig@lists.linaro.org
-
Christian König