Re: [PATCH v6 14/15] drm/sched: Queue all free credits in one worker invocation

["Christian König" <christian.koenig@amd.com>]

On 08.07.25 14:54, Tvrtko Ursulin wrote:
> 
> On 08/07/2025 13:37, Christian König wrote:
>> On 08.07.25 11:51, Tvrtko Ursulin wrote:
>>> There is no reason to queue just a single job if scheduler can take more
>>> and re-queue the worker to queue more.
>>
>> That's not correct. This was intentionally avoided.
>>
>> If more than just the scheduler is using the single threaded workqeueu other workers, especially the timeout worker, can jump in and execute first.
>>
>> We explicitely removed submitting more than one job in each worker run.
> 
> I wanted to ask why, but then I had a look to see if anyone actually does this. And I did not find any driver sharing a single threaded workqueue between submit and timeout.
> 
> The only driver which even passes in the same workqueue for both is PVR, but it is not a single threaded one.
> 
> Or perhaps I misunderstood what you said. Could you please clarify either way?

You correctly understood that.

The argument was that submitting more than one job in a worker is simply not beneficial and other work items can jump in and execute.

I have no idea if that is actually used or not. You would need to dig up the discussion when we switched from a kernel thread to work items for the full background.

But in general to do as less work as possible in each worker and then re-submit it is usually a good idea.

Regards,
Christian.

> 
> Regards,
> 
> Tvrtko
> 
>>> We can simply feed the hardware
>>> with as much as it can take in one go and hopefully win some latency.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> Cc: Philipp Stanner <phasta@kernel.org>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_internal.h |   2 -
>>>   drivers/gpu/drm/scheduler/sched_main.c     | 132 ++++++++++-----------
>>>   drivers/gpu/drm/scheduler/sched_rq.c       |  12 +-
>>>   3 files changed, 64 insertions(+), 82 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h
>>> index 15d78abc48df..1a5c2f255223 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_internal.h
>>> +++ b/drivers/gpu/drm/scheduler/sched_internal.h
>>> @@ -22,8 +22,6 @@ struct drm_sched_entity_stats {
>>>       u64        vruntime;
>>>   };
>>>   -bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
>>> -             struct drm_sched_entity *entity);
>>>   void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>>>     void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 35025edea669..1fb3f1da4821 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -95,35 +95,6 @@ static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
>>>       return credits;
>>>   }
>>>   -/**
>>> - * drm_sched_can_queue -- Can we queue more to the hardware?
>>> - * @sched: scheduler instance
>>> - * @entity: the scheduler entity
>>> - *
>>> - * Return true if we can push at least one more job from @entity, false
>>> - * otherwise.
>>> - */
>>> -bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
>>> -             struct drm_sched_entity *entity)
>>> -{
>>> -    struct drm_sched_job *s_job;
>>> -
>>> -    s_job = drm_sched_entity_queue_peek(entity);
>>> -    if (!s_job)
>>> -        return false;
>>> -
>>> -    /* If a job exceeds the credit limit, truncate it to the credit limit
>>> -     * itself to guarantee forward progress.
>>> -     */
>>> -    if (s_job->credits > sched->credit_limit) {
>>> -        dev_WARN(sched->dev,
>>> -             "Jobs may not exceed the credit limit, truncate.\n");
>>> -        s_job->credits = sched->credit_limit;
>>> -    }
>>> -
>>> -    return drm_sched_available_credits(sched) >= s_job->credits;
>>> -}
>>> -
>>>   /**
>>>    * drm_sched_run_job_queue - enqueue run-job work
>>>    * @sched: scheduler instance
>>> @@ -940,54 +911,77 @@ static void drm_sched_run_job_work(struct work_struct *w)
>>>   {
>>>       struct drm_gpu_scheduler *sched =
>>>           container_of(w, struct drm_gpu_scheduler, work_run_job);
>>> +    u32 job_credits, submitted_credits = 0;
>>>       struct drm_sched_entity *entity;
>>> -    struct dma_fence *fence;
>>>       struct drm_sched_fence *s_fence;
>>>       struct drm_sched_job *sched_job;
>>> -    int r;
>>> +    struct dma_fence *fence;
>>>   -    /* Find entity with a ready job */
>>> -    entity = drm_sched_rq_select_entity(sched, sched->rq);
>>> -    if (IS_ERR_OR_NULL(entity))
>>> -        return;    /* No more work */
>>> +    while (!READ_ONCE(sched->pause_submit)) {
>>> +        /* Find entity with a ready job */
>>> +        entity = drm_sched_rq_select_entity(sched, sched->rq);
>>> +        if (!entity)
>>> +            break;    /* No more work */
>>> +
>>> +        sched_job = drm_sched_entity_queue_peek(entity);
>>> +        if (!sched_job) {
>>> +            complete_all(&entity->entity_idle);
>>> +            continue;
>>> +        }
>>> +
>>> +        job_credits = sched_job->credits;
>>> +        /*
>>> +         * If a job exceeds the credit limit truncate it to guarantee
>>> +         * forward progress.
>>> +         */
>>> +        if (dev_WARN_ONCE(sched->dev, job_credits > sched->credit_limit,
>>> +                  "Jobs may not exceed the credit limit, truncating.\n"))
>>> +            job_credits = sched_job->credits = sched->credit_limit;
>>> +
>>> +        if (job_credits > drm_sched_available_credits(sched)) {
>>> +            complete_all(&entity->entity_idle);
>>> +            break;
>>> +        }
>>> +
>>> +        sched_job = drm_sched_entity_pop_job(entity);
>>> +        if (!sched_job) {
>>> +            /* Top entity is not yet runnable after all */
>>> +            complete_all(&entity->entity_idle);
>>> +            continue;
>>> +        }
>>> +
>>> +        s_fence = sched_job->s_fence;
>>> +        drm_sched_job_begin(sched_job);
>>> +        trace_drm_sched_job_run(sched_job, entity);
>>> +        submitted_credits += job_credits;
>>> +        atomic_add(job_credits, &sched->credit_count);
>>> +
>>> +        fence = sched->ops->run_job(sched_job);
>>> +        drm_sched_fence_scheduled(s_fence, fence);
>>> +
>>> +        if (!IS_ERR_OR_NULL(fence)) {
>>> +            int r;
>>> +
>>> +            /* Drop for original kref_init of the fence */
>>> +            dma_fence_put(fence);
>>> +
>>> +            r = dma_fence_add_callback(fence, &sched_job->cb,
>>> +                           drm_sched_job_done_cb);
>>> +            if (r == -ENOENT)
>>> +                drm_sched_job_done(sched_job, fence->error);
>>> +            else if (r)
>>> +                DRM_DEV_ERROR(sched->dev,
>>> +                          "fence add callback failed (%d)\n", r);
>>> +        } else {
>>> +            drm_sched_job_done(sched_job, IS_ERR(fence) ?
>>> +                              PTR_ERR(fence) : 0);
>>> +        }
>>>   -    sched_job = drm_sched_entity_pop_job(entity);
>>> -    if (!sched_job) {
>>>           complete_all(&entity->entity_idle);
>>> -        drm_sched_run_job_queue(sched);
>>> -        return;
>>>       }
>>>   -    s_fence = sched_job->s_fence;
>>> -
>>> -    atomic_add(sched_job->credits, &sched->credit_count);
>>> -    drm_sched_job_begin(sched_job);
>>> -
>>> -    trace_drm_sched_job_run(sched_job, entity);
>>> -    /*
>>> -     * The run_job() callback must by definition return a fence whose
>>> -     * refcount has been incremented for the scheduler already.
>>> -     */
>>> -    fence = sched->ops->run_job(sched_job);
>>> -    complete_all(&entity->entity_idle);
>>> -    drm_sched_fence_scheduled(s_fence, fence);
>>> -
>>> -    if (!IS_ERR_OR_NULL(fence)) {
>>> -        r = dma_fence_add_callback(fence, &sched_job->cb,
>>> -                       drm_sched_job_done_cb);
>>> -        if (r == -ENOENT)
>>> -            drm_sched_job_done(sched_job, fence->error);
>>> -        else if (r)
>>> -            DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n", r);
>>> -
>>> -        dma_fence_put(fence);
>>> -    } else {
>>> -        drm_sched_job_done(sched_job, IS_ERR(fence) ?
>>> -                   PTR_ERR(fence) : 0);
>>> -    }
>>> -
>>> -    wake_up(&sched->job_scheduled);
>>> -    drm_sched_run_job_queue(sched);
>>> +    if (submitted_credits)
>>> +        wake_up(&sched->job_scheduled);
>>>   }
>>>     static struct workqueue_struct *drm_sched_alloc_wq(const char *name)
>>> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
>>> index e22f9ff88822..f0afdc0bd417 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_rq.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
>>> @@ -197,9 +197,7 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
>>>    *
>>>    * Find oldest waiting ready entity.
>>>    *
>>> - * Return an entity if one is found; return an error-pointer (!NULL) if an
>>> - * entity was ready, but the scheduler had insufficient credits to accommodate
>>> - * its job; return NULL, if no ready entity was found.
>>> + * Return an entity if one is found or NULL if no ready entity was found.
>>>    */
>>>   struct drm_sched_entity *
>>>   drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
>>> @@ -213,14 +211,6 @@ drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
>>>             entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
>>>           if (drm_sched_entity_is_ready(entity)) {
>>> -            /* If we can't queue yet, preserve the current entity in
>>> -             * terms of fairness.
>>> -             */
>>> -            if (!drm_sched_can_queue(sched, entity)) {
>>> -                spin_unlock(&rq->lock);
>>> -                return ERR_PTR(-ENOSPC);
>>> -            }
>>> -
>>>               reinit_completion(&entity->entity_idle);
>>>               break;
>>>           }
>>
>