[PATCH v5 07/16] drm/sched: Implement RR via FIFO

[Tvrtko Ursulin]

On 04/07/2025 14:18, Maíra Canal wrote:
> Hi Tvrtko,
> 
> On 23/06/25 09:27, Tvrtko Ursulin wrote:
>> Round-robin being the non-default policy and unclear how much it is used,
>> we can notice that it can be implemented using the FIFO data 
>> structures if
>> we only invent a fake submit timestamp which is monotonically increasing
>> inside drm_sched_rq instances.
>>
>> So instead of remembering which was the last entity the scheduler worker
>> picked, we can bump the picked one to the bottom of the tree, achieving
>> the same round-robin behaviour.
>>
>> Advantage is that we can consolidate to a single code path and remove a
>> bunch of code. Downside is round-robin mode now needs to lock on the job
>> pop path but that should not be visible.
>>
>> 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_entity.c | 45 ++++++++------
>>   drivers/gpu/drm/scheduler/sched_main.c   | 76 ++----------------------
>>   include/drm/gpu_scheduler.h              |  5 +-
>>   3 files changed, 36 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/ 
>> drm/scheduler/sched_entity.c
>> index 0bdf52e27461..e69176765697 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -470,9 +470,19 @@ drm_sched_job_dependency(struct drm_sched_job *job,
>>       return NULL;
>>   }
>> +static ktime_t
>> +drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
>> +{
>> +    lockdep_assert_held(&rq->lock);
>> +
>> +    rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
>> +
>> +    return rq->rr_deadline;
>> +}
>> +
>>   struct drm_sched_job *drm_sched_entity_pop_job(struct 
>> drm_sched_entity *entity)
>>   {
>> -    struct drm_sched_job *sched_job;
>> +    struct drm_sched_job *sched_job, *next_job;
>>       sched_job = drm_sched_entity_queue_peek(entity);
>>       if (!sched_job)
>> @@ -507,21 +517,22 @@ struct drm_sched_job 
>> *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>>        * Update the entity's location in the min heap according to
>>        * the timestamp of the next job, if any.
>>        */
>> -    if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) {
>> -        struct drm_sched_job *next;
>> +    next_job = drm_sched_entity_queue_peek(entity);
>> +    if (next_job) {
>> +        struct drm_sched_rq *rq;
>> +        ktime_t ts;
>> -        next = drm_sched_entity_queue_peek(entity);
>> -        if (next) {
>> -            struct drm_sched_rq *rq;
>> +        if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> +            ts = next_job->submit_ts;
>> +        else
>> +            ts = drm_sched_rq_get_rr_deadline(rq);
> 
> I know that this chunk is going to be deleted in the next patches.
> However, to keep things bisectable, I believe you need to set `rq =
> entity->rq;` before calling `drm_sched_rq_get_rr_deadline(rq)`.
> Otherwise, rq is undefined.
> 
> Moreover, IIUC you need to hold rq->lock to call
> `drm_sched_rq_get_rr_deadline()`.

Well spotted! Must have been a rebase mistake. I've fixed it locally.

> 
>> -            spin_lock(&entity->lock);
>> -            rq = entity->rq;
>> -            spin_lock(&rq->lock);
>> -            drm_sched_rq_update_fifo_locked(entity, rq,
>> -                            next->submit_ts);
>> -            spin_unlock(&rq->lock);
>> -            spin_unlock(&entity->lock);
>> -        }
>> +        spin_lock(&entity->lock);
>> +        rq = entity->rq;
>> +        spin_lock(&rq->lock);
>> +        drm_sched_rq_update_fifo_locked(entity, rq, ts);
>> +        spin_unlock(&rq->lock);
>> +        spin_unlock(&entity->lock);
>>       }
>>       /* Jobs and entities might have different lifecycles. Since we're
> 
> [...]
> 
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index e62a7214e052..9f8b3b78d24d 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -239,8 +239,7 @@ struct drm_sched_entity {
>>    * struct drm_sched_rq - queue of entities to be scheduled.
>>    *
>>    * @sched: the scheduler to which this rq belongs to.
>> - * @lock: protects @entities, @rb_tree_root and @current_entity.
>> - * @current_entity: the entity which is to be scheduled.
>> + * @lock: protects @entities, @rb_tree_root and @rr_deadline.
> 
> Could you add an entry for @rr_deadline?

Ditto.

Regards,

Tvrtko

>>    * @entities: list of the entities to be scheduled.
>>    * @rb_tree_root: root of time based priority queue of entities for 
>> FIFO scheduling
>>    *
>> @@ -253,7 +252,7 @@ struct drm_sched_rq {
>>       spinlock_t            lock;
>>       /* Following members are protected by the @lock: */
>> -    struct drm_sched_entity        *current_entity;
>> +    ktime_t                rr_deadline;
>>       struct list_head        entities;
>>       struct rb_root_cached        rb_tree_root;
>>   };
>