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 <[email protected]> > > > Cc: Christian König <[email protected]> > > > Cc: Danilo Krummrich <[email protected]> > > > Cc: Matthew Brost <[email protected]> > > > Cc: Philipp Stanner <[email protected]> > > > --- > > > 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 <[email protected]> > > > > > > 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. > > >
