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