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

[Philipp Stanner <phasta@mailbox.org>]

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.

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