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

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

On 12/01/2026 12:51, Christian König wrote:
> 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.

Would you be able to test the first two patches with the AMD test suite? 
Or put it into amd-staging-next if it would be automatically validated 
there?

Or even all three patches.

On the overall I am keen to get a clear picture whether last patch will 
be a go or no-go because my fair scheduler rewrite conflicts with it.

Regards,

Tvrtko

>>> 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.
>>>>
>>>
>>
>