On Wed, 2026-01-14 at 12:30 +0100, Christian König wrote:
> On 1/13/26 17:12, Philipp Stanner wrote:
> > On Tue, 2026-01-13 at 16:16 +0100, Christian König wrote:
> > > Using the inline lock is now the recommended way for dma_fence
> > > implementations.
> > >
> > > For the scheduler fence use the inline lock for the scheduled fence part
> > > and then the lock from the scheduled fence as external lock for the
> > > finished fence.
> > >
> > > This way there is no functional difference, except for saving the space
> > > for the separate lock.
> > >
> > > v2: re-work the patch to avoid any functional difference
> >
> > *cough cough*
> >
> > >
> > > Signed-off-by: Christian König <[email protected]>
> > > ---
> > > drivers/gpu/drm/scheduler/sched_fence.c | 6 +++---
> > > include/drm/gpu_scheduler.h | 4 ----
> > > 2 files changed, 3 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/scheduler/sched_fence.c
> > > b/drivers/gpu/drm/scheduler/sched_fence.c
> > > index 724d77694246..112677231f9a 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_fence.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_fence.c
> > > @@ -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,10 @@ 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);
> > > + dma_fence_spinlock(&fence->scheduled),
> >
> > I think while you are correct that this is no functional difference, it
> > is still a bad idea which violates the entire idea of your series:
> >
> > All fences are now independent from each other and the fence context –
> > except for those two.
> >
> > Some fences are more equal than others ;)
>
> Yeah, I was going back and forth once more if I should keep this patch at all
> or just drop it.
>
> > By implementing this, you would also show to people browsing the code
> > that it can be a good idea or can be done to have fences share locks.
> > Do you want that?
>
> Good question. For almost all cases we don't want this, but once more the
> scheduler is special.
>
> In the scheduler we have two fences in one, the scheduled one and the
> finished one.
>
> So here it technically makes sense to have this construct to be defensive.
>
> But on the other hand it has no practical value because it still doesn't
> allow us to unload the scheduler module. We would need a much wider rework
> for being able to do that.
>
> So maybe I should just really drop this patch or at least keep it back until
> we had time to figure out what the next steps are.
>
> > As far as I have learned from you and our discussions, that would be a
> > very bombastic violation of the sacred "dma-fence-rules".
>
> Well using the inline fence is "only" a strong recommendation. It's not as
> heavy as the signaling rules because when you mess up those you can easily
> kill the whole system.
>
> > I believe it's definitely worth sacrificing some bytes so that those
> > two fences get fully decoupled. Who will have it on their radar that
> > they are special? Think about future reworks.
>
> This doesn't even save any bytes, my thinking was more that this is the more
> defensive approach should anybody use the spinlock pointer from the scheduler
> fence to do some locking.
>
Using the scheduler's internal locks is not legal (anymore). With the
sched_fence it's a bit special though because that is the
synchronization object for the driver, actually. So I don't know
either.
I would say either separate the locks, or drop the patch as you
suggest.
P.
> > Besides that, no objections from my side.
>
> Thanks,
> Christian.
>
> >
> >
> > P.
> >
> > > + 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 78e07c2507c7..ad3704685163 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
> > > */
> >
>
