[PATCH 0/6] dma-fence: Remove return code of dma_fence_signal() et al.

[PATCH 0/6] dma-fence: Remove return code of dma_fence_signal() et al.

[Philipp Stanner]

Barely anyone uses dma_fence_signal()'s (and similar functions') return
code. Checking it is pretty much useless anyways, because what are you
going to do if a fence was already signal it? Unsignal it and signal it
again? ;p

Removing the return code simplifies the API and makes it easier for me
to sit on top with Rust DmaFence.

Philipp Stanner (6):
  dma-buf/dma-fence: Add dma_fence_test_signaled_flag()
  amd/amdkfd: Ignore return code of dma_fence_signal()
  drm/gpu/xe: Ignore dma_fenc_signal() return code
  dma-buf: Don't misuse dma_fence_signal()
  drm/ttm: Remove return check of dma_fence_signal()
  dma-buf/dma-fence: Remove return code of signaling-functions

 drivers/dma-buf/dma-fence.c                   | 59 ++++++-------------
 drivers/dma-buf/st-dma-fence.c                |  7 +--
 drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  5 +-
 .../gpu/drm/ttm/tests/ttm_bo_validate_test.c  |  3 +-
 drivers/gpu/drm/xe/xe_hw_fence.c              |  5 +-
 include/linux/dma-fence.h                     | 33 ++++++++---
 6 files changed, 53 insertions(+), 59 deletions(-)

-- 
2.49.0

[PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Philipp Stanner]

The dma_fence framework checks at many places whether the signaled flag
of a fence is already set. The code can be simplified and made more
readable by providing a helper function for that.

Add dma_fence_test_signaled_flag(), which only checks whether a fence is
signaled. Use it internally.

Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c | 19 +++++++++----------
 include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
 2 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 39e6f93dc310..25117a906846 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 
 	lockdep_assert_held(fence->lock);
 
-	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
-				      &fence->flags)))
+	if (unlikely(dma_fence_test_signaled_flag(fence)))
 		return -EINVAL;
 
 	/* Stash the cb_list before replacing it with the timestamp */
@@ -545,7 +544,7 @@ void dma_fence_release(struct kref *kref)
 	trace_dma_fence_destroy(fence);
 
 	if (!list_empty(&fence->cb_list) &&
-	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+	    !dma_fence_test_signaled_flag(fence)) {
 		const char __rcu *timeline;
 		const char __rcu *driver;
 		unsigned long flags;
@@ -602,7 +601,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
 	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
 				   &fence->flags);
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (dma_fence_test_signaled_flag(fence))
 		return false;
 
 	if (!was_set && fence->ops->enable_signaling) {
@@ -666,7 +665,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 	if (WARN_ON(!fence || !func))
 		return -EINVAL;
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+	if (dma_fence_test_signaled_flag(fence)) {
 		INIT_LIST_HEAD(&cb->node);
 		return -ENOENT;
 	}
@@ -783,7 +782,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 
 	spin_lock_irqsave(fence->lock, flags);
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (dma_fence_test_signaled_flag(fence))
 		goto out;
 
 	if (intr && signal_pending(current)) {
@@ -800,7 +799,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
 	cb.task = current;
 	list_add(&cb.base.node, &fence->cb_list);
 
-	while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
+	while (!dma_fence_test_signaled_flag(fence) && ret > 0) {
 		if (intr)
 			__set_current_state(TASK_INTERRUPTIBLE);
 		else
@@ -832,7 +831,7 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
 
 	for (i = 0; i < count; ++i) {
 		struct dma_fence *fence = fences[i];
-		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+		if (dma_fence_test_signaled_flag(fence)) {
 			if (idx)
 				*idx = i;
 			return true;
@@ -1108,7 +1107,7 @@ 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");
 
-	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (!dma_fence_test_signaled_flag(fence))
 		return fence->ops->get_driver_name(fence);
 	else
 		return "detached-driver";
@@ -1140,7 +1139,7 @@ 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");
 
-	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (!dma_fence_test_signaled_flag(fence))
 		return fence->ops->get_timeline_name(fence);
 	else
 		return "signaled-timeline";
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 64639e104110..19972f5d176f 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -401,6 +401,26 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
 const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
 const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
 
+/*
+ * dma_fence_test_signaled_flag - Only check whether a fence is signaled yet.
+ * @fence: the fence to check
+ *
+ * This function just checks whether @fence is signaled, without interacting
+ * with the fence in any way. The user must, therefore, ensure through other
+ * means that fences get signaled eventually.
+ *
+ * This function uses test_bit(), which is thread-safe. Naturally, this function
+ * should be used opportunistically; a fence could get signaled at any moment
+ * after the check is done.
+ *
+ * Return: true if signaled, false otherwise.
+ */
+static inline bool
+dma_fence_test_signaled_flag(struct dma_fence *fence)
+{
+	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
+}
+
 /**
  * dma_fence_is_signaled_locked - Return an indication if the fence
  *                                is signaled yet.
@@ -418,7 +438,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
 static inline bool
 dma_fence_is_signaled_locked(struct dma_fence *fence)
 {
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (dma_fence_test_signaled_flag(fence))
 		return true;
 
 	if (fence->ops->signaled && fence->ops->signaled(fence)) {
@@ -448,7 +468,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
 static inline bool
 dma_fence_is_signaled(struct dma_fence *fence)
 {
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+	if (dma_fence_test_signaled_flag(fence))
 		return true;
 
 	if (fence->ops->signaled && fence->ops->signaled(fence)) {
-- 
2.49.0

[PATCH 2/6] amd/amdkfd: Ignore return code of dma_fence_signal()

[Philipp Stanner]

The return code of dma_fence_signal() is not really useful as there is
nothing reasonable to do if a fence was already signaled. That return
code shall be removed from the kernel.

Ignore dma_fence_signal()'s return code.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
index ddfe30c13e9d..950fafa4b3c3 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
@@ -1986,7 +1986,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);
@@ -1994,10 +1993,10 @@ static int signal_eviction_fence(struct kfd_process *p)
 	if (!ef)
 		return -EINVAL;
 
-	ret = dma_fence_signal(ef);
+	dma_fence_signal(ef);
 	dma_fence_put(ef);
 
-	return ret;
+	return 0;
 }
 
 static void evict_process_worker(struct work_struct *work)
-- 
2.49.0

[PATCH 3/6] drm/gpu/xe: Ignore dma_fenc_signal() return code

[Philipp Stanner]

The return code of dma_fence_signal() is not really useful as there is
nothing reasonable to do if a fence was already signaled. That return
code shall be removed from the kernel.

Ignore dma_fence_signal()'s return code.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/xe/xe_hw_fence.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
index b2a0c46dfcd4..959b30dde724 100644
--- a/drivers/gpu/drm/xe/xe_hw_fence.c
+++ b/drivers/gpu/drm/xe/xe_hw_fence.c
@@ -85,7 +85,6 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
 {
 	struct xe_hw_fence *fence, *next;
 	unsigned long flags;
-	int err;
 	bool tmp;
 
 	if (XE_WARN_ON(!list_empty(&irq->pending))) {
@@ -93,9 +92,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
 		spin_lock_irqsave(&irq->lock, flags);
 		list_for_each_entry_safe(fence, next, &irq->pending, irq_link) {
 			list_del_init(&fence->irq_link);
-			err = dma_fence_signal_locked(&fence->dma);
+			XE_WARN_ON(dma_fence_test_signaled_flag(&fence->dma));
+			dma_fence_signal_locked(&fence->dma);
 			dma_fence_put(&fence->dma);
-			XE_WARN_ON(err);
 		}
 		spin_unlock_irqrestore(&irq->lock, flags);
 		dma_fence_end_signalling(tmp);
-- 
2.49.0

[PATCH 4/6] dma-buf: Don't misuse dma_fence_signal()

[Philipp Stanner]

The return code of dma_fence_signal() is not really useful as there is
nothing reasonable to do if a fence was already signaled. That return
code shall be removed from the kernel.

Moreover, dma_fence_signal() should not be used to check whether fences
are signaled. That's what dma_fence_is_signaled() and
dma_fence_test_signaled_flag() exist for.

Replace the non-canonical usage of dma_fence_signal().

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/st-dma-fence.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/dma-buf/st-dma-fence.c b/drivers/dma-buf/st-dma-fence.c
index 27a36045410b..9fc93859f36f 100644
--- a/drivers/dma-buf/st-dma-fence.c
+++ b/drivers/dma-buf/st-dma-fence.c
@@ -126,17 +126,14 @@ static int test_signaling(void *arg)
 		goto err_free;
 	}
 
-	if (dma_fence_signal(f)) {
-		pr_err("Fence reported being already signaled\n");
-		goto err_free;
-	}
+	dma_fence_signal(f);
 
 	if (!dma_fence_is_signaled(f)) {
 		pr_err("Fence not reporting signaled\n");
 		goto err_free;
 	}
 
-	if (!dma_fence_signal(f)) {
+	if (!dma_fence_test_signaled_flag(f)) {
 		pr_err("Fence reported not being already signaled\n");
 		goto err_free;
 	}
-- 
2.49.0

[PATCH 5/6] drm/ttm: Remove return check of dma_fence_signal()

[Philipp Stanner]

The return code of dma_fence_signal() is not useful and shall be removed
from the kernel. To do so, all users must be removed.

Remove usage of dma_fence_signal()'s return code.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
index 1bcc67977f48..34957624910f 100644
--- a/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
+++ b/drivers/gpu/drm/ttm/tests/ttm_bo_validate_test.c
@@ -692,7 +692,8 @@ static int threaded_fence_signal(void *arg)
 
 	msleep(20);
 
-	return dma_fence_signal(fence);
+	dma_fence_signal(fence);
+	return 0;
 }
 
 static void ttm_bo_validate_move_fence_not_signaled(struct kunit *test)
-- 
2.49.0

[PATCH 6/6] dma-buf/dma-fence: Remove return code of signaling-functions

[Philipp Stanner]

All functions used for signaling a fence return an error code whose sole
purpose is to tell whether a fence was already signaled.

This is racy and has been used by almost no party in the kernel, and the
few users have been removed in preceding cleanup commits.

Turn all signaling-functions into void-functions.

Suggested-by: Christian König <christian.koenig@amd.com>
Signed-off-by: Philipp Stanner <phasta@kernel.org>
---
 drivers/dma-buf/dma-fence.c | 40 ++++++++++---------------------------
 include/linux/dma-fence.h   |  9 ++++-----
 2 files changed, 14 insertions(+), 35 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 25117a906846..bed8d0c27217 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -360,11 +360,8 @@ void __dma_fence_might_wait(void)
  *
  * Unlike dma_fence_signal_timestamp(), this function must be called with
  * &dma_fence.lock held.
- *
- * Returns 0 on success and a negative error value when @fence has been
- * signalled already.
  */
-int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
+void dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 				      ktime_t timestamp)
 {
 	struct dma_fence_cb *cur, *tmp;
@@ -373,7 +370,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 	lockdep_assert_held(fence->lock);
 
 	if (unlikely(dma_fence_test_signaled_flag(fence)))
-		return -EINVAL;
+		return;
 
 	/* Stash the cb_list before replacing it with the timestamp */
 	list_replace(&fence->cb_list, &cb_list);
@@ -386,8 +383,6 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
 		INIT_LIST_HEAD(&cur->node);
 		cur->func(fence, cur);
 	}
-
-	return 0;
 }
 EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
 
@@ -402,23 +397,17 @@ EXPORT_SYMBOL(dma_fence_signal_timestamp_locked);
  * can only go from the unsignaled to the signaled state and not back, it will
  * only be effective the first time. Set the timestamp provided as the fence
  * signal timestamp.
- *
- * Returns 0 on success and a negative error value when @fence has been
- * signalled already.
  */
-int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp)
+void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp)
 {
 	unsigned long flags;
-	int ret;
 
 	if (WARN_ON(!fence))
-		return -EINVAL;
+		return;
 
 	spin_lock_irqsave(fence->lock, flags);
-	ret = dma_fence_signal_timestamp_locked(fence, timestamp);
+	dma_fence_signal_timestamp_locked(fence, timestamp);
 	spin_unlock_irqrestore(fence->lock, flags);
-
-	return ret;
 }
 EXPORT_SYMBOL(dma_fence_signal_timestamp);
 
@@ -434,13 +423,10 @@ EXPORT_SYMBOL(dma_fence_signal_timestamp);
  *
  * Unlike dma_fence_signal(), this function must be called with &dma_fence.lock
  * held.
- *
- * Returns 0 on success and a negative error value when @fence has been
- * signalled already.
  */
-int dma_fence_signal_locked(struct dma_fence *fence)
+void dma_fence_signal_locked(struct dma_fence *fence)
 {
-	return dma_fence_signal_timestamp_locked(fence, ktime_get());
+	dma_fence_signal_timestamp_locked(fence, ktime_get());
 }
 EXPORT_SYMBOL(dma_fence_signal_locked);
 
@@ -453,28 +439,22 @@ EXPORT_SYMBOL(dma_fence_signal_locked);
  * dma_fence_add_callback(). Can be called multiple times, but since a fence
  * can only go from the unsignaled to the signaled state and not back, it will
  * only be effective the first time.
- *
- * Returns 0 on success and a negative error value when @fence has been
- * signalled already.
  */
-int dma_fence_signal(struct dma_fence *fence)
+void dma_fence_signal(struct dma_fence *fence)
 {
 	unsigned long flags;
-	int ret;
 	bool tmp;
 
 	if (WARN_ON(!fence))
-		return -EINVAL;
+		return;
 
 	tmp = dma_fence_begin_signalling();
 
 	spin_lock_irqsave(fence->lock, flags);
-	ret = dma_fence_signal_timestamp_locked(fence, ktime_get());
+	dma_fence_signal_timestamp_locked(fence, ktime_get());
 	spin_unlock_irqrestore(fence->lock, flags);
 
 	dma_fence_end_signalling(tmp);
-
-	return ret;
 }
 EXPORT_SYMBOL(dma_fence_signal);
 
diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
index 19972f5d176f..188f7641050f 100644
--- a/include/linux/dma-fence.h
+++ b/include/linux/dma-fence.h
@@ -364,11 +364,10 @@ static inline void dma_fence_end_signalling(bool cookie) {}
 static inline void __dma_fence_might_wait(void) {}
 #endif
 
-int dma_fence_signal(struct dma_fence *fence);
-int dma_fence_signal_locked(struct dma_fence *fence);
-int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
-int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
-				      ktime_t timestamp);
+void dma_fence_signal(struct dma_fence *fence);
+void dma_fence_signal_locked(struct dma_fence *fence);
+void dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t timestamp);
+void dma_fence_signal_timestamp_locked(struct dma_fence *fence, ktime_t timestamp);
 signed long dma_fence_default_wait(struct dma_fence *fence,
 				   bool intr, signed long timeout);
 int dma_fence_add_callback(struct dma_fence *fence,
-- 
2.49.0

Re: [PATCH 0/6] dma-fence: Remove return code of dma_fence_signal() et al.

[Christian König]

On 11/26/25 14:19, Philipp Stanner wrote:
> Barely anyone uses dma_fence_signal()'s (and similar functions') return
> code. Checking it is pretty much useless anyways, because what are you
> going to do if a fence was already signal it? Unsignal it and signal it
> again? ;p

Reviewed-by: Christian König <christian.koenig@amd.com> for the entire series.

Please push to drm-misc-next or leave me a note when I should pick it up.

> Removing the return code simplifies the API and makes it easier for me
> to sit on top with Rust DmaFence.

BTW, I have an rb for embedding the lock and I'm now writing test cases.

When that is done you should be able to base the Rust DmaFence abstraction on that as well.

Regards,
Christian.

> 
> Philipp Stanner (6):
>   dma-buf/dma-fence: Add dma_fence_test_signaled_flag()
>   amd/amdkfd: Ignore return code of dma_fence_signal()
>   drm/gpu/xe: Ignore dma_fenc_signal() return code
>   dma-buf: Don't misuse dma_fence_signal()
>   drm/ttm: Remove return check of dma_fence_signal()
>   dma-buf/dma-fence: Remove return code of signaling-functions
> 
>  drivers/dma-buf/dma-fence.c                   | 59 ++++++-------------
>  drivers/dma-buf/st-dma-fence.c                |  7 +--
>  drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  5 +-
>  .../gpu/drm/ttm/tests/ttm_bo_validate_test.c  |  3 +-
>  drivers/gpu/drm/xe/xe_hw_fence.c              |  5 +-
>  include/linux/dma-fence.h                     | 33 ++++++++---
>  6 files changed, 53 insertions(+), 59 deletions(-)
> 

Re: [PATCH 0/6] dma-fence: Remove return code of dma_fence_signal() et al.

[Philipp Stanner]

On Wed, 2025-11-26 at 15:02 +0100, Christian König wrote:
> On 11/26/25 14:19, Philipp Stanner wrote:
> > Barely anyone uses dma_fence_signal()'s (and similar functions') return
> > code. Checking it is pretty much useless anyways, because what are you
> > going to do if a fence was already signal it? Unsignal it and signal it
> > again? ;p
> 
> Reviewed-by: Christian König <christian.koenig@amd.com> for the entire series.
> 
> Please push to drm-misc-next or leave me a note when I should pick it up.

Thx! I can push it. Let's wait a while to see if some of the other
folks have sth to say.

> 
> > Removing the return code simplifies the API and makes it easier for me
> > to sit on top with Rust DmaFence.
> 
> BTW, I have an rb for embedding the lock and I'm now writing test cases.
> 
> When that is done you should be able to base the Rust DmaFence abstraction on that as well.

Yeah, thank you, that will actually help since I was in the process of
solving the same life time issues in Rust.

I will give your series a review ~tomorrow, too. Or should I wait for
v4 with the tests?

P.

> 
> Regards,
> Christian.
> 
> > 
> > Philipp Stanner (6):
> >   dma-buf/dma-fence: Add dma_fence_test_signaled_flag()
> >   amd/amdkfd: Ignore return code of dma_fence_signal()
> >   drm/gpu/xe: Ignore dma_fenc_signal() return code
> >   dma-buf: Don't misuse dma_fence_signal()
> >   drm/ttm: Remove return check of dma_fence_signal()
> >   dma-buf/dma-fence: Remove return code of signaling-functions
> > 
> >  drivers/dma-buf/dma-fence.c                   | 59 ++++++-------------
> >  drivers/dma-buf/st-dma-fence.c                |  7 +--
> >  drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  5 +-
> >  .../gpu/drm/ttm/tests/ttm_bo_validate_test.c  |  3 +-
> >  drivers/gpu/drm/xe/xe_hw_fence.c              |  5 +-
> >  include/linux/dma-fence.h                     | 33 ++++++++---
> >  6 files changed, 53 insertions(+), 59 deletions(-)
> > 
> 

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Matthew Brost]

On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote:
> The dma_fence framework checks at many places whether the signaled flag
> of a fence is already set. The code can be simplified and made more
> readable by providing a helper function for that.
> 
> Add dma_fence_test_signaled_flag(), which only checks whether a fence is
> signaled. Use it internally.
> 
> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>

This is a nice cleanp:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
>  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
>  2 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 39e6f93dc310..25117a906846 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  
>  	lockdep_assert_held(fence->lock);
>  
> -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> -				      &fence->flags)))
> +	if (unlikely(dma_fence_test_signaled_flag(fence)))
>  		return -EINVAL;
>  
>  	/* Stash the cb_list before replacing it with the timestamp */
> @@ -545,7 +544,7 @@ void dma_fence_release(struct kref *kref)
>  	trace_dma_fence_destroy(fence);
>  
>  	if (!list_empty(&fence->cb_list) &&
> -	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	    !dma_fence_test_signaled_flag(fence)) {
>  		const char __rcu *timeline;
>  		const char __rcu *driver;
>  		unsigned long flags;
> @@ -602,7 +601,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
>  	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>  				   &fence->flags);
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (dma_fence_test_signaled_flag(fence))
>  		return false;
>  
>  	if (!was_set && fence->ops->enable_signaling) {
> @@ -666,7 +665,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
>  	if (WARN_ON(!fence || !func))
>  		return -EINVAL;
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +	if (dma_fence_test_signaled_flag(fence)) {
>  		INIT_LIST_HEAD(&cb->node);
>  		return -ENOENT;
>  	}
> @@ -783,7 +782,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  
>  	spin_lock_irqsave(fence->lock, flags);
>  
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (dma_fence_test_signaled_flag(fence))
>  		goto out;
>  
>  	if (intr && signal_pending(current)) {
> @@ -800,7 +799,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>  	cb.task = current;
>  	list_add(&cb.base.node, &fence->cb_list);
>  
> -	while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
> +	while (!dma_fence_test_signaled_flag(fence) && ret > 0) {
>  		if (intr)
>  			__set_current_state(TASK_INTERRUPTIBLE);
>  		else
> @@ -832,7 +831,7 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
>  
>  	for (i = 0; i < count; ++i) {
>  		struct dma_fence *fence = fences[i];
> -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> +		if (dma_fence_test_signaled_flag(fence)) {
>  			if (idx)
>  				*idx = i;
>  			return true;
> @@ -1108,7 +1107,7 @@ 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");
>  
> -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (!dma_fence_test_signaled_flag(fence))
>  		return fence->ops->get_driver_name(fence);
>  	else
>  		return "detached-driver";
> @@ -1140,7 +1139,7 @@ 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");
>  
> -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (!dma_fence_test_signaled_flag(fence))
>  		return fence->ops->get_timeline_name(fence);
>  	else
>  		return "signaled-timeline";
> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> index 64639e104110..19972f5d176f 100644
> --- a/include/linux/dma-fence.h
> +++ b/include/linux/dma-fence.h
> @@ -401,6 +401,26 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>  const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
>  const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>  
> +/*
> + * dma_fence_test_signaled_flag - Only check whether a fence is signaled yet.
> + * @fence: the fence to check
> + *
> + * This function just checks whether @fence is signaled, without interacting
> + * with the fence in any way. The user must, therefore, ensure through other
> + * means that fences get signaled eventually.
> + *
> + * This function uses test_bit(), which is thread-safe. Naturally, this function
> + * should be used opportunistically; a fence could get signaled at any moment
> + * after the check is done.
> + *
> + * Return: true if signaled, false otherwise.
> + */
> +static inline bool
> +dma_fence_test_signaled_flag(struct dma_fence *fence)
> +{
> +	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> +}
> +
>  /**
>   * dma_fence_is_signaled_locked - Return an indication if the fence
>   *                                is signaled yet.
> @@ -418,7 +438,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>  static inline bool
>  dma_fence_is_signaled_locked(struct dma_fence *fence)
>  {
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (dma_fence_test_signaled_flag(fence))
>  		return true;
>  
>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> @@ -448,7 +468,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>  static inline bool
>  dma_fence_is_signaled(struct dma_fence *fence)
>  {
> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> +	if (dma_fence_test_signaled_flag(fence))
>  		return true;
>  
>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> -- 
> 2.49.0
> 

Re: [PATCH 3/6] drm/gpu/xe: Ignore dma_fenc_signal() return code

[Matthew Brost]

On Wed, Nov 26, 2025 at 02:19:12PM +0100, Philipp Stanner wrote:
> The return code of dma_fence_signal() is not really useful as there is
> nothing reasonable to do if a fence was already signaled. That return
> code shall be removed from the kernel.
> 
> Ignore dma_fence_signal()'s return code.
> 
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/gpu/drm/xe/xe_hw_fence.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> index b2a0c46dfcd4..959b30dde724 100644
> --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> @@ -85,7 +85,6 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
>  {
>  	struct xe_hw_fence *fence, *next;
>  	unsigned long flags;
> -	int err;
>  	bool tmp;
>  
>  	if (XE_WARN_ON(!list_empty(&irq->pending))) {
> @@ -93,9 +92,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
>  		spin_lock_irqsave(&irq->lock, flags);
>  		list_for_each_entry_safe(fence, next, &irq->pending, irq_link) {
>  			list_del_init(&fence->irq_link);
> -			err = dma_fence_signal_locked(&fence->dma);
> +			XE_WARN_ON(dma_fence_test_signaled_flag(&fence->dma));
> +			dma_fence_signal_locked(&fence->dma);

If you also want fix Xe to use dma_fence_test_signaled_flag in all
places where we manually check DMA_FENCE_FLAG_SIGNALED_BIT, I'm not
going to complain. Ofc I can also do this in follow if patch when patch
#1 merges too.

Anyways this patch LGTM:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

>  			dma_fence_put(&fence->dma);
> -			XE_WARN_ON(err);
>  		}
>  		spin_unlock_irqrestore(&irq->lock, flags);
>  		dma_fence_end_signalling(tmp);
> -- 
> 2.49.0
> 

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Matthew Brost]

On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote:
> On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote:
> > The dma_fence framework checks at many places whether the signaled flag
> > of a fence is already set. The code can be simplified and made more
> > readable by providing a helper function for that.
> > 
> > Add dma_fence_test_signaled_flag(), which only checks whether a fence is
> > signaled. Use it internally.
> > 
> > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> 
> This is a nice cleanp:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> > ---
> >  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
> >  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
> >  2 files changed, 31 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 39e6f93dc310..25117a906846 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> >  
> >  	lockdep_assert_held(fence->lock);
> >  
> > -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > -				      &fence->flags)))

I need to read a little better, I think this change isn't quite right.
The original code is test and set, the updated code is test only (i.e.,
you are missing the set step). So maybe just leave this line as is.

Matt

> > +	if (unlikely(dma_fence_test_signaled_flag(fence)))
> >  		return -EINVAL;
> >  
> >  	/* Stash the cb_list before replacing it with the timestamp */
> > @@ -545,7 +544,7 @@ void dma_fence_release(struct kref *kref)
> >  	trace_dma_fence_destroy(fence);
> >  
> >  	if (!list_empty(&fence->cb_list) &&
> > -	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > +	    !dma_fence_test_signaled_flag(fence)) {
> >  		const char __rcu *timeline;
> >  		const char __rcu *driver;
> >  		unsigned long flags;
> > @@ -602,7 +601,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
> >  	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
> >  				   &fence->flags);
> >  
> > -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > +	if (dma_fence_test_signaled_flag(fence))
> >  		return false;
> >  
> >  	if (!was_set && fence->ops->enable_signaling) {
> > @@ -666,7 +665,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
> >  	if (WARN_ON(!fence || !func))
> >  		return -EINVAL;
> >  
> > -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > +	if (dma_fence_test_signaled_flag(fence)) {
> >  		INIT_LIST_HEAD(&cb->node);
> >  		return -ENOENT;
> >  	}
> > @@ -783,7 +782,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> >  
> >  	spin_lock_irqsave(fence->lock, flags);
> >  
> > -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > +	if (dma_fence_test_signaled_flag(fence))
> >  		goto out;
> >  
> >  	if (intr && signal_pending(current)) {
> > @@ -800,7 +799,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> >  	cb.task = current;
> >  	list_add(&cb.base.node, &fence->cb_list);
> >  
> > -	while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
> > +	while (!dma_fence_test_signaled_flag(fence) && ret > 0) {
> >  		if (intr)
> >  			__set_current_state(TASK_INTERRUPTIBLE);
> >  		else
> > @@ -832,7 +831,7 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
> >  
> >  	for (i = 0; i < count; ++i) {
> >  		struct dma_fence *fence = fences[i];
> > -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
> > +		if (dma_fence_test_signaled_flag(fence)) {
> >  			if (idx)
> >  				*idx = i;
> >  			return true;
> > @@ -1108,7 +1107,7 @@ 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");
> >  
> > -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > +	if (!dma_fence_test_signaled_flag(fence))
> >  		return fence->ops->get_driver_name(fence);
> >  	else
> >  		return "detached-driver";
> > @@ -1140,7 +1139,7 @@ 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");
> >  
> > -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > +	if (!dma_fence_test_signaled_flag(fence))
> >  		return fence->ops->get_timeline_name(fence);
> >  	else
> >  		return "signaled-timeline";
> > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > index 64639e104110..19972f5d176f 100644
> > --- a/include/linux/dma-fence.h
> > +++ b/include/linux/dma-fence.h
> > @@ -401,6 +401,26 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
> >  const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
> >  const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
> >  
> > +/*
> > + * dma_fence_test_signaled_flag - Only check whether a fence is signaled yet.
> > + * @fence: the fence to check
> > + *
> > + * This function just checks whether @fence is signaled, without interacting
> > + * with the fence in any way. The user must, therefore, ensure through other
> > + * means that fences get signaled eventually.
> > + *
> > + * This function uses test_bit(), which is thread-safe. Naturally, this function
> > + * should be used opportunistically; a fence could get signaled at any moment
> > + * after the check is done.
> > + *
> > + * Return: true if signaled, false otherwise.
> > + */
> > +static inline bool
> > +dma_fence_test_signaled_flag(struct dma_fence *fence)
> > +{
> > +	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
> > +}
> > +
> >  /**
> >   * dma_fence_is_signaled_locked - Return an indication if the fence
> >   *                                is signaled yet.
> > @@ -418,7 +438,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
> >  static inline bool
> >  dma_fence_is_signaled_locked(struct dma_fence *fence)
> >  {
> > -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > +	if (dma_fence_test_signaled_flag(fence))
> >  		return true;
> >  
> >  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> > @@ -448,7 +468,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
> >  static inline bool
> >  dma_fence_is_signaled(struct dma_fence *fence)
> >  {
> > -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> > +	if (dma_fence_test_signaled_flag(fence))
> >  		return true;
> >  
> >  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
> > -- 
> > 2.49.0
> > 

Re: [PATCH 0/6] dma-fence: Remove return code of dma_fence_signal() et al.

[Matthew Brost]

On Wed, Nov 26, 2025 at 03:09:26PM +0100, Philipp Stanner wrote:
> On Wed, 2025-11-26 at 15:02 +0100, Christian König wrote:
> > On 11/26/25 14:19, Philipp Stanner wrote:
> > > Barely anyone uses dma_fence_signal()'s (and similar functions') return
> > > code. Checking it is pretty much useless anyways, because what are you
> > > going to do if a fence was already signal it? Unsignal it and signal it
> > > again? ;p
> > 
> > Reviewed-by: Christian König <christian.koenig@amd.com> for the entire series.
> > 
> > Please push to drm-misc-next or leave me a note when I should pick it up.
> 
> Thx! I can push it. Let's wait a while to see if some of the other
> folks have sth to say.
> 

I think you have a small bug in patch #1 [1]. Also Intel's CI [2] is
complaining, it would nice to figure that out too + get a clean(ish) run
ahead of merging.

Matt

[1] https://patchwork.freedesktop.org/patch/690505/?series=158108&rev=1#comment_1268421
[2] https://patchwork.freedesktop.org/series/158108/

> > 
> > > Removing the return code simplifies the API and makes it easier for me
> > > to sit on top with Rust DmaFence.
> > 
> > BTW, I have an rb for embedding the lock and I'm now writing test cases.
> > 
> > When that is done you should be able to base the Rust DmaFence abstraction on that as well.
> 
> Yeah, thank you, that will actually help since I was in the process of
> solving the same life time issues in Rust.
> 
> I will give your series a review ~tomorrow, too. Or should I wait for
> v4 with the tests?
> 
> P.
> 
> > 
> > Regards,
> > Christian.
> > 
> > > 
> > > Philipp Stanner (6):
> > >   dma-buf/dma-fence: Add dma_fence_test_signaled_flag()
> > >   amd/amdkfd: Ignore return code of dma_fence_signal()
> > >   drm/gpu/xe: Ignore dma_fenc_signal() return code
> > >   dma-buf: Don't misuse dma_fence_signal()
> > >   drm/ttm: Remove return check of dma_fence_signal()
> > >   dma-buf/dma-fence: Remove return code of signaling-functions
> > > 
> > >  drivers/dma-buf/dma-fence.c                   | 59 ++++++-------------
> > >  drivers/dma-buf/st-dma-fence.c                |  7 +--
> > >  drivers/gpu/drm/amd/amdkfd/kfd_process.c      |  5 +-
> > >  .../gpu/drm/ttm/tests/ttm_bo_validate_test.c  |  3 +-
> > >  drivers/gpu/drm/xe/xe_hw_fence.c              |  5 +-
> > >  include/linux/dma-fence.h                     | 33 ++++++++---
> > >  6 files changed, 53 insertions(+), 59 deletions(-)
> > > 
> > 
> 

Re: [PATCH 2/6] amd/amdkfd: Ignore return code of dma_fence_signal()

[Kuehling, Felix]

On 2025-11-26 08:19, Philipp Stanner wrote:
> The return code of dma_fence_signal() is not really useful as there is
> nothing reasonable to do if a fence was already signaled. That return
> code shall be removed from the kernel.
>
> Ignore dma_fence_signal()'s return code.

I think this is not correct. Looking at the comment in 
evict_process_worker, we use the return value to decide a race 
conditions where multiple threads are trying to signal the eviction 
fence. Only one of them should schedule the restore work. And the other 
ones need to increment the reference count to keep evictions balanced.

Regards,
   Felix


>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Philipp Stanner <phasta@kernel.org>
> ---
>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> index ddfe30c13e9d..950fafa4b3c3 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> @@ -1986,7 +1986,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);
> @@ -1994,10 +1993,10 @@ static int signal_eviction_fence(struct kfd_process *p)
>   	if (!ef)
>   		return -EINVAL;
>   
> -	ret = dma_fence_signal(ef);
> +	dma_fence_signal(ef);
>   	dma_fence_put(ef);
>   
> -	return ret;
> +	return 0;
>   }
>   
>   static void evict_process_worker(struct work_struct *work)

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Andi Shyti]

Hi Philipp,

> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 39e6f93dc310..25117a906846 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>  
>  	lockdep_assert_held(fence->lock);
>  
> -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> -				      &fence->flags)))
> +	if (unlikely(dma_fence_test_signaled_flag(fence)))
>  		return -EINVAL;

Please, drop this change.

Andi

Re: [PATCH 3/6] drm/gpu/xe: Ignore dma_fenc_signal() return code

[Andi Shyti]

Hi Philipp,

in the subject /dma_fenc_signal/dma_fence_signal/

> @@ -85,7 +85,6 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
>  {
>  	struct xe_hw_fence *fence, *next;
>  	unsigned long flags;
> -	int err;
>  	bool tmp;
>  
>  	if (XE_WARN_ON(!list_empty(&irq->pending))) {
> @@ -93,9 +92,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
>  		spin_lock_irqsave(&irq->lock, flags);
>  		list_for_each_entry_safe(fence, next, &irq->pending, irq_link) {
>  			list_del_init(&fence->irq_link);
> -			err = dma_fence_signal_locked(&fence->dma);

why don't we do

XE_WARN_ON(dma_fence_signal_locked(..))

instead?

Andi

> +			XE_WARN_ON(dma_fence_test_signaled_flag(&fence->dma));
> +			dma_fence_signal_locked(&fence->dma);
>  			dma_fence_put(&fence->dma);
> -			XE_WARN_ON(err);
>  		}
>  		spin_unlock_irqrestore(&irq->lock, flags);
>  		dma_fence_end_signalling(tmp);
> -- 
> 2.49.0
> 

Re: [PATCH 3/6] drm/gpu/xe: Ignore dma_fenc_signal() return code

[Matthew Brost]

On Wed, Nov 26, 2025 at 11:56:57PM +0100, Andi Shyti wrote:
> Hi Philipp,
> 
> in the subject /dma_fenc_signal/dma_fence_signal/
> 
> > @@ -85,7 +85,6 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
> >  {
> >  	struct xe_hw_fence *fence, *next;
> >  	unsigned long flags;
> > -	int err;
> >  	bool tmp;
> >  
> >  	if (XE_WARN_ON(!list_empty(&irq->pending))) {
> > @@ -93,9 +92,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
> >  		spin_lock_irqsave(&irq->lock, flags);
> >  		list_for_each_entry_safe(fence, next, &irq->pending, irq_link) {
> >  			list_del_init(&fence->irq_link);
> > -			err = dma_fence_signal_locked(&fence->dma);
> 
> why don't we do
> 
> XE_WARN_ON(dma_fence_signal_locked(..))
> 

IIRC the above statement can compile out. So the patch looks correct to me.

Matt

> instead?
> 
> Andi
> 
> > +			XE_WARN_ON(dma_fence_test_signaled_flag(&fence->dma));
> > +			dma_fence_signal_locked(&fence->dma);
> >  			dma_fence_put(&fence->dma);
> > -			XE_WARN_ON(err);
> >  		}
> >  		spin_unlock_irqrestore(&irq->lock, flags);
> >  		dma_fence_end_signalling(tmp);
> > -- 
> > 2.49.0
> > 

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Christian König]

On 11/26/25 17:55, Matthew Brost wrote:
> On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote:
>> On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote:
>>> The dma_fence framework checks at many places whether the signaled flag
>>> of a fence is already set. The code can be simplified and made more
>>> readable by providing a helper function for that.
>>>
>>> Add dma_fence_test_signaled_flag(), which only checks whether a fence is
>>> signaled. Use it internally.
>>>
>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>>
>> This is a nice cleanp:
>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>
>>> ---
>>>  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
>>>  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
>>>  2 files changed, 31 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>> index 39e6f93dc310..25117a906846 100644
>>> --- a/drivers/dma-buf/dma-fence.c
>>> +++ b/drivers/dma-buf/dma-fence.c
>>> @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>  
>>>  	lockdep_assert_held(fence->lock);
>>>  
>>> -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
>>> -				      &fence->flags)))
> 
> I need to read a little better, I think this change isn't quite right.
> The original code is test and set, the updated code is test only (i.e.,
> you are missing the set step). So maybe just leave this line as is.

Oh, good point! I've totally missed that as well.

But that means that this patch set hasn't even been smoke tested.

Regards,
Christian.

> 
> Matt
> 
>>> +	if (unlikely(dma_fence_test_signaled_flag(fence)))
>>>  		return -EINVAL;
>>>  
>>>  	/* Stash the cb_list before replacing it with the timestamp */
>>> @@ -545,7 +544,7 @@ void dma_fence_release(struct kref *kref)
>>>  	trace_dma_fence_destroy(fence);
>>>  
>>>  	if (!list_empty(&fence->cb_list) &&
>>> -	    !test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
>>> +	    !dma_fence_test_signaled_flag(fence)) {
>>>  		const char __rcu *timeline;
>>>  		const char __rcu *driver;
>>>  		unsigned long flags;
>>> @@ -602,7 +601,7 @@ static bool __dma_fence_enable_signaling(struct dma_fence *fence)
>>>  	was_set = test_and_set_bit(DMA_FENCE_FLAG_ENABLE_SIGNAL_BIT,
>>>  				   &fence->flags);
>>>  
>>> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +	if (dma_fence_test_signaled_flag(fence))
>>>  		return false;
>>>  
>>>  	if (!was_set && fence->ops->enable_signaling) {
>>> @@ -666,7 +665,7 @@ int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
>>>  	if (WARN_ON(!fence || !func))
>>>  		return -EINVAL;
>>>  
>>> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
>>> +	if (dma_fence_test_signaled_flag(fence)) {
>>>  		INIT_LIST_HEAD(&cb->node);
>>>  		return -ENOENT;
>>>  	}
>>> @@ -783,7 +782,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>>>  
>>>  	spin_lock_irqsave(fence->lock, flags);
>>>  
>>> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +	if (dma_fence_test_signaled_flag(fence))
>>>  		goto out;
>>>  
>>>  	if (intr && signal_pending(current)) {
>>> @@ -800,7 +799,7 @@ dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
>>>  	cb.task = current;
>>>  	list_add(&cb.base.node, &fence->cb_list);
>>>  
>>> -	while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
>>> +	while (!dma_fence_test_signaled_flag(fence) && ret > 0) {
>>>  		if (intr)
>>>  			__set_current_state(TASK_INTERRUPTIBLE);
>>>  		else
>>> @@ -832,7 +831,7 @@ dma_fence_test_signaled_any(struct dma_fence **fences, uint32_t count,
>>>  
>>>  	for (i = 0; i < count; ++i) {
>>>  		struct dma_fence *fence = fences[i];
>>> -		if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
>>> +		if (dma_fence_test_signaled_flag(fence)) {
>>>  			if (idx)
>>>  				*idx = i;
>>>  			return true;
>>> @@ -1108,7 +1107,7 @@ 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");
>>>  
>>> -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +	if (!dma_fence_test_signaled_flag(fence))
>>>  		return fence->ops->get_driver_name(fence);
>>>  	else
>>>  		return "detached-driver";
>>> @@ -1140,7 +1139,7 @@ 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");
>>>  
>>> -	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +	if (!dma_fence_test_signaled_flag(fence))
>>>  		return fence->ops->get_timeline_name(fence);
>>>  	else
>>>  		return "signaled-timeline";
>>> diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
>>> index 64639e104110..19972f5d176f 100644
>>> --- a/include/linux/dma-fence.h
>>> +++ b/include/linux/dma-fence.h
>>> @@ -401,6 +401,26 @@ void dma_fence_enable_sw_signaling(struct dma_fence *fence);
>>>  const char __rcu *dma_fence_driver_name(struct dma_fence *fence);
>>>  const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>>>  
>>> +/*
>>> + * dma_fence_test_signaled_flag - Only check whether a fence is signaled yet.
>>> + * @fence: the fence to check
>>> + *
>>> + * This function just checks whether @fence is signaled, without interacting
>>> + * with the fence in any way. The user must, therefore, ensure through other
>>> + * means that fences get signaled eventually.
>>> + *
>>> + * This function uses test_bit(), which is thread-safe. Naturally, this function
>>> + * should be used opportunistically; a fence could get signaled at any moment
>>> + * after the check is done.
>>> + *
>>> + * Return: true if signaled, false otherwise.
>>> + */
>>> +static inline bool
>>> +dma_fence_test_signaled_flag(struct dma_fence *fence)
>>> +{
>>> +	return test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags);
>>> +}
>>> +
>>>  /**
>>>   * dma_fence_is_signaled_locked - Return an indication if the fence
>>>   *                                is signaled yet.
>>> @@ -418,7 +438,7 @@ const char __rcu *dma_fence_timeline_name(struct dma_fence *fence);
>>>  static inline bool
>>>  dma_fence_is_signaled_locked(struct dma_fence *fence)
>>>  {
>>> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +	if (dma_fence_test_signaled_flag(fence))
>>>  		return true;
>>>  
>>>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
>>> @@ -448,7 +468,7 @@ dma_fence_is_signaled_locked(struct dma_fence *fence)
>>>  static inline bool
>>>  dma_fence_is_signaled(struct dma_fence *fence)
>>>  {
>>> -	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
>>> +	if (dma_fence_test_signaled_flag(fence))
>>>  		return true;
>>>  
>>>  	if (fence->ops->signaled && fence->ops->signaled(fence)) {
>>> -- 
>>> 2.49.0
>>>

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Philipp Stanner]

On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote:
> On 11/26/25 17:55, Matthew Brost wrote:
> > On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote:
> > > On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote:
> > > > The dma_fence framework checks at many places whether the signaled flag
> > > > of a fence is already set. The code can be simplified and made more
> > > > readable by providing a helper function for that.
> > > > 
> > > > Add dma_fence_test_signaled_flag(), which only checks whether a fence is
> > > > signaled. Use it internally.
> > > > 
> > > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > > 
> > > This is a nice cleanp:
> > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > > 
> > > > ---
> > > >  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
> > > >  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
> > > >  2 files changed, 31 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > > index 39e6f93dc310..25117a906846 100644
> > > > --- a/drivers/dma-buf/dma-fence.c
> > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> > > >  
> > > >  	lockdep_assert_held(fence->lock);
> > > >  
> > > > -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > > > -				      &fence->flags)))
> > 
> > I need to read a little better, I think this change isn't quite right.
> > The original code is test and set, the updated code is test only (i.e.,
> > you are missing the set step). So maybe just leave this line as is.
> 
> Oh, good point! I've totally missed that as well.

Oh dear; I also just saw it when opening the mail client ._.

> 
> But that means that this patch set hasn't even been smoke tested.

I've built it and did some basic testing with my Nouveau system. Any
suggestions? Do you have a CI that one can trigger?

Thx
P.

Re: [PATCH 2/6] amd/amdkfd: Ignore return code of dma_fence_signal()

[Philipp Stanner]

On Wed, 2025-11-26 at 16:24 -0500, Kuehling, Felix wrote:
> 
> On 2025-11-26 08:19, Philipp Stanner wrote:
> > The return code of dma_fence_signal() is not really useful as there is
> > nothing reasonable to do if a fence was already signaled. That return
> > code shall be removed from the kernel.
> > 
> > Ignore dma_fence_signal()'s return code.
> 
> I think this is not correct. Looking at the comment in 
> evict_process_worker, we use the return value to decide a race 
> conditions where multiple threads are trying to signal the eviction 
> fence. Only one of them should schedule the restore work. And the other 
> ones need to increment the reference count to keep evictions balanced.

Thank you for pointing that out. Seems then amdkfd is the only user who
actually relies on the feature. See below

> 
> Regards,
>    Felix
> 
> 
> > 
> > Suggested-by: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > ---
> >   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 ++---
> >   1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > index ddfe30c13e9d..950fafa4b3c3 100644
> > --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
> > @@ -1986,7 +1986,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);
> > @@ -1994,10 +1993,10 @@ static int signal_eviction_fence(struct kfd_process *p)
> >   	if (!ef)
> >   		return -EINVAL;
> >   
> > -	ret = dma_fence_signal(ef);
> > +	dma_fence_signal(ef);

The issue now is that dma_fence_signal()'s return code is actually non-
racy, because check + bit-set are protected by lock.

Christian's new spinlock series would add a lock function for fences:
https://lore.kernel.org/dri-devel/20251113145332.16805-5-christian.koenig@amd.com/


So I suppose this should work:

dma_fence_lock_irqsave(ef, flags);
if (dma_fence_test_signaled_flag(ef)) {
	dma_fence_unlock_irqrestore(ef, flags);
	return true;
}
dma_fence_signal_locked(ef);
dma_fence_unlock_irqrestore(ef, flags);

return false;


+ some cosmetic adjustments for the boolean of course.


Would that fly and be reasonable? @Felix, Christian.


P.

Re: [PATCH 2/6] amd/amdkfd: Ignore return code of dma_fence_signal()

[Christian König]

On 11/27/25 10:48, Philipp Stanner wrote:
> On Wed, 2025-11-26 at 16:24 -0500, Kuehling, Felix wrote:
>>
>> On 2025-11-26 08:19, Philipp Stanner wrote:
>>> The return code of dma_fence_signal() is not really useful as there is
>>> nothing reasonable to do if a fence was already signaled. That return
>>> code shall be removed from the kernel.
>>>
>>> Ignore dma_fence_signal()'s return code.
>>
>> I think this is not correct. Looking at the comment in 
>> evict_process_worker, we use the return value to decide a race 
>> conditions where multiple threads are trying to signal the eviction 
>> fence. Only one of them should schedule the restore work. And the other 
>> ones need to increment the reference count to keep evictions balanced.
> 
> Thank you for pointing that out. Seems then amdkfd is the only user who
> actually relies on the feature. See below
> 
>>
>> Regards,
>>    Felix
>>
>>
>>>
>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>>> ---
>>>   drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 ++---
>>>   1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> index ddfe30c13e9d..950fafa4b3c3 100644
>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>> @@ -1986,7 +1986,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);
>>> @@ -1994,10 +1993,10 @@ static int signal_eviction_fence(struct kfd_process *p)
>>>   	if (!ef)
>>>   		return -EINVAL;
>>>   
>>> -	ret = dma_fence_signal(ef);
>>> +	dma_fence_signal(ef);
> 
> The issue now is that dma_fence_signal()'s return code is actually non-
> racy, because check + bit-set are protected by lock.
> 
> Christian's new spinlock series would add a lock function for fences:
> https://lore.kernel.org/dri-devel/20251113145332.16805-5-christian.koenig@amd.com/
> 
> 
> So I suppose this should work:
> 
> dma_fence_lock_irqsave(ef, flags);
> if (dma_fence_test_signaled_flag(ef)) {
> 	dma_fence_unlock_irqrestore(ef, flags);
> 	return true;
> }
> dma_fence_signal_locked(ef);
> dma_fence_unlock_irqrestore(ef, flags);
> 
> return false;
> 
> 
> + some cosmetic adjustments for the boolean of course.
> 
> 
> Would that fly and be reasonable? @Felix, Christian.

I was just about to reply with the same idea when your mail arrived.

So yes looks totally reasonable to me.

Regards,
Christian.

> 
> 
> P.

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Christian König]

On 11/27/25 10:16, Philipp Stanner wrote:
> On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote:
>> On 11/26/25 17:55, Matthew Brost wrote:
>>> On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote:
>>>> On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote:
>>>>> The dma_fence framework checks at many places whether the signaled flag
>>>>> of a fence is already set. The code can be simplified and made more
>>>>> readable by providing a helper function for that.
>>>>>
>>>>> Add dma_fence_test_signaled_flag(), which only checks whether a fence is
>>>>> signaled. Use it internally.
>>>>>
>>>>> Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>>>>
>>>> This is a nice cleanp:
>>>> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>>>>
>>>>> ---
>>>>>  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
>>>>>  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
>>>>>  2 files changed, 31 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
>>>>> index 39e6f93dc310..25117a906846 100644
>>>>> --- a/drivers/dma-buf/dma-fence.c
>>>>> +++ b/drivers/dma-buf/dma-fence.c
>>>>> @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
>>>>>  
>>>>>  	lockdep_assert_held(fence->lock);
>>>>>  
>>>>> -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
>>>>> -				      &fence->flags)))
>>>
>>> I need to read a little better, I think this change isn't quite right.
>>> The original code is test and set, the updated code is test only (i.e.,
>>> you are missing the set step). So maybe just leave this line as is.
>>
>> Oh, good point! I've totally missed that as well.
> 
> Oh dear; I also just saw it when opening the mail client ._.
> 
>>
>> But that means that this patch set hasn't even been smoke tested.
> 
> I've built it and did some basic testing with my Nouveau system. Any
> suggestions? Do you have a CI that one can trigger?

DMA-buf has CONFIG_DMABUF_SELFTESTS which should be able to catch things like that.

But even running Nouveau should have found this since basically no fence at would signal any more.

Regards,
Christian.

> 
> Thx
> P.

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Philipp Stanner]

On Thu, 2025-11-27 at 11:01 +0100, Christian König wrote:
> On 11/27/25 10:16, Philipp Stanner wrote:
> > On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote:
> > > On 11/26/25 17:55, Matthew Brost wrote:
> > > > On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote:
> > > > > On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote:
> > > > > > The dma_fence framework checks at many places whether the signaled flag
> > > > > > of a fence is already set. The code can be simplified and made more
> > > > > > readable by providing a helper function for that.
> > > > > > 
> > > > > > Add dma_fence_test_signaled_flag(), which only checks whether a fence is
> > > > > > signaled. Use it internally.
> > > > > > 
> > > > > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > > > > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > > > > 
> > > > > This is a nice cleanp:
> > > > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > > > > 
> > > > > > ---
> > > > > >  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
> > > > > >  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
> > > > > >  2 files changed, 31 insertions(+), 12 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > > > > index 39e6f93dc310..25117a906846 100644
> > > > > > --- a/drivers/dma-buf/dma-fence.c
> > > > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > > > @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> > > > > >  
> > > > > >  	lockdep_assert_held(fence->lock);
> > > > > >  
> > > > > > -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > > > > > -				      &fence->flags)))
> > > > 
> > > > I need to read a little better, I think this change isn't quite right.
> > > > The original code is test and set, the updated code is test only (i.e.,
> > > > you are missing the set step). So maybe just leave this line as is.
> > > 
> > > Oh, good point! I've totally missed that as well.
> > 
> > Oh dear; I also just saw it when opening the mail client ._.
> > 
> > > 
> > > But that means that this patch set hasn't even been smoke tested.
> > 
> > I've built it and did some basic testing with my Nouveau system. Any
> > suggestions? Do you have a CI that one can trigger?
> 
> DMA-buf has CONFIG_DMABUF_SELFTESTS which should be able to catch things like that.
> 
> But even running Nouveau should have found this since basically no fence at would signal any more.

They will signal and execute their callbacks, but all is_signaled()
checks are then broken.

Thx for the feedback, I'll be more careful with changes like those and
test more extensively.

P.

Re: [PATCH 3/6] drm/gpu/xe: Ignore dma_fenc_signal() return code

[Andi Shyti]

Hi Matt,

On Wed, Nov 26, 2025 at 03:56:32PM -0800, Matthew Brost wrote:
> On Wed, Nov 26, 2025 at 11:56:57PM +0100, Andi Shyti wrote:
> > > @@ -85,7 +85,6 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
> > > @@ -93,9 +92,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
> > >  		spin_lock_irqsave(&irq->lock, flags);
> > >  		list_for_each_entry_safe(fence, next, &irq->pending, irq_link) {
> > >  			list_del_init(&fence->irq_link);
> > > -			err = dma_fence_signal_locked(&fence->dma);
> > 
> > why don't we do
> > 
> > XE_WARN_ON(dma_fence_signal_locked(..))
> > 
> 
> IIRC the above statement can compile out. So the patch looks correct to me.

you have defined XE_WARN_ON as WARN_ON that should always
evaluate the content and, depending on the configuration, it
prints the logs or not.

What I don't like from this patch is that we end up checking
twice for the DMA_FENCE_FLAG_SIGNALED_BIT bit.

Thanks,
Andi

Re: [PATCH 3/6] drm/gpu/xe: Ignore dma_fenc_signal() return code

[Philipp Stanner]

On Thu, 2025-11-27 at 14:37 +0100, Andi Shyti wrote:
> Hi Matt,
> 
> On Wed, Nov 26, 2025 at 03:56:32PM -0800, Matthew Brost wrote:
> > On Wed, Nov 26, 2025 at 11:56:57PM +0100, Andi Shyti wrote:
> > > > @@ -85,7 +85,6 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
> > > > @@ -93,9 +92,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
> > > >  		spin_lock_irqsave(&irq->lock, flags);
> > > >  		list_for_each_entry_safe(fence, next, &irq->pending, irq_link) {
> > > >  			list_del_init(&fence->irq_link);
> > > > -			err = dma_fence_signal_locked(&fence->dma);
> > > 
> > > why don't we do
> > > 
> > > XE_WARN_ON(dma_fence_signal_locked(..))

because it's impossible because the series is about removing the return
codes from the dma_fence_signal_* functions.

> > > 
> > 
> > IIRC the above statement can compile out. So the patch looks correct to me.
> 
> you have defined XE_WARN_ON as WARN_ON that should always
> evaluate the content and, depending on the configuration, it
> prints the logs or not.
> 
> What I don't like from this patch is that we end up checking
> twice for the DMA_FENCE_FLAG_SIGNALED_BIT bit.

Depends on what you mean by "we". The Xe code checks it only once, with
dma_fence_test_signaled_flag(). The dma_fence backend checks it yet
again, as it always does, to avoid signaling a signaled fence.

That's not racy here, however, because the fence lock is already being
held, as evidenced by the current usage of dma_fence_signal_locked().


P.

Re: [PATCH 2/6] amd/amdkfd: Ignore return code of dma_fence_signal()

[Kuehling, Felix]

On 2025-11-27 04:55, Christian König wrote:
> On 11/27/25 10:48, Philipp Stanner wrote:
>> On Wed, 2025-11-26 at 16:24 -0500, Kuehling, Felix wrote:
>>> On 2025-11-26 08:19, Philipp Stanner wrote:
>>>> The return code of dma_fence_signal() is not really useful as there is
>>>> nothing reasonable to do if a fence was already signaled. That return
>>>> code shall be removed from the kernel.
>>>>
>>>> Ignore dma_fence_signal()'s return code.
>>> I think this is not correct. Looking at the comment in
>>> evict_process_worker, we use the return value to decide a race
>>> conditions where multiple threads are trying to signal the eviction
>>> fence. Only one of them should schedule the restore work. And the other
>>> ones need to increment the reference count to keep evictions balanced.
>> Thank you for pointing that out. Seems then amdkfd is the only user who
>> actually relies on the feature. See below
>>
>>> Regards,
>>>     Felix
>>>
>>>
>>>> Suggested-by: Christian König <christian.koenig@amd.com>
>>>> Signed-off-by: Philipp Stanner <phasta@kernel.org>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdkfd/kfd_process.c | 5 ++---
>>>>    1 file changed, 2 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_process.c b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> index ddfe30c13e9d..950fafa4b3c3 100644
>>>> --- a/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_process.c
>>>> @@ -1986,7 +1986,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);
>>>> @@ -1994,10 +1993,10 @@ static int signal_eviction_fence(struct kfd_process *p)
>>>>    	if (!ef)
>>>>    		return -EINVAL;
>>>>    
>>>> -	ret = dma_fence_signal(ef);
>>>> +	dma_fence_signal(ef);
>> The issue now is that dma_fence_signal()'s return code is actually non-
>> racy, because check + bit-set are protected by lock.
>>
>> Christian's new spinlock series would add a lock function for fences:
>> https://lore.kernel.org/dri-devel/20251113145332.16805-5-christian.koenig@amd.com/
>>
>>
>> So I suppose this should work:
>>
>> dma_fence_lock_irqsave(ef, flags);
>> if (dma_fence_test_signaled_flag(ef)) {
>> 	dma_fence_unlock_irqrestore(ef, flags);
>> 	return true;
>> }
>> dma_fence_signal_locked(ef);
>> dma_fence_unlock_irqrestore(ef, flags);
>>
>> return false;
>>
>>
>> + some cosmetic adjustments for the boolean of course.
>>
>>
>> Would that fly and be reasonable? @Felix, Christian.
> I was just about to reply with the same idea when your mail arrived.

I agree as well. The important feature is that we need to test and 
signal the fence atomically. It may even make sense to add a function 
for that "dma_fence_test_and_signal" that preserves the original 
behaviour of dma_fence_signal. ;)

Regards,
   Felix


>
> So yes looks totally reasonable to me.
>
> Regards,
> Christian.
>
>>
>> P.

Re: [PATCH 2/6] amd/amdkfd: Ignore return code of dma_fence_signal()

[Philipp Stanner]

On Thu, 2025-11-27 at 10:08 -0500, Kuehling, Felix wrote:
> On 2025-11-27 04:55, Christian König wrote:
> > On 11/27/25 10:48, Philipp Stanner wrote:
> > > 

[…]

> > > The issue now is that dma_fence_signal()'s return code is actually non-
> > > racy, because check + bit-set are protected by lock.
> > > 
> > > Christian's new spinlock series would add a lock function for fences:
> > > https://lore.kernel.org/dri-devel/20251113145332.16805-5-christian.koenig@amd.com/
> > > 
> > > 
> > > So I suppose this should work:
> > > 
> > > dma_fence_lock_irqsave(ef, flags);
> > > if (dma_fence_test_signaled_flag(ef)) {
> > > 	dma_fence_unlock_irqrestore(ef, flags);
> > > 	return true;
> > > }
> > > dma_fence_signal_locked(ef);
> > > dma_fence_unlock_irqrestore(ef, flags);
> > > 
> > > return false;
> > > 
> > > 
> > > + some cosmetic adjustments for the boolean of course.
> > > 
> > > 
> > > Would that fly and be reasonable? @Felix, Christian.
> > I was just about to reply with the same idea when your mail arrived.
> 
> I agree as well. The important feature is that we need to test and 
> signal the fence atomically. It may even make sense to add a function
> for that "dma_fence_test_and_signal" that preserves the original 
> behaviour of dma_fence_signal. ;)

Fine by me if the maintainer agrees

P.

Re: [PATCH 3/6] drm/gpu/xe: Ignore dma_fenc_signal() return code

[Andi Shyti]

On Thu, Nov 27, 2025 at 02:51:39PM +0100, Philipp Stanner wrote:
> On Thu, 2025-11-27 at 14:37 +0100, Andi Shyti wrote:
> > On Wed, Nov 26, 2025 at 03:56:32PM -0800, Matthew Brost wrote:
> > > On Wed, Nov 26, 2025 at 11:56:57PM +0100, Andi Shyti wrote:
> > > > > @@ -85,7 +85,6 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
> > > > > @@ -93,9 +92,9 @@ void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq)
> > > > >  		spin_lock_irqsave(&irq->lock, flags);
> > > > >  		list_for_each_entry_safe(fence, next, &irq->pending, irq_link) {
> > > > >  			list_del_init(&fence->irq_link);
> > > > > -			err = dma_fence_signal_locked(&fence->dma);
> > > > 
> > > > why don't we do
> > > > 
> > > > XE_WARN_ON(dma_fence_signal_locked(..))
> 
> because it's impossible because the series is about removing the return
> codes from the dma_fence_signal_* functions.

oh yes, the last patch. Sorry, I went on reviewing and lost the
final target from sight.

> > > > 
> > > 
> > > IIRC the above statement can compile out. So the patch looks correct to me.
> > 
> > you have defined XE_WARN_ON as WARN_ON that should always
> > evaluate the content and, depending on the configuration, it
> > prints the logs or not.
> > 
> > What I don't like from this patch is that we end up checking
> > twice for the DMA_FENCE_FLAG_SIGNALED_BIT bit.
> 
> Depends on what you mean by "we". The Xe code checks it only once, with
> dma_fence_test_signaled_flag(). The dma_fence backend checks it yet
> again, as it always does, to avoid signaling a signaled fence.
> 
> That's not racy here, however, because the fence lock is already being
> held, as evidenced by the current usage of dma_fence_signal_locked().

I haven't said it's racy, I just didn't like that we are testing
for the DMA_FENCE_FLAG_SIGNALED_BIT twice. On the other hand,
with dma_fence_signal_locked() being void, I wouldn't know how to
do it better. So that I guess it's fine.

Andi

Re: [PATCH 1/6] dma-buf/dma-fence: Add dma_fence_test_signaled_flag()

[Matthew Brost]

On Thu, Nov 27, 2025 at 11:16:26AM +0100, Philipp Stanner wrote:
> On Thu, 2025-11-27 at 11:01 +0100, Christian König wrote:
> > On 11/27/25 10:16, Philipp Stanner wrote:
> > > On Thu, 2025-11-27 at 09:11 +0100, Christian König wrote:
> > > > On 11/26/25 17:55, Matthew Brost wrote:
> > > > > On Wed, Nov 26, 2025 at 08:41:27AM -0800, Matthew Brost wrote:
> > > > > > On Wed, Nov 26, 2025 at 02:19:10PM +0100, Philipp Stanner wrote:
> > > > > > > The dma_fence framework checks at many places whether the signaled flag
> > > > > > > of a fence is already set. The code can be simplified and made more
> > > > > > > readable by providing a helper function for that.
> > > > > > > 
> > > > > > > Add dma_fence_test_signaled_flag(), which only checks whether a fence is
> > > > > > > signaled. Use it internally.
> > > > > > > 
> > > > > > > Suggested-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > > > > > Signed-off-by: Philipp Stanner <phasta@kernel.org>
> > > > > > 
> > > > > > This is a nice cleanp:
> > > > > > Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> > > > > > 
> > > > > > > ---
> > > > > > >  drivers/dma-buf/dma-fence.c | 19 +++++++++----------
> > > > > > >  include/linux/dma-fence.h   | 24 ++++++++++++++++++++++--
> > > > > > >  2 files changed, 31 insertions(+), 12 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > > > > > index 39e6f93dc310..25117a906846 100644
> > > > > > > --- a/drivers/dma-buf/dma-fence.c
> > > > > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > > > > @@ -372,8 +372,7 @@ int dma_fence_signal_timestamp_locked(struct dma_fence *fence,
> > > > > > >  
> > > > > > >  	lockdep_assert_held(fence->lock);
> > > > > > >  
> > > > > > > -	if (unlikely(test_and_set_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > > > > > > -				      &fence->flags)))
> > > > > 
> > > > > I need to read a little better, I think this change isn't quite right.
> > > > > The original code is test and set, the updated code is test only (i.e.,
> > > > > you are missing the set step). So maybe just leave this line as is.
> > > > 
> > > > Oh, good point! I've totally missed that as well.
> > > 
> > > Oh dear; I also just saw it when opening the mail client ._.
> > > 
> > > > 
> > > > But that means that this patch set hasn't even been smoke tested.
> > > 
> > > I've built it and did some basic testing with my Nouveau system. Any
> > > suggestions? Do you have a CI that one can trigger?

Intel's Xe list will trigger CI - you are authorized to trigger that
flow. Navigate to Intel's list patchworks [1], find your series and see
the results. It is noisy, so also certainly to say failure but I let you
know if anything is actually a problem.

Matt

[1] https://patchwork.freedesktop.org/project/intel-xe/series/?ordering=-last_updated

> > 
> > DMA-buf has CONFIG_DMABUF_SELFTESTS which should be able to catch things like that.
> > 
> > But even running Nouveau should have found this since basically no fence at would signal any more.
> 
> They will signal and execute their callbacks, but all is_signaled()
> checks are then broken.
> 
> Thx for the feedback, I'll be more careful with changes like those and
> test more extensively.
> 
> P.