[PATCH v5 04/16] drm/sched: Avoid double re-lock on the job free path
Philipp Stanner
phasta at mailbox.org
Fri Jul 4 13:59:56 UTC 2025
On Fri, 2025-07-04 at 14:30 +0100, Tvrtko Ursulin wrote:
>
> On 04/07/2025 13:56, Philipp Stanner wrote:
> > On Fri, 2025-07-04 at 09:29 -0300, Maíra Canal wrote:
> > > Hi Tvrtko,
> > >
> > > On 23/06/25 09:27, Tvrtko Ursulin wrote:
> > > > Currently the job free work item will lock sched->job_list_lock
> > > > first time
> > > > to see if there are any jobs, free a single job, and then lock
> > > > again to
> > > > decide whether to re-queue itself if there are more finished
> > > > jobs.
> > > >
> > > > Since drm_sched_get_finished_job() already looks at the second
> > > > job
> > > > in the
> > > > queue we can simply add the signaled check and have it return
> > > > the
> > > > presence
> > > > of more jobs to free to the caller. That way the work item does
> > > > not
> > > > have
> > > > to lock the list again and repeat the signaled check.
> > > >
> > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at igalia.com>
> > > > Cc: Christian König <christian.koenig at amd.com>
> > > > Cc: Danilo Krummrich <dakr at kernel.org>
> > > > Cc: Matthew Brost <matthew.brost at intel.com>
> > > > Cc: Philipp Stanner <phasta at kernel.org>
> > > > ---
> > > > drivers/gpu/drm/scheduler/sched_main.c | 39 +++++++++++-----
> > > > -----
> > > > -----
> > > > 1 file changed, 16 insertions(+), 23 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index 1f077782ec12..c6c26aec07b6 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -366,22 +366,6 @@ static void
> > > > __drm_sched_run_free_queue(struct
> > > > drm_gpu_scheduler *sched)
> > > > queue_work(sched->submit_wq, &sched-
> > > > > work_free_job);
> > > > }
> > > >
> > > > -/**
> > > > - * drm_sched_run_free_queue - enqueue free-job work if ready
> > > > - * @sched: scheduler instance
> > > > - */
> > > > -static void drm_sched_run_free_queue(struct drm_gpu_scheduler
> > > > *sched)
> > > > -{
> > > > - struct drm_sched_job *job;
> > > > -
> > > > - spin_lock(&sched->job_list_lock);
> > > > - job = list_first_entry_or_null(&sched->pending_list,
> > > > - struct drm_sched_job,
> > > > list);
> > > > - if (job && dma_fence_is_signaled(&job->s_fence-
> > > > >finished))
> > > > - __drm_sched_run_free_queue(sched);
> > > > - spin_unlock(&sched->job_list_lock);
> > > > -}
> > > > -
> > > > /**
> > > > * drm_sched_job_done - complete a job
> > > > * @s_job: pointer to the job which is done
> > > > @@ -1102,12 +1086,13 @@ drm_sched_select_entity(struct
> > > > drm_gpu_scheduler *sched)
> > > > * drm_sched_get_finished_job - fetch the next finished job
> > > > to be
> > > > destroyed
> > > > *
> > > > * @sched: scheduler instance
> > > > + * @have_more: are there more finished jobs on the list
> > > > *
> > > > * Returns the next finished job from the pending list (if
> > > > there
> > > > is one)
> > > > * ready for it to be destroyed.
> > > > */
> > > > static struct drm_sched_job *
> > > > -drm_sched_get_finished_job(struct drm_gpu_scheduler *sched)
> > > > +drm_sched_get_finished_job(struct drm_gpu_scheduler *sched,
> > > > bool
> > > > *have_more)
> > > > {
> > > > struct drm_sched_job *job, *next;
> > > >
> > > > @@ -1115,22 +1100,27 @@ drm_sched_get_finished_job(struct
> > > > drm_gpu_scheduler *sched)
> > > >
> > > > job = list_first_entry_or_null(&sched->pending_list,
> > > > struct drm_sched_job,
> > > > list);
> > > > -
> > > > if (job && dma_fence_is_signaled(&job->s_fence-
> > > > >finished))
> > > > {
> > > > /* remove job from pending_list */
> > > > list_del_init(&job->list);
> > > >
> > > > /* cancel this job's TO timer */
> > > > cancel_delayed_work(&sched->work_tdr);
> > > > - /* make the scheduled timestamp more accurate
> > > > */
> > > > +
> > > > + *have_more = false;
> > > > next = list_first_entry_or_null(&sched-
> > > > > pending_list,
> > > > typeof(*next),
> > > > list);
> > > > -
> > > > if (next) {
> > > > + /* make the scheduled timestamp more
> > > > accurate */
> > > > if
> > > > (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT,
> > > > &next->s_fence-
> > > > > scheduled.flags))
> > > > next->s_fence-
> > > > >scheduled.timestamp
> > > > =
> > > > dma_fence_timestamp(&j
> > > > ob-
> > > > > s_fence->finished);
> > > > +
> > > > + if
> > > > (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT,
> > > > + &next->s_fence-
> > > > > finished.flags))
> > >
> > > Shouldn't we use dma_fence_is_signaled() to keep the same check
> > > that
> > > we
> > > have in drm_sched_run_free_queue()?
> >
> > There is a paused-ongoing discussion about this function:
> >
> > https://lore.kernel.org/all/20250522112540.161411-2-phasta@kernel.org/
> >
> >
> > dma_fence_is_signaled() can have side effects by actually
> > signaling,
> > instead of just checking.
> >
> > Not sure if Tvrtko wanted to bypass that behavior here, though.
>
> No, no ulterior motives here. :)
>
> It is ages I wrote this, but now I revisited it, and AFAICT I don't
> see
> that it matters in this case.
>
> It is a scheduler fence which does not implement fence->ops-
> >signaled()
> so opportunistic signaling does not come into the picture.
>
> I am happy to change it to dma_fence_is_signaled() if that is the
> preference.
Its our (scheduler's) fence, so we can be sure dma_fence_is_signaled()
is OK.
I'd still prefer if we could get Christian to accept a function with a
superior name, though..
P.
>
> Regards,
>
> Tvrtko
>
> > > > + *have_more = true;
> > > > +
> > > > /* start TO timer for next job */
> > > > drm_sched_start_timeout(sched);
> > > > }
> > >
> > >
> >
>
More information about the dri-devel
mailing list