Re: [PATCH 1/2] drm/sched: Do not restore unsaved virtual runtime

Fri, 14 Aug 2026 02:58:37 -0700 


On 14/08/2026 09:53, Matthew Brost wrote:

On Fri, Aug 14, 2026 at 08:57:38AM +0100, Tvrtko Ursulin wrote:

Prevent pushing a new job to an entity seeing it being the first in the
queue, and hence entering the drm_sched_rq_add_entity() path, if the pop
side in drm_sched_entity_pop_job() has de-queued the job but not yet
updated the saved virtual time.

We do this by pulling the locked sections out to encompass both the queue
push/pop and corresponding rbtree management.

Signed-off-by: Tvrtko Ursulin <[email protected]>
Fixes: 2fa4d8e2c109 ("drm/sched: Add fair scheduling policy")
Suggested-by: [email protected] # via Claude Opus
Tested-by: [email protected]
Cc: Christian König <[email protected]>
Cc: Danilo Krummrich <[email protected]>
Cc: Philipp Stanner <[email protected]>
Cc: Pierre-Eric Pelloux-Prayer <[email protected]>
Cc: Matthew Brost <[email protected]>


Ugh. Idiom:

"How much longer are we going to keep polishing this turd?"

The revert of FAIR IMO shows it basically time for a rewrite, cough -
DRM dep.



To give any external readers clarity - hypothetical new DRM dep and fair 
policy for the current scheduler address separate and distinct kernel 
drivers.



Or to put it differently, the regression which warranted a revert does 
not apply at all to any of the drivers DRM dep would be the new solution 
for. Better scheduler for drivers not covered by the DRM dep proposal is 
still required should DRM dep happen or not.


Having clarified that...


If we can agree, I'll prioritize, if not we'll be here seemingly
forever.



...for me that's fine albeit unfortunate. I proposed refactoring the 
existing scheduler to split it at the backend submission level by sched 
ops and rq ops, with a future route to split the data structures as 
well, but that was rejected as a direction. So it seems some flavour of 
two schedulers will happen, but will it be DRM dep, or jobq in Rust it's 
up to the (more) interested parties to agree upon.


Regards,

Tvrtko


Cc: Vitaly Prosyak <[email protected]>
---
  drivers/gpu/drm/scheduler/sched_entity.c |  8 +++++++-
  drivers/gpu/drm/scheduler/sched_rq.c     | 20 +++++++++-----------
  2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
b/drivers/gpu/drm/scheduler/sched_entity.c
index a4a7efdbf229..673ca9cbf362 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -559,9 +559,10 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct 
drm_sched_entity *entity)
         */
        smp_wmb();

  
+	spin_lock(&entity->lock);

        spsc_queue_pop(&entity->job_queue);
-
        drm_sched_rq_pop_entity(entity);
+       spin_unlock(&entity->lock);

  
  	/* Jobs and entities might have different lifecycles. Since we're

         * removing the job from the entities queue, set the jobs entity pointer
@@ -647,6 +648,9 @@ void drm_sched_entity_push_job(struct drm_sched_job 
*sched_job)
         * Make sure to set the submit_ts first, to avoid a race.
         */
        sched_job->submit_ts = submit_ts = ktime_get();
+
+       spin_lock(&entity->lock);
+
        first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);

  
  	/* first job wakes up scheduler */

@@ -657,5 +661,7 @@ void drm_sched_entity_push_job(struct drm_sched_job 
*sched_job)
                if (sched)
                        drm_sched_wakeup(sched);
        }
+
+       spin_unlock(&entity->lock);
  }
  EXPORT_SYMBOL(drm_sched_entity_push_job);
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c 
b/drivers/gpu/drm/scheduler/sched_rq.c
index 0464d324d98d..23f46ec610e7 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -257,19 +257,17 @@ static ktime_t drm_sched_entity_get_job_ts(struct 
drm_sched_entity *entity)
  struct drm_gpu_scheduler *
  drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
  {
+       struct drm_sched_rq *rq = entity->rq;
        struct drm_gpu_scheduler *sched;
-       struct drm_sched_rq *rq;

  
  	/* Add the entity to the run queue */

-       spin_lock(&entity->lock);
+       lockdep_assert_held(&entity->lock);
+
        if (entity->stopped) {
-               spin_unlock(&entity->lock);
-
                DRM_ERROR("Trying to push to a killed entity\n");
                return NULL;
        }

  
-	rq = entity->rq;

        spin_lock(&rq->lock);
        sched = rq->sched;

  
@@ -289,7 +287,6 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)

        drm_sched_rq_update_fifo_locked(entity, rq, ts);

  
  	spin_unlock(&rq->lock);

-       spin_unlock(&entity->lock);

  
  	return sched;

  }
@@ -343,16 +340,17 @@ drm_sched_rq_next_rr_ts(struct drm_sched_rq *rq,
   */
  void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
  {
+       struct drm_sched_rq *rq = entity->rq;
        struct drm_sched_job *next_job;
-       struct drm_sched_rq *rq;
+
+       lockdep_assert_held(&entity->lock);
+
+       spin_lock(&rq->lock);

  
  	/*

         * Update the entity's location in the min heap according to
         * the timestamp of the next job, if any.
         */
-       spin_lock(&entity->lock);
-       rq = entity->rq;
-       spin_lock(&rq->lock);
        next_job = drm_sched_entity_queue_peek(entity);
        if (next_job) {
                ktime_t ts;
@@ -375,8 +373,8 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity 
*entity)
                        drm_sched_entity_save_vruntime(entity, min_vruntime);
                }
        }
+
        spin_unlock(&rq->lock);
-       spin_unlock(&entity->lock);
  }

  
  /**

--
2.54.0

























					Reply via email to