Re: [PATCH] drm/sched: Remove redundant entity->rq initialization and checks

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

On 03/06/2026 09:37, Philipp Stanner wrote:
> On Tue, 2026-06-02 at 16:33 +0100, Tvrtko Ursulin wrote:
>> Commit
>> 28c5bf28763d ("drm/sched: Disallow initializing entities with no schedulers")
>> failed to notice clearing of entity->rq in drm_sched_entity_init() is now
> 
> By clearing you also mean the setting to NULL?
> 
> I'd just use "initialization" consistently, like in the commit title.

It is initialized properly a bit lower down so clearing is I think more 
accurate.

>> redundant and can be removed.
>>
>> Given that entity->rq can now never be NULL, we also remove two impossible
>> checks, from drm_sched_entity_kill() and drm_sched_entity_flush()
>> respectively.
>>
>> Similarly, we can also remove the !entity->rq check in
>> drm_sched_job_init(). And for the better, given that the error message, if
>> it ever triggered, would have dereferenced the yet un-initialized job->
>> sched (only initialized later in drm_sched_job_arm()). This appears to
>> have been theoretically broken ever since commit
>> 56e449603f0a ("drm/sched: Convert the GPU scheduler to variable number of run-queues")
>> .
>>
>> 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 | 11 ++---------
>>   drivers/gpu/drm/scheduler/sched_main.c   |  9 ---------
>>   2 files changed, 2 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 4ebb513255ed..c51101ec70c1 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -129,7 +129,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>>   		return -ENOMEM;
>>   
>>   	INIT_LIST_HEAD(&entity->list);
>> -	entity->rq = NULL;
> 
> It would seem that has always been redundant because of the memset(0)
> directly above.

True, ever since 1decbf6bb0b4 ("drm/sched: Fix entities with 0 rqs."). 
Just that in this patch the redundant is focusing on:

   entity->rq = NULL;
...
   entity->rq = &sched_list[0]->rq;

Good enough or you want a respin and if so in what flavour?

Regards,

Tvrtko

> 
>>   	entity->guilty = guilty;
>>   	entity->priority = priority;
>>   	entity->last_user = current->group_leader;
>> @@ -280,9 +279,6 @@ void drm_sched_entity_kill(struct drm_sched_entity *entity)
>>   	struct drm_sched_job *job;
>>   	struct dma_fence *prev;
>>   
>> -	if (!entity->rq)
>> -		return;
>> -
>>   	spin_lock(&entity->lock);
>>   	entity->stopped = true;
>>   	drm_sched_rq_remove_entity(entity->rq, entity);
>> @@ -329,14 +325,11 @@ EXPORT_SYMBOL(drm_sched_entity_kill);
>>    */
>>   long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
>>   {
>> -	struct drm_gpu_scheduler *sched;
>> +	struct drm_gpu_scheduler *sched =
>> +		container_of(entity->rq, typeof(*sched), rq);
>>   	struct task_struct *last_user;
>>   	long ret = timeout;
>>   
>> -	if (!entity->rq)
>> -		return 0;
>> -
>> -	sched = container_of(entity->rq, typeof(*sched), rq);
>>   	/*
>>   	 * The client will not queue more jobs during this fini - consume
>>   	 * existing queued ones, or discard them on SIGKILL.
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index 818d3d4434b5..d2ca01b31ee4 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -588,15 +588,6 @@ int drm_sched_job_init(struct drm_sched_job *job,
>>   		       u32 credits, void *owner,
>>   		       uint64_t drm_client_id)
>>   {
>> -	if (!entity->rq) {
>> -		/* This will most likely be followed by missing frames
>> -		 * or worse--a blank screen--leave a trail in the
>> -		 * logs, so this can be debugged easier.
>> -		 */
>> -		dev_err(job->sched->dev, "%s: entity has no rq!\n", __func__);
>> -		return -ENOENT;
>> -	}
>> -
>>   	if (unlikely(!credits)) {
>>   		pr_err("*ERROR* %s: credits cannot be 0!\n", __func__);
>>   		return -EINVAL;
> 
> But overall a very nice cleanup
> 
> 
> P.