On 02/11/2026 10:42, Philipp Stanner wrote:
On Wed, 2026-10-21 at 10:42 +0100, Tvrtko Ursulin wrote:
Another consequence of the recent revert of
77a6809f1dc3 ("drm/sched: Remove FIFO and RR and simplify to a single run
queue")
is that after the revert we need to adjust entity->rq_priority when
runtime priority changes are requested.
Only amdgpu does that so the impact is minimal.
Also considering dynamic
priority changes are largely broken there, due currently broken entity
scheduler list handling where the special case of a single scheduler can
never change it's run-queue. And coupled with the fact for the rest a two-
tier approach of drm_sched_entity_set_priority() and
drm_sched_entity_modify_sched() is used, where later directly maps to
different hardware queue priority by replacing the set of schedulers
completely.
Hence the regression is limited to rings which have multiple instances
other than compute and graphics, which would be SDMA only, for which I am
not sure any userspace actually does dynamic priority changes in the first
place.
I can't fully grasp what the bug actually is. So is it that we forgot
to adjust the priority in the CFS series, or is it that the partial
revert made this necessary?
Partial revert makes this necessary, albeit mostly on paper. In reality
it is complicated. Only amdgpu calls it, plus, for single scheduler IP
blocks there is not bug because of the "single scheduler special case"
run-queue cannot change at all (bug I am trying to address in that
different patch, and which also got re-uncovered due the partial revert
of the fair series), while in the multiple scheduler case there is no
bug because amdgpu pairs drm_sched_entity_set_priority() with
drm_sched_entity_modify_sched(), which actually changes the priority in
the driver. This leaves SDMA as the only potential place where there can
be a regression, and there I am not sure those do dynamic priority
changes from the userspace side in the first place. But theorethicaly,
from the UAPI point of view, it could be a regression so needs to be fixed.
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 | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
b/drivers/gpu/drm/scheduler/sched_entity.c
index e12709b7bd0c..03e68b4102d5 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -456,6 +456,8 @@ void drm_sched_entity_set_priority(struct drm_sched_entity
*entity,
{
spin_lock(&entity->lock);
entity->priority = priority;
+ if (drm_sched_policy != DRM_SCHED_POLICY_FAIR)
+ entity->rq_priority = priority;
spin_unlock(&entity->lock);
So is there always a bug, or only for FAIR?
With FIFO and RR, on amdgpu, SDMA rings, only if more than one, *if*
there exists userspace which does dynamic priority changes (calls
DRM_IOCTL_AMDGPU_SCHED / AMDGPU_SCHED_OP_*_PRIORITY_OVERRIDE) on
entities using those SDMA engines directly. All clear now? :))
Regards,
Tvrtko
}
EXPORT_SYMBOL(drm_sched_entity_set_priority);
