Re: [PATCH] drm/sched: Simplify idle entity check

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

\
On 07/01/2026 14:11, Philipp Stanner wrote:
> Happy 2026, Tvrtko!
> 
> 
> On Wed, 2026-01-07 at 13:44 +0000, Tvrtko Ursulin wrote:
>> drm_sched_entity_is_idle() is called when flushing the entity before
>> destroying it and currently decides on the idle status based either on
>> number of jobs in its queue being zero, or whether the entity is not part
>> of any run-queue.
>>
>> If entity is not part of a run-queue it is implied it can have no jobs in
>> its queue, from which it follows it is redundant to look at the both and
>> we can simplify to only look at the queue.
>>
>> The list_empty() check was added in
>> a875f58e237a ("drm/scheduler: stop setting rq to NULL")
>> where it replaced the entity->rq check which was added in
>> 741f01e636b7 ("drm/scheduler: Avoid using wait_event_killable for dying process (V4)").
>>
>> Since for the submit race involving exiting entities, explicit handling
>> via entity->stopped was added soon after in
>> 62347a33001c ("drm/scheduler: Add stopped flag to drm_sched_entity")
>> it indeed looks we are safe to remove the list_empty() check.
>>
>> This mean we can remove the memory barrier as well and fewer memory
>> barriers the better.
> 
> I am not convinced that this change is worth it.
> 
> Not to long ago we discussed that the spsc_queue should be removed and
> replaced by some sort of list, with proper locks. Christian has agreed
> that this should fly.
> 
> The spsc queue has only 1 user in the kernel and it's super hard to
> understand how it's supposed to work and when any why barriers and
> READ_ONCE's are necessary (that's, of course, also not documented in
> the queue).
> 
> Until that is done I don't really want to touch any of those memory
> barriers..

(I had a branch with spsc gone more than a year ago but I abandoned it 
for now since it contained some other too ambitious changes. So no 
complaints from me. Who is doing it BTW?)

Back to the point - this patch can wait, no problem. To explain the 
context though.

I wanted to get rid of looking at the list_empty here because I have a 
branch which improves the flow for the 1:1 sched:entity drivers.

Why are the two related? If you remember in the fair scheduler series 
all the run-queue stuff is nicely grouped in sched_rq.c and encapsulated 
in the rq API, which made it possible to follow up with virtualizing the 
rq operations.

The yet another relevant thing is the patch I sent this week which 
removes the special case where entity can be initialized with no schedulers.

If we combined all these three pre-requisites, my branch allows the 
fully invariant sched:entity and 1:1:1 sched:rq:entity. Run-queue vfuncs 
for the 1:1 drivers become mostly no-ops (remove and pop entity are not 
needed at all, while add just checks for entity->stopped). So all the 
list and tree management needed by M:N drivers simply does not happen.

In that branch 1:1 entities do not take part in the rq->entities_list 
so, going back to this patch, the list_empty check will be in the way.

Anyway, just for context, I will park this cleanup for now but you can 
mull it over whether the bigger picture sounds interesting to you.

Regards,

Tvrtko

>>
>> While at it, we add READ_ONCE annotation on the entity->stopped check to
>> mark the unlocked read.
> 
> This would effectively legalize the lockless access. But it is illegal
> and undefined behavior. See drm_sched_fini().
> 
> P.
> 
> 
>>
>> 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 | 10 ++--------
>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>> index bb7e5fc47f99..2ed84504cfd6 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -137,14 +137,8 @@ EXPORT_SYMBOL(drm_sched_entity_modify_sched);
>>   
>>   static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity)
>>   {
>> -	rmb(); /* for list_empty to work without lock */
>> -
>> -	if (list_empty(&entity->list) ||
>> -	    spsc_queue_count(&entity->job_queue) == 0 ||
>> -	    entity->stopped)
>> -		return true;
>> -
>> -	return false;
>> +	return spsc_queue_count(&entity->job_queue) == 0 ||
>> +	       READ_ONCE(entity->stopped);
>>   }
>>   
>>   /**
>