On Wed, 2026-10-21 at 10:42 +0100, Tvrtko Ursulin wrote: > When the out of range priority at entity init time is clamped to the > number of run queues in the supplied scheduler the internal run-queue > priority needs to be adjusted as well. > > There is no impact since none of the drivers hit the clamping path, but > we still need to address it by moving the entity->rq_priority assignment > to after the clamping is done. > > Note that a lot of this will hopefully get removed once again when the > single run-queue simplification lands.
So this is no existing bug out there, but merely for formal correctness? > > Signed-off-by: Tvrtko Ursulin <[email protected]> > Fixes: 2fa4d8e2c109 ("drm/sched: Add fair scheduling policy") > Cc: Christian König <[email protected]> > Cc: Danilo Krummrich <[email protected]> > Cc: Matthew Brost <[email protected]> > Cc: Philipp Stanner <[email protected]> > Cc: <[email protected]> # v7.2+ > --- > drivers/gpu/drm/scheduler/sched_entity.c | 27 +++++++++++------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/scheduler/sched_entity.c > b/drivers/gpu/drm/scheduler/sched_entity.c > index 7be0387bac3c..e12709b7bd0c 100644 > --- a/drivers/gpu/drm/scheduler/sched_entity.c > +++ b/drivers/gpu/drm/scheduler/sched_entity.c > @@ -130,10 +130,7 @@ int drm_sched_entity_init(struct drm_sched_entity > *entity, > > INIT_LIST_HEAD(&entity->list); > entity->guilty = guilty; > - entity->priority = priority; > entity->last_user = current->group_leader; > - entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ? > - DRM_SCHED_PRIORITY_KERNEL : priority; > entity->num_sched_list = num_sched_list; > entity->sched_list = num_sched_list > 1 ? sched_list : NULL; > RB_CLEAR_NODE(&entity->rb_tree_node); > @@ -144,25 +141,25 @@ int drm_sched_entity_init(struct drm_sched_entity > *entity, > * not to do this and to fix their DRM calling order. > */ > pr_warn("%s: called with uninitialized scheduler\n", __func__); > - } else { > - enum drm_sched_priority p = entity->priority; > - > + } else if (priority >= sched_list[0]->num_user_rqs) { > /* > * 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. > */ > - if (p >= sched_list[0]->num_user_rqs) { > - dev_err(sched_list[0]->dev, "entity with out-of-bounds > priority:%u num_user_rqs:%u\n", > - p, sched_list[0]->num_user_rqs); > - p = max_t(s32, > - (s32)sched_list[0]->num_user_rqs - 1, > - (s32)DRM_SCHED_PRIORITY_KERNEL); > - entity->priority = p; > - } > + dev_err(sched_list[0]->dev, "entity with out-of-bounds > priority:%u num_user_rqs:%u\n", > + priority, sched_list[0]->num_user_rqs); > + priority = max_t(s32, > + (s32)sched_list[0]->num_user_rqs - 1, > + (s32)DRM_SCHED_PRIORITY_KERNEL); > + } > + > + entity->priority = priority; > + entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ? > + DRM_SCHED_PRIORITY_KERNEL : priority; I guess my main question regarding the code is why you moved those lines? > + if (sched_list[0]->sched_rq) Couldn't that remain in a terminating `else`, reducing the git churn? > entity->rq = sched_list[0]->sched_rq[entity->rq_priority]; > - } > > init_completion(&entity->entity_idle); >
