[Linaro-mm-sig] Re: [PATCH 10/18] drm/amdgpu: independence for the amdgpu_fence!

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