Re: [PATCH 05/28] drm/sched: Consolidate entity run queue management

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 14/10/2025 09:52, Philipp Stanner wrote:
> On Tue, 2025-10-14 at 08:26 +0100, Tvrtko Ursulin wrote:
>>
>> On 14/10/2025 07:53, Philipp Stanner wrote:
>>> On Sat, 2025-10-11 at 15:19 +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 10/10/2025 11:49, Philipp Stanner wrote:
>>>>> On Wed, 2025-10-08 at 09:53 +0100, Tvrtko Ursulin wrote:
>>>>>> Move the code dealing with entities entering and exiting run queues to
>>>>>> helpers to logically separate it from jobs entering and exiting entities.
>>>>>>
>>>>>> 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_entity.c   | 64 ++-------------
>>>>>>     drivers/gpu/drm/scheduler/sched_internal.h |  8 +-
>>>>>>     drivers/gpu/drm/scheduler/sched_main.c     | 95 +++++++++++++++++++---
>>>>>>     3 files changed, 91 insertions(+), 76 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> index 4852006f2308..7a0a52ba87bf 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>>>> @@ -456,24 +456,9 @@ drm_sched_job_dependency(struct drm_sched_job *job,
>>>>>>     	return NULL;
>>>>>>     }
>>>>>>     
>>>>>> -static ktime_t
>>>>>> -drm_sched_rq_get_rr_ts(struct drm_sched_rq *rq, struct drm_sched_entity *entity)
>>>>>> -{
>>>>>> -	ktime_t ts;
>>>>>> -
>>>>>> -	lockdep_assert_held(&entity->lock);
>>>>>> -	lockdep_assert_held(&rq->lock);
>>>>>> -
>>>>>> -	ts = ktime_add_ns(rq->rr_ts, 1);
>>>>>> -	entity->rr_ts = ts;
>>>>>> -	rq->rr_ts = ts;
>>>>>> -
>>>>>> -	return ts;
>>>>>> -}
>>>>>> -
>>>>>>     struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>>>>>>     {
>>>>>> -	struct drm_sched_job *sched_job, *next_job;
>>>>>> +	struct drm_sched_job *sched_job;
>>>>>
>>>>> `next_job` has been added in a previous job. Have you tried whether
>>>>> patch-order can be reversed?
>>>>>
>>>>> Just asking; I don't want to cause unnecessary work here
>>>>
>>>> You are correct that there would be some knock on effect on a few other
>>>> patches in the series but it is definitely doable. Because for certain
>>>> argument can be made it would be logical to have it like that. Both this
>>>> patch and "drm/sched: Move run queue related code into a separate file"
>>>> would be then moved ahead of "drm/sched: Implement RR via FIFO". If you
>>>> prefer it like that I can reshuffle no problem.
>>>
>>> I mean, it seems to make the overall git diff smaller, which is nice?
>>>
>>> If you don't see a significant reason against it, I'd say it's a good
>>> idea.
>>
>> Okay deal. It isn't anything significant, just re-ordering patches with
>> compile testing patches to ensure every step still builds.

Completed locally.

>>>>>>     
>>>>>>     	sched_job = drm_sched_entity_queue_peek(entity);
>>>>>>     	if (!sched_job)
>>>>>> @@ -502,26 +487,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>>>>>>     
>>>>>>     	spsc_queue_pop(&entity->job_queue);
>>>>>>     
>>>>>> -	/*
>>>>>> -	 * Update the entity's location in the min heap according to
>>>>>> -	 * the timestamp of the next job, if any.
>>>>>> -	 */
>>>>>> -	next_job = drm_sched_entity_queue_peek(entity);
>>>>>> -	if (next_job) {
>>>>>> -		struct drm_sched_rq *rq;
>>>>>> -		ktime_t ts;
>>>>>> -
>>>>>> -		spin_lock(&entity->lock);
>>>>>> -		rq = entity->rq;
>>>>>> -		spin_lock(&rq->lock);
>>>>>> -		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>>>>>> -			ts = next_job->submit_ts;
>>>>>> -		else
>>>>>> -			ts = drm_sched_rq_get_rr_ts(rq, entity);
>>>>>> -		drm_sched_rq_update_fifo_locked(entity, rq, ts);
>>>>>> -		spin_unlock(&rq->lock);
>>>>>> -		spin_unlock(&entity->lock);
>>>>>> -	}
>>>>>> +	drm_sched_rq_pop_entity(entity);
>>>>>>     
>>>>>>     	/* Jobs and entities might have different lifecycles. Since we're
>>>>>>     	 * removing the job from the entities queue, set the jobs entity pointer
>>>>>> @@ -611,30 +577,10 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>>>>>>     	/* first job wakes up scheduler */
>>>>>>     	if (first) {
>>>>>>     		struct drm_gpu_scheduler *sched;
>>>>>> -		struct drm_sched_rq *rq;
>>>>>>     
>>>>>> -		/* Add the entity to the run queue */
>>>>>> -		spin_lock(&entity->lock);
>>>>>> -		if (entity->stopped) {
>>>>>> -			spin_unlock(&entity->lock);
>>>>>> -
>>>>>> -			DRM_ERROR("Trying to push to a killed entity\n");
>>>>>> -			return;
>>>>>> -		}
>>>>>> -
>>>>>> -		rq = entity->rq;
>>>>>> -		sched = rq->sched;
>>>>>> -
>>>>>> -		spin_lock(&rq->lock);
>>>>>> -		drm_sched_rq_add_entity(rq, entity);
>>>>>> -		if (drm_sched_policy == DRM_SCHED_POLICY_RR)
>>>>>> -			submit_ts = entity->rr_ts;
>>>>>> -		drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
>>>>>> -
>>>>>> -		spin_unlock(&rq->lock);
>>>>>> -		spin_unlock(&entity->lock);
>>>>>> -
>>>>>> -		drm_sched_wakeup(sched);
>>>>>> +		sched = drm_sched_rq_add_entity(entity, submit_ts);
>>>>>> +		if (sched)
>>>>>> +			drm_sched_wakeup(sched);
>>>>>>     	}
>>>>>>     }
>>>>>>     EXPORT_SYMBOL(drm_sched_entity_push_job);
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h
>>>>>> index 7ea5a6736f98..8269c5392a82 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_internal.h
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_internal.h
>>>>>> @@ -12,13 +12,11 @@ extern int drm_sched_policy;
>>>>>>     
>>>>>>     void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
>>>>>>     
>>>>>> -void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>>>>>> -			     struct drm_sched_entity *entity);
>>>>>> +struct drm_gpu_scheduler *
>>>>>> +drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts);
>>>>>>     void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>>>>>>     				struct drm_sched_entity *entity);
>>>>>> -
>>>>>> -void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
>>>>>> -				     struct drm_sched_rq *rq, ktime_t ts);
>>>>>> +void drm_sched_rq_pop_entity(struct drm_sched_entity *entity);
>>>>>>     
>>>>>>     void drm_sched_entity_select_rq(struct drm_sched_entity *entity);
>>>>>>     struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity);
>>>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> index 8e62541b439a..e5d02c28665c 100644
>>>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>>>> @@ -151,9 +151,9 @@ static void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
>>>>>>     	}
>>>>>>     }
>>>>>>     
>>>>>> -void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
>>>>>> -				     struct drm_sched_rq *rq,
>>>>>> -				     ktime_t ts)
>>>>>> +static void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
>>>>>> +					    struct drm_sched_rq *rq,
>>>>>> +					    ktime_t ts)
>>>>>>     {
>>>>>>     	/*
>>>>>>     	 * Both locks need to be grabbed, one to protect from entity->rq change
>>>>>> @@ -191,22 +191,45 @@ static void drm_sched_rq_init(struct drm_sched_rq *rq,
>>>>>>     /**
>>>>>>      * drm_sched_rq_add_entity - add an entity
>>>>>>      *
>>>>>> - * @rq: scheduler run queue
>>>>>>      * @entity: scheduler entity
>>>>>> + * @ts: submission timestamp
>>>>>>      *
>>>>>>      * Adds a scheduler entity to the run queue.
>>>>>> + *
>>>>>> + * Returns a DRM scheduler pre-selected to handle this entity.
>>>>>>      */
>>>>>> -void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>>>>>> -			     struct drm_sched_entity *entity)
>>>>>> +struct drm_gpu_scheduler *
>>>>>> +drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
>>>>>>     {
>>>>>
>>>>> I'm not sure if it's a good idea to have the scheduler returned from
>>>>> that function. That doesn't make a whole lot of sense semantically.
>>>>>
>>>>> At the very least the function's docstring, maybe even its name, should
>>>>> be adjusted to detail why this makes sense. The commit message, too.
>>>>> It's not trivially understood.
>>>>>
>>>>> I think I get why it's being done, but writing it down black on white
>>>>> gives us something to grasp.
>>>>>
>>>>> Sth like "adds an entity to a runqueue, selects to appropriate
>>>>> scheduler and returns it for the purpose of XYZ"
>>>>
>>>> Yeah. Remeber your unlocked rq access slide and the discussion around it?
>>>
>>> Sure. Is that related, though? The slide was about many readers being
>>> totally unlocked. The current drm_sched_entity_push_job() locks readers
>>> correctly if I'm not mistaken.
>>>
>>>>
>>>> Currently we have this:
>>>>
>>>> drm_sched_entity_push_job()
>>>> {
>>>> ...
>>>> 		spin_lock(&entity->lock);
>>>> ...
>>>> 		rq = entity->rq;
>>>> 		sched = rq->sched;
>>>> ...
>>>> 		spin_unlock(&rq->lock);
>>>> 		spin_unlock(&entity->lock);
>>>>
>>>> 		drm_sched_wakeup(sched);
>>>>
>>>> Ie. we know entity->rq and rq->sched are guaranteed to be stable and
>>>> present at this point because job is already in the queue and
>>>> drm_sched_entity_select_rq() guarantees that.
>>>>
>>>> In this patch I moved all this block into drm_sched_rq_add_entity() but
>>>> I wanted to leave drm_sched_wakeup() outside. Because I thought it is
>>>> not the job of the run queue handling, and semantically the logic was
>>>> "only once added to the entity we know the rq and scheduler for
>>>> certain". That would open the door for future improvements and late
>>>> rq/scheduler selection.
>>>>
>>>> But now I think it is premature and it would be better I simply move the
>>>> wakekup inside drm_sched_rq_add_entity() together with all the rest.
>>>>
>>>> Does that sound like a plan for now?
>>>
>>> Hmmm. What I'm wondering most about if it really is a good idea to have
>>> drm_sched_wakeup() in rq_add_entity().
>>>
>>> Do you think that makes semantically more sense than just reading:
>>>
>>> drm_sched_entity_push_job()
>>> {
>>>      foo
>>>      bar
>>>      more_foo
>>>
>>>      /* New job was added. Right time to wake up scheduler. */
>>>      drm_sched_wakeup();
>>
>> Problem here always is you need a sched pointer so question is simply
>> how and where to get it.
>>
>>> I think both can make sense, but the above / current version seems to
>>> make more sense to me.
>>
>> Current as in this patch or current as in the upstream codebase?
>>
>> In all cases the knowledge it is safe to use sched after unlocking is
>> implicit.
>>
>> I see only two options:
>>
>> current)
>>
>> drm_sched_entity_push_job()
>> {
>> ...
>> 	spin_unlock(&rq->lock);
>> 	spin_unlock(&entity->lock);
>>
>> 	drm_sched_wakeup(sched);
>>
>> a)
>>
>> drm_sched_entity_push_job()
>> {
>> ...
>> 	sched = drm_sched_rq_add_entity(entity, submit_ts);
>> 	if (sched)
>> 		drm_sched_wakeup(sched);
>>
>> b)
>>
>> drm_sched_rq_add_entity()
>> {
>> ...
>> 	spin_unlock(&rq->lock);
>> 	spin_unlock(&entity->lock);
>>
>> 	drm_sched_wakeup(sched);
>>
>>
>> drm_sched_entity_push_job()
>> {
>> ...
>> 	drm_sched_rq_add_entity(entity, submit_ts);
>>
>>
>> b) is the same as today, a) perhaps a bit premature. Which do you prefer?
> 
> Alright, I looked through everything now.
> 
> The thing is just that I believe that it's a semantically confusing and
> unclean concept of having drm_sched_rq_add_entity() return a scheduler
> – except for when the entity is stopped. Then "there is no scheduler"
> actually means "there is a scheduler, but that entity is stopped"
> 
> In an ideal world:
> 
> a) drm_sched_entity_push_job() wakes up the scheduler (as in your code,
> and as in the current mainline code) and
> 
> b) drm_sched_entity_push_job() is the one who checks whether the entity
> is stopped. rq_add_entity() should just, well, add an entity to a
> runqueue.
> 
> Option b) then would need locks again and could race. So that's not so
> cool.
> 
> Possible solutions I can see is:
> 
> 1. Have drm_sched_rq_add_entity() return an ERR_PTR instead of NULL.

Maybe I am misunderstanding the idea, but what would be the benefit of 
this option?

To clarify, I have:

drm_sched_rq_add_entity()
{
...
	if (entity->stopped) {
...
		return NULL;

drm_sched_entity_push_job()
{
...
		sched = drm_sched_rq_add_entity(entity);
		if (sched)
			drm_sched_wakeup(sched);

And you propose:

drm_sched_rq_add_entity()
{
...
	if (entity->stopped) {
...
		return ERR_PTR(-ESOMETHING);

drm_sched_entity_push_job()
{
...
		sched = drm_sched_rq_add_entity(entity);
		if (!IS_ERR(sched))
			drm_sched_wakeup(sched);


?

> 2. Rename rq_add_entity()

You mean to something signify it is also doing the wakeup? Or simply 
drm_sched_rq_add_first_entity()?

> 3. Potentially leave it as is? I guess that doesn't work for your rq-
> simplification?

Leave drm_sched_wakeup in push job? Yeah that doesn't work for moving 
the rq handling into own helpers.
  > Option 1 would almost be my preference. What do you think?

Lets see if I understand the option 1. I am fine with that one as 
described, and also with option 2.

Regards,

Tvrtko