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

[Maíra Canal]

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()`.

>   
> -			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?

Best Regards,
- Maíra

>    * @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;
>   };