On 13/08/2026 07:42, [email protected] wrote:
Ok, after a lot of A/B testing, it appears that trying your
"drm-intel/drm-sched-fair-fixed" branch, the stuttering is still there. It's definitely
paced differently though, I will attach a video. It's "choppier" when it happens. I
tested in a few different areas / times of day, in case you notice the scenery isn't the same; this
is representative of what I experienced in other places. I will note, that I tried playing 4 times,
3/4 times, shortly after boot. 1/4 times I left my desktop to idle for about 30 minutes while I did
something else, and I didn't encounter the issue at all in 10 minutes. Not sure how significant
that is, or if it's just noise.
https://www.youtube.com/watch?v=GWyIVEuhooM
To make sure I wasn't insane, I went back to the patch Claude gave me, applied
against a clean 7.2.0-rc7. At least again in a few sessions after boot, no
issue. I'll post that exact code below.
Yes, I had a brain fart yesterday and had only pulled the lock out on
the pop side. I have now pushed the updated branch, with both the add
and pop side made symmetric in lock taking aspect.
If you could pull and re-test once more that would be great.
Regards,
Tvrtko
If you need any other info from me let me know.
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -2,6 +2,7 @@
/* Copyright 2015 Advanced Micro Devices, Inc. */
/* Copyright (c) 2025 Valve Corporation */
+#include <linux/moduleparam.h>
#include <linux/rbtree.h>
#include <drm/drm_print.h>
@@ -9,6 +10,32 @@
#include "sched_internal.h"
+/*
+ * Diagnostic counters. Not for submission.
+ *
+ * dbg_pop_stayed - pops where the entity had another job queued and so stayed
+ * in the tree. No save/restore of vruntime occurs.
+ * dbg_pop_left - pops where the entity queue drained, so it left the tree
+ * and drm_sched_entity_save_vruntime() ran. Only this path
+ * arms a later restore.
+ * dbg_add_restore - calls to drm_sched_rq_add_entity(), i.e. restores.
+ * dbg_add_race - restores where the entity was still linked in the tree,
+ * meaning the vruntime being restored is still absolute.
+ *
+ * Incremented under rq->lock, so exact per scheduler and only mildly lossy
+ * when summed across rings. Writable so they can be reset between runs:
+ * echo 0 > /sys/module/gpu_sched/parameters/dbg_pop_left
+ */
+static unsigned long dbg_pop_stayed;
+static unsigned long dbg_pop_left;
+static unsigned long dbg_add_restore;
+static unsigned long dbg_add_race;
+
+module_param(dbg_pop_stayed, ulong, 0644);
+module_param(dbg_pop_left, ulong, 0644);
+module_param(dbg_add_restore, ulong, 0644);
+module_param(dbg_add_race, ulong, 0644);
+
static __always_inline bool
drm_sched_entity_compare_before(struct rb_node *a, const struct rb_node *b)
{
@@ -266,14 +293,40 @@
sched = container_of(rq, typeof(*sched), rq);
spin_lock(&rq->lock);
+ dbg_add_restore++;
+ if (!RB_EMPTY_NODE(&entity->rb_tree_node)) {
+ dbg_add_race++;
+ pr_warn_ratelimited("drm_sched: vruntime race on ring %s (comm
%s)\n",
+ sched->name, current->comm);
+ }
+
if (list_empty(&entity->list)) {
atomic_inc(sched->score);
list_add_tail(&entity->list, &rq->entities);
}
- ts = drm_sched_rq_get_min_vruntime(rq);
- ts = drm_sched_entity_restore_vruntime(entity, ts, rq->head_prio);
- drm_sched_rq_update_tree_locked(entity, rq, ts);
+ /*
+ * Only restore the vruntime if the entity actually left the run queue.
+ *
+ * drm_sched_entity_pop_job() dequeues the last job and only afterwards
+ * calls drm_sched_rq_pop_entity(), which is where the entity is removed
+ * from the tree and drm_sched_entity_save_vruntime() converts its
+ * vruntime to min_vruntime-relative form. A push landing in that window
+ * sees an empty queue, takes the "first job" path to here, and would
+ * restore a vruntime which is still absolute -- adding min_vruntime to
+ * it a second time. If the entity is also the leftmost one,
+ * drm_sched_rq_get_min_vruntime() returns the entity's own vruntime and
+ * the value is doubled, placing it far to the right of the tree where
it
+ * will not be selected again until the run queue catches up.
+ *
+ * A still-linked entity never left, so its vruntime is already absolute
+ * and its tree position is valid. The concurrent pop will update both.
+ */
+ if (RB_EMPTY_NODE(&entity->rb_tree_node)) {
+ ts = drm_sched_rq_get_min_vruntime(rq);
+ ts = drm_sched_entity_restore_vruntime(entity, ts,
rq->head_prio);
+ drm_sched_rq_update_tree_locked(entity, rq, ts);
+ }
spin_unlock(&rq->lock);
spin_unlock(&entity->lock);
@@ -330,11 +383,13 @@
if (next_job) {
ktime_t ts;
+ dbg_pop_stayed++;
ts = drm_sched_entity_update_vruntime(entity);
drm_sched_rq_update_tree_locked(entity, rq, ts);
} else {
ktime_t min_vruntime;
+ dbg_pop_left++;
drm_sched_rq_remove_tree_locked(entity, rq);
min_vruntime = drm_sched_rq_get_min_vruntime(rq);
drm_sched_entity_save_vruntime(entity, min_vruntime);
