Hi everyone,
dma_fences have ever lived under the tyranny dictated by the module lifetime of their issuer, leading to crashes should anybody still holding a reference to a dma_fence when the module of the issuer was unloaded.
The basic problem is that when buffer are shared between drivers dma_fence objects can leak into external drivers and stay there even after they are signaled. The dma_resv object for example only lazy releases dma_fences.
So what happens is that when the module who originally created the dma_fence unloads the dma_fence_ops function table becomes unavailable as well and so any attempt to release the fence crashes the system.
Previously various approaches have been discussed, including changing the locking semantics of the dma_fence callbacks (by me) as well as using the drm scheduler as intermediate layer (by Sima) to disconnect dma_fences from their actual users, but none of them are actually solving all problems.
Tvrtko did some really nice prerequisite work by protecting the returned strings of the dma_fence_ops by RCU. This way dma_fence creators where able to just wait for an RCU grace period after fence signaling before they could be save to free those data structures.
Now this patch set here goes a step further and protects the whole dma_fence_ops structure by RCU, so that after the fence signals the pointer to the dma_fence_ops is set to NULL when there is no wait nor release callback given. All functionality which use the dma_fence_ops reference are put inside an RCU critical section, except for the deprecated issuer specific wait and of course the optional release callback.
Additional to the RCU changes the lock protecting the dma_fence state previously had to be allocated external. This set here now changes the functionality to make that external lock optional and allows dma_fences to use an inline lock and be self contained.
v4:
Rebases the whole set on upstream changes, especially the cleanup from Philip in patch "drm/amdgpu: independence for the amdkfd_fence!".
Adding two patches which brings the DMA-fence self tests up to date. The first selftest changes removes the mock_wait and so actually starts testing the default behavior instead of some hacky implementation in the test. This one should probably go upstream independent of this set. The second drops the mock_fence as well and tests the new RCU and inline spinlock functionality.
Especially the first patch still needs a Reviewed-by, apart from that I think I've addressed all review comments.
The plan is to push the core DMA-buf changes to drm-misc-next and then the driver specific changes through the driver channels as approprite.
Please review and comment, Christian.
At first glance it is counter intuitive to protect a constant function pointer table by RCU, but this allows modules providing the function table to unload by waiting for an RCU grace period.
v2: make one the now duplicated lockdep warnings a comment instead. v3: Add more documentation to ->wait and ->release callback. v4: fix typo in documentation v5: rebased on drm-tip
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/dma-fence.c | 69 +++++++++++++++++++++++++------------ include/linux/dma-fence.h | 29 ++++++++++++++-- 2 files changed, 73 insertions(+), 25 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 21c5c30b4f34..e3e74f98f58d 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -522,6 +522,7 @@ EXPORT_SYMBOL(dma_fence_signal); signed long dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) { + const struct dma_fence_ops *ops; signed long ret;
if (WARN_ON(timeout < 0)) @@ -533,15 +534,21 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout)
dma_fence_enable_sw_signaling(fence);
- if (trace_dma_fence_wait_start_enabled()) { - rcu_read_lock(); - trace_dma_fence_wait_start(fence); + rcu_read_lock(); + ops = rcu_dereference(fence->ops); + trace_dma_fence_wait_start(fence); + if (ops->wait) { + /* + * Implementing the wait ops is deprecated and not supported for + * issuer independent fences, so it is ok to use the ops outside + * the RCU protected section. + */ + rcu_read_unlock(); + ret = ops->wait(fence, intr, timeout); + } else { rcu_read_unlock(); - } - if (fence->ops->wait) - ret = fence->ops->wait(fence, intr, timeout); - else ret = dma_fence_default_wait(fence, intr, timeout); + } if (trace_dma_fence_wait_end_enabled()) { rcu_read_lock(); trace_dma_fence_wait_end(fence); @@ -562,6 +569,7 @@ void dma_fence_release(struct kref *kref) { struct dma_fence *fence = container_of(kref, struct dma_fence, refcount); + const struct dma_fence_ops *ops;
rcu_read_lock(); trace_dma_fence_destroy(fence); @@ -593,12 +601,12 @@ void dma_fence_release(struct kref *kref) spin_unlock_irqrestore(fence->lock, flags); }
- rcu_read_unlock(); - - if (fence->ops->release) - fence->ops->release(fence); + ops = rcu_dereference(fence->ops); + if (ops->release) + ops->release(fence); else dma_fence_free(fence); + rcu_read_unlock(); } EXPORT_SYMBOL(dma_fence_release);
@@ -617,6 +625,7 @@ EXPORT_SYMBOL(dma_fence_free);
static bool __dma_fence_enable_signaling(struct dma_fence *fence) { + const struct dma_fence_ops *ops; bool was_set;
lockdep_assert_held(fence->lock); @@ -627,14 +636,18 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence) if (dma_fence_test_signaled_flag(fence)) return false;
- if (!was_set && fence->ops->enable_signaling) { + rcu_read_lock(); + ops = rcu_dereference(fence->ops); + if (!was_set && ops->enable_signaling) { trace_dma_fence_enable_signal(fence);
- if (!fence->ops->enable_signaling(fence)) { + if (!ops->enable_signaling(fence)) { + rcu_read_unlock(); dma_fence_signal_locked(fence); return false; } } + rcu_read_unlock();
return true; } @@ -1007,8 +1020,13 @@ EXPORT_SYMBOL(dma_fence_wait_any_timeout); */ void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline) { - if (fence->ops->set_deadline && !dma_fence_is_signaled(fence)) - fence->ops->set_deadline(fence, deadline); + const struct dma_fence_ops *ops; + + rcu_read_lock(); + ops = rcu_dereference(fence->ops); + if (ops->set_deadline && !dma_fence_is_signaled(fence)) + ops->set_deadline(fence, deadline); + rcu_read_unlock(); } EXPORT_SYMBOL(dma_fence_set_deadline);
@@ -1049,7 +1067,12 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
kref_init(&fence->refcount); - fence->ops = ops; + /* + * At first glance it is counter intuitive to protect a constant + * function pointer table by RCU, but this allows modules providing the + * function table to unload by waiting for an RCU grace period. + */ + RCU_INIT_POINTER(fence->ops, ops); INIT_LIST_HEAD(&fence->cb_list); fence->lock = lock; fence->context = context; @@ -1129,11 +1152,12 @@ EXPORT_SYMBOL(dma_fence_init64); */ const char __rcu *dma_fence_driver_name(struct dma_fence *fence) { - RCU_LOCKDEP_WARN(!rcu_read_lock_held(), - "RCU protection is required for safe access to returned string"); + const struct dma_fence_ops *ops;
+ /* RCU protection is required for safe access to returned string */ + ops = rcu_dereference(fence->ops); if (!dma_fence_test_signaled_flag(fence)) - return fence->ops->get_driver_name(fence); + return ops->get_driver_name(fence); else return "detached-driver"; } @@ -1161,11 +1185,12 @@ EXPORT_SYMBOL(dma_fence_driver_name); */ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence) { - RCU_LOCKDEP_WARN(!rcu_read_lock_held(), - "RCU protection is required for safe access to returned string"); + const struct dma_fence_ops *ops;
+ /* RCU protection is required for safe access to returned string */ + ops = rcu_dereference(fence->ops); if (!dma_fence_test_signaled_flag(fence)) - return fence->ops->get_timeline_name(fence); + return ops->get_timeline_name(fence); else return "signaled-timeline"; } diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index d4c92fd35092..eea674acdfa6 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -66,7 +66,7 @@ struct seq_file; */ struct dma_fence { spinlock_t *lock; - const struct dma_fence_ops *ops; + const struct dma_fence_ops __rcu *ops; /* * We clear the callback list on kref_put so that by the time we * release the fence it is unused. No one should be adding to the @@ -218,6 +218,10 @@ struct dma_fence_ops { * timed out. Can also return other error values on custom implementations, * which should be treated as if the fence is signaled. For example a hardware * lockup could be reported like that. + * + * Implementing this callback prevents the fence from detaching after + * signaling and so it is mandatory for the module providing the + * dma_fence_ops to stay loaded as long as the dma_fence exists. */ signed long (*wait)(struct dma_fence *fence, bool intr, signed long timeout); @@ -229,6 +233,13 @@ struct dma_fence_ops { * Can be called from irq context. This callback is optional. If it is * NULL, then dma_fence_free() is instead called as the default * implementation. + * + * Implementing this callback prevents the fence from detaching after + * signaling and so it is mandatory for the module providing the + * dma_fence_ops to stay loaded as long as the dma_fence exists. + * + * If the callback is implemented the memory backing the dma_fence + * object must be freed RCU safe. */ void (*release)(struct dma_fence *fence);
@@ -439,13 +450,19 @@ dma_fence_test_signaled_flag(struct dma_fence *fence) static inline bool dma_fence_is_signaled_locked(struct dma_fence *fence) { + const struct dma_fence_ops *ops; + if (dma_fence_test_signaled_flag(fence)) return true;
- if (fence->ops->signaled && fence->ops->signaled(fence)) { + rcu_read_lock(); + ops = rcu_dereference(fence->ops); + if (ops->signaled && ops->signaled(fence)) { + rcu_read_unlock(); dma_fence_signal_locked(fence); return true; } + rcu_read_unlock();
return false; } @@ -469,13 +486,19 @@ dma_fence_is_signaled_locked(struct dma_fence *fence) static inline bool dma_fence_is_signaled(struct dma_fence *fence) { + const struct dma_fence_ops *ops; + if (dma_fence_test_signaled_flag(fence)) return true;
- if (fence->ops->signaled && fence->ops->signaled(fence)) { + rcu_read_lock(); + ops = rcu_dereference(fence->ops); + if (ops->signaled && ops->signaled(fence)) { + rcu_read_unlock(); dma_fence_signal(fence); return true; } + rcu_read_unlock();
return false; }
When neither a release nor a wait backend ops is specified it is possible to let the dma_fence live on independently of the module who issued it.
This makes it possible to unload drivers and only wait for all their fences to signal.
v2: fix typo in comment
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Reviewed-by: Philipp Stanner phasta@kernel.org --- drivers/dma-buf/dma-fence.c | 16 ++++++++++++---- include/linux/dma-fence.h | 4 ++-- 2 files changed, 14 insertions(+), 6 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index e3e74f98f58d..481f1cd9aae2 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -371,6 +371,14 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, &fence->flags))) return;
+ /* + * When neither a release nor a wait operation is specified set the ops + * pointer to NULL to allow the fence structure to become independent + * from who originally issued it. + */ + if (!fence->ops->release && !fence->ops->wait) + RCU_INIT_POINTER(fence->ops, NULL); + /* Stash the cb_list before replacing it with the timestamp */ list_replace(&fence->cb_list, &cb_list);
@@ -537,7 +545,7 @@ dma_fence_wait_timeout(struct dma_fence *fence, bool intr, signed long timeout) rcu_read_lock(); ops = rcu_dereference(fence->ops); trace_dma_fence_wait_start(fence); - if (ops->wait) { + if (ops && ops->wait) { /* * Implementing the wait ops is deprecated and not supported for * issuer independent fences, so it is ok to use the ops outside @@ -602,7 +610,7 @@ void dma_fence_release(struct kref *kref) }
ops = rcu_dereference(fence->ops); - if (ops->release) + if (ops && ops->release) ops->release(fence); else dma_fence_free(fence); @@ -638,7 +646,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
rcu_read_lock(); ops = rcu_dereference(fence->ops); - if (!was_set && ops->enable_signaling) { + if (!was_set && ops && ops->enable_signaling) { trace_dma_fence_enable_signal(fence);
if (!ops->enable_signaling(fence)) { @@ -1024,7 +1032,7 @@ void dma_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
rcu_read_lock(); ops = rcu_dereference(fence->ops); - if (ops->set_deadline && !dma_fence_is_signaled(fence)) + if (ops && ops->set_deadline && !dma_fence_is_signaled(fence)) ops->set_deadline(fence, deadline); rcu_read_unlock(); } diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index eea674acdfa6..e6d753107143 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -457,7 +457,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
rcu_read_lock(); ops = rcu_dereference(fence->ops); - if (ops->signaled && ops->signaled(fence)) { + if (ops && ops->signaled && ops->signaled(fence)) { rcu_read_unlock(); dma_fence_signal_locked(fence); return true; @@ -493,7 +493,7 @@ dma_fence_is_signaled(struct dma_fence *fence)
rcu_read_lock(); ops = rcu_dereference(fence->ops); - if (ops->signaled && ops->signaled(fence)) { + if (ops && ops->signaled && ops->signaled(fence)) { rcu_read_unlock(); dma_fence_signal(fence); return true;
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
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com --- drivers/dma-buf/dma-fence.c | 69 +++++++++++++----------- drivers/dma-buf/st-dma-fence.c | 6 ++- drivers/dma-buf/sw_sync.c | 14 ++--- drivers/dma-buf/sync_debug.h | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 12 ++--- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_writeback.c | 2 +- drivers/gpu/drm/nouveau/nouveau_drm.c | 5 +- drivers/gpu/drm/nouveau/nouveau_fence.c | 3 +- drivers/gpu/drm/qxl/qxl_release.c | 3 +- drivers/gpu/drm/scheduler/sched_fence.c | 4 +- drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 3 +- drivers/gpu/drm/xe/xe_hw_fence.c | 3 +- drivers/gpu/drm/xe/xe_sched_job.c | 4 +- include/linux/dma-fence.h | 42 ++++++++++++++- 17 files changed, 117 insertions(+), 65 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 481f1cd9aae2..b3c1fb990621 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -343,7 +343,6 @@ void __dma_fence_might_wait(void) } #endif
- /** * dma_fence_signal_timestamp_locked - signal completion of a fence * @fence: the fence to signal @@ -365,7 +364,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, struct dma_fence_cb *cur, *tmp; struct list_head cb_list;
- lockdep_assert_held(fence->lock); + lockdep_assert_held(dma_fence_spinlock(fence));
if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))) @@ -412,9 +411,9 @@ void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp) if (WARN_ON(!fence)) return;
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); dma_fence_signal_timestamp_locked(fence, timestamp); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); } EXPORT_SYMBOL(dma_fence_signal_timestamp);
@@ -473,9 +472,9 @@ bool dma_fence_check_and_signal(struct dma_fence *fence) unsigned long flags; bool ret;
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); ret = dma_fence_check_and_signal_locked(fence); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
return ret; } @@ -501,9 +500,9 @@ void dma_fence_signal(struct dma_fence *fence)
tmp = dma_fence_begin_signalling();
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); dma_fence_signal_timestamp_locked(fence, ktime_get()); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
dma_fence_end_signalling(tmp); } @@ -603,10 +602,10 @@ void dma_fence_release(struct kref *kref) * don't leave chains dangling. We set the error flag first * so that the callbacks know this signal is due to an error. */ - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); fence->error = -EDEADLK; dma_fence_signal_locked(fence); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); }
ops = rcu_dereference(fence->ops); @@ -636,7 +635,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence) const struct dma_fence_ops *ops; bool was_set;
- lockdep_assert_held(fence->lock); + lockdep_assert_held(dma_fence_spinlock(fence));
was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags); @@ -672,9 +671,9 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence) { unsigned long flags;
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); __dma_fence_enable_signaling(fence); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); } EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
@@ -714,8 +713,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, return -ENOENT; }
- spin_lock_irqsave(fence->lock, flags); - + dma_fence_lock_irqsave(fence, flags); if (__dma_fence_enable_signaling(fence)) { cb->func = func; list_add_tail(&cb->node, &fence->cb_list); @@ -723,8 +721,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, INIT_LIST_HEAD(&cb->node); ret = -ENOENT; } - - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
return ret; } @@ -747,9 +744,9 @@ int dma_fence_get_status(struct dma_fence *fence) unsigned long flags; int status;
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); status = dma_fence_get_status_locked(fence); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
return status; } @@ -779,13 +776,11 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb) unsigned long flags; bool ret;
- spin_lock_irqsave(fence->lock, flags); - + dma_fence_lock_irqsave(fence, flags); ret = !list_empty(&cb->node); if (ret) list_del_init(&cb->node); - - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
return ret; } @@ -824,7 +819,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) unsigned long flags; signed long ret = timeout ? timeout : 1;
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags);
if (dma_fence_test_signaled_flag(fence)) goto out; @@ -848,11 +843,11 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) __set_current_state(TASK_INTERRUPTIBLE); else __set_current_state(TASK_UNINTERRUPTIBLE); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
ret = schedule_timeout(ret);
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (ret > 0 && intr && signal_pending(current)) ret = -ERESTARTSYS; } @@ -862,7 +857,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) __set_current_state(TASK_RUNNING);
out: - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); return ret; } EXPORT_SYMBOL(dma_fence_default_wait); @@ -1071,7 +1066,6 @@ static void __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, u64 seqno, unsigned long flags) { - BUG_ON(!lock); BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
kref_init(&fence->refcount); @@ -1082,10 +1076,15 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, */ RCU_INIT_POINTER(fence->ops, ops); INIT_LIST_HEAD(&fence->cb_list); - fence->lock = lock; fence->context = context; fence->seqno = seqno; fence->flags = flags; + if (lock) { + fence->extern_lock = lock; + } else { + spin_lock_init(&fence->inline_lock); + fence->flags |= BIT(DMA_FENCE_FLAG_INLINE_LOCK_BIT); + } fence->error = 0;
trace_dma_fence_init(fence); @@ -1095,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 * @@ -1105,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 + * for legacy use cases when multiple fences need to be prevented from + * signaling out of order. */ void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, @@ -1118,7 +1121,7 @@ EXPORT_SYMBOL(dma_fence_init); * dma_fence_init64 - Initialize a custom fence with 64-bit seqno support. * @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 * @@ -1128,6 +1131,10 @@ EXPORT_SYMBOL(dma_fence_init); * * 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 + * 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/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index 4dbe39c58bfb..5cb8f90a7448 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -451,8 +451,10 @@ struct race_thread {
static void __wait_for_callbacks(struct dma_fence *f) { - spin_lock_irq(f->lock); - spin_unlock_irq(f->lock); + unsigned long flags; + + dma_fence_lock_irqsave(f, flags); + dma_fence_unlock_irqrestore(f, flags); }
static int thread_signal_callback(void *arg) diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 6f09d13be6b6..4c81a37dd682 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -156,12 +156,12 @@ static void timeline_fence_release(struct dma_fence *fence) struct sync_timeline *parent = dma_fence_parent(fence); unsigned long flags;
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (!list_empty(&pt->link)) { list_del(&pt->link); rb_erase(&pt->node, &parent->pt_tree); } - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
sync_timeline_put(parent); dma_fence_free(fence); @@ -179,7 +179,7 @@ static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadlin struct sync_pt *pt = dma_fence_to_sync_pt(fence); unsigned long flags;
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { if (ktime_before(deadline, pt->deadline)) pt->deadline = deadline; @@ -187,7 +187,7 @@ static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadlin pt->deadline = deadline; __set_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags); } - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); }
static const struct dma_fence_ops timeline_fence_ops = { @@ -431,13 +431,13 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a goto put_fence; }
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (!test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { ret = -ENOENT; goto unlock; } data.deadline_ns = ktime_to_ns(pt->deadline); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
dma_fence_put(fence);
@@ -450,7 +450,7 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return 0;
unlock: - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags); put_fence: dma_fence_put(fence);
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); }
/** diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c index c596b6df2e2d..60922463b415 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c @@ -474,10 +474,10 @@ bool amdgpu_ring_soft_recovery(struct amdgpu_ring *ring, unsigned int vmid, if (amdgpu_sriov_vf(ring->adev) || !ring->funcs->soft_recovery || !fence) return false;
- spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags); if (!dma_fence_is_signaled_locked(fence)) dma_fence_set_error(fence, -ENODATA); - spin_unlock_irqrestore(fence->lock, flags); + dma_fence_unlock_irqrestore(fence, flags);
while (!dma_fence_is_signaled(fence) && ktime_to_ns(ktime_sub(deadline, ktime_get())) > 0) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index a67285118c37..9ea196e88588 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -2782,8 +2782,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm) dma_fence_put(vm->last_unlocked); dma_fence_wait(vm->last_tlb_flush, false); /* Make sure that all fence callbacks have completed */ - spin_lock_irqsave(vm->last_tlb_flush->lock, flags); - spin_unlock_irqrestore(vm->last_tlb_flush->lock, flags); + dma_fence_lock_irqsave(vm->last_tlb_flush, flags); + dma_fence_unlock_irqrestore(vm->last_tlb_flush, flags); dma_fence_put(vm->last_tlb_flush);
list_for_each_entry_safe(mapping, tmp, &vm->freed, list) { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 15d757c016cb..eae105a6cf37 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -627,20 +627,20 @@ bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct amdgpu_bo *bo); */ static inline uint64_t amdgpu_vm_tlb_seq(struct amdgpu_vm *vm) { + struct dma_fence *fence; unsigned long flags; - spinlock_t *lock;
/* * Workaround to stop racing between the fence signaling and handling - * the cb. The lock is static after initially setting it up, just make - * sure that the dma_fence structure isn't freed up. + * the cb. */ rcu_read_lock(); - lock = vm->last_tlb_flush->lock; + fence = dma_fence_get_rcu(vm->last_tlb_flush); rcu_read_unlock();
- spin_lock_irqsave(lock, flags); - spin_unlock_irqrestore(lock, flags); + dma_fence_lock_irqsave(fence, flags); + dma_fence_unlock_irqrestore(fence, flags); + dma_fence_put(fence);
return atomic64_read(&vm->tlb_seq); } diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a7797d260f1e..17472915842f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -159,7 +159,7 @@ static const struct dma_fence_ops drm_crtc_fence_ops; static struct drm_crtc *fence_to_crtc(struct dma_fence *fence) { BUG_ON(fence->ops != &drm_crtc_fence_ops); - return container_of(fence->lock, struct drm_crtc, fence_lock); + return container_of(fence->extern_lock, struct drm_crtc, fence_lock); }
static const char *drm_crtc_fence_get_driver_name(struct dma_fence *fence) diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 95b8a2e4bda6..624a4e8b6c99 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -81,7 +81,7 @@ * From userspace, this property will always read as zero. */
-#define fence_to_wb_connector(x) container_of(x->lock, \ +#define fence_to_wb_connector(x) container_of(x->extern_lock, \ struct drm_writeback_connector, \ fence_lock)
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 1527b801f013..ec4dfa3ea725 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -156,12 +156,13 @@ nouveau_name(struct drm_device *dev) static inline bool nouveau_cli_work_ready(struct dma_fence *fence) { + unsigned long flags; bool ret = true;
- spin_lock_irq(fence->lock); + dma_fence_lock_irqsave(fence, flags); if (!dma_fence_is_signaled_locked(fence)) ret = false; - spin_unlock_irq(fence->lock); + dma_fence_unlock_irqrestore(fence, flags);
if (ret == true) dma_fence_put(fence); diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 4a193b7d6d9e..c282c94138b2 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -41,7 +41,8 @@ static const struct dma_fence_ops nouveau_fence_ops_legacy; static inline struct nouveau_fence_chan * nouveau_fctx(struct nouveau_fence *fence) { - return container_of(fence->base.lock, struct nouveau_fence_chan, lock); + return container_of(fence->base.extern_lock, struct nouveau_fence_chan, + lock); }
static bool diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 7b3c9a6016db..de05e392df88 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -62,7 +62,8 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr, struct qxl_device *qdev; unsigned long cur, end = jiffies + timeout;
- qdev = container_of(fence->lock, struct qxl_device, release_lock); + qdev = container_of(fence->extern_lock, struct qxl_device, + release_lock);
if (!wait_event_timeout(qdev->release_event, (dma_fence_is_signaled(fence) || diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 9391d6f0dc01..08ccbde8b2f5 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -156,7 +156,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, struct dma_fence *parent; unsigned long flags;
- spin_lock_irqsave(&fence->lock, flags); + dma_fence_lock_irqsave(f, flags);
/* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && @@ -168,7 +168,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
- spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags);
/* * smp_load_aquire() to ensure that if we are racing another diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c index 00be92da5509..621aa0aa8406 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c @@ -47,7 +47,8 @@ struct vmw_event_fence_action { static struct vmw_fence_manager * fman_from_fence(struct vmw_fence_obj *fence) { - return container_of(fence->base.lock, struct vmw_fence_manager, lock); + return container_of(fence->base.extern_lock, struct vmw_fence_manager, + lock); }
static void vmw_fence_obj_destroy(struct dma_fence *f) diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c index f6057456e460..3d89c660ea73 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.c +++ b/drivers/gpu/drm/xe/xe_hw_fence.c @@ -142,7 +142,8 @@ static struct xe_hw_fence *to_xe_hw_fence(struct dma_fence *fence);
static struct xe_hw_fence_irq *xe_hw_fence_irq(struct xe_hw_fence *fence) { - return container_of(fence->dma.lock, struct xe_hw_fence_irq, lock); + return container_of(fence->dma.extern_lock, struct xe_hw_fence_irq, + lock); }
static const char *xe_hw_fence_get_driver_name(struct dma_fence *dma_fence) diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c index cb674a322113..9a43ed689e65 100644 --- a/drivers/gpu/drm/xe/xe_sched_job.c +++ b/drivers/gpu/drm/xe/xe_sched_job.c @@ -189,11 +189,11 @@ static bool xe_fence_set_error(struct dma_fence *fence, int error) unsigned long irq_flags; bool signaled;
- spin_lock_irqsave(fence->lock, irq_flags); + dma_fence_lock_irqsave(fence, irq_flags); signaled = test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); if (!signaled) dma_fence_set_error(fence, error); - spin_unlock_irqrestore(fence->lock, irq_flags); + dma_fence_unlock_irqrestore(fence, irq_flags);
return signaled; } diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index e6d753107143..eb8851ea5fa1 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 + * @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, @@ -48,6 +49,7 @@ struct seq_file; * atomic ops (bit_*), so taking the spinlock will not be needed most * of the time. * + * 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; + }; const struct dma_fence_ops __rcu *ops; /* * We clear the callback list on kref_put so that by the time we @@ -98,6 +103,7 @@ struct dma_fence { };
enum dma_fence_flag_bits { + DMA_FENCE_FLAG_INLINE_LOCK_BIT, DMA_FENCE_FLAG_SEQNO64_BIT, DMA_FENCE_FLAG_SIGNALED_BIT, DMA_FENCE_FLAG_TIMESTAMP_BIT, @@ -362,6 +368,38 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep) } while (1); }
+/** + * 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; +} + +/** + * dma_fence_lock_irqsave - irqsave lock the fence + * @fence: the fence to lock + * @flags: where to store the CPU flags. + * + * Lock the fence, preventing it from changing to the signaled state. + */ +#define dma_fence_lock_irqsave(fence, flags) \ + spin_lock_irqsave(dma_fence_spinlock(fence), flags) + +/** + * dma_fence_unlock_irqrestore - unlock the fence and irqrestore + * @fence: the fence to unlock + * @flags the CPU flags to restore + * + * Unlock the fence, allowing it to change it's state to signaled again. + */ +#define dma_fence_unlock_irqrestore(fence, flags) \ + spin_unlock_irqrestore(dma_fence_spinlock(fence), flags) + #ifdef CONFIG_LOCKDEP bool dma_fence_begin_signalling(void); void dma_fence_end_signalling(bool cookie);
Hi,
On 11/12/2025 13:16, 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
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com
I don't think I gave r-b on this one. Not just yet at least. Maybe you have missed the comments I had in the previous two rounds? I will repeat them below.
drivers/dma-buf/dma-fence.c | 69 +++++++++++++----------- drivers/dma-buf/st-dma-fence.c | 6 ++- drivers/dma-buf/sw_sync.c | 14 ++--- drivers/dma-buf/sync_debug.h | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 12 ++--- drivers/gpu/drm/drm_crtc.c | 2 +- drivers/gpu/drm/drm_writeback.c | 2 +- drivers/gpu/drm/nouveau/nouveau_drm.c | 5 +- drivers/gpu/drm/nouveau/nouveau_fence.c | 3 +- drivers/gpu/drm/qxl/qxl_release.c | 3 +- drivers/gpu/drm/scheduler/sched_fence.c | 4 +- drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 3 +- drivers/gpu/drm/xe/xe_hw_fence.c | 3 +- drivers/gpu/drm/xe/xe_sched_job.c | 4 +- include/linux/dma-fence.h | 42 ++++++++++++++- 17 files changed, 117 insertions(+), 65 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 481f1cd9aae2..b3c1fb990621 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -343,7 +343,6 @@ void __dma_fence_might_wait(void) } #endif
- /**
- dma_fence_signal_timestamp_locked - signal completion of a fence
- @fence: the fence to signal
@@ -365,7 +364,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, struct dma_fence_cb *cur, *tmp; struct list_head cb_list;
- lockdep_assert_held(fence->lock);
- lockdep_assert_held(dma_fence_spinlock(fence));
if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))) @@ -412,9 +411,9 @@ void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp) if (WARN_ON(!fence)) return;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags);
For the locking wrappers I think it would be better to introduce them in a purely mechanical patch preceding this one. That is, just add the wrappers and nothing else.
dma_fence_signal_timestamp_locked(fence, timestamp);
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags); } EXPORT_SYMBOL(dma_fence_signal_timestamp);
@@ -473,9 +472,9 @@ bool dma_fence_check_and_signal(struct dma_fence *fence) unsigned long flags; bool ret;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); ret = dma_fence_check_and_signal_locked(fence);
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags);
return ret; } @@ -501,9 +500,9 @@ void dma_fence_signal(struct dma_fence *fence) tmp = dma_fence_begin_signalling();
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); dma_fence_signal_timestamp_locked(fence, ktime_get());
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags);
dma_fence_end_signalling(tmp); } @@ -603,10 +602,10 @@ void dma_fence_release(struct kref *kref) * don't leave chains dangling. We set the error flag first * so that the callbacks know this signal is due to an error. */
spin_lock_irqsave(fence->lock, flags);
dma_fence_lock_irqsave(fence, flags);
spin_unlock_irqrestore(fence->lock, flags);
dma_fence_unlock_irqrestore(fence, flags);ops = rcu_dereference(fence->ops); @@ -636,7 +635,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence) const struct dma_fence_ops *ops; bool was_set;
- lockdep_assert_held(fence->lock);
- lockdep_assert_held(dma_fence_spinlock(fence));
was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags); @@ -672,9 +671,9 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence) { unsigned long flags;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); __dma_fence_enable_signaling(fence);
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags); } EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
@@ -714,8 +713,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, return -ENOENT; }
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); if (__dma_fence_enable_signaling(fence)) { cb->func = func; list_add_tail(&cb->node, &fence->cb_list);
@@ -723,8 +721,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb, INIT_LIST_HEAD(&cb->node); ret = -ENOENT; }
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags);
return ret; } @@ -747,9 +744,9 @@ int dma_fence_get_status(struct dma_fence *fence) unsigned long flags; int status;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); status = dma_fence_get_status_locked(fence);
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags);
return status; } @@ -779,13 +776,11 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb) unsigned long flags; bool ret;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); ret = !list_empty(&cb->node); if (ret) list_del_init(&cb->node);
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags);
return ret; } @@ -824,7 +819,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) unsigned long flags; signed long ret = timeout ? timeout : 1;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags);
if (dma_fence_test_signaled_flag(fence)) goto out; @@ -848,11 +843,11 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) __set_current_state(TASK_INTERRUPTIBLE); else __set_current_state(TASK_UNINTERRUPTIBLE);
spin_unlock_irqrestore(fence->lock, flags);
dma_fence_unlock_irqrestore(fence, flags);ret = schedule_timeout(ret);
spin_lock_irqsave(fence->lock, flags);
dma_fence_lock_irqsave(fence, flags);@@ -862,7 +857,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout) __set_current_state(TASK_RUNNING); out:
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags); return ret; } EXPORT_SYMBOL(dma_fence_default_wait);
@@ -1071,7 +1066,6 @@ static void __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, spinlock_t *lock, u64 context, u64 seqno, unsigned long flags) {
- BUG_ON(!lock); BUG_ON(!ops || !ops->get_driver_name || !ops->get_timeline_name);
kref_init(&fence->refcount); @@ -1082,10 +1076,15 @@ __dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops, */ RCU_INIT_POINTER(fence->ops, ops); INIT_LIST_HEAD(&fence->cb_list);
- fence->lock = lock; fence->context = context; fence->seqno = seqno; fence->flags = flags;
- if (lock) {
fence->extern_lock = lock;- } else {
spin_lock_init(&fence->inline_lock);fence->flags |= BIT(DMA_FENCE_FLAG_INLINE_LOCK_BIT);- } fence->error = 0;
trace_dma_fence_init(fence); @@ -1095,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
@@ -1105,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
- for legacy use cases when multiple fences need to be prevented from
*/ void dma_fence_init(struct dma_fence *fence, const struct dma_fence_ops *ops,
- signaling out of order.
@@ -1118,7 +1121,7 @@ EXPORT_SYMBOL(dma_fence_init);
- dma_fence_init64 - Initialize a custom fence with 64-bit seqno support.
- @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
@@ -1128,6 +1131,10 @@ EXPORT_SYMBOL(dma_fence_init);
- 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
- for legacy use cases when multiple fences need to be prevented from
*/ void dma_fence_init64(struct dma_fence *fence, const struct dma_fence_ops *ops,
- signaling out of order.
diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index 4dbe39c58bfb..5cb8f90a7448 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -451,8 +451,10 @@ struct race_thread { static void __wait_for_callbacks(struct dma_fence *f) {
- spin_lock_irq(f->lock);
- spin_unlock_irq(f->lock);
- unsigned long flags;
- dma_fence_lock_irqsave(f, flags);
- dma_fence_unlock_irqrestore(f, flags); }
static int thread_signal_callback(void *arg) diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 6f09d13be6b6..4c81a37dd682 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -156,12 +156,12 @@ static void timeline_fence_release(struct dma_fence *fence) struct sync_timeline *parent = dma_fence_parent(fence); unsigned long flags;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); if (!list_empty(&pt->link)) { list_del(&pt->link); rb_erase(&pt->node, &parent->pt_tree); }
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags);
sync_timeline_put(parent); dma_fence_free(fence); @@ -179,7 +179,7 @@ static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadlin struct sync_pt *pt = dma_fence_to_sync_pt(fence); unsigned long flags;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); if (test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { if (ktime_before(deadline, pt->deadline)) pt->deadline = deadline;
@@ -187,7 +187,7 @@ static void timeline_fence_set_deadline(struct dma_fence *fence, ktime_t deadlin pt->deadline = deadline; __set_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags); }
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags); }
static const struct dma_fence_ops timeline_fence_ops = { @@ -431,13 +431,13 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a goto put_fence; }
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); if (!test_bit(SW_SYNC_HAS_DEADLINE_BIT, &fence->flags)) { ret = -ENOENT; goto unlock; } data.deadline_ns = ktime_to_ns(pt->deadline);
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags);
dma_fence_put(fence); @@ -450,7 +450,7 @@ static int sw_sync_ioctl_get_deadline(struct sync_timeline *obj, unsigned long a return 0; unlock:
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags); put_fence: dma_fence_put(fence);
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); }
/** diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c index c596b6df2e2d..60922463b415 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c @@ -474,10 +474,10 @@ bool amdgpu_ring_soft_recovery(struct amdgpu_ring *ring, unsigned int vmid, if (amdgpu_sriov_vf(ring->adev) || !ring->funcs->soft_recovery || !fence) return false;
- spin_lock_irqsave(fence->lock, flags);
- dma_fence_lock_irqsave(fence, flags); if (!dma_fence_is_signaled_locked(fence)) dma_fence_set_error(fence, -ENODATA);
- spin_unlock_irqrestore(fence->lock, flags);
- dma_fence_unlock_irqrestore(fence, flags);
while (!dma_fence_is_signaled(fence) && ktime_to_ns(ktime_sub(deadline, ktime_get())) > 0) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index a67285118c37..9ea196e88588 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -2782,8 +2782,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm) dma_fence_put(vm->last_unlocked); dma_fence_wait(vm->last_tlb_flush, false); /* Make sure that all fence callbacks have completed */
- spin_lock_irqsave(vm->last_tlb_flush->lock, flags);
- spin_unlock_irqrestore(vm->last_tlb_flush->lock, flags);
- dma_fence_lock_irqsave(vm->last_tlb_flush, flags);
- dma_fence_unlock_irqrestore(vm->last_tlb_flush, flags); dma_fence_put(vm->last_tlb_flush);
list_for_each_entry_safe(mapping, tmp, &vm->freed, list) { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h index 15d757c016cb..eae105a6cf37 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -627,20 +627,20 @@ bool amdgpu_vm_is_bo_always_valid(struct amdgpu_vm *vm, struct amdgpu_bo *bo); */ static inline uint64_t amdgpu_vm_tlb_seq(struct amdgpu_vm *vm) {
- struct dma_fence *fence; unsigned long flags;
- spinlock_t *lock;
/* * Workaround to stop racing between the fence signaling and handling
* the cb. The lock is static after initially setting it up, just make* sure that the dma_fence structure isn't freed up.
* the cb.
- lock = vm->last_tlb_flush->lock;
- fence = dma_fence_get_rcu(vm->last_tlb_flush);
Why does this belong here? If taking a reference fixes some race it needs to be a separate patch. If it doesn't then this patch shouldn't be adding it.
rcu_read_unlock();
- spin_lock_irqsave(lock, flags);
- spin_unlock_irqrestore(lock, flags);
- dma_fence_lock_irqsave(fence, flags);
- dma_fence_unlock_irqrestore(fence, flags);
- dma_fence_put(fence);
return atomic64_read(&vm->tlb_seq); } diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index a7797d260f1e..17472915842f 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -159,7 +159,7 @@ static const struct dma_fence_ops drm_crtc_fence_ops; static struct drm_crtc *fence_to_crtc(struct dma_fence *fence) { BUG_ON(fence->ops != &drm_crtc_fence_ops);
- return container_of(fence->lock, struct drm_crtc, fence_lock);
- return container_of(fence->extern_lock, struct drm_crtc, fence_lock); }
static const char *drm_crtc_fence_get_driver_name(struct dma_fence *fence) diff --git a/drivers/gpu/drm/drm_writeback.c b/drivers/gpu/drm/drm_writeback.c index 95b8a2e4bda6..624a4e8b6c99 100644 --- a/drivers/gpu/drm/drm_writeback.c +++ b/drivers/gpu/drm/drm_writeback.c @@ -81,7 +81,7 @@
- From userspace, this property will always read as zero.
*/ -#define fence_to_wb_connector(x) container_of(x->lock, \ +#define fence_to_wb_connector(x) container_of(x->extern_lock, \ struct drm_writeback_connector, \ fence_lock) diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c index 1527b801f013..ec4dfa3ea725 100644 --- a/drivers/gpu/drm/nouveau/nouveau_drm.c +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c @@ -156,12 +156,13 @@ nouveau_name(struct drm_device *dev) static inline bool nouveau_cli_work_ready(struct dma_fence *fence) {
- unsigned long flags; bool ret = true;
- spin_lock_irq(fence->lock);
- dma_fence_lock_irqsave(fence, flags); if (!dma_fence_is_signaled_locked(fence)) ret = false;
- spin_unlock_irq(fence->lock);
- dma_fence_unlock_irqrestore(fence, flags);
if (ret == true) dma_fence_put(fence); diff --git a/drivers/gpu/drm/nouveau/nouveau_fence.c b/drivers/gpu/drm/nouveau/nouveau_fence.c index 4a193b7d6d9e..c282c94138b2 100644 --- a/drivers/gpu/drm/nouveau/nouveau_fence.c +++ b/drivers/gpu/drm/nouveau/nouveau_fence.c @@ -41,7 +41,8 @@ static const struct dma_fence_ops nouveau_fence_ops_legacy; static inline struct nouveau_fence_chan * nouveau_fctx(struct nouveau_fence *fence) {
- return container_of(fence->base.lock, struct nouveau_fence_chan, lock);
- return container_of(fence->base.extern_lock, struct nouveau_fence_chan,
lock);static bool diff --git a/drivers/gpu/drm/qxl/qxl_release.c b/drivers/gpu/drm/qxl/qxl_release.c index 7b3c9a6016db..de05e392df88 100644 --- a/drivers/gpu/drm/qxl/qxl_release.c +++ b/drivers/gpu/drm/qxl/qxl_release.c @@ -62,7 +62,8 @@ static long qxl_fence_wait(struct dma_fence *fence, bool intr, struct qxl_device *qdev; unsigned long cur, end = jiffies + timeout;
- qdev = container_of(fence->lock, struct qxl_device, release_lock);
- qdev = container_of(fence->extern_lock, struct qxl_device,
release_lock);if (!wait_event_timeout(qdev->release_event, (dma_fence_is_signaled(fence) || diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 9391d6f0dc01..08ccbde8b2f5 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -156,7 +156,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, struct dma_fence *parent; unsigned long flags;
- spin_lock_irqsave(&fence->lock, flags);
- dma_fence_lock_irqsave(f, flags);
/* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && @@ -168,7 +168,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
- spin_unlock_irqrestore(&fence->lock, flags);
- dma_fence_unlock_irqrestore(f, flags);
/* * smp_load_aquire() to ensure that if we are racing another diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c index 00be92da5509..621aa0aa8406 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c @@ -47,7 +47,8 @@ struct vmw_event_fence_action { static struct vmw_fence_manager * fman_from_fence(struct vmw_fence_obj *fence) {
- return container_of(fence->base.lock, struct vmw_fence_manager, lock);
- return container_of(fence->base.extern_lock, struct vmw_fence_manager,
lock);static void vmw_fence_obj_destroy(struct dma_fence *f) diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c index f6057456e460..3d89c660ea73 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.c +++ b/drivers/gpu/drm/xe/xe_hw_fence.c @@ -142,7 +142,8 @@ static struct xe_hw_fence *to_xe_hw_fence(struct dma_fence *fence); static struct xe_hw_fence_irq *xe_hw_fence_irq(struct xe_hw_fence *fence) {
- return container_of(fence->dma.lock, struct xe_hw_fence_irq, lock);
- return container_of(fence->dma.extern_lock, struct xe_hw_fence_irq,
lock);static const char *xe_hw_fence_get_driver_name(struct dma_fence *dma_fence) diff --git a/drivers/gpu/drm/xe/xe_sched_job.c b/drivers/gpu/drm/xe/xe_sched_job.c index cb674a322113..9a43ed689e65 100644 --- a/drivers/gpu/drm/xe/xe_sched_job.c +++ b/drivers/gpu/drm/xe/xe_sched_job.c @@ -189,11 +189,11 @@ static bool xe_fence_set_error(struct dma_fence *fence, int error) unsigned long irq_flags; bool signaled;
- spin_lock_irqsave(fence->lock, irq_flags);
- dma_fence_lock_irqsave(fence, irq_flags); signaled = test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags); if (!signaled) dma_fence_set_error(fence, error);
- spin_unlock_irqrestore(fence->lock, irq_flags);
- dma_fence_unlock_irqrestore(fence, irq_flags);
return signaled; } diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index e6d753107143..eb8851ea5fa1 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
- @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,
@@ -48,6 +49,7 @@ struct seq_file;
- atomic ops (bit_*), so taking the spinlock will not be needed most
- of the time.
- 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;- }; const struct dma_fence_ops __rcu *ops; /*
- We clear the callback list on kref_put so that by the time we
@@ -98,6 +103,7 @@ struct dma_fence { }; enum dma_fence_flag_bits {
- DMA_FENCE_FLAG_INLINE_LOCK_BIT, DMA_FENCE_FLAG_SEQNO64_BIT, DMA_FENCE_FLAG_SIGNALED_BIT, DMA_FENCE_FLAG_TIMESTAMP_BIT,
@@ -362,6 +368,38 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep) } while (1); } +/**
- 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;
Is sprinkling of conditionals better than growing the struct? Probably yes, since branch misses are cheaper than cache misses. Unless the code grows significantly on some hot path and we get instruction cache misses instead. Who knows. But let say in the commit message we considered it and decided on this solution due xyz.
On a quick grep there is one arch where this grows the struct past a cache line anyway, but as it is PA-RISC I guess no one cares. Lets mention that in the commit message as well.
Regards,
Tvrtko
+}
+/**
- dma_fence_lock_irqsave - irqsave lock the fence
- @fence: the fence to lock
- @flags: where to store the CPU flags.
- Lock the fence, preventing it from changing to the signaled state.
- */
+#define dma_fence_lock_irqsave(fence, flags) \
- spin_lock_irqsave(dma_fence_spinlock(fence), flags)
+/**
- dma_fence_unlock_irqrestore - unlock the fence and irqrestore
- @fence: the fence to unlock
- @flags the CPU flags to restore
- Unlock the fence, allowing it to change it's state to signaled again.
- */
+#define dma_fence_unlock_irqrestore(fence, flags) \
- spin_unlock_irqrestore(dma_fence_spinlock(fence), flags)
- #ifdef CONFIG_LOCKDEP bool dma_fence_begin_signalling(void); void dma_fence_end_signalling(bool cookie);
On 12/11/25 15:35, Tvrtko Ursulin wrote:
Hi,
On 11/12/2025 13:16, 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
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com
I don't think I gave r-b on this one. Not just yet at least. Maybe you have missed the comments I had in the previous two rounds? I will repeat them below.
I was already wondering why you gave comments and an rb but though that the comments might just be optional.
Going to remove that and see on the comments below.
@@ -365,7 +364,7 @@ void dma_fence_signal_timestamp_locked(struct dma_fence *fence, struct dma_fence_cb *cur, *tmp; struct list_head cb_list; - lockdep_assert_held(fence->lock); + lockdep_assert_held(dma_fence_spinlock(fence)); if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))) @@ -412,9 +411,9 @@ void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp) if (WARN_ON(!fence)) return; - spin_lock_irqsave(fence->lock, flags); + dma_fence_lock_irqsave(fence, flags);
For the locking wrappers I think it would be better to introduce them in a purely mechanical patch preceding this one. That is, just add the wrappers and nothing else.
That doesn't fully work for all cases, but I will separate it out a bit more.
static inline uint64_t amdgpu_vm_tlb_seq(struct amdgpu_vm *vm) { + struct dma_fence *fence; unsigned long flags; - spinlock_t *lock; /* * Workaround to stop racing between the fence signaling and handling - * the cb. The lock is static after initially setting it up, just make - * sure that the dma_fence structure isn't freed up. + * the cb. */ rcu_read_lock(); - lock = vm->last_tlb_flush->lock; + fence = dma_fence_get_rcu(vm->last_tlb_flush);
Why does this belong here? If taking a reference fixes some race it needs to be a separate patch. If it doesn't then this patch shouldn't be adding it.
The code previously assumed that the lock is global and can't go away while the function is called. When we start to use an inline lock that assumption is not true any more.
But you're right that can be a separate patch.
@@ -362,6 +368,38 @@ dma_fence_get_rcu_safe(struct dma_fence __rcu **fencep) } while (1); } +/**
- 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;
Is sprinkling of conditionals better than growing the struct? Probably yes, since branch misses are cheaper than cache misses. Unless the code grows significantly on some hot path and we get instruction cache misses instead. Who knows. But let say in the commit message we considered it and decided on this solution due xyz.
Sure.
On a quick grep there is one arch where this grows the struct past a cache line anyway, but as it is PA-RISC I guess no one cares. Lets mention that in the commit message as well.
Interesting, I was aware of the problems on Sparc regarding spinlocks but that PA-RISC also has something more complicated then an int is news to me.
Anyway I agree it doesn't really matter.
Regards, Christian.
Regards,
Tvrtko
+}
+/**
- dma_fence_lock_irqsave - irqsave lock the fence
- @fence: the fence to lock
- @flags: where to store the CPU flags.
- Lock the fence, preventing it from changing to the signaled state.
- */
+#define dma_fence_lock_irqsave(fence, flags) \ + spin_lock_irqsave(dma_fence_spinlock(fence), flags)
+/**
- dma_fence_unlock_irqrestore - unlock the fence and irqrestore
- @fence: the fence to unlock
- @flags the CPU flags to restore
- Unlock the fence, allowing it to change it's state to signaled again.
- */
+#define dma_fence_unlock_irqrestore(fence, flags) \ + spin_unlock_irqrestore(dma_fence_spinlock(fence), flags)
#ifdef CONFIG_LOCKDEP bool dma_fence_begin_signalling(void); void dma_fence_end_signalling(bool cookie);
Actually test the documented expectation of dma_fence_wait() instead of comming up with a mock_wait implementation which uses undocumented and non standard return codes and behavior.
Additional to that increase the timeout to one second, otherwise the test case is a bit unreliable.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/st-dma-fence.c | 51 ++++------------------------------ 1 file changed, 5 insertions(+), 46 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index 5cb8f90a7448..5d0d9abc6e21 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -33,50 +33,9 @@ static void mock_fence_release(struct dma_fence *f) kmem_cache_free(slab_fences, to_mock_fence(f)); }
-struct wait_cb { - struct dma_fence_cb cb; - struct task_struct *task; -}; - -static void mock_wakeup(struct dma_fence *f, struct dma_fence_cb *cb) -{ - wake_up_process(container_of(cb, struct wait_cb, cb)->task); -} - -static long mock_wait(struct dma_fence *f, bool intr, long timeout) -{ - const int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE; - struct wait_cb cb = { .task = current }; - - if (dma_fence_add_callback(f, &cb.cb, mock_wakeup)) - return timeout; - - while (timeout) { - set_current_state(state); - - if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &f->flags)) - break; - - if (signal_pending_state(state, current)) - break; - - timeout = schedule_timeout(timeout); - } - __set_current_state(TASK_RUNNING); - - if (!dma_fence_remove_callback(f, &cb.cb)) - return timeout; - - if (signal_pending_state(state, current)) - return -ERESTARTSYS; - - return -ETIME; -} - static const struct dma_fence_ops mock_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name, - .wait = mock_wait, .release = mock_fence_release, };
@@ -349,14 +308,14 @@ static int test_wait(void *arg)
dma_fence_enable_sw_signaling(f);
- if (dma_fence_wait_timeout(f, false, 0) != -ETIME) { + if (dma_fence_wait_timeout(f, false, 0) != 0) { pr_err("Wait reported complete before being signaled\n"); goto err_free; }
dma_fence_signal(f);
- if (dma_fence_wait_timeout(f, false, 0) != 0) { + if (dma_fence_wait_timeout(f, false, 0) != 1) { pr_err("Wait reported incomplete after being signaled\n"); goto err_free; } @@ -393,16 +352,16 @@ static int test_wait_timeout(void *arg)
dma_fence_enable_sw_signaling(wt.f);
- if (dma_fence_wait_timeout(wt.f, false, 1) != -ETIME) { + if (dma_fence_wait_timeout(wt.f, false, 1) != 0) { pr_err("Wait reported complete before being signaled\n"); goto err_free; }
mod_timer(&wt.timer, jiffies + 1);
- if (dma_fence_wait_timeout(wt.f, false, 2) == -ETIME) { + if (dma_fence_wait_timeout(wt.f, false, HZ) == 0) { if (timer_pending(&wt.timer)) { - pr_notice("Timer did not fire within the jiffy!\n"); + pr_notice("Timer did not fire within one HZ!\n"); err = 0; /* not our fault! */ } else { pr_err("Wait reported incomplete after timeout\n");
On 11/12/2025 13:16, Christian König wrote:
Actually test the documented expectation of dma_fence_wait() instead of comming up with a mock_wait implementation which uses undocumented and non standard return codes and behavior.
Additional to that increase the timeout to one second, otherwise the test case is a bit unreliable.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/dma-buf/st-dma-fence.c | 51 ++++------------------------------ 1 file changed, 5 insertions(+), 46 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index 5cb8f90a7448..5d0d9abc6e21 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -33,50 +33,9 @@ static void mock_fence_release(struct dma_fence *f) kmem_cache_free(slab_fences, to_mock_fence(f)); } -struct wait_cb {
- struct dma_fence_cb cb;
- struct task_struct *task;
-};
-static void mock_wakeup(struct dma_fence *f, struct dma_fence_cb *cb) -{
- wake_up_process(container_of(cb, struct wait_cb, cb)->task);
-}
-static long mock_wait(struct dma_fence *f, bool intr, long timeout) -{
- const int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
- struct wait_cb cb = { .task = current };
- if (dma_fence_add_callback(f, &cb.cb, mock_wakeup))
return timeout;- while (timeout) {
set_current_state(state);if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &f->flags))break;if (signal_pending_state(state, current))break;timeout = schedule_timeout(timeout);- }
- __set_current_state(TASK_RUNNING);
- if (!dma_fence_remove_callback(f, &cb.cb))
return timeout;- if (signal_pending_state(state, current))
return -ERESTARTSYS;- return -ETIME;
-}
- static const struct dma_fence_ops mock_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name,
- .wait = mock_wait, .release = mock_fence_release, };
@@ -349,14 +308,14 @@ static int test_wait(void *arg) dma_fence_enable_sw_signaling(f);
- if (dma_fence_wait_timeout(f, false, 0) != -ETIME) {
- if (dma_fence_wait_timeout(f, false, 0) != 0) { pr_err("Wait reported complete before being signaled\n"); goto err_free; }
dma_fence_signal(f);
- if (dma_fence_wait_timeout(f, false, 0) != 0) {
- if (dma_fence_wait_timeout(f, false, 0) != 1) { pr_err("Wait reported incomplete after being signaled\n"); goto err_free; }
@@ -393,16 +352,16 @@ static int test_wait_timeout(void *arg) dma_fence_enable_sw_signaling(wt.f);
- if (dma_fence_wait_timeout(wt.f, false, 1) != -ETIME) {
- if (dma_fence_wait_timeout(wt.f, false, 1) != 0) { pr_err("Wait reported complete before being signaled\n"); goto err_free; }
mod_timer(&wt.timer, jiffies + 1);
- if (dma_fence_wait_timeout(wt.f, false, 2) == -ETIME) {
- if (dma_fence_wait_timeout(wt.f, false, HZ) == 0) { if (timer_pending(&wt.timer)) {
pr_notice("Timer did not fire within the jiffy!\n");
pr_notice("Timer did not fire within one HZ!\n"); err = 0; /* not our fault! */
Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com
Regards,
Tvrtko
Drop the mock_fence and the kmem_cache, instead use the inline lock and test if the ops are properly dropped after signaling.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/dma-buf/st-dma-fence.c | 44 ++++++++-------------------------- 1 file changed, 10 insertions(+), 34 deletions(-)
diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c index 5d0d9abc6e21..65221270fd06 100644 --- a/drivers/dma-buf/st-dma-fence.c +++ b/drivers/dma-buf/st-dma-fence.c @@ -14,43 +14,26 @@
#include "selftest.h"
-static struct kmem_cache *slab_fences; - -static struct mock_fence { - struct dma_fence base; - struct spinlock lock; -} *to_mock_fence(struct dma_fence *f) { - return container_of(f, struct mock_fence, base); -} - static const char *mock_name(struct dma_fence *f) { return "mock"; }
-static void mock_fence_release(struct dma_fence *f) -{ - kmem_cache_free(slab_fences, to_mock_fence(f)); -} - static const struct dma_fence_ops mock_ops = { .get_driver_name = mock_name, .get_timeline_name = mock_name, - .release = mock_fence_release, };
static struct dma_fence *mock_fence(void) { - struct mock_fence *f; + struct dma_fence *f;
- f = kmem_cache_alloc(slab_fences, GFP_KERNEL); + f = kmalloc(sizeof(*f), GFP_KERNEL); if (!f) return NULL;
- spin_lock_init(&f->lock); - dma_fence_init(&f->base, &mock_ops, &f->lock, 0, 0); - - return &f->base; + dma_fence_init(f, &mock_ops, NULL, 0, 0); + return f; }
static int sanitycheck(void *arg) @@ -90,6 +73,11 @@ static int test_signaling(void *arg) goto err_free; }
+ if (rcu_dereference_protected(f->ops, true)) { + pr_err("Fence ops not cleared on signal\n"); + goto err_free; + } + if (!dma_fence_is_signaled(f)) { pr_err("Fence not reporting signaled\n"); goto err_free; @@ -540,19 +528,7 @@ int dma_fence(void) SUBTEST(test_stub), SUBTEST(race_signal_callback), }; - int ret;
pr_info("sizeof(dma_fence)=%zu\n", sizeof(struct dma_fence)); - - slab_fences = KMEM_CACHE(mock_fence, - SLAB_TYPESAFE_BY_RCU | - SLAB_HWCACHE_ALIGN); - if (!slab_fences) - return -ENOMEM; - - ret = subtests(tests, NULL); - - kmem_cache_destroy(slab_fences); - - return ret; + return subtests(tests, NULL); }
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the framework's internal fences as well.
Also saves about 4 bytes for the external spinlock.
v2: drop unecessary changes
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com --- drivers/dma-buf/dma-fence.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index b3c1fb990621..d1a9a1cdd64b 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -24,7 +24,6 @@ EXPORT_TRACEPOINT_SYMBOL(dma_fence_emit); EXPORT_TRACEPOINT_SYMBOL(dma_fence_enable_signal); EXPORT_TRACEPOINT_SYMBOL(dma_fence_signaled);
-static DEFINE_SPINLOCK(dma_fence_stub_lock); static struct dma_fence dma_fence_stub;
/* @@ -123,12 +122,9 @@ static const struct dma_fence_ops dma_fence_stub_ops = {
static int __init dma_fence_init_stub(void) { - dma_fence_init(&dma_fence_stub, &dma_fence_stub_ops, - &dma_fence_stub_lock, 0, 0); - + dma_fence_init(&dma_fence_stub, &dma_fence_stub_ops, NULL, 0, 0); set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &dma_fence_stub.flags); - dma_fence_signal(&dma_fence_stub); return 0; } @@ -160,11 +156,7 @@ struct dma_fence *dma_fence_allocate_private_stub(ktime_t timestamp) if (fence == NULL) return NULL;
- dma_fence_init(fence, - &dma_fence_stub_ops, - &dma_fence_stub_lock, - 0, 0); - + dma_fence_init(fence, &dma_fence_stub_ops, NULL, 0, 0); set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags);
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the framework's internal fences as well.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Reviewed-by: Philipp Stanner phasta@kernel.org --- drivers/dma-buf/dma-fence-array.c | 5 ++--- include/linux/dma-fence-array.h | 1 - 2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/dma-buf/dma-fence-array.c b/drivers/dma-buf/dma-fence-array.c index 6657d4b30af9..c2119a8049fe 100644 --- a/drivers/dma-buf/dma-fence-array.c +++ b/drivers/dma-buf/dma-fence-array.c @@ -204,9 +204,8 @@ void dma_fence_array_init(struct dma_fence_array *array,
array->num_fences = num_fences;
- spin_lock_init(&array->lock); - dma_fence_init(&array->base, &dma_fence_array_ops, &array->lock, - context, seqno); + dma_fence_init(&array->base, &dma_fence_array_ops, NULL, context, + seqno); init_irq_work(&array->work, irq_dma_fence_array_work);
atomic_set(&array->num_pending, signal_on_any ? 1 : num_fences); diff --git a/include/linux/dma-fence-array.h b/include/linux/dma-fence-array.h index 079b3dec0a16..370b3d2bba37 100644 --- a/include/linux/dma-fence-array.h +++ b/include/linux/dma-fence-array.h @@ -38,7 +38,6 @@ struct dma_fence_array_cb { struct dma_fence_array { struct dma_fence base;
- spinlock_t lock; unsigned num_fences; atomic_t num_pending; struct dma_fence **fences;
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the framework's internal fences as well.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com Reviewed-by: Philipp Stanner phasta@kernel.org --- drivers/dma-buf/dma-fence-chain.c | 3 +-- include/linux/dma-fence-chain.h | 1 - 2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/dma-buf/dma-fence-chain.c b/drivers/dma-buf/dma-fence-chain.c index a8a90acf4f34..a707792b6025 100644 --- a/drivers/dma-buf/dma-fence-chain.c +++ b/drivers/dma-buf/dma-fence-chain.c @@ -245,7 +245,6 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, struct dma_fence_chain *prev_chain = to_dma_fence_chain(prev); uint64_t context;
- spin_lock_init(&chain->lock); rcu_assign_pointer(chain->prev, prev); chain->fence = fence; chain->prev_seqno = 0; @@ -261,7 +260,7 @@ void dma_fence_chain_init(struct dma_fence_chain *chain, seqno = max(prev->seqno, seqno); }
- dma_fence_init64(&chain->base, &dma_fence_chain_ops, &chain->lock, + dma_fence_init64(&chain->base, &dma_fence_chain_ops, NULL, context, seqno);
/* diff --git a/include/linux/dma-fence-chain.h b/include/linux/dma-fence-chain.h index 68c3c1e41014..d39ce7a2e599 100644 --- a/include/linux/dma-fence-chain.h +++ b/include/linux/dma-fence-chain.h @@ -46,7 +46,6 @@ struct dma_fence_chain { */ struct irq_work work; }; - spinlock_t lock; };
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the scheduler fences as well just in case if anybody uses this as blueprint for its own implementation.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- include/drm/gpu_scheduler.h | 4 ---- 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 08ccbde8b2f5..47471b9e43f9 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, /* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { - spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags); return; }
@@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity,
fence->owner = owner; fence->drm_client_id = drm_client_id; - spin_lock_init(&fence->lock);
return fence; } @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, fence->sched = entity->rq->sched; seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, - &fence->lock, entity->fence_context, seq); + NULL, entity->fence_context, seq); dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, - &fence->lock, entity->fence_context + 1, seq); + NULL, entity->fence_context + 1, seq); }
module_init(drm_sched_fence_slab_init); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index fb88301b3c45..b77f24a783e3 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -297,10 +297,6 @@ struct drm_sched_fence { * belongs to. */ struct drm_gpu_scheduler *sched; - /** - * @lock: the lock used by the scheduled and the finished fences. - */ - spinlock_t lock; /** * @owner: job owner for debugging */
On 11/12/2025 13:16, Christian König wrote:
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the scheduler fences as well just in case if anybody uses this as blueprint for its own implementation.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- include/drm/gpu_scheduler.h | 4 ---- 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 08ccbde8b2f5..47471b9e43f9 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, /* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) {
spin_unlock_irqrestore(&fence->lock, flags);
dma_fence_unlock_irqrestore(f, flags);
Rebase error I guess. Pull into the locking helpers patch.
Regards,
Tvrtko
return;} @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, fence->owner = owner; fence->drm_client_id = drm_client_id;
- spin_lock_init(&fence->lock);
return fence; } @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, fence->sched = entity->rq->sched; seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled,
&fence->lock, entity->fence_context, seq);
NULL, entity->fence_context, seq);
&fence->lock, entity->fence_context + 1, seq);
NULL, entity->fence_context + 1, seq);module_init(drm_sched_fence_slab_init); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index fb88301b3c45..b77f24a783e3 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -297,10 +297,6 @@ struct drm_sched_fence { * belongs to. */ struct drm_gpu_scheduler *sched;
/*** @lock: the lock used by the scheduled and the finished fences.*/- spinlock_t lock; /** * @owner: job owner for debugging */
On 12/11/25 16:13, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the scheduler fences as well just in case if anybody uses this as blueprint for its own implementation.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- include/drm/gpu_scheduler.h | 4 ---- 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 08ccbde8b2f5..47471b9e43f9 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, /* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { - spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags);
Rebase error I guess. Pull into the locking helpers patch.
No that is actually completely intentional here.
Previously we had a separate lock which protected both the DMA-fences as well as the deadline state.
Now we turn that upside down by dropping the separate lock and protecting the deadline state with the dma_fence lock instead.
Regards, Christian.
Regards,
Tvrtko
return; } @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, fence->owner = owner; fence->drm_client_id = drm_client_id; - spin_lock_init(&fence->lock); return fence; } @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, fence->sched = entity->rq->sched; seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, - &fence->lock, entity->fence_context, seq); + NULL, entity->fence_context, seq); dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, - &fence->lock, entity->fence_context + 1, seq); + NULL, entity->fence_context + 1, seq); } module_init(drm_sched_fence_slab_init); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index fb88301b3c45..b77f24a783e3 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -297,10 +297,6 @@ struct drm_sched_fence { * belongs to. */ struct drm_gpu_scheduler *sched; - /** - * @lock: the lock used by the scheduled and the finished fences. - */ - spinlock_t lock; /** * @owner: job owner for debugging */
On 12/12/2025 15:50, Christian König wrote:
On 12/11/25 16:13, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the scheduler fences as well just in case if anybody uses this as blueprint for its own implementation.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- include/drm/gpu_scheduler.h | 4 ---- 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 08ccbde8b2f5..47471b9e43f9 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, /* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { - spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags);
Rebase error I guess. Pull into the locking helpers patch.
No that is actually completely intentional here.
Previously we had a separate lock which protected both the DMA-fences as well as the deadline state.
Now we turn that upside down by dropping the separate lock and protecting the deadline state with the dma_fence lock instead.
I don't follow. The code is currently like this:
static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, ktime_t deadline) { struct drm_sched_fence *fence = to_drm_sched_fence(f); struct dma_fence *parent; unsigned long flags;
spin_lock_irqsave(&fence->lock, flags);
/* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { spin_unlock_irqrestore(&fence->lock, flags); return; }
fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
spin_unlock_irqrestore(&fence->lock, flags);...
The diff changes one out of the three lock/unlock operations. Other two are changed in 3/19. All three should surely be changed in the same patch.
Regards,
Tvrtko
Regards, Christian.
Regards,
Tvrtko
return; } @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, fence->owner = owner; fence->drm_client_id = drm_client_id; - spin_lock_init(&fence->lock); return fence; } @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, fence->sched = entity->rq->sched; seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, - &fence->lock, entity->fence_context, seq); + NULL, entity->fence_context, seq); dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, - &fence->lock, entity->fence_context + 1, seq); + NULL, entity->fence_context + 1, seq); } module_init(drm_sched_fence_slab_init); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index fb88301b3c45..b77f24a783e3 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -297,10 +297,6 @@ struct drm_sched_fence { * belongs to. */ struct drm_gpu_scheduler *sched; - /** - * @lock: the lock used by the scheduled and the finished fences. - */ - spinlock_t lock; /** * @owner: job owner for debugging */
On 12/15/25 10:20, Tvrtko Ursulin wrote:
On 12/12/2025 15:50, Christian König wrote:
On 12/11/25 16:13, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the scheduler fences as well just in case if anybody uses this as blueprint for its own implementation.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- include/drm/gpu_scheduler.h | 4 ---- 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 08ccbde8b2f5..47471b9e43f9 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, /* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { - spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags);
Rebase error I guess. Pull into the locking helpers patch.
No that is actually completely intentional here.
Previously we had a separate lock which protected both the DMA-fences as well as the deadline state.
Now we turn that upside down by dropping the separate lock and protecting the deadline state with the dma_fence lock instead.
I don't follow. The code is currently like this:
static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, ktime_t deadline) { struct drm_sched_fence *fence = to_drm_sched_fence(f); struct dma_fence *parent; unsigned long flags;
spin_lock_irqsave(&fence->lock, flags);
/* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { spin_unlock_irqrestore(&fence->lock, flags); return; }
fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
spin_unlock_irqrestore(&fence->lock, flags);...
The diff changes one out of the three lock/unlock operations. Other two are changed in 3/19. All three should surely be changed in the same patch.
We could change those spin_lock/unlock calls in patch #3, but I don't think that this is clean.
See the code here currently uses fence->lock and patch #3 would change it to use fence->finished->lock instead. That might be the pointer at the moment, but that is just by coincident and not design.
Only this change here ontop makes it intentional that we use fence->finished->lock for everything.
Regards, Christian.
Regards,
Tvrtko
Regards, Christian.
Regards,
Tvrtko
return; } @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, fence->owner = owner; fence->drm_client_id = drm_client_id; - spin_lock_init(&fence->lock); return fence; } @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, fence->sched = entity->rq->sched; seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, - &fence->lock, entity->fence_context, seq); + NULL, entity->fence_context, seq); dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, - &fence->lock, entity->fence_context + 1, seq); + NULL, entity->fence_context + 1, seq); } module_init(drm_sched_fence_slab_init); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index fb88301b3c45..b77f24a783e3 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -297,10 +297,6 @@ struct drm_sched_fence { * belongs to. */ struct drm_gpu_scheduler *sched; - /** - * @lock: the lock used by the scheduled and the finished fences. - */ - spinlock_t lock; /** * @owner: job owner for debugging */
On 15/12/2025 15:38, Christian König wrote:
On 12/15/25 10:20, Tvrtko Ursulin wrote:
On 12/12/2025 15:50, Christian König wrote:
On 12/11/25 16:13, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the scheduler fences as well just in case if anybody uses this as blueprint for its own implementation.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- include/drm/gpu_scheduler.h | 4 ---- 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 08ccbde8b2f5..47471b9e43f9 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, /* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { - spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags);
Rebase error I guess. Pull into the locking helpers patch.
No that is actually completely intentional here.
Previously we had a separate lock which protected both the DMA-fences as well as the deadline state.
Now we turn that upside down by dropping the separate lock and protecting the deadline state with the dma_fence lock instead.
I don't follow. The code is currently like this:
static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, ktime_t deadline) { struct drm_sched_fence *fence = to_drm_sched_fence(f); struct dma_fence *parent; unsigned long flags;
spin_lock_irqsave(&fence->lock, flags);
/* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { spin_unlock_irqrestore(&fence->lock, flags); return; }
fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
spin_unlock_irqrestore(&fence->lock, flags);...
The diff changes one out of the three lock/unlock operations. Other two are changed in 3/19. All three should surely be changed in the same patch.
We could change those spin_lock/unlock calls in patch #3, but I don't think that this is clean.
See the code here currently uses fence->lock and patch #3 would change it to use fence->finished->lock instead. That might be the pointer at the moment, but that is just by coincident and not design.
Only this change here ontop makes it intentional that we use fence->finished->lock for everything.
Sorry I still don't follow. After 3/19 and before this 9/19 the function looks like this:
static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, ktime_t deadline) { struct drm_sched_fence *fence = to_drm_sched_fence(f); struct dma_fence *parent; unsigned long flags;
dma_fence_lock_irqsave(f, flags);
/* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { spin_unlock_irqrestore(&fence->lock, flags); return; }
fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
dma_fence_unlock_irqrestore(f, flags);
Notice the lonely spin_unlock_irqrestore on the early return path while other two use the dma_fence_(un)lock helpers. Am I blind or how is that clean?
Regards,
Tvrtko
Regards, Christian.
Regards,
Tvrtko
Regards, Christian.
Regards,
Tvrtko
return; } @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, fence->owner = owner; fence->drm_client_id = drm_client_id; - spin_lock_init(&fence->lock); return fence; } @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, fence->sched = entity->rq->sched; seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, - &fence->lock, entity->fence_context, seq); + NULL, entity->fence_context, seq); dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, - &fence->lock, entity->fence_context + 1, seq); + NULL, entity->fence_context + 1, seq); } module_init(drm_sched_fence_slab_init); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index fb88301b3c45..b77f24a783e3 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -297,10 +297,6 @@ struct drm_sched_fence { * belongs to. */ struct drm_gpu_scheduler *sched; - /** - * @lock: the lock used by the scheduled and the finished fences. - */ - spinlock_t lock; /** * @owner: job owner for debugging */
On 12/15/25 16:53, Tvrtko Ursulin wrote:
On 15/12/2025 15:38, Christian König wrote:
On 12/15/25 10:20, Tvrtko Ursulin wrote:
On 12/12/2025 15:50, Christian König wrote:
On 12/11/25 16:13, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the scheduler fences as well just in case if anybody uses this as blueprint for its own implementation.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/scheduler/sched_fence.c | 7 +++---- include/drm/gpu_scheduler.h | 4 ---- 2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 08ccbde8b2f5..47471b9e43f9 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, /* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { - spin_unlock_irqrestore(&fence->lock, flags); + dma_fence_unlock_irqrestore(f, flags);
Rebase error I guess. Pull into the locking helpers patch.
No that is actually completely intentional here.
Previously we had a separate lock which protected both the DMA-fences as well as the deadline state.
Now we turn that upside down by dropping the separate lock and protecting the deadline state with the dma_fence lock instead.
I don't follow. The code is currently like this:
static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, ktime_t deadline) { struct drm_sched_fence *fence = to_drm_sched_fence(f); struct dma_fence *parent; unsigned long flags;
spin_lock_irqsave(&fence->lock, flags);
/* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { spin_unlock_irqrestore(&fence->lock, flags); return; }
fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
spin_unlock_irqrestore(&fence->lock, flags);...
The diff changes one out of the three lock/unlock operations. Other two are changed in 3/19. All three should surely be changed in the same patch.
We could change those spin_lock/unlock calls in patch #3, but I don't think that this is clean.
See the code here currently uses fence->lock and patch #3 would change it to use fence->finished->lock instead. That might be the pointer at the moment, but that is just by coincident and not design.
Only this change here ontop makes it intentional that we use fence->finished->lock for everything.
Sorry I still don't follow. After 3/19 and before this 9/19 the function looks like this:
static void drm_sched_fence_set_deadline_finished(struct dma_fence *f, ktime_t deadline) { struct drm_sched_fence *fence = to_drm_sched_fence(f); struct dma_fence *parent; unsigned long flags;
dma_fence_lock_irqsave(f, flags);
/* If we already have an earlier deadline, keep it: */ if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) && ktime_before(fence->deadline, deadline)) { spin_unlock_irqrestore(&fence->lock, flags); return; }
fence->deadline = deadline; set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
dma_fence_unlock_irqrestore(f, flags);
Notice the lonely spin_unlock_irqrestore on the early return path while other two use the dma_fence_(un)lock helpers. Am I blind or how is that clean?
Oh, that's what you mean. Sorry I was blind!
Yeah that is clearly unintentional.
Thanks, Christian.
Regards,
Tvrtko
Regards, Christian.
Regards,
Tvrtko
Regards, Christian.
Regards,
Tvrtko
return; } @@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, fence->owner = owner; fence->drm_client_id = drm_client_id; - spin_lock_init(&fence->lock); return fence; } @@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence, fence->sched = entity->rq->sched; seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, - &fence->lock, entity->fence_context, seq); + NULL, entity->fence_context, seq); dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, - &fence->lock, entity->fence_context + 1, seq); + NULL, entity->fence_context + 1, seq); } module_init(drm_sched_fence_slab_init); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index fb88301b3c45..b77f24a783e3 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -297,10 +297,6 @@ struct drm_sched_fence { * belongs to. */ struct drm_gpu_scheduler *sched; - /** - * @lock: the lock used by the scheduled and the finished fences. - */ - spinlock_t lock; /** * @owner: job owner for debugging */
Calling dma_fence_is_signaled() here is illegal!
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c index 1ef758ac5076..09c919f72b6c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c @@ -120,12 +120,6 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f) { struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
- if (!fence) - return false; - - if (dma_fence_is_signaled(f)) - return true; - if (!fence->svm_bo) { if (!kgd2kfd_schedule_evict_and_restore_process(fence->mm, f)) return true;
This allows amdgpu_fences to outlive the amdgpu module.
v2: use dma_fence_get_rcu_safe to be NULL safe here.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 63 +++++++---------------- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 - 2 files changed, 20 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index c7843e336310..c636347801c1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -112,8 +112,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct amdgpu_fence *af, af->ring = ring;
seq = ++ring->fence_drv.sync_seq; - dma_fence_init(fence, &amdgpu_fence_ops, - &ring->fence_drv.lock, + dma_fence_init(fence, &amdgpu_fence_ops, NULL, adev->fence_context + ring->idx, seq);
amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr, @@ -467,7 +466,6 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring) timer_setup(&ring->fence_drv.fallback_timer, amdgpu_fence_fallback, 0);
ring->fence_drv.num_fences_mask = ring->num_hw_submission * 2 - 1; - spin_lock_init(&ring->fence_drv.lock); ring->fence_drv.fences = kcalloc(ring->num_hw_submission * 2, sizeof(void *), GFP_KERNEL);
@@ -654,16 +652,20 @@ void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error) struct amdgpu_fence_driver *drv = &ring->fence_drv; unsigned long flags;
- spin_lock_irqsave(&drv->lock, flags); + rcu_read_lock(); for (unsigned int i = 0; i <= drv->num_fences_mask; ++i) { struct dma_fence *fence;
- fence = rcu_dereference_protected(drv->fences[i], - lockdep_is_held(&drv->lock)); - if (fence && !dma_fence_is_signaled_locked(fence)) + fence = dma_fence_get_rcu(drv->fences[i]); + if (!fence) + continue; + + dma_fence_lock_irqsave(fence, flags); + if (!dma_fence_is_signaled_locked(fence)) dma_fence_set_error(fence, error); + dma_fence_unlock_irqrestore(fence, flags); } - spin_unlock_irqrestore(&drv->lock, flags); + rcu_read_unlock(); }
/** @@ -714,16 +716,19 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) seq = ring->fence_drv.sync_seq & ring->fence_drv.num_fences_mask;
/* mark all fences from the guilty context with an error */ - spin_lock_irqsave(&ring->fence_drv.lock, flags); + rcu_read_lock(); do { last_seq++; last_seq &= ring->fence_drv.num_fences_mask;
ptr = &ring->fence_drv.fences[last_seq]; - rcu_read_lock(); - unprocessed = rcu_dereference(*ptr); + unprocessed = dma_fence_get_rcu_safe(ptr); + + if (!unprocessed) + continue;
- if (unprocessed && !dma_fence_is_signaled_locked(unprocessed)) { + dma_fence_lock_irqsave(unprocessed, flags); + if (dma_fence_is_signaled_locked(unprocessed)) { fence = container_of(unprocessed, struct amdgpu_fence, base);
if (fence == af) @@ -731,9 +736,10 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) else if (fence->context == af->context) dma_fence_set_error(&fence->base, -ECANCELED); } - rcu_read_unlock(); + dma_fence_unlock_irqrestore(unprocessed, flags); + dma_fence_put(unprocessed); } while (last_seq != seq); - spin_unlock_irqrestore(&ring->fence_drv.lock, flags); + rcu_read_unlock(); /* signal the guilty fence */ amdgpu_fence_write(ring, (u32)af->base.seqno); amdgpu_fence_process(ring); @@ -823,39 +829,10 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f) return true; }
-/** - * amdgpu_fence_free - free up the fence memory - * - * @rcu: RCU callback head - * - * Free up the fence memory after the RCU grace period. - */ -static void amdgpu_fence_free(struct rcu_head *rcu) -{ - struct dma_fence *f = container_of(rcu, struct dma_fence, rcu); - - /* free fence_slab if it's separated fence*/ - kfree(to_amdgpu_fence(f)); -} - -/** - * amdgpu_fence_release - callback that fence can be freed - * - * @f: fence - * - * This function is called when the reference count becomes zero. - * It just RCU schedules freeing up the fence. - */ -static void amdgpu_fence_release(struct dma_fence *f) -{ - call_rcu(&f->rcu, amdgpu_fence_free); -} - static const struct dma_fence_ops amdgpu_fence_ops = { .get_driver_name = amdgpu_fence_get_driver_name, .get_timeline_name = amdgpu_fence_get_timeline_name, .enable_signaling = amdgpu_fence_enable_signaling, - .release = amdgpu_fence_release, };
/* diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h index 7a27c6c4bb44..9cbf63454004 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h @@ -125,7 +125,6 @@ struct amdgpu_fence_driver { unsigned irq_type; struct timer_list fallback_timer; unsigned num_fences_mask; - spinlock_t lock; struct dma_fence **fences; };
On 11/12/2025 13:16, Christian König wrote:
This allows amdgpu_fences to outlive the amdgpu module.
v2: use dma_fence_get_rcu_safe to be NULL safe here.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 63 +++++++---------------- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 - 2 files changed, 20 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index c7843e336310..c636347801c1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -112,8 +112,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct amdgpu_fence *af, af->ring = ring; seq = ++ring->fence_drv.sync_seq;
- dma_fence_init(fence, &amdgpu_fence_ops,
&ring->fence_drv.lock,
- dma_fence_init(fence, &amdgpu_fence_ops, NULL, adev->fence_context + ring->idx, seq);
amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr, @@ -467,7 +466,6 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring) timer_setup(&ring->fence_drv.fallback_timer, amdgpu_fence_fallback, 0); ring->fence_drv.num_fences_mask = ring->num_hw_submission * 2 - 1;
- spin_lock_init(&ring->fence_drv.lock); ring->fence_drv.fences = kcalloc(ring->num_hw_submission * 2, sizeof(void *), GFP_KERNEL);
@@ -654,16 +652,20 @@ void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error) struct amdgpu_fence_driver *drv = &ring->fence_drv; unsigned long flags;
- spin_lock_irqsave(&drv->lock, flags);
- rcu_read_lock(); for (unsigned int i = 0; i <= drv->num_fences_mask; ++i) { struct dma_fence *fence;
fence = rcu_dereference_protected(drv->fences[i],lockdep_is_held(&drv->lock));if (fence && !dma_fence_is_signaled_locked(fence))
fence = dma_fence_get_rcu(drv->fences[i]);
dma_fence_get_rcu is not safe against passing a NULL fence in, while the existing code makes it look like drv->fence[] slot can contain NULL at this point?
amdgpu_fence_process() is the place which can NULL the slots? Irq context? Why is that safe with no reference held from clearing the slot to operating on the fence?
if (!fence)continue;dma_fence_lock_irqsave(fence, flags);if (!dma_fence_is_signaled_locked(fence)) dma_fence_set_error(fence, error);dma_fence_unlock_irqrestore(fence, flags);
- spin_unlock_irqrestore(&drv->lock, flags);
- rcu_read_unlock(); }
/** @@ -714,16 +716,19 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) seq = ring->fence_drv.sync_seq & ring->fence_drv.num_fences_mask; /* mark all fences from the guilty context with an error */
- spin_lock_irqsave(&ring->fence_drv.lock, flags);
- rcu_read_lock(); do { last_seq++; last_seq &= ring->fence_drv.num_fences_mask;
ptr = &ring->fence_drv.fences[last_seq];
rcu_read_lock();unprocessed = rcu_dereference(*ptr);
unprocessed = dma_fence_get_rcu_safe(ptr);
Similar concern like the above.
Regards,
Tvrtko
if (!unprocessed)continue;
if (unprocessed && !dma_fence_is_signaled_locked(unprocessed)) {
dma_fence_lock_irqsave(unprocessed, flags);if (dma_fence_is_signaled_locked(unprocessed)) { fence = container_of(unprocessed, struct amdgpu_fence, base);if (fence == af) @@ -731,9 +736,10 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) else if (fence->context == af->context) dma_fence_set_error(&fence->base, -ECANCELED); }
rcu_read_unlock();
dma_fence_unlock_irqrestore(unprocessed, flags);dma_fence_put(unprocessed);
- spin_unlock_irqrestore(&ring->fence_drv.lock, flags);
- rcu_read_unlock(); /* signal the guilty fence */ amdgpu_fence_write(ring, (u32)af->base.seqno); amdgpu_fence_process(ring);
@@ -823,39 +829,10 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f) return true; } -/**
- amdgpu_fence_free - free up the fence memory
- @rcu: RCU callback head
- Free up the fence memory after the RCU grace period.
- */
-static void amdgpu_fence_free(struct rcu_head *rcu) -{
- struct dma_fence *f = container_of(rcu, struct dma_fence, rcu);
- /* free fence_slab if it's separated fence*/
- kfree(to_amdgpu_fence(f));
-}
-/**
- amdgpu_fence_release - callback that fence can be freed
- @f: fence
- This function is called when the reference count becomes zero.
- It just RCU schedules freeing up the fence.
- */
-static void amdgpu_fence_release(struct dma_fence *f) -{
- call_rcu(&f->rcu, amdgpu_fence_free);
-}
- static const struct dma_fence_ops amdgpu_fence_ops = { .get_driver_name = amdgpu_fence_get_driver_name, .get_timeline_name = amdgpu_fence_get_timeline_name, .enable_signaling = amdgpu_fence_enable_signaling,
- .release = amdgpu_fence_release, };
/* diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h index 7a27c6c4bb44..9cbf63454004 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h @@ -125,7 +125,6 @@ struct amdgpu_fence_driver { unsigned irq_type; struct timer_list fallback_timer; unsigned num_fences_mask;
- spinlock_t lock; struct dma_fence **fences; };
On 12/11/25 16:53, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
This allows amdgpu_fences to outlive the amdgpu module.
v2: use dma_fence_get_rcu_safe to be NULL safe here.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 63 +++++++---------------- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 - 2 files changed, 20 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index c7843e336310..c636347801c1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -112,8 +112,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct amdgpu_fence *af, af->ring = ring; seq = ++ring->fence_drv.sync_seq; - dma_fence_init(fence, &amdgpu_fence_ops, - &ring->fence_drv.lock, + dma_fence_init(fence, &amdgpu_fence_ops, NULL, adev->fence_context + ring->idx, seq); amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr, @@ -467,7 +466,6 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring) timer_setup(&ring->fence_drv.fallback_timer, amdgpu_fence_fallback, 0); ring->fence_drv.num_fences_mask = ring->num_hw_submission * 2 - 1; - spin_lock_init(&ring->fence_drv.lock); ring->fence_drv.fences = kcalloc(ring->num_hw_submission * 2, sizeof(void *), GFP_KERNEL); @@ -654,16 +652,20 @@ void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error) struct amdgpu_fence_driver *drv = &ring->fence_drv; unsigned long flags; - spin_lock_irqsave(&drv->lock, flags); + rcu_read_lock(); for (unsigned int i = 0; i <= drv->num_fences_mask; ++i) { struct dma_fence *fence; - fence = rcu_dereference_protected(drv->fences[i], - lockdep_is_held(&drv->lock)); - if (fence && !dma_fence_is_signaled_locked(fence)) + fence = dma_fence_get_rcu(drv->fences[i]);
dma_fence_get_rcu is not safe against passing a NULL fence in, while the existing code makes it look like drv->fence[] slot can contain NULL at this point?
amdgpu_fence_process() is the place which can NULL the slots? Irq context? Why is that safe with no reference held from clearing the slot to operating on the fence?
The slots are never cleared. It can only be that they are NULL when the driver is loaded.
I've switched over to dma_fence_get_rcu_safe() where appropriated.
Regards, Christian.
+ if (!fence) + continue;
+ dma_fence_lock_irqsave(fence, flags); + if (!dma_fence_is_signaled_locked(fence)) dma_fence_set_error(fence, error); + dma_fence_unlock_irqrestore(fence, flags); } - spin_unlock_irqrestore(&drv->lock, flags); + rcu_read_unlock(); } /** @@ -714,16 +716,19 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) seq = ring->fence_drv.sync_seq & ring->fence_drv.num_fences_mask; /* mark all fences from the guilty context with an error */ - spin_lock_irqsave(&ring->fence_drv.lock, flags); + rcu_read_lock(); do { last_seq++; last_seq &= ring->fence_drv.num_fences_mask; ptr = &ring->fence_drv.fences[last_seq]; - rcu_read_lock(); - unprocessed = rcu_dereference(*ptr); + unprocessed = dma_fence_get_rcu_safe(ptr);
Similar concern like the above.
Regards,
Tvrtko
+ if (!unprocessed) + continue; - if (unprocessed && !dma_fence_is_signaled_locked(unprocessed)) { + dma_fence_lock_irqsave(unprocessed, flags); + if (dma_fence_is_signaled_locked(unprocessed)) { fence = container_of(unprocessed, struct amdgpu_fence, base); if (fence == af) @@ -731,9 +736,10 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) else if (fence->context == af->context) dma_fence_set_error(&fence->base, -ECANCELED); } - rcu_read_unlock(); + dma_fence_unlock_irqrestore(unprocessed, flags); + dma_fence_put(unprocessed); } while (last_seq != seq); - spin_unlock_irqrestore(&ring->fence_drv.lock, flags); + rcu_read_unlock(); /* signal the guilty fence */ amdgpu_fence_write(ring, (u32)af->base.seqno); amdgpu_fence_process(ring); @@ -823,39 +829,10 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f) return true; } -/**
- amdgpu_fence_free - free up the fence memory
- @rcu: RCU callback head
- Free up the fence memory after the RCU grace period.
- */
-static void amdgpu_fence_free(struct rcu_head *rcu) -{ - struct dma_fence *f = container_of(rcu, struct dma_fence, rcu);
- /* free fence_slab if it's separated fence*/ - kfree(to_amdgpu_fence(f)); -}
-/**
- amdgpu_fence_release - callback that fence can be freed
- @f: fence
- This function is called when the reference count becomes zero.
- It just RCU schedules freeing up the fence.
- */
-static void amdgpu_fence_release(struct dma_fence *f) -{ - call_rcu(&f->rcu, amdgpu_fence_free); -}
static const struct dma_fence_ops amdgpu_fence_ops = { .get_driver_name = amdgpu_fence_get_driver_name, .get_timeline_name = amdgpu_fence_get_timeline_name, .enable_signaling = amdgpu_fence_enable_signaling, - .release = amdgpu_fence_release, }; /* diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h index 7a27c6c4bb44..9cbf63454004 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h @@ -125,7 +125,6 @@ struct amdgpu_fence_driver { unsigned irq_type; struct timer_list fallback_timer; unsigned num_fences_mask; - spinlock_t lock; struct dma_fence **fences; };
On 12/12/2025 15:49, Christian König wrote:
On 12/11/25 16:53, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
This allows amdgpu_fences to outlive the amdgpu module.
v2: use dma_fence_get_rcu_safe to be NULL safe here.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 63 +++++++---------------- drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h | 1 - 2 files changed, 20 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c index c7843e336310..c636347801c1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c @@ -112,8 +112,7 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct amdgpu_fence *af, af->ring = ring; seq = ++ring->fence_drv.sync_seq; - dma_fence_init(fence, &amdgpu_fence_ops, - &ring->fence_drv.lock, + dma_fence_init(fence, &amdgpu_fence_ops, NULL, adev->fence_context + ring->idx, seq); amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr, @@ -467,7 +466,6 @@ int amdgpu_fence_driver_init_ring(struct amdgpu_ring *ring) timer_setup(&ring->fence_drv.fallback_timer, amdgpu_fence_fallback, 0); ring->fence_drv.num_fences_mask = ring->num_hw_submission * 2 - 1; - spin_lock_init(&ring->fence_drv.lock); ring->fence_drv.fences = kcalloc(ring->num_hw_submission * 2, sizeof(void *), GFP_KERNEL); @@ -654,16 +652,20 @@ void amdgpu_fence_driver_set_error(struct amdgpu_ring *ring, int error) struct amdgpu_fence_driver *drv = &ring->fence_drv; unsigned long flags; - spin_lock_irqsave(&drv->lock, flags); + rcu_read_lock(); for (unsigned int i = 0; i <= drv->num_fences_mask; ++i) { struct dma_fence *fence; - fence = rcu_dereference_protected(drv->fences[i], - lockdep_is_held(&drv->lock)); - if (fence && !dma_fence_is_signaled_locked(fence)) + fence = dma_fence_get_rcu(drv->fences[i]);
dma_fence_get_rcu is not safe against passing a NULL fence in, while the existing code makes it look like drv->fence[] slot can contain NULL at this point?
amdgpu_fence_process() is the place which can NULL the slots? Irq context? Why is that safe with no reference held from clearing the slot to operating on the fence?
The slots are never cleared. It can only be that they are NULL when the driver is loaded.
Are you sure?
bool amdgpu_fence_process(struct amdgpu_ring *ring) { ... ptr = &drv->fences[last_seq];
/* There is always exactly one thread signaling this fence slot */ fence = rcu_dereference_protected(*ptr, 1); RCU_INIT_POINTER(*ptr, NULL);
The above looks like it can set slots to NULL. At least if the code is reachable. I don't claim that it is because I could not easily follow the logic behind the high level scheme fence driver and associated state implements.
I've switched over to dma_fence_get_rcu_safe() where appropriated.
You mean below in amdgpu_fence_driver_guilty_force_completion? In amdgpu_fence_driver_set_error you are sure it is not needed so the existing code is too careful?
Regards,
Tvrtko
+ if (!fence) + continue;
+ dma_fence_lock_irqsave(fence, flags); + if (!dma_fence_is_signaled_locked(fence)) dma_fence_set_error(fence, error); + dma_fence_unlock_irqrestore(fence, flags); } - spin_unlock_irqrestore(&drv->lock, flags); + rcu_read_unlock(); } /** @@ -714,16 +716,19 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) seq = ring->fence_drv.sync_seq & ring->fence_drv.num_fences_mask; /* mark all fences from the guilty context with an error */ - spin_lock_irqsave(&ring->fence_drv.lock, flags); + rcu_read_lock(); do { last_seq++; last_seq &= ring->fence_drv.num_fences_mask; ptr = &ring->fence_drv.fences[last_seq]; - rcu_read_lock(); - unprocessed = rcu_dereference(*ptr); + unprocessed = dma_fence_get_rcu_safe(ptr);
Similar concern like the above.
Regards,
Tvrtko
+ if (!unprocessed) + continue; - if (unprocessed && !dma_fence_is_signaled_locked(unprocessed)) { + dma_fence_lock_irqsave(unprocessed, flags); + if (dma_fence_is_signaled_locked(unprocessed)) { fence = container_of(unprocessed, struct amdgpu_fence, base); if (fence == af) @@ -731,9 +736,10 @@ void amdgpu_fence_driver_guilty_force_completion(struct amdgpu_fence *af) else if (fence->context == af->context) dma_fence_set_error(&fence->base, -ECANCELED); } - rcu_read_unlock(); + dma_fence_unlock_irqrestore(unprocessed, flags); + dma_fence_put(unprocessed); } while (last_seq != seq); - spin_unlock_irqrestore(&ring->fence_drv.lock, flags); + rcu_read_unlock(); /* signal the guilty fence */ amdgpu_fence_write(ring, (u32)af->base.seqno); amdgpu_fence_process(ring); @@ -823,39 +829,10 @@ static bool amdgpu_fence_enable_signaling(struct dma_fence *f) return true; } -/**
- amdgpu_fence_free - free up the fence memory
- @rcu: RCU callback head
- Free up the fence memory after the RCU grace period.
- */
-static void amdgpu_fence_free(struct rcu_head *rcu) -{ - struct dma_fence *f = container_of(rcu, struct dma_fence, rcu);
- /* free fence_slab if it's separated fence*/ - kfree(to_amdgpu_fence(f)); -}
-/**
- amdgpu_fence_release - callback that fence can be freed
- @f: fence
- This function is called when the reference count becomes zero.
- It just RCU schedules freeing up the fence.
- */
-static void amdgpu_fence_release(struct dma_fence *f) -{ - call_rcu(&f->rcu, amdgpu_fence_free); -}
static const struct dma_fence_ops amdgpu_fence_ops = { .get_driver_name = amdgpu_fence_get_driver_name, .get_timeline_name = amdgpu_fence_get_timeline_name, .enable_signaling = amdgpu_fence_enable_signaling, - .release = amdgpu_fence_release, }; /* diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h index 7a27c6c4bb44..9cbf63454004 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h @@ -125,7 +125,6 @@ struct amdgpu_fence_driver { unsigned irq_type; struct timer_list fallback_timer; unsigned num_fences_mask; - spinlock_t lock; struct dma_fence **fences; };
This allows amdgpu_eviction_fences to outlive the amdgpu module.
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 3 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h | 1 - 2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c index 23d7d0b0d625..95ee22c43ceb 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c @@ -167,9 +167,8 @@ amdgpu_eviction_fence_create(struct amdgpu_eviction_fence_mgr *evf_mgr)
ev_fence->evf_mgr = evf_mgr; get_task_comm(ev_fence->timeline_name, current); - spin_lock_init(&ev_fence->lock); dma_fence_init64(&ev_fence->base, &amdgpu_eviction_fence_ops, - &ev_fence->lock, evf_mgr->ev_fence_ctx, + NULL, evf_mgr->ev_fence_ctx, atomic_inc_return(&evf_mgr->ev_fence_seq)); return ev_fence; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h index fcd867b7147d..fb70efb54338 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h @@ -27,7 +27,6 @@
struct amdgpu_eviction_fence { struct dma_fence base; - spinlock_t lock; char timeline_name[TASK_COMM_LEN]; struct amdgpu_eviction_fence_mgr *evf_mgr; };
This allows amdgpu_vm_tlb_fences to outlive the amdgpu module.
Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c index 5d26797356a3..27bf1f569830 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c @@ -33,7 +33,6 @@ struct amdgpu_tlb_fence { struct amdgpu_device *adev; struct dma_fence *dependency; struct work_struct work; - spinlock_t lock; uint16_t pasid;
}; @@ -98,9 +97,8 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm *vm f->dependency = *fence; f->pasid = vm->pasid; INIT_WORK(&f->work, amdgpu_tlb_fence_work); - spin_lock_init(&f->lock);
- dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, &f->lock, + dma_fence_init64(&f->base, &amdgpu_tlb_fence_ops, NULL, vm->tlb_fence_context, atomic64_read(&vm->tlb_seq));
/* TODO: We probably need a separate wq here */
This allows amdkfd_fences to outlive the amdgpu module.
v2: implement Felix suggestion to lock the fence while signaling it. v3: fix typos v4: fix return code in signal_eviction_fence
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 7 +++ .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 44 +++++++++---------- drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 +- drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 +- 4 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index 8bdfcde2029b..2f2b277cfaed 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -196,6 +196,7 @@ int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data); #endif #if IS_ENABLED(CONFIG_HSA_AMD) bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm); +bool amdkfd_fence_signal(struct dma_fence *f); struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f); void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo); int amdgpu_amdkfd_evict_userptr(struct mmu_interval_notifier *mni, @@ -210,6 +211,12 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) return false; }
+static inline +bool amdkfd_fence_signal(struct dma_fence *f) +{ + return false; +} + static inline struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f) { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c index 09c919f72b6c..9cd413e325f0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c @@ -127,29 +127,9 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f) if (!svm_range_schedule_evict_svm_bo(fence)) return true; } - return false; -} - -/** - * amdkfd_fence_release - callback that fence can be freed - * - * @f: dma_fence - * - * This function is called when the reference count becomes zero. - * Drops the mm_struct reference and RCU schedules freeing up the fence. - */ -static void amdkfd_fence_release(struct dma_fence *f) -{ - struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f); - - /* Unconditionally signal the fence. The process is getting - * terminated. - */ - if (WARN_ON(!fence)) - return; /* Not an amdgpu_amdkfd_fence */ - mmdrop(fence->mm); - kfree_rcu(f, rcu); + fence->mm = NULL; + return false; }
/** @@ -174,9 +154,27 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) return false; }
+bool amdkfd_fence_signal(struct dma_fence *f) +{ + struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f); + unsigned long flags; + bool was_signaled; + + dma_fence_lock_irqsave(f, flags); + if (fence->mm) { + mmdrop(fence->mm); + fence->mm = NULL; + } + was_signaled = dma_fence_is_signaled_locked(f); + if (!was_signaled) + dma_fence_signal_locked(f); + dma_fence_unlock_irqrestore(f, flags); + + return was_signaled; +} + static const struct dma_fence_ops amdkfd_fence_ops = { .get_driver_name = amdkfd_fence_get_driver_name, .get_timeline_name = amdkfd_fence_get_timeline_name, .enable_signaling = amdkfd_fence_enable_signaling, - .release = amdkfd_fence_release, }; diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c index bb252ec43733..2cf39e3d3fae 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c @@ -1173,7 +1173,7 @@ static void kfd_process_wq_release(struct work_struct *work) synchronize_rcu(); ef = rcu_access_pointer(p->ef); if (ef) - dma_fence_signal(ef); + amdkfd_fence_signal(ef);
kfd_process_remove_sysfs(p); kfd_debugfs_remove_process(p); diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index 97c2270f278f..0e94f3a976b1 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -428,7 +428,7 @@ static void svm_range_bo_release(struct kref *kref)
if (!dma_fence_is_signaled(&svm_bo->eviction_fence->base)) /* We're not in the eviction worker. Signal the fence. */ - dma_fence_signal(&svm_bo->eviction_fence->base); + amdkfd_fence_signal(&svm_bo->eviction_fence->base); dma_fence_put(&svm_bo->eviction_fence->base); amdgpu_bo_unref(&svm_bo->bo); kfree(svm_bo); @@ -3628,7 +3628,7 @@ static void svm_range_evict_svm_bo_worker(struct work_struct *work) mmap_read_unlock(mm); mmput(mm);
- dma_fence_signal(&svm_bo->eviction_fence->base); + amdkfd_fence_signal(&svm_bo->eviction_fence->base);
/* This is the last reference to svm_bo, after svm_range_vram_node_free * has been called in svm_migrate_vram_to_ram
On Thu, 2025-12-11 at 13:16 +0100, Christian König wrote:
This allows amdkfd_fences to outlive the amdgpu module.
v2: implement Felix suggestion to lock the fence while signaling it. v3: fix typos v4: fix return code in signal_eviction_fence
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 7 +++ .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 44 +++++++++---------- drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 +- drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 +- 4 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index 8bdfcde2029b..2f2b277cfaed 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -196,6 +196,7 @@ int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data); #endif #if IS_ENABLED(CONFIG_HSA_AMD) bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm); +bool amdkfd_fence_signal(struct dma_fence *f); struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f); void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo); int amdgpu_amdkfd_evict_userptr(struct mmu_interval_notifier *mni, @@ -210,6 +211,12 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) return false; } +static inline +bool amdkfd_fence_signal(struct dma_fence *f) +{
- return false;
+}
Huh? What's that?
That function seems to be just a NOP. It's return code is used nowhere, is it?
static inline struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f) { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c index 09c919f72b6c..9cd413e325f0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c @@ -127,29 +127,9 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f) if (!svm_range_schedule_evict_svm_bo(fence)) return true; }
- return false;
-}
-/**
- amdkfd_fence_release - callback that fence can be freed
- @f: dma_fence
- This function is called when the reference count becomes zero.
- Drops the mm_struct reference and RCU schedules freeing up the fence.
- */
-static void amdkfd_fence_release(struct dma_fence *f) -{
- struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
- /* Unconditionally signal the fence. The process is getting
* terminated.*/- if (WARN_ON(!fence))
return; /* Not an amdgpu_amdkfd_fence */mmdrop(fence->mm);
- kfree_rcu(f, rcu);
- fence->mm = NULL;
- return false;
} /** @@ -174,9 +154,27 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) return false; } +bool amdkfd_fence_signal(struct dma_fence *f) +{
- struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
- unsigned long flags;
- bool was_signaled;
- dma_fence_lock_irqsave(f, flags);
- if (fence->mm) {
mmdrop(fence->mm);fence->mm = NULL;- }
- was_signaled = dma_fence_is_signaled_locked(f);
- if (!was_signaled)
dma_fence_signal_locked(f);- dma_fence_unlock_irqrestore(f, flags);
- return was_signaled;
+}
static const struct dma_fence_ops amdkfd_fence_ops = { .get_driver_name = amdkfd_fence_get_driver_name, .get_timeline_name = amdkfd_fence_get_timeline_name, .enable_signaling = amdkfd_fence_enable_signaling,
- .release = amdkfd_fence_release,
}; diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c index bb252ec43733..2cf39e3d3fae 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c @@ -1173,7 +1173,7 @@ static void kfd_process_wq_release(struct work_struct *work) synchronize_rcu(); ef = rcu_access_pointer(p->ef); if (ef)
dma_fence_signal(ef);
amdkfd_fence_signal(ef);kfd_process_remove_sysfs(p); kfd_debugfs_remove_process(p); diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index 97c2270f278f..0e94f3a976b1 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -428,7 +428,7 @@ static void svm_range_bo_release(struct kref *kref) if (!dma_fence_is_signaled(&svm_bo->eviction_fence->base)) /* We're not in the eviction worker. Signal the fence. */
dma_fence_signal(&svm_bo->eviction_fence->base);
amdkfd_fence_signal(&svm_bo->eviction_fence->base);dma_fence_put(&svm_bo->eviction_fence->base); amdgpu_bo_unref(&svm_bo->bo); kfree(svm_bo); @@ -3628,7 +3628,7 @@ static void svm_range_evict_svm_bo_worker(struct work_struct *work) mmap_read_unlock(mm); mmput(mm);
- dma_fence_signal(&svm_bo->eviction_fence->base);
- amdkfd_fence_signal(&svm_bo->eviction_fence->base);
And why do you do those changes and why doesn't the commit message explain it?
You stop signalling those fences, after all.
P.
/* This is the last reference to svm_bo, after svm_range_vram_node_free * has been called in svm_migrate_vram_to_ram
On 12/11/25 16:08, Philipp Stanner wrote:
On Thu, 2025-12-11 at 13:16 +0100, Christian König wrote:
This allows amdkfd_fences to outlive the amdgpu module.
v2: implement Felix suggestion to lock the fence while signaling it. v3: fix typos v4: fix return code in signal_eviction_fence
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h | 7 +++ .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c | 44 +++++++++---------- drivers/gpu/drm/amd/amdkfd/kfd_process.c | 2 +- drivers/gpu/drm/amd/amdkfd/kfd_svm.c | 4 +- 4 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index 8bdfcde2029b..2f2b277cfaed 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h @@ -196,6 +196,7 @@ int kfd_debugfs_kfd_mem_limits(struct seq_file *m, void *data); #endif #if IS_ENABLED(CONFIG_HSA_AMD) bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm); +bool amdkfd_fence_signal(struct dma_fence *f); struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f); void amdgpu_amdkfd_remove_all_eviction_fences(struct amdgpu_bo *bo); int amdgpu_amdkfd_evict_userptr(struct mmu_interval_notifier *mni, @@ -210,6 +211,12 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) return false; } +static inline +bool amdkfd_fence_signal(struct dma_fence *f) +{
- return false;
+}
Huh? What's that?
That function seems to be just a NOP. It's return code is used nowhere, is it?
It's the dummy which is used when CONFIG_HSA_AMD isn't enabled.
Not sure if it's actually used or not, but we have dummies for all functions declared in this file.
static inline struct amdgpu_amdkfd_fence *to_amdgpu_amdkfd_fence(struct dma_fence *f) { diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c index 09c919f72b6c..9cd413e325f0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c @@ -127,29 +127,9 @@ static bool amdkfd_fence_enable_signaling(struct dma_fence *f) if (!svm_range_schedule_evict_svm_bo(fence)) return true; }
- return false;
-}
-/**
- amdkfd_fence_release - callback that fence can be freed
- @f: dma_fence
- This function is called when the reference count becomes zero.
- Drops the mm_struct reference and RCU schedules freeing up the fence.
- */
-static void amdkfd_fence_release(struct dma_fence *f) -{
- struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
- /* Unconditionally signal the fence. The process is getting
* terminated.*/- if (WARN_ON(!fence))
return; /* Not an amdgpu_amdkfd_fence */mmdrop(fence->mm);
- kfree_rcu(f, rcu);
- fence->mm = NULL;
- return false;
} /** @@ -174,9 +154,27 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm) return false; } +bool amdkfd_fence_signal(struct dma_fence *f) +{
- struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
- unsigned long flags;
- bool was_signaled;
- dma_fence_lock_irqsave(f, flags);
- if (fence->mm) {
mmdrop(fence->mm);fence->mm = NULL;- }
- was_signaled = dma_fence_is_signaled_locked(f);
- if (!was_signaled)
dma_fence_signal_locked(f);- dma_fence_unlock_irqrestore(f, flags);
- return was_signaled;
+}
static const struct dma_fence_ops amdkfd_fence_ops = { .get_driver_name = amdkfd_fence_get_driver_name, .get_timeline_name = amdkfd_fence_get_timeline_name, .enable_signaling = amdkfd_fence_enable_signaling,
- .release = amdkfd_fence_release,
}; diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c index bb252ec43733..2cf39e3d3fae 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c @@ -1173,7 +1173,7 @@ static void kfd_process_wq_release(struct work_struct *work) synchronize_rcu(); ef = rcu_access_pointer(p->ef); if (ef)
dma_fence_signal(ef);
amdkfd_fence_signal(ef);kfd_process_remove_sysfs(p); kfd_debugfs_remove_process(p); diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index 97c2270f278f..0e94f3a976b1 100644 --- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c +++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c @@ -428,7 +428,7 @@ static void svm_range_bo_release(struct kref *kref) if (!dma_fence_is_signaled(&svm_bo->eviction_fence->base)) /* We're not in the eviction worker. Signal the fence. */
dma_fence_signal(&svm_bo->eviction_fence->base);
amdkfd_fence_signal(&svm_bo->eviction_fence->base);dma_fence_put(&svm_bo->eviction_fence->base); amdgpu_bo_unref(&svm_bo->bo); kfree(svm_bo); @@ -3628,7 +3628,7 @@ static void svm_range_evict_svm_bo_worker(struct work_struct *work) mmap_read_unlock(mm); mmput(mm);
- dma_fence_signal(&svm_bo->eviction_fence->base);
- amdkfd_fence_signal(&svm_bo->eviction_fence->base);
And why do you do those changes and why doesn't the commit message explain it?
You stop signalling those fences, after all.
Hui? I don't stop signaling the fences. I just delegate signaling into a separate helper function which does some extra cleanup before signaling the fence.
Regards, Christian.
P.
/* This is the last reference to svm_bo, after svm_range_vram_node_free * has been called in svm_migrate_vram_to_ram
This allows amdgpu_userq_fences to outlive the amdgpu module.
Signed-off-by: Christian König christian.koenig@amd.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 +---- .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 54 ++++--------------- .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h | 8 --- 3 files changed, 11 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 2dfbddcef9ab..f206297aae8b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -3155,11 +3155,7 @@ static int __init amdgpu_init(void)
r = amdgpu_sync_init(); if (r) - goto error_sync; - - r = amdgpu_userq_fence_slab_init(); - if (r) - goto error_fence; + return r;
DRM_INFO("amdgpu kernel modesetting enabled.\n"); amdgpu_register_atpx_handler(); @@ -3176,12 +3172,6 @@ static int __init amdgpu_init(void)
/* let modprobe override vga console setting */ return pci_register_driver(&amdgpu_kms_pci_driver); - -error_fence: - amdgpu_sync_fini(); - -error_sync: - return r; }
static void __exit amdgpu_exit(void) @@ -3191,7 +3181,6 @@ static void __exit amdgpu_exit(void) amdgpu_unregister_atpx_handler(); amdgpu_acpi_release(); amdgpu_sync_fini(); - amdgpu_userq_fence_slab_fini(); mmu_notifier_synchronize(); amdgpu_xcp_drv_release(); } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c index eba9fb359047..bb19f72770b0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c @@ -33,26 +33,6 @@ #include "amdgpu_userq_fence.h"
static const struct dma_fence_ops amdgpu_userq_fence_ops; -static struct kmem_cache *amdgpu_userq_fence_slab; - -int amdgpu_userq_fence_slab_init(void) -{ - amdgpu_userq_fence_slab = kmem_cache_create("amdgpu_userq_fence", - sizeof(struct amdgpu_userq_fence), - 0, - SLAB_HWCACHE_ALIGN, - NULL); - if (!amdgpu_userq_fence_slab) - return -ENOMEM; - - return 0; -} - -void amdgpu_userq_fence_slab_fini(void) -{ - rcu_barrier(); - kmem_cache_destroy(amdgpu_userq_fence_slab); -}
static inline struct amdgpu_userq_fence *to_amdgpu_userq_fence(struct dma_fence *f) { @@ -227,7 +207,7 @@ void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv)
static int amdgpu_userq_fence_alloc(struct amdgpu_userq_fence **userq_fence) { - *userq_fence = kmem_cache_alloc(amdgpu_userq_fence_slab, GFP_ATOMIC); + *userq_fence = kmalloc(sizeof(**userq_fence), GFP_ATOMIC); return *userq_fence ? 0 : -ENOMEM; }
@@ -243,12 +223,11 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq, if (!fence_drv) return -EINVAL;
- spin_lock_init(&userq_fence->lock); INIT_LIST_HEAD(&userq_fence->link); fence = &userq_fence->base; userq_fence->fence_drv = fence_drv;
- dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, + dma_fence_init64(fence, &amdgpu_userq_fence_ops, NULL, fence_drv->context, seq);
amdgpu_userq_fence_driver_get(fence_drv); @@ -318,35 +297,22 @@ static bool amdgpu_userq_fence_signaled(struct dma_fence *f) rptr = amdgpu_userq_fence_read(fence_drv); wptr = fence->base.seqno;
- if (rptr >= wptr) + if (rptr >= wptr) { + amdgpu_userq_fence_driver_put(fence->fence_drv); + fence->fence_drv = NULL; + + kvfree(fence->fence_drv_array); + fence->fence_drv_array = NULL; return true; + }
return false; }
-static void amdgpu_userq_fence_free(struct rcu_head *rcu) -{ - struct dma_fence *fence = container_of(rcu, struct dma_fence, rcu); - struct amdgpu_userq_fence *userq_fence = to_amdgpu_userq_fence(fence); - struct amdgpu_userq_fence_driver *fence_drv = userq_fence->fence_drv; - - /* Release the fence driver reference */ - amdgpu_userq_fence_driver_put(fence_drv); - - kvfree(userq_fence->fence_drv_array); - kmem_cache_free(amdgpu_userq_fence_slab, userq_fence); -} - -static void amdgpu_userq_fence_release(struct dma_fence *f) -{ - call_rcu(&f->rcu, amdgpu_userq_fence_free); -} - static const struct dma_fence_ops amdgpu_userq_fence_ops = { .get_driver_name = amdgpu_userq_fence_get_driver_name, .get_timeline_name = amdgpu_userq_fence_get_timeline_name, .signaled = amdgpu_userq_fence_signaled, - .release = amdgpu_userq_fence_release, };
/** @@ -560,7 +526,7 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data, r = amdgpu_userq_fence_create(queue, userq_fence, wptr, &fence); if (r) { mutex_unlock(&userq_mgr->userq_mutex); - kmem_cache_free(amdgpu_userq_fence_slab, userq_fence); + kfree(userq_fence); goto put_gobj_write; }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h index d76add2afc77..6f04782f3ea9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h @@ -31,11 +31,6 @@
struct amdgpu_userq_fence { struct dma_fence base; - /* - * This lock is necessary to synchronize the - * userqueue dma fence operations. - */ - spinlock_t lock; struct list_head link; unsigned long fence_drv_array_count; struct amdgpu_userq_fence_driver *fence_drv; @@ -58,9 +53,6 @@ struct amdgpu_userq_fence_driver { char timeline_name[TASK_COMM_LEN]; };
-int amdgpu_userq_fence_slab_init(void); -void amdgpu_userq_fence_slab_fini(void); - void amdgpu_userq_fence_driver_get(struct amdgpu_userq_fence_driver *fence_drv); void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv); int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
On 11/12/2025 13:16, Christian König wrote:
This allows amdgpu_userq_fences to outlive the amdgpu module.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 +---- .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 54 ++++--------------- .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h | 8 --- 3 files changed, 11 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 2dfbddcef9ab..f206297aae8b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -3155,11 +3155,7 @@ static int __init amdgpu_init(void) r = amdgpu_sync_init(); if (r)
goto error_sync;- r = amdgpu_userq_fence_slab_init();
- if (r)
goto error_fence;
return r;DRM_INFO("amdgpu kernel modesetting enabled.\n"); amdgpu_register_atpx_handler(); @@ -3176,12 +3172,6 @@ static int __init amdgpu_init(void) /* let modprobe override vga console setting */ return pci_register_driver(&amdgpu_kms_pci_driver);
-error_fence:
- amdgpu_sync_fini();
-error_sync:
- return r; }
static void __exit amdgpu_exit(void) @@ -3191,7 +3181,6 @@ static void __exit amdgpu_exit(void) amdgpu_unregister_atpx_handler(); amdgpu_acpi_release(); amdgpu_sync_fini();
- amdgpu_userq_fence_slab_fini(); mmu_notifier_synchronize(); amdgpu_xcp_drv_release(); }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c index eba9fb359047..bb19f72770b0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c @@ -33,26 +33,6 @@ #include "amdgpu_userq_fence.h" static const struct dma_fence_ops amdgpu_userq_fence_ops; -static struct kmem_cache *amdgpu_userq_fence_slab;
-int amdgpu_userq_fence_slab_init(void) -{
- amdgpu_userq_fence_slab = kmem_cache_create("amdgpu_userq_fence",
sizeof(struct amdgpu_userq_fence),0,SLAB_HWCACHE_ALIGN,NULL);- if (!amdgpu_userq_fence_slab)
return -ENOMEM;- return 0;
-}
-void amdgpu_userq_fence_slab_fini(void) -{
- rcu_barrier();
What was this rcu_barrier() for? Cargo culted or more to it?
- kmem_cache_destroy(amdgpu_userq_fence_slab);
-} static inline struct amdgpu_userq_fence *to_amdgpu_userq_fence(struct dma_fence *f) { @@ -227,7 +207,7 @@ void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv) static int amdgpu_userq_fence_alloc(struct amdgpu_userq_fence **userq_fence) {
- *userq_fence = kmem_cache_alloc(amdgpu_userq_fence_slab, GFP_ATOMIC);
- *userq_fence = kmalloc(sizeof(**userq_fence), GFP_ATOMIC);
This GFP_ATOMIC is suboptimal for sure being on the ioctl path. It is outside of the scope for this patch, but once my userq cleanup patches get reviewed next on my list was to try and understand this.
return *userq_fence ? 0 : -ENOMEM; } @@ -243,12 +223,11 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq, if (!fence_drv) return -EINVAL;
- spin_lock_init(&userq_fence->lock); INIT_LIST_HEAD(&userq_fence->link); fence = &userq_fence->base; userq_fence->fence_drv = fence_drv;
- dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock,
- dma_fence_init64(fence, &amdgpu_userq_fence_ops, NULL, fence_drv->context, seq);
amdgpu_userq_fence_driver_get(fence_drv); @@ -318,35 +297,22 @@ static bool amdgpu_userq_fence_signaled(struct dma_fence *f) rptr = amdgpu_userq_fence_read(fence_drv); wptr = fence->base.seqno;
- if (rptr >= wptr)
- if (rptr >= wptr) {
amdgpu_userq_fence_driver_put(fence->fence_drv);
fence_drv is in a local already.
fence->fence_drv = NULL;
amdgpu_userq_fence_get_timeline_name could now oops somehow?
kvfree(fence->fence_drv_array);fence->fence_drv_array = NULL;
Not sure if this is safe either. amdgpu_userq_fence_driver_process() drops its reference before it unlinks the fence from the list. Can someone external trigger the fence_is_signaled check, before the interrupt processing kicks in, which will clear fence_drv_array, and so amdgpu_userq_fence_driver_process() would oops?
Regards,
Tvrtko
return true;
- }
return false; } -static void amdgpu_userq_fence_free(struct rcu_head *rcu) -{
- struct dma_fence *fence = container_of(rcu, struct dma_fence, rcu);
- struct amdgpu_userq_fence *userq_fence = to_amdgpu_userq_fence(fence);
- struct amdgpu_userq_fence_driver *fence_drv = userq_fence->fence_drv;
- /* Release the fence driver reference */
- amdgpu_userq_fence_driver_put(fence_drv);
- kvfree(userq_fence->fence_drv_array);
- kmem_cache_free(amdgpu_userq_fence_slab, userq_fence);
-}
-static void amdgpu_userq_fence_release(struct dma_fence *f) -{
- call_rcu(&f->rcu, amdgpu_userq_fence_free);
-}
- static const struct dma_fence_ops amdgpu_userq_fence_ops = { .get_driver_name = amdgpu_userq_fence_get_driver_name, .get_timeline_name = amdgpu_userq_fence_get_timeline_name, .signaled = amdgpu_userq_fence_signaled,
- .release = amdgpu_userq_fence_release, };
/** @@ -560,7 +526,7 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data, r = amdgpu_userq_fence_create(queue, userq_fence, wptr, &fence); if (r) { mutex_unlock(&userq_mgr->userq_mutex);
kmem_cache_free(amdgpu_userq_fence_slab, userq_fence);
kfree(userq_fence);diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h index d76add2afc77..6f04782f3ea9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h @@ -31,11 +31,6 @@ struct amdgpu_userq_fence { struct dma_fence base;
- /*
* This lock is necessary to synchronize the* userqueue dma fence operations.*/- spinlock_t lock; struct list_head link; unsigned long fence_drv_array_count; struct amdgpu_userq_fence_driver *fence_drv;
@@ -58,9 +53,6 @@ struct amdgpu_userq_fence_driver { char timeline_name[TASK_COMM_LEN]; }; -int amdgpu_userq_fence_slab_init(void); -void amdgpu_userq_fence_slab_fini(void);
- void amdgpu_userq_fence_driver_get(struct amdgpu_userq_fence_driver *fence_drv); void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv); int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
On 12/11/25 17:12, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
This allows amdgpu_userq_fences to outlive the amdgpu module.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 +---- .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 54 ++++--------------- .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h | 8 --- 3 files changed, 11 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 2dfbddcef9ab..f206297aae8b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -3155,11 +3155,7 @@ static int __init amdgpu_init(void) r = amdgpu_sync_init(); if (r) - goto error_sync;
- r = amdgpu_userq_fence_slab_init(); - if (r) - goto error_fence; + return r; DRM_INFO("amdgpu kernel modesetting enabled.\n"); amdgpu_register_atpx_handler(); @@ -3176,12 +3172,6 @@ static int __init amdgpu_init(void) /* let modprobe override vga console setting */ return pci_register_driver(&amdgpu_kms_pci_driver);
-error_fence: - amdgpu_sync_fini();
-error_sync: - return r; } static void __exit amdgpu_exit(void) @@ -3191,7 +3181,6 @@ static void __exit amdgpu_exit(void) amdgpu_unregister_atpx_handler(); amdgpu_acpi_release(); amdgpu_sync_fini(); - amdgpu_userq_fence_slab_fini(); mmu_notifier_synchronize(); amdgpu_xcp_drv_release(); } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c index eba9fb359047..bb19f72770b0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c @@ -33,26 +33,6 @@ #include "amdgpu_userq_fence.h" static const struct dma_fence_ops amdgpu_userq_fence_ops; -static struct kmem_cache *amdgpu_userq_fence_slab;
-int amdgpu_userq_fence_slab_init(void) -{ - amdgpu_userq_fence_slab = kmem_cache_create("amdgpu_userq_fence", - sizeof(struct amdgpu_userq_fence), - 0, - SLAB_HWCACHE_ALIGN, - NULL); - if (!amdgpu_userq_fence_slab) - return -ENOMEM;
- return 0; -}
-void amdgpu_userq_fence_slab_fini(void) -{ - rcu_barrier();
What was this rcu_barrier() for? Cargo culted or more to it?
All dma_fences are RCU protected. When they are backed by a kmem_cache you need to make sure to wait for an RCU grace period to pass before destroying the kmem_cache.
Since the dma_fence framework now uses kfree_rcu that shouldn't be problematic any more.
- kmem_cache_destroy(amdgpu_userq_fence_slab); -} static inline struct amdgpu_userq_fence *to_amdgpu_userq_fence(struct dma_fence *f) { @@ -227,7 +207,7 @@ void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv) static int amdgpu_userq_fence_alloc(struct amdgpu_userq_fence **userq_fence) { - *userq_fence = kmem_cache_alloc(amdgpu_userq_fence_slab, GFP_ATOMIC); + *userq_fence = kmalloc(sizeof(**userq_fence), GFP_ATOMIC);
This GFP_ATOMIC is suboptimal for sure being on the ioctl path. It is outside of the scope for this patch, but once my userq cleanup patches get reviewed next on my list was to try and understand this.
return *userq_fence ? 0 : -ENOMEM; } @@ -243,12 +223,11 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq, if (!fence_drv) return -EINVAL; - spin_lock_init(&userq_fence->lock); INIT_LIST_HEAD(&userq_fence->link); fence = &userq_fence->base; userq_fence->fence_drv = fence_drv; - dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, + dma_fence_init64(fence, &amdgpu_userq_fence_ops, NULL, fence_drv->context, seq); amdgpu_userq_fence_driver_get(fence_drv); @@ -318,35 +297,22 @@ static bool amdgpu_userq_fence_signaled(struct dma_fence *f) rptr = amdgpu_userq_fence_read(fence_drv); wptr = fence->base.seqno; - if (rptr >= wptr) + if (rptr >= wptr) { + amdgpu_userq_fence_driver_put(fence->fence_drv);
fence_drv is in a local already.
+ fence->fence_drv = NULL;
amdgpu_userq_fence_get_timeline_name could now oops somehow?
+ kvfree(fence->fence_drv_array); + fence->fence_drv_array = NULL;
Not sure if this is safe either. amdgpu_userq_fence_driver_process() drops its reference before it unlinks the fence from the list. Can someone external trigger the fence_is_signaled check, before the interrupt processing kicks in, which will clear fence_drv_array, and so amdgpu_userq_fence_driver_process() would oops?
Oh, good question. I need to double check that.
Thanks, Christian.
Regards,
Tvrtko
return true; + } return false; } -static void amdgpu_userq_fence_free(struct rcu_head *rcu) -{ - struct dma_fence *fence = container_of(rcu, struct dma_fence, rcu); - struct amdgpu_userq_fence *userq_fence = to_amdgpu_userq_fence(fence); - struct amdgpu_userq_fence_driver *fence_drv = userq_fence->fence_drv;
- /* Release the fence driver reference */ - amdgpu_userq_fence_driver_put(fence_drv);
- kvfree(userq_fence->fence_drv_array); - kmem_cache_free(amdgpu_userq_fence_slab, userq_fence); -}
-static void amdgpu_userq_fence_release(struct dma_fence *f) -{ - call_rcu(&f->rcu, amdgpu_userq_fence_free); -}
static const struct dma_fence_ops amdgpu_userq_fence_ops = { .get_driver_name = amdgpu_userq_fence_get_driver_name, .get_timeline_name = amdgpu_userq_fence_get_timeline_name, .signaled = amdgpu_userq_fence_signaled, - .release = amdgpu_userq_fence_release, }; /** @@ -560,7 +526,7 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data, r = amdgpu_userq_fence_create(queue, userq_fence, wptr, &fence); if (r) { mutex_unlock(&userq_mgr->userq_mutex); - kmem_cache_free(amdgpu_userq_fence_slab, userq_fence); + kfree(userq_fence); goto put_gobj_write; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h index d76add2afc77..6f04782f3ea9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h @@ -31,11 +31,6 @@ struct amdgpu_userq_fence { struct dma_fence base; - /* - * This lock is necessary to synchronize the - * userqueue dma fence operations. - */ - spinlock_t lock; struct list_head link; unsigned long fence_drv_array_count; struct amdgpu_userq_fence_driver *fence_drv; @@ -58,9 +53,6 @@ struct amdgpu_userq_fence_driver { char timeline_name[TASK_COMM_LEN]; }; -int amdgpu_userq_fence_slab_init(void); -void amdgpu_userq_fence_slab_fini(void);
void amdgpu_userq_fence_driver_get(struct amdgpu_userq_fence_driver *fence_drv); void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv); int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
On 12/12/2025 15:46, Christian König wrote:
On 12/11/25 17:12, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
This allows amdgpu_userq_fences to outlive the amdgpu module.
Signed-off-by: Christian König christian.koenig@amd.com
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 13 +---- .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.c | 54 ++++--------------- .../gpu/drm/amd/amdgpu/amdgpu_userq_fence.h | 8 --- 3 files changed, 11 insertions(+), 64 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 2dfbddcef9ab..f206297aae8b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -3155,11 +3155,7 @@ static int __init amdgpu_init(void) r = amdgpu_sync_init(); if (r) - goto error_sync;
- r = amdgpu_userq_fence_slab_init(); - if (r) - goto error_fence; + return r; DRM_INFO("amdgpu kernel modesetting enabled.\n"); amdgpu_register_atpx_handler(); @@ -3176,12 +3172,6 @@ static int __init amdgpu_init(void) /* let modprobe override vga console setting */ return pci_register_driver(&amdgpu_kms_pci_driver);
-error_fence: - amdgpu_sync_fini();
-error_sync: - return r; } static void __exit amdgpu_exit(void) @@ -3191,7 +3181,6 @@ static void __exit amdgpu_exit(void) amdgpu_unregister_atpx_handler(); amdgpu_acpi_release(); amdgpu_sync_fini(); - amdgpu_userq_fence_slab_fini(); mmu_notifier_synchronize(); amdgpu_xcp_drv_release(); } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c index eba9fb359047..bb19f72770b0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.c @@ -33,26 +33,6 @@ #include "amdgpu_userq_fence.h" static const struct dma_fence_ops amdgpu_userq_fence_ops; -static struct kmem_cache *amdgpu_userq_fence_slab;
-int amdgpu_userq_fence_slab_init(void) -{ - amdgpu_userq_fence_slab = kmem_cache_create("amdgpu_userq_fence", - sizeof(struct amdgpu_userq_fence), - 0, - SLAB_HWCACHE_ALIGN, - NULL); - if (!amdgpu_userq_fence_slab) - return -ENOMEM;
- return 0; -}
-void amdgpu_userq_fence_slab_fini(void) -{ - rcu_barrier();
What was this rcu_barrier() for? Cargo culted or more to it?
All dma_fences are RCU protected. When they are backed by a kmem_cache you need to make sure to wait for an RCU grace period to pass before destroying the kmem_cache.
You are right, on a deeper look the built-in protection inside kmem_cache_destroy() only handles the simple kfree_rcu() case and not arbitrary call_rcu() destructors.
Since the dma_fence framework now uses kfree_rcu that shouldn't be problematic any more.
- kmem_cache_destroy(amdgpu_userq_fence_slab); -} static inline struct amdgpu_userq_fence *to_amdgpu_userq_fence(struct dma_fence *f) { @@ -227,7 +207,7 @@ void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv) static int amdgpu_userq_fence_alloc(struct amdgpu_userq_fence **userq_fence) { - *userq_fence = kmem_cache_alloc(amdgpu_userq_fence_slab, GFP_ATOMIC); + *userq_fence = kmalloc(sizeof(**userq_fence), GFP_ATOMIC);
This GFP_ATOMIC is suboptimal for sure being on the ioctl path. It is outside of the scope for this patch, but once my userq cleanup patches get reviewed next on my list was to try and understand this.
return *userq_fence ? 0 : -ENOMEM; } @@ -243,12 +223,11 @@ static int amdgpu_userq_fence_create(struct amdgpu_usermode_queue *userq, if (!fence_drv) return -EINVAL; - spin_lock_init(&userq_fence->lock); INIT_LIST_HEAD(&userq_fence->link); fence = &userq_fence->base; userq_fence->fence_drv = fence_drv; - dma_fence_init64(fence, &amdgpu_userq_fence_ops, &userq_fence->lock, + dma_fence_init64(fence, &amdgpu_userq_fence_ops, NULL, fence_drv->context, seq); amdgpu_userq_fence_driver_get(fence_drv); @@ -318,35 +297,22 @@ static bool amdgpu_userq_fence_signaled(struct dma_fence *f) rptr = amdgpu_userq_fence_read(fence_drv); wptr = fence->base.seqno; - if (rptr >= wptr) + if (rptr >= wptr) { + amdgpu_userq_fence_driver_put(fence->fence_drv);
fence_drv is in a local already.
+ fence->fence_drv = NULL;
amdgpu_userq_fence_get_timeline_name could now oops somehow?
+ kvfree(fence->fence_drv_array); + fence->fence_drv_array = NULL;
Not sure if this is safe either. amdgpu_userq_fence_driver_process() drops its reference before it unlinks the fence from the list. Can someone external trigger the fence_is_signaled check, before the interrupt processing kicks in, which will clear fence_drv_array, and so amdgpu_userq_fence_driver_process() would oops?
Oh, good question. I need to double check that.
Okay thanks, I will await the next re-spin.
Regards,
Tvrtko
return true; + } return false; } -static void amdgpu_userq_fence_free(struct rcu_head *rcu) -{ - struct dma_fence *fence = container_of(rcu, struct dma_fence, rcu); - struct amdgpu_userq_fence *userq_fence = to_amdgpu_userq_fence(fence); - struct amdgpu_userq_fence_driver *fence_drv = userq_fence->fence_drv;
- /* Release the fence driver reference */ - amdgpu_userq_fence_driver_put(fence_drv);
- kvfree(userq_fence->fence_drv_array); - kmem_cache_free(amdgpu_userq_fence_slab, userq_fence); -}
-static void amdgpu_userq_fence_release(struct dma_fence *f) -{ - call_rcu(&f->rcu, amdgpu_userq_fence_free); -}
static const struct dma_fence_ops amdgpu_userq_fence_ops = { .get_driver_name = amdgpu_userq_fence_get_driver_name, .get_timeline_name = amdgpu_userq_fence_get_timeline_name, .signaled = amdgpu_userq_fence_signaled, - .release = amdgpu_userq_fence_release, }; /** @@ -560,7 +526,7 @@ int amdgpu_userq_signal_ioctl(struct drm_device *dev, void *data, r = amdgpu_userq_fence_create(queue, userq_fence, wptr, &fence); if (r) { mutex_unlock(&userq_mgr->userq_mutex); - kmem_cache_free(amdgpu_userq_fence_slab, userq_fence); + kfree(userq_fence); goto put_gobj_write; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h index d76add2afc77..6f04782f3ea9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_userq_fence.h @@ -31,11 +31,6 @@ struct amdgpu_userq_fence { struct dma_fence base; - /* - * This lock is necessary to synchronize the - * userqueue dma fence operations. - */ - spinlock_t lock; struct list_head link; unsigned long fence_drv_array_count; struct amdgpu_userq_fence_driver *fence_drv; @@ -58,9 +53,6 @@ struct amdgpu_userq_fence_driver { char timeline_name[TASK_COMM_LEN]; }; -int amdgpu_userq_fence_slab_init(void); -void amdgpu_userq_fence_slab_fini(void);
void amdgpu_userq_fence_driver_get(struct amdgpu_userq_fence_driver *fence_drv); void amdgpu_userq_fence_driver_put(struct amdgpu_userq_fence_driver *fence_drv); int amdgpu_userq_fence_driver_alloc(struct amdgpu_device *adev,
From: Matthew Brost matthew.brost@intel.com
Preempt, tlb invalidation, and OA fences now use embedded fence lock.
Signed-off-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/xe/xe_oa.c | 5 +---- drivers/gpu/drm/xe/xe_preempt_fence.c | 3 +-- drivers/gpu/drm/xe/xe_preempt_fence_types.h | 2 -- drivers/gpu/drm/xe/xe_tlb_inval.c | 5 +---- drivers/gpu/drm/xe/xe_tlb_inval_types.h | 2 -- 5 files changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c index cc48663c2b48..2dc33f645bdd 100644 --- a/drivers/gpu/drm/xe/xe_oa.c +++ b/drivers/gpu/drm/xe/xe_oa.c @@ -112,8 +112,6 @@ struct xe_oa_config_bo { struct xe_oa_fence { /* @base: dma fence base */ struct dma_fence base; - /* @lock: lock for the fence */ - spinlock_t lock; /* @work: work to signal @base */ struct delayed_work work; /* @cb: callback to schedule @work */ @@ -1017,8 +1015,7 @@ static int xe_oa_emit_oa_config(struct xe_oa_stream *stream, struct xe_oa_config }
/* Point of no return: initialize and set fence to signal */ - spin_lock_init(&ofence->lock); - dma_fence_init(&ofence->base, &xe_oa_fence_ops, &ofence->lock, 0, 0); + dma_fence_init(&ofence->base, &xe_oa_fence_ops, NULL, 0, 0);
for (i = 0; i < stream->num_syncs; i++) { if (stream->syncs[i].flags & DRM_XE_SYNC_FLAG_SIGNAL) diff --git a/drivers/gpu/drm/xe/xe_preempt_fence.c b/drivers/gpu/drm/xe/xe_preempt_fence.c index 7f587ca3947d..75f433aee747 100644 --- a/drivers/gpu/drm/xe/xe_preempt_fence.c +++ b/drivers/gpu/drm/xe/xe_preempt_fence.c @@ -145,9 +145,8 @@ xe_preempt_fence_arm(struct xe_preempt_fence *pfence, struct xe_exec_queue *q, { list_del_init(&pfence->link); pfence->q = xe_exec_queue_get(q); - spin_lock_init(&pfence->lock); dma_fence_init(&pfence->base, &preempt_fence_ops, - &pfence->lock, context, seqno); + NULL, context, seqno);
return &pfence->base; } diff --git a/drivers/gpu/drm/xe/xe_preempt_fence_types.h b/drivers/gpu/drm/xe/xe_preempt_fence_types.h index ac125c697a41..a98de8d1c723 100644 --- a/drivers/gpu/drm/xe/xe_preempt_fence_types.h +++ b/drivers/gpu/drm/xe/xe_preempt_fence_types.h @@ -25,8 +25,6 @@ struct xe_preempt_fence { struct xe_exec_queue *q; /** @preempt_work: work struct which issues preemption */ struct work_struct preempt_work; - /** @lock: dma-fence fence lock */ - spinlock_t lock; /** @error: preempt fence is in error state */ int error; }; diff --git a/drivers/gpu/drm/xe/xe_tlb_inval.c b/drivers/gpu/drm/xe/xe_tlb_inval.c index 918a59e686ea..5c23e76b0241 100644 --- a/drivers/gpu/drm/xe/xe_tlb_inval.c +++ b/drivers/gpu/drm/xe/xe_tlb_inval.c @@ -133,7 +133,6 @@ int xe_gt_tlb_inval_init_early(struct xe_gt *gt) tlb_inval->seqno = 1; INIT_LIST_HEAD(&tlb_inval->pending_fences); spin_lock_init(&tlb_inval->pending_lock); - spin_lock_init(&tlb_inval->lock); INIT_DELAYED_WORK(&tlb_inval->fence_tdr, xe_tlb_inval_fence_timeout);
err = drmm_mutex_init(&xe->drm, &tlb_inval->seqno_lock); @@ -420,10 +419,8 @@ void xe_tlb_inval_fence_init(struct xe_tlb_inval *tlb_inval, { xe_pm_runtime_get_noresume(tlb_inval->xe);
- spin_lock_irq(&tlb_inval->lock); - dma_fence_init(&fence->base, &inval_fence_ops, &tlb_inval->lock, + dma_fence_init(&fence->base, &inval_fence_ops, NULL, dma_fence_context_alloc(1), 1); - spin_unlock_irq(&tlb_inval->lock); INIT_LIST_HEAD(&fence->link); if (stack) set_bit(FENCE_STACK_BIT, &fence->base.flags); diff --git a/drivers/gpu/drm/xe/xe_tlb_inval_types.h b/drivers/gpu/drm/xe/xe_tlb_inval_types.h index 8f8b060e9005..80e893950099 100644 --- a/drivers/gpu/drm/xe/xe_tlb_inval_types.h +++ b/drivers/gpu/drm/xe/xe_tlb_inval_types.h @@ -104,8 +104,6 @@ struct xe_tlb_inval { struct delayed_work fence_tdr; /** @job_wq: schedules TLB invalidation jobs */ struct workqueue_struct *job_wq; - /** @tlb_inval.lock: protects TLB invalidation fences */ - spinlock_t lock; };
/**
From: Matthew Brost matthew.brost@intel.com
Helps with disconnecting fences from Xe module.
Signed-off-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/xe/xe_hw_fence.c | 25 ++----------------------- drivers/gpu/drm/xe/xe_hw_fence.h | 3 --- drivers/gpu/drm/xe/xe_module.c | 5 ----- 3 files changed, 2 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c index 3d89c660ea73..86bd554bba3e 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.c +++ b/drivers/gpu/drm/xe/xe_hw_fence.c @@ -6,7 +6,6 @@ #include "xe_hw_fence.h"
#include <linux/device.h> -#include <linux/slab.h>
#include "xe_bo.h" #include "xe_device.h" @@ -16,28 +15,9 @@ #include "xe_map.h" #include "xe_trace.h"
-static struct kmem_cache *xe_hw_fence_slab; - -int __init xe_hw_fence_module_init(void) -{ - xe_hw_fence_slab = kmem_cache_create("xe_hw_fence", - sizeof(struct xe_hw_fence), 0, - SLAB_HWCACHE_ALIGN, NULL); - if (!xe_hw_fence_slab) - return -ENOMEM; - - return 0; -} - -void xe_hw_fence_module_exit(void) -{ - rcu_barrier(); - kmem_cache_destroy(xe_hw_fence_slab); -} - static struct xe_hw_fence *fence_alloc(void) { - return kmem_cache_zalloc(xe_hw_fence_slab, GFP_KERNEL); + return kzalloc(sizeof(struct xe_hw_fence), GFP_KERNEL); }
static void fence_free(struct rcu_head *rcu) @@ -45,8 +25,7 @@ static void fence_free(struct rcu_head *rcu) struct xe_hw_fence *fence = container_of(rcu, struct xe_hw_fence, dma.rcu);
- if (!WARN_ON_ONCE(!fence)) - kmem_cache_free(xe_hw_fence_slab, fence); + kfree(fence); }
static void hw_fence_irq_run_cb(struct irq_work *work) diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h index f13a1c4982c7..96f34332fd8d 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.h +++ b/drivers/gpu/drm/xe/xe_hw_fence.h @@ -11,9 +11,6 @@ /* Cause an early wrap to catch wrapping errors */ #define XE_FENCE_INITIAL_SEQNO (-127)
-int xe_hw_fence_module_init(void); -void xe_hw_fence_module_exit(void); - void xe_hw_fence_irq_init(struct xe_hw_fence_irq *irq); void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq); void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq); diff --git a/drivers/gpu/drm/xe/xe_module.c b/drivers/gpu/drm/xe/xe_module.c index d08338fc3bc1..32517bcd533c 100644 --- a/drivers/gpu/drm/xe/xe_module.c +++ b/drivers/gpu/drm/xe/xe_module.c @@ -12,7 +12,6 @@
#include "xe_drv.h" #include "xe_configfs.h" -#include "xe_hw_fence.h" #include "xe_pci.h" #include "xe_pm.h" #include "xe_observation.h" @@ -114,10 +113,6 @@ static const struct init_funcs init_funcs[] = { .init = xe_configfs_init, .exit = xe_configfs_exit, }, - { - .init = xe_hw_fence_module_init, - .exit = xe_hw_fence_module_exit, - }, { .init = xe_sched_job_module_init, .exit = xe_sched_job_module_exit,
From: Matthew Brost matthew.brost@intel.com
Help disconnect fences from the Xe module.
Signed-off-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/xe/xe_exec_queue.c | 2 +- drivers/gpu/drm/xe/xe_gt.c | 7 ++++-- drivers/gpu/drm/xe/xe_gt_types.h | 2 +- drivers/gpu/drm/xe/xe_hw_engine.c | 2 +- drivers/gpu/drm/xe/xe_hw_fence.c | 35 ++++++++++++++++++++++++-- drivers/gpu/drm/xe/xe_hw_fence.h | 2 +- drivers/gpu/drm/xe/xe_hw_fence_types.h | 4 +++ 7 files changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c index 226d07a3d852..a4cac728cb17 100644 --- a/drivers/gpu/drm/xe/xe_exec_queue.c +++ b/drivers/gpu/drm/xe/xe_exec_queue.c @@ -141,7 +141,7 @@ static struct xe_exec_queue *__xe_exec_queue_alloc(struct xe_device *xe, q->width = width; q->msix_vec = XE_IRQ_DEFAULT_MSIX; q->logical_mask = logical_mask; - q->fence_irq = >->fence_irq[hwe->class]; + q->fence_irq = gt->fence_irq[hwe->class]; q->ring_ops = gt->ring_ops[hwe->class]; q->ops = gt->exec_queue_ops; INIT_LIST_HEAD(&q->lr.link); diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c index 7caf781ba9e8..3a3866f77a38 100644 --- a/drivers/gpu/drm/xe/xe_gt.c +++ b/drivers/gpu/drm/xe/xe_gt.c @@ -589,7 +589,8 @@ static void xe_gt_fini(void *arg) xe_pm_runtime_put(gt_to_xe(gt));
for (i = 0; i < XE_ENGINE_CLASS_MAX; ++i) - xe_hw_fence_irq_finish(>->fence_irq[i]); + if (gt->fence_irq[i]) + xe_hw_fence_irq_finish(gt->fence_irq[i]);
xe_gt_disable_host_l2_vram(gt); } @@ -603,7 +604,9 @@ int xe_gt_init(struct xe_gt *gt)
for (i = 0; i < XE_ENGINE_CLASS_MAX; ++i) { gt->ring_ops[i] = xe_ring_ops_get(gt, i); - xe_hw_fence_irq_init(>->fence_irq[i]); + gt->fence_irq[i] = xe_hw_fence_irq_init(); + if (!gt->fence_irq[i]) + return -ENOMEM; }
err = devm_add_action_or_reset(gt_to_xe(gt)->drm.dev, xe_gt_fini, gt); diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h index 0a728180b6fe..88a05edab51b 100644 --- a/drivers/gpu/drm/xe/xe_gt_types.h +++ b/drivers/gpu/drm/xe/xe_gt_types.h @@ -240,7 +240,7 @@ struct xe_gt { const struct xe_ring_ops *ring_ops[XE_ENGINE_CLASS_MAX];
/** @fence_irq: fence IRQs (1 per engine class) */ - struct xe_hw_fence_irq fence_irq[XE_ENGINE_CLASS_MAX]; + struct xe_hw_fence_irq *fence_irq[XE_ENGINE_CLASS_MAX];
/** @default_lrc: default LRC state */ void *default_lrc[XE_ENGINE_CLASS_MAX]; diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c b/drivers/gpu/drm/xe/xe_hw_engine.c index 6a9e2a4272dd..480972c3da84 100644 --- a/drivers/gpu/drm/xe/xe_hw_engine.c +++ b/drivers/gpu/drm/xe/xe_hw_engine.c @@ -524,7 +524,7 @@ static void hw_engine_init_early(struct xe_gt *gt, struct xe_hw_engine *hwe, info->irq_offset; hwe->domain = info->domain; hwe->name = info->name; - hwe->fence_irq = >->fence_irq[info->class]; + hwe->fence_irq = gt->fence_irq[info->class]; hwe->engine_id = id;
hwe->eclass = >->eclass[hwe->class]; diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c index 86bd554bba3e..d0508b855410 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.c +++ b/drivers/gpu/drm/xe/xe_hw_fence.c @@ -15,6 +15,25 @@ #include "xe_map.h" #include "xe_trace.h"
+static void xe_hw_fence_irq_destroy(struct kref *ref) +{ + struct xe_hw_fence_irq *irq = container_of(ref, typeof(*irq), refcount); + + kfree(irq); +} + +static void xe_hw_fence_irq_put(struct xe_hw_fence_irq *irq) +{ + if (irq) + kref_put(&irq->refcount, xe_hw_fence_irq_destroy); +} + +static struct xe_hw_fence_irq *xe_hw_fence_irq_get(struct xe_hw_fence_irq *irq) +{ + kref_get(&irq->refcount); + return irq; +} + static struct xe_hw_fence *fence_alloc(void) { return kzalloc(sizeof(struct xe_hw_fence), GFP_KERNEL); @@ -25,6 +44,7 @@ static void fence_free(struct rcu_head *rcu) struct xe_hw_fence *fence = container_of(rcu, struct xe_hw_fence, dma.rcu);
+ xe_hw_fence_irq_put(fence->irq); kfree(fence); }
@@ -52,12 +72,20 @@ static void hw_fence_irq_run_cb(struct irq_work *work) dma_fence_end_signalling(tmp); }
-void xe_hw_fence_irq_init(struct xe_hw_fence_irq *irq) +struct xe_hw_fence_irq *xe_hw_fence_irq_init(void) { + struct xe_hw_fence_irq *irq = kzalloc(sizeof(*irq), GFP_KERNEL); + + if (!irq) + return NULL; + + kref_init(&irq->refcount); spin_lock_init(&irq->lock); init_irq_work(&irq->work, hw_fence_irq_run_cb); INIT_LIST_HEAD(&irq->pending); irq->enabled = true; + + return irq; }
void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq) @@ -80,6 +108,8 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
/* Safe release of the irq->lock used in dma_fence_init. */ synchronize_rcu(); + + xe_hw_fence_irq_put(irq); }
void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq) @@ -231,13 +261,14 @@ void xe_hw_fence_free(struct dma_fence *fence) void xe_hw_fence_init(struct dma_fence *fence, struct xe_hw_fence_ctx *ctx, struct iosys_map seqno_map) { - struct xe_hw_fence *hw_fence = + struct xe_hw_fence *hw_fence = container_of(fence, typeof(*hw_fence), dma);
hw_fence->xe = gt_to_xe(ctx->gt); snprintf(hw_fence->name, sizeof(hw_fence->name), "%s", ctx->name); hw_fence->seqno_map = seqno_map; INIT_LIST_HEAD(&hw_fence->irq_link); + hw_fence->irq = xe_hw_fence_irq_get(ctx->irq);
dma_fence_init(fence, &xe_hw_fence_ops, &ctx->irq->lock, ctx->dma_fence_ctx, ctx->next_seqno++); diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h index 96f34332fd8d..fa1620203b90 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.h +++ b/drivers/gpu/drm/xe/xe_hw_fence.h @@ -11,7 +11,7 @@ /* Cause an early wrap to catch wrapping errors */ #define XE_FENCE_INITIAL_SEQNO (-127)
-void xe_hw_fence_irq_init(struct xe_hw_fence_irq *irq); +struct xe_hw_fence_irq *xe_hw_fence_irq_init(void); void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq); void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq); void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq); diff --git a/drivers/gpu/drm/xe/xe_hw_fence_types.h b/drivers/gpu/drm/xe/xe_hw_fence_types.h index 58a8d09afe5c..0682c12520e9 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence_types.h +++ b/drivers/gpu/drm/xe/xe_hw_fence_types.h @@ -28,6 +28,8 @@ struct xe_hw_fence_irq { struct irq_work work; /** @pending: list of pending xe_hw_fences */ struct list_head pending; + /** @refcount: ref count of this exec queue */ + struct kref refcount; /** @enabled: fence signaling enabled */ bool enabled; }; @@ -62,6 +64,8 @@ struct xe_hw_fence_ctx { struct xe_hw_fence { /** @dma: base dma fence for hardware fence context */ struct dma_fence dma; + /** @irq: fence irq handler */ + struct xe_hw_fence_irq *irq; /** @xe: Xe device for hw fence driver name */ struct xe_device *xe; /** @name: name of hardware fence context */
From: Matthew Brost matthew.brost@intel.com
Be safe when dereferencing fence->xe.
Signed-off-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/xe/xe_hw_fence.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c index d0508b855410..1bf4d234a315 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.c +++ b/drivers/gpu/drm/xe/xe_hw_fence.c @@ -157,9 +157,7 @@ static struct xe_hw_fence_irq *xe_hw_fence_irq(struct xe_hw_fence *fence)
static const char *xe_hw_fence_get_driver_name(struct dma_fence *dma_fence) { - struct xe_hw_fence *fence = to_xe_hw_fence(dma_fence); - - return dev_name(fence->xe->drm.dev); + return "xe"; }
static const char *xe_hw_fence_get_timeline_name(struct dma_fence *dma_fence) @@ -173,10 +171,13 @@ static bool xe_hw_fence_signaled(struct dma_fence *dma_fence) { struct xe_hw_fence *fence = to_xe_hw_fence(dma_fence); struct xe_device *xe = fence->xe; - u32 seqno = xe_map_rd(xe, &fence->seqno_map, 0, u32); + u32 seqno; + + if (dma_fence->error) + return true;
- return dma_fence->error || - !__dma_fence_is_later(dma_fence, dma_fence->seqno, seqno); + seqno = xe_map_rd(xe, &fence->seqno_map, 0, u32); + return !__dma_fence_is_later(dma_fence, dma_fence->seqno, seqno); }
static bool xe_hw_fence_enable_signaling(struct dma_fence *dma_fence)
On Thu, 2025-12-11 at 13:16 +0100, Christian König wrote:
Hi everyone,
dma_fences have ever lived under the tyranny dictated by the module lifetime of their issuer, leading to crashes should anybody still holding a reference to a dma_fence when the module of the issuer was unloaded.
The basic problem is that when buffer are shared between drivers dma_fence objects can leak into external drivers and stay there even after they are signaled. The dma_resv object for example only lazy releases dma_fences.
So what happens is that when the module who originally created the dma_fence unloads the dma_fence_ops function table becomes unavailable as well and so any attempt to release the fence crashes the system.
Previously various approaches have been discussed, including changing the locking semantics of the dma_fence callbacks (by me) as well as using the drm scheduler as intermediate layer (by Sima) to disconnect dma_fences from their actual users, but none of them are actually solving all problems.
Tvrtko did some really nice prerequisite work by protecting the returned strings of the dma_fence_ops by RCU. This way dma_fence creators where able to just wait for an RCU grace period after fence signaling before they could be save to free those data structures.
Now this patch set here goes a step further and protects the whole dma_fence_ops structure by RCU, so that after the fence signals the pointer to the dma_fence_ops is set to NULL when there is no wait nor release callback given. All functionality which use the dma_fence_ops reference are put inside an RCU critical section, except for the deprecated issuer specific wait and of course the optional release callback.
Additional to the RCU changes the lock protecting the dma_fence state previously had to be allocated external. This set here now changes the functionality to make that external lock optional and allows dma_fences to use an inline lock and be self contained.
v4:
Rebases the whole set on upstream changes, especially the cleanup from Philip in patch "drm/amdgpu: independence for the amdkfd_fence!".
Adding two patches which brings the DMA-fence self tests up to date. The first selftest changes removes the mock_wait and so actually starts testing the default behavior instead of some hacky implementation in the test. This one should probably go upstream independent of this set. The second drops the mock_fence as well and tests the new RCU and inline spinlock functionality.
Especially the first patch still needs a Reviewed-by, apart from that I think I've addressed all review comments.
The plan is to push the core DMA-buf changes to drm-misc-next and then the driver specific changes through the driver channels as approprite.
This does not apply to drm-misc-next (unless I'm screwing up badly).
Where can I apply it? I'd like to test the drm_sched changes before this gets merged.
P.
Please review and comment, Christian.
On 12/11/25 13:33, Philipp Stanner wrote:
On Thu, 2025-12-11 at 13:16 +0100, Christian König wrote:
Hi everyone,
dma_fences have ever lived under the tyranny dictated by the module lifetime of their issuer, leading to crashes should anybody still holding a reference to a dma_fence when the module of the issuer was unloaded.
The basic problem is that when buffer are shared between drivers dma_fence objects can leak into external drivers and stay there even after they are signaled. The dma_resv object for example only lazy releases dma_fences.
So what happens is that when the module who originally created the dma_fence unloads the dma_fence_ops function table becomes unavailable as well and so any attempt to release the fence crashes the system.
Previously various approaches have been discussed, including changing the locking semantics of the dma_fence callbacks (by me) as well as using the drm scheduler as intermediate layer (by Sima) to disconnect dma_fences from their actual users, but none of them are actually solving all problems.
Tvrtko did some really nice prerequisite work by protecting the returned strings of the dma_fence_ops by RCU. This way dma_fence creators where able to just wait for an RCU grace period after fence signaling before they could be save to free those data structures.
Now this patch set here goes a step further and protects the whole dma_fence_ops structure by RCU, so that after the fence signals the pointer to the dma_fence_ops is set to NULL when there is no wait nor release callback given. All functionality which use the dma_fence_ops reference are put inside an RCU critical section, except for the deprecated issuer specific wait and of course the optional release callback.
Additional to the RCU changes the lock protecting the dma_fence state previously had to be allocated external. This set here now changes the functionality to make that external lock optional and allows dma_fences to use an inline lock and be self contained.
v4:
Rebases the whole set on upstream changes, especially the cleanup from Philip in patch "drm/amdgpu: independence for the amdkfd_fence!".
Adding two patches which brings the DMA-fence self tests up to date. The first selftest changes removes the mock_wait and so actually starts testing the default behavior instead of some hacky implementation in the test. This one should probably go upstream independent of this set. The second drops the mock_fence as well and tests the new RCU and inline spinlock functionality.
Especially the first patch still needs a Reviewed-by, apart from that I think I've addressed all review comments.
The plan is to push the core DMA-buf changes to drm-misc-next and then the driver specific changes through the driver channels as approprite.
This does not apply to drm-misc-next (unless I'm screwing up badly).
Where can I apply it? I'd like to test the drm_sched changes before this gets merged.
drm-tip from a few days ago, otherwise the xe changes won't work.
Regards, Christian.
P.
Please review and comment, Christian.
linaro-mm-sig@lists.linaro.org
-
Christian König -
Christian König -
Philipp Stanner -
Tvrtko Ursulin