On 02/11/2026 10:46, Philipp Stanner wrote:
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?
Yep.
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?
It could; the minimal diff would be:
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index 7be0387bac3c..0dec0d22c997 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -160,6 +160,8 @@ int drm_sched_entity_init(struct drm_sched_entity
*entity,
(s32)sched_list[0]->num_user_rqs - 1,
(s32)DRM_SCHED_PRIORITY_KERNEL);
entity->priority = p;
+ entity->rq_priority = drm_sched_policy ==
DRM_SCHED_POLICY_FAIR ?
+ DRM_SCHED_PRIORITY_KERNEL
: priority;
}
entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
}
But that leaves the "ugly" sequence of assigning prio back and forth,
and even adds more ugliness to it by repeating the drm_sched_policy
conditional:
entity->priority = priority;
...
entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
DRM_SCHED_PRIORITY_KERNEL : priority;
...
} else {
enum drm_sched_priority p = entity->priority;
...
if (p >= sched_list[0]->num_user_rqs) {
...
entity->priority = p;
+ entity->rq_priority = drm_sched_policy ==
DRM_SCHED_POLICY_FAIR ?
+ DRM_SCHED_PRIORITY_KERNEL : p;
So I thought we can tidy in up in one go and end up with a completely
streamlined and localised:
} else if (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 don't think there was any churn to this area so backporting shouldn't
be a problem.
Regards,
Tvrtko
entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
- }
init_completion(&entity->entity_idle);
