On 13/11/2025 14:51, Christian König wrote:

The driver and timeline name are meaningless for signaled fences.

Drop them and also print the context number.

v2: avoid the calls when the BO is already signaled. v3: use same format as trace points for context and seqno.

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

drivers/dma-buf/dma-fence.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index c5d1478b28dd..b4f5c8635276 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -997,19 +997,21 @@ EXPORT_SYMBOL(dma_fence_set_deadline); */ void dma_fence_describe(struct dma_fence *fence, struct seq_file *seq) {

• const char __rcu *timeline;
• const char __rcu *driver;

• const char __rcu *timeline = "";
• const char __rcu *driver = "";
• const char *signaled = "";

rcu_read_lock();

• timeline = dma_fence_timeline_name(fence);
• driver = dma_fence_driver_name(fence);

• if (!dma_fence_is_signaled(fence)) {

• timeline = dma_fence_timeline_name(fence);
  

• driver = dma_fence_driver_name(fence);
  

• signaled = "un";
  

• }

• seq_printf(seq, "%s %s seq %llu %ssignalled\n",

•    rcu_dereference(driver),
  

•    rcu_dereference(timeline),
  

•    fence->seqno,
  

•    dma_fence_is_signaled(fence) ? "" : "un");
  

• seq_printf(seq, "%llu:%llu %s %s %ssignalled\n",

•    fence->context, fence->seqno, timeline, driver,
  

•    signaled);
  

rcu_read_unlock(); }

I thought I r-b-ed this one but apparently not.

Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com

Regards,

Tvrtko

On 11/14/25 11:50, Tvrtko Ursulin wrote:

...

@@ -569,12 +577,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();

Risk being a spin lock in the release callback will trigger a warning on PREEMPT_RT. But at least the current code base does not have anything like that AFAICS so I guess it is okay.

I don't think that this is a problem. When PREEMPT_RT is enabled both RCU and spinlocks become preemptible.

So as far as I know it is perfectly valid to grab a spinlock under an rcu read side critical section.

...

diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h index 64639e104110..77f07735f556 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 BO from detaching after

s/BO/fence/

...

+     * 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 BO from detaching after

Ditto.

Both fixed, thanks.

...

+     * 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);   @@ -418,13 +429,19 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);   static inline bool   dma_fence_is_signaled_locked(struct dma_fence *fence)   { +    const struct dma_fence_ops *ops;

if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))           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;   } @@ -448,13 +465,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 (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))           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();

With the unlocked version two threads could race and one could make the fence->lock go away just around here, before the dma_fence_signal below will take it. It seems it is only safe to rcu_read_unlock before signaling if using the embedded fence (later in the series). Can you think of a downside to holding the rcu read lock to after signaling? that would make it safe I think.

Well it's good to talk about it but I think that it is not necessary to protect the lock in this particular case.

See the RCU protection is only for the fence->ops pointer, but the lock can be taken way after the fence is already signaled.

That's why I came up with the patch to move the lock into the fence in the first place.

Regards, Christian.

Regards,

Tvrtko

...

dma_fence_signal(fence);           return true;       } +    rcu_read_unlock();         return false;   }

On 11/18/25 17:03, Tvrtko Ursulin wrote:

...

...

...

@@ -448,13 +465,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 (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))            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();

With the unlocked version two threads could race and one could make the fence->lock go away just around here, before the dma_fence_signal below will take it. It seems it is only safe to rcu_read_unlock before signaling if using the embedded fence (later in the series). Can you think of a downside to holding the rcu read lock to after signaling? that would make it safe I think.

Well it's good to talk about it but I think that it is not necessary to protect the lock in this particular case.

See the RCU protection is only for the fence->ops pointer, but the lock can be taken way after the fence is already signaled.

That's why I came up with the patch to move the lock into the fence in the first place.

Right. And you think there is nothing to gain with the option of keeping the rcu_read_unlock() to after signalling? Ie. why not plug a potential race if we can for no negative effect.

I thought quite a bit over that, but at least of hand I can't come up with a reason why we should do this. The signaling path doesn't need the RCU read side lock as far as I can see.

Regards, Christian.

Regards,

Tvrtko

On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote:

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: 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 ec21be9b089a..7074347f506d 100644 --- a/drivers/dma-buf/dma-fence.c +++ b/drivers/dma-buf/dma-fence.c @@ -374,6 +374,14 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,         &fence->flags)))   return -EINVAL;

• /*

• * 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);   @@ -513,7 +521,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 @@ -578,7 +586,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); @@ -614,7 +622,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)) { @@ -1000,7 +1008,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 77f07735f556..eb57bcc8712f 100644 --- a/include/linux/dma-fence.h +++ b/include/linux/dma-fence.h @@ -436,7 +436,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; @@ -472,7 +472,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;

On 13/11/2025 14:51, 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

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

drivers/dma-buf/dma-fence.c | 66 +++++++++++++----------- 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 ++++++++++++++- 16 files changed, 111 insertions(+), 62 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 7074347f506d..9a5aa9e44e13 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

@@ -368,7 +367,7 @@ int 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))) @@ -421,9 +420,9 @@ int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp) if (WARN_ON(!fence)) return -EINVAL;

• spin_lock_irqsave(fence->lock, flags);

• dma_fence_lock_irqsave(fence, flags); ret = dma_fence_signal_timestamp_locked(fence, timestamp);

• spin_unlock_irqrestore(fence->lock, flags);

• dma_fence_unlock_irqrestore(fence, flags);

return ret; } @@ -475,9 +474,9 @@ int dma_fence_signal(struct dma_fence *fence) tmp = dma_fence_begin_signalling();

• spin_lock_irqsave(fence->lock, flags);

• dma_fence_lock_irqsave(fence, flags); ret = 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); @@ -579,10 +578,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); @@ -612,7 +611,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); @@ -648,9 +647,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);

@@ -690,8 +689,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);

@@ -699,8 +697,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; } @@ -723,9 +720,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; } @@ -755,13 +752,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; } @@ -800,8 +795,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 (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) goto out;

@@ -824,11 +818,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; }

@@ -838,7 +832,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);

@@ -1047,7 +1041,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); @@ -1058,10 +1051,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); @@ -1071,7 +1069,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

@@ -1081,6 +1079,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,

@@ -1094,7 +1096,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

@@ -1104,6 +1106,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/sw_sync.c b/drivers/dma-buf/sw_sync.c index 3c20f1d31cf5..956a3ad7a075 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -155,12 +155,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); @@ -178,7 +178,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;

@@ -186,7 +186,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 = { @@ -427,13 +427,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); @@ -446,7 +446,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 cd8873c6931a..ab41488b0df9 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 700b4a776532..4b5360bc4813 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 cf0ec94e8a07..82492a5759a0 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h @@ -631,20 +631,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);

This part to me still looks it should be a separate patch.

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);

I also commented before how I think a helper for this would be better.

If some implementation switches to the embedded lock these container_ofs will still compile but crash.

Helper would be able to at least output a more easily understandable message along the lines "invalid usage of fence embedded lock" or something. (If bit is not set.) It is not perfect but worth at least discussing.

Also, you did not comment on whether or not you would be happy to grow the size of struct dma_fence for all architectures? This patch as is keeps it in 64B for most, but some architectures still go over due their spinlock implementation.

If instead of an union you would keep the pointer, then it could point to the either the embedded or external lock and dma_fence_(un)lock* call sites would be able to lose the conditional. In fact they could even stay without the wrapper, although I don't have anything against the wrapper.

} 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 869d4335c0f4..a7512c22c85f 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 b2a0c46dfcd4..3456bec93c70 100644 --- a/drivers/gpu/drm/xe/xe_hw_fence.c +++ b/drivers/gpu/drm/xe/xe_hw_fence.c @@ -144,7 +144,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 eb57bcc8712f..d493d15ad52a 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;
  

• };

I assumed there would be a patch which just adds the spin_(un)lock* helpers and does nothing else. Eg. a purely mechanical patch. But I guess this is passable, and we definitely need to close on teh union vs pointer before churning further.

Regards,

Tvrtko

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);

On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote:

Using the inline lock is now the recommended way for dma_fence implementations.

So use this approach for the framework internal fences as well.

nit: s/framework/framework's

Also saves about 4 bytes for the external spinlock.

On all platforms or just AMD64?

Signed-off-by: Christian König christian.koenig@amd.com Reviewed-by: Tvrtko Ursulin tvrtko.ursulin@igalia.com

---

drivers/dma-buf/dma-fence.c | 20 ++++----------------  1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c index 9a5aa9e44e13..e6d26c2e0391 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,8 @@ 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);
  

• set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,

• &dma_fence_stub.flags);
  

That change is unrelated, isn't it? Enlarges the diff and breaks git- blame.

• 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,16 +155,9 @@ 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);
  

• set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,

• &fence->flags);
  

Same.

• dma_fence_init(fence, &dma_fence_stub_ops, NULL, 0, 0);
• set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT, &fence->flags);

dma_fence_signal_timestamp(fence, timestamp);

I wouldn't remove that empty line (nit).

return fence;  }  EXPORT_SYMBOL(dma_fence_allocate_private_stub);

On 11/27/25 11:57, Philipp Stanner wrote:

On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote:

...

Calling dma_fence_is_signaled() here is illegal!

OK, but why is that patch in this series?

Because the next patch depends on it, otherwise the series won't compile.

My plan is to push the amdgpu patches through amd-staging-drm-next as soon as Alex rebased that branch on drm-next during the next cycle.

Regards, Christian.

P.

...

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;

On 13/11/2025 14:51, Christian König wrote:

This should allow amdgpu_fences to outlive the amdgpu module.

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..990fcbbe90a0 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(*ptr);
  

Both here and in amdgpu_fence_driver_set_error, the old/existing code makes me think the fetched fence pointer can be NULL (the "if (unprocesses && .. below"). If true then it needs to be checked before dma_fence_get_rcu and after.

Although I am a bit confused with the RCU usage here in general. For example struct amdgpu_fence_driver->fences is not rcu annotated so what is actually guarded by RCU? Pointers in the array or fence objects pointer to by those pointers?

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 Thu, 2025-11-13 at 15:51 +0100, Christian König wrote:

This should allow amdgpu_fences to outlive the amdgpu module.

amdgpu_eviction_fences you mean.

And I would drop the "should" from this and the previous patch. We're confident it works, aren't we? ^^'

P.

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;  };

On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote:

This should allow amdkfd_fences to outlive the amdgpu module.

v2: implement Felix suggestion to lock the fence while signaling it.

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

drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h    |  6 +++  .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_fence.c  | 39 ++++++++-----------  drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  7 ++--  drivers/gpu/drm/amd/amdkfd/kfd_svm.c          |  4 +-  4 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd.h index 8bdfcde2029b..6254cef04213 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); +void 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,11 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm)   return false;  }   +static inline +void amdkfd_fence_signal(struct dma_fence *f) +{

I would add a short comment here: "Empty function because …"

+}

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..f76c3c52a2a1 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;

That the storage actually takes place is guaranteed by the lock taken when calling the fence ops?

• return false;

}    /** @@ -174,9 +154,22 @@ bool amdkfd_fence_check_mm(struct dma_fence *f, struct mm_struct *mm)   return false;  }   +void amdkfd_fence_signal(struct dma_fence *f) +{

• struct amdgpu_amdkfd_fence *fence = to_amdgpu_amdkfd_fence(f);
• long flags;
• dma_fence_lock_irqsafe(f, flags)
• if (fence->mm) {

• mmdrop(fence->mm);
  

• fence->mm = NULL;
  

• }
• dma_fence_signal_locked(f);
• dma_fence_unlock_irqrestore(f, flags)

+}

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 a085faac9fe1..8fac70b839ed 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); @@ -1990,7 +1990,6 @@ kfd_process_gpuid_from_node(struct kfd_process *p, struct kfd_node *node,  static int signal_eviction_fence(struct kfd_process *p)  {   struct dma_fence *ef;

• int ret;

rcu_read_lock();   ef = dma_fence_get_rcu_safe(&p->ef); @@ -1998,10 +1997,10 @@ static int signal_eviction_fence(struct kfd_process *p)   if (!ef)   return -EINVAL;

• ret = dma_fence_signal(ef);

• amdkfd_fence_signal(ef);

dma_fence_put(ef);

• return ret;

• return 0;

Oh wait, that's the code I'm also touching in my return code series!

https://lore.kernel.org/dri-devel/cef83fed-5994-4c77-962c-9c7aac9f7306@amd.c...

Does this series then solve the problem Felix pointed out in evict_process_worker()?

P.

}    static void evict_process_worker(struct work_struct *work) diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c index c30dfb8ec236..566950702b7d 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 Fri, 2025-11-28 at 11:06 +0100, Christian König wrote:

On 11/27/25 12:10, Philipp Stanner wrote:

...

On Thu, 2025-11-13 at 15:51 +0100, Christian König wrote:

...

This should allow amdkfd_fences to outlive the amdgpu module.

v2: implement Felix suggestion to lock the fence while signaling it.

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

[…]

...

...

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c index a085faac9fe1..8fac70b839ed 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); @@ -1990,7 +1990,6 @@ kfd_process_gpuid_from_node(struct kfd_process *p, struct kfd_node *node,  static int signal_eviction_fence(struct kfd_process *p)  {   struct dma_fence *ef;

• int ret;

rcu_read_lock();   ef = dma_fence_get_rcu_safe(&p->ef); @@ -1998,10 +1997,10 @@ static int signal_eviction_fence(struct kfd_process *p)   if (!ef)   return -EINVAL;

• ret = dma_fence_signal(ef);

• amdkfd_fence_signal(ef);

dma_fence_put(ef);

• return ret;

• return 0;

Oh wait, that's the code I'm also touching in my return code series!

https://lore.kernel.org/dri-devel/cef83fed-5994-4c77-962c-9c7aac9f7306@amd.c...

Does this series then solve the problem Felix pointed out in evict_process_worker()?

No it doesn't, I wasn't aware that the higher level code actually needs the status. After all Felix is the maintainer of this part.

This patch here needs to be rebased on top of yours and changed accordingly to still return the fence status correctly.

But thanks for pointing that out.

Alright, so my (repaired, v2) status-code-removal series shall enter drm-misc-next first, and then your series here. ACK?

P.