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

["Christian König" <christian.koenig@amd.com>]

On 1/12/26 13:49, Philipp Stanner wrote:
> On Mon, 2026-01-12 at 10:29 +0000, Tvrtko Ursulin wrote:
>>
>> 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.
> 
> Works for me.

Sounds like a plan to me as well.

Regards,
Christian.

> 
>>
>> 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.
> 
> Fine by me. Though a little sentence about that cosmetical change in
> the commit message would have made that clearer.
> 
> 
> Greetings
> P.
> 
>>
>> 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.
>>>
>>
>