On 15/12/2025 15:38, Christian König wrote:
On 12/15/25 10:20, Tvrtko Ursulin wrote:
On 12/12/2025 15:50, Christian König wrote:
On 12/11/25 16:13, Tvrtko Ursulin wrote:
On 11/12/2025 13:16, Christian König wrote:
Using the inline lock is now the recommended way for dma_fence implementations.
So use this approach for the scheduler fences as well just in case if
anybody uses this as blueprint for its own implementation.
Also saves about 4 bytes for the external spinlock.
Signed-off-by: Christian König <[email protected]>
---
drivers/gpu/drm/scheduler/sched_fence.c | 7 +++----
include/drm/gpu_scheduler.h | 4 ----
2 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c
b/drivers/gpu/drm/scheduler/sched_fence.c
index 08ccbde8b2f5..47471b9e43f9 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -161,7 +161,7 @@ static void drm_sched_fence_set_deadline_finished(struct
dma_fence *f,
/* If we already have an earlier deadline, keep it: */
if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) &&
ktime_before(fence->deadline, deadline)) {
- spin_unlock_irqrestore(&fence->lock, flags);
+ dma_fence_unlock_irqrestore(f, flags);
Rebase error I guess. Pull into the locking helpers patch.
No that is actually completely intentional here.
Previously we had a separate lock which protected both the DMA-fences as well
as the deadline state.
Now we turn that upside down by dropping the separate lock and protecting the
deadline state with the dma_fence lock instead.
I don't follow. The code is currently like this:
static void drm_sched_fence_set_deadline_finished(struct dma_fence *f,
ktime_t deadline)
{
struct drm_sched_fence *fence = to_drm_sched_fence(f);
struct dma_fence *parent;
unsigned long flags;
spin_lock_irqsave(&fence->lock, flags);
/* If we already have an earlier deadline, keep it: */
if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) &&
ktime_before(fence->deadline, deadline)) {
spin_unlock_irqrestore(&fence->lock, flags);
return;
}
fence->deadline = deadline;
set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
spin_unlock_irqrestore(&fence->lock, flags);...
The diff changes one out of the three lock/unlock operations. Other two are
changed in 3/19. All three should surely be changed in the same patch.
We could change those spin_lock/unlock calls in patch #3, but I don't think
that this is clean.
See the code here currently uses fence->lock and patch #3 would change it to use
fence->finished->lock instead. That might be the pointer at the moment, but that
is just by coincident and not design.
Only this change here ontop makes it intentional that we use
fence->finished->lock for everything.
Sorry I still don't follow. After 3/19 and before this 9/19 the function
looks like this:
static void drm_sched_fence_set_deadline_finished(struct dma_fence *f,
ktime_t deadline)
{
struct drm_sched_fence *fence = to_drm_sched_fence(f);
struct dma_fence *parent;
unsigned long flags;
dma_fence_lock_irqsave(f, flags);
/* If we already have an earlier deadline, keep it: */
if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags) &&
ktime_before(fence->deadline, deadline)) {
spin_unlock_irqrestore(&fence->lock, flags);
return;
}
fence->deadline = deadline;
set_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &f->flags);
dma_fence_unlock_irqrestore(f, flags);
Notice the lonely spin_unlock_irqrestore on the early return path while
other two use the dma_fence_(un)lock helpers. Am I blind or how is that
clean?
Regards,
Tvrtko
Regards,
Christian.
Regards,
Tvrtko
Regards,
Christian.
Regards,
Tvrtko
return;
}
@@ -217,7 +217,6 @@ struct drm_sched_fence *drm_sched_fence_alloc(struct
drm_sched_entity *entity,
fence->owner = owner;
fence->drm_client_id = drm_client_id;
- spin_lock_init(&fence->lock);
return fence;
}
@@ -230,9 +229,9 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
fence->sched = entity->rq->sched;
seq = atomic_inc_return(&entity->fence_seq);
dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled,
- &fence->lock, entity->fence_context, seq);
+ NULL, entity->fence_context, seq);
dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished,
- &fence->lock, entity->fence_context + 1, seq);
+ NULL, entity->fence_context + 1, seq);
}
module_init(drm_sched_fence_slab_init);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index fb88301b3c45..b77f24a783e3 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -297,10 +297,6 @@ struct drm_sched_fence {
* belongs to.
*/
struct drm_gpu_scheduler *sched;
- /**
- * @lock: the lock used by the scheduled and the finished fences.
- */
- spinlock_t lock;
/**
* @owner: job owner for debugging
*/
