Re: [RFC 3/3] drm/sched: Disallow initializing entities with no schedulers

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

On 08/01/2026 13:54, Philipp Stanner wrote:
> What's the merge plan for this series? Christian?

It sounds that staged merge would be safest. First two patches could go 
to amd-next and if everything will look fine, I would follow up by 
sending the DRM scheduler patch once amdgpu patches land to drm-next.

Or if DRM scheduler maintainers are happy for the DRM scheduler patch to 
also go via amd-next that is another option.
  > On Wed, 2026-01-07 at 12:43 +0000, Tvrtko Ursulin wrote:
>> Since we have removed the case where amdgpu was initializing entitites
>> with either no schedulers on the list, or with a single NULL scheduler,
>> and there appears no other drivers which rely on this, we can simplify the
>> scheduler by explictly rejecting that early.
>>
>> 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 | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
>> index fe174a4857be..bb7e5fc47f99 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -61,32 +61,27 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>>   			  unsigned int num_sched_list,
>>   			  atomic_t *guilty)
>>   {
>> -	if (!(entity && sched_list && (num_sched_list == 0 || sched_list[0])))
>> +	if (!entity || !sched_list || !num_sched_list || !sched_list[0])
> 
> I personally am a fan of checking integers explicitly against a number,
> which would make the diff a bit more straightforward, too. But I accept
> that like that is common kernel practice.
> 
>>   		return -EINVAL;
>>   
>>   	memset(entity, 0, sizeof(struct drm_sched_entity));
>>   	INIT_LIST_HEAD(&entity->list);
>>   	entity->rq = NULL;
>>   	entity->guilty = guilty;
>> -	entity->num_sched_list = num_sched_list;
>>   	entity->priority = priority;
>>   	entity->last_user = current->group_leader;
>> -	/*
>> -	 * It's perfectly valid to initialize an entity without having a valid
>> -	 * scheduler attached. It's just not valid to use the scheduler before it
>> -	 * is initialized itself.
>> -	 */
>> +	entity->num_sched_list = num_sched_list;
> 
> Why do you move that line downwards? Just leave it where it was?
> num_sched_list isn't changed or anything, so I don't see a logical
> connection to the line below so that grouping would make sense.

It looks completely logical to me to have both lines dealing with the 
same scheduler list, accessing the same input parameter even, next to 
each other:

   entity->num_sched_list = num_sched_list;
   entity->sched_list = num_sched_list > 1 ? sched_list : NULL;

No? In other words, I can respin if you insist but I don't see the need.

Regards,

Tvrtko

> 
> With that:
> Acked-by: Philipp Stanner <phasta@kernel.org>
> 
> 
> P.
> 
>>   	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
>>   	RCU_INIT_POINTER(entity->last_scheduled, NULL);
>>   	RB_CLEAR_NODE(&entity->rb_tree_node);
>>   
>> -	if (num_sched_list && !sched_list[0]->sched_rq) {
>> +	if (!sched_list[0]->sched_rq) {
>>   		/* Since every entry covered by num_sched_list
>>   		 * should be non-NULL and therefore we warn drivers
>>   		 * not to do this and to fix their DRM calling order.
>>   		 */
>>   		pr_warn("%s: called with uninitialized scheduler\n", __func__);
>> -	} else if (num_sched_list) {
>> +	} else {
>>   		/* The "priority" of an entity cannot exceed the number of run-queues of a
>>   		 * scheduler. Protect against num_rqs being 0, by converting to signed. Choose
>>   		 * the lowest priority available.
>