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

[Matthew Brost <matthew.brost@intel.com>]

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 <tvrtko.ursulin@igalia.com>
> Fixes: 2fa4d8e2c109 ("drm/sched: Add fair scheduling policy")
> Suggested-by: Luke.Wildhardt@proton.me # via Claude Opus
> Tested-by: Luke.Wildhardt@proton.me
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <phasta@kernel.org>
> Cc: Pierre-Eric Pelloux-Prayer <pierre-eric.pelloux-prayer@amd.com>
> Cc: Matthew Brost <matthew.brost@intel.com>

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.

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

Matt

> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
> ---
>  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
>