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

[Philipp Stanner <phasta@mailbox.org>]

What's the merge plan for this series? Christian?

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.

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.