[PATCH 0/7] drm/sched: Replace SPSC queue with a list and spinlock

[PATCH 0/7] drm/sched: Replace SPSC queue with a list and spinlock

[Tvrtko Ursulin]

[First four patches are just my queue to avoid conflicts. I am still hoping I
can bring them in a state where all is good since all these together simplify
things.]

So as requested by Philipp and as the title says - replacing the SPSC queue with
a list and a spinlock. A first cut which passes unit tests and smoke testing on
AMD.

But it wasn't easy, since we can't really get away from some of the deeply
ingrained scheduler design points.

Patches 5 and 6 are where the action is at so please focus there first. Please
have a look and see what you think.

Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>

Tvrtko Ursulin (7):
  drm/sched: Disallow initializing entities with un-initialized
    schedulers
  drm/sched: Ignore stopped entity attempt to change its run-queue
  drm/sched: Replace completion with a flush
  drm/sched: Remove the single scheduler special case
  drm/amdgpu: Rework the amdgpu_job_stop_all_jobs_on_sched() hack
  drm/sched: Replace the SPSC queue with a list and a spinlock
  drm/sched: Address the fence_seq fixme

 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c       | 110 ++++++++++---
 .../gpu/drm/scheduler/gpu_scheduler_trace.h   |   2 +-
 drivers/gpu/drm/scheduler/sched_entity.c      | 153 +++++++++++-------
 drivers/gpu/drm/scheduler/sched_fence.c       |   6 +-
 drivers/gpu/drm/scheduler/sched_internal.h    |  70 ++++----
 drivers/gpu/drm/scheduler/sched_main.c        |  56 ++-----
 drivers/gpu/drm/scheduler/sched_rq.c          |  77 +++++++--
 .../gpu/drm/scheduler/tests/tests_scheduler.c |   2 +-
 include/drm/gpu_scheduler.h                   |  85 +++++-----
 include/drm/spsc_queue.h                      | 124 --------------
 10 files changed, 349 insertions(+), 336 deletions(-)
 delete mode 100644 include/drm/spsc_queue.h

-- 
2.56.0

[PATCH 1/7] drm/sched: Disallow initializing entities with un-initialized schedulers

[Tvrtko Ursulin]

Simplify the code and theoretical runtime combinations by disallowing
initializing entities with un-initialized schedulers.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
I do not *think* any of the drivers relies on this inverted initialization
order but it needs more eyes for sure.
---
 drivers/gpu/drm/scheduler/sched_entity.c | 30 +++++++++++++++---------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 03e68b4102d5..2844049d4c66 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -119,9 +119,16 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 			  unsigned int num_sched_list,
 			  atomic_t *guilty)
 {
-	if (!entity || !sched_list || !num_sched_list || !sched_list[0])
+	unsigned int i;
+
+	if (!entity || !sched_list || !num_sched_list)
 		return -EINVAL;
 
+	for (i = 0; i < num_sched_list; i++) {
+		if (!sched_list[i] || !sched_list[i]->sched_rq)
+			return -EINVAL;
+	}
+
 	memset(entity, 0, sizeof(struct drm_sched_entity));
 
 	entity->stats = drm_sched_entity_stats_new();
@@ -135,13 +142,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
 	RB_CLEAR_NODE(&entity->rb_tree_node);
 
-	if (!sched_list[0]->sched_rq) {
-		/* Since every entry covered by num_sched_list
-		 * should be non-NULL and therefore we warn drivers
-		 * not to do this and to fix their DRM calling order.
-		 */
-		pr_warn("%s: called with uninitialized scheduler\n", __func__);
-	} else if (priority >= sched_list[0]->num_user_rqs) {
+	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,
@@ -158,8 +159,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->priority = priority;
 	entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
 			      DRM_SCHED_PRIORITY_KERNEL : priority;
-	if (sched_list[0]->sched_rq)
-		entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
+	entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
 
 	init_completion(&entity->entity_idle);
 
@@ -192,7 +192,15 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
 				    struct drm_gpu_scheduler **sched_list,
 				    unsigned int num_sched_list)
 {
-	WARN_ON(!num_sched_list || !sched_list);
+	unsigned int i;
+
+	if (WARN_ON(!num_sched_list || !sched_list))
+		return;
+
+	for (i = 0; i < num_sched_list; i++) {
+		if (WARN_ON(!sched_list[i] || !sched_list[i]->sched_rq))
+			return;
+	}
 
 	spin_lock(&entity->lock);
 	entity->sched_list = sched_list;
-- 
2.56.0

[PATCH 2/7] drm/sched: Ignore stopped entity attempt to change its run-queue

[Tvrtko Ursulin]

Jobs submitted to a stopped entity do not go anywhere due how they are
handled in drm_sched_rq_add_entity() so there is no point to allow them
to change their run-queue. We can simply bail out early which makes
entity->stopped a clearer boundary than it currently is.

This is completely safe because it is always guaranteed entity->rq is
valid if drm_sched_entity_init() succeeded.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 2844049d4c66..a18f637f9e99 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -594,13 +594,16 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 		return;
 
 	spin_lock(&entity->lock);
+
+	/* Cannot submit to stopped entity anyway so bail out early. */
+	if (entity->stopped)
+		goto out;
+
 	fence = entity->last_scheduled;
 
 	/* stay on the same engine if the previous job hasn't finished */
-	if (fence && !dma_fence_is_signaled(fence)) {
-		spin_unlock(&entity->lock);
-		return;
-	}
+	if (fence && !dma_fence_is_signaled(fence))
+		goto out;
 
 	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
 	rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
@@ -612,6 +615,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	if (entity->num_sched_list == 1)
 		entity->sched_list = NULL;
 
+out:
 	spin_unlock(&entity->lock);
 }
 
-- 
2.56.0

[PATCH 3/7] drm/sched: Replace completion with a flush

[Tvrtko Ursulin]

Due the scheduler locking design, and the inability to always lock both
the entity and the run-queue in the consistent order, a completion exists
which effectively marks the entity as in use from a call path which is not
able to lock it.

When entity is selected from the run job worker, its completion is marked
as non-idle all until the code is sure it will not be dereferencing it any
more, at which point it signals it as idle, releasing the potential
parallel cleanup path.

We can remove the need for this completion by implementing the identical
guarantee by simply flushing the run job work from the cleanup path, after
having removed the entity from the run queue.

We then know that the entity is no longer reachable by the run queue
selection logic, so as soon as any pending work is done the cleanup can
safely proceed. And because we have marked the entity as stopped, we also
know that the entity cannot re-enter the run queue.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: amd-gfx@lists.freedesktop.org
Cc: intel-xe@lists.freedesktop.org
---
 drivers/gpu/drm/scheduler/sched_entity.c   | 18 ++++++++++--------
 drivers/gpu/drm/scheduler/sched_internal.h | 14 ++++++++++++--
 drivers/gpu/drm/scheduler/sched_main.c     |  2 --
 drivers/gpu/drm/scheduler/sched_rq.c       | 19 +++++++++++++------
 include/drm/gpu_scheduler.h                |  9 ---------
 5 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index a18f637f9e99..2a827522e23f 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -161,11 +161,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 			      DRM_SCHED_PRIORITY_KERNEL : priority;
 	entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
 
-	init_completion(&entity->entity_idle);
-
-	/* We start in an idle state. */
-	complete_all(&entity->entity_idle);
-
 	spin_lock_init(&entity->lock);
 	spsc_queue_init(&entity->job_queue);
 
@@ -312,16 +307,23 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
  */
 void drm_sched_entity_kill(struct drm_sched_entity *entity)
 {
+	struct drm_gpu_scheduler *sched;
 	struct drm_sched_job *job;
 	struct dma_fence *prev;
 
 	spin_lock(&entity->lock);
 	entity->stopped = true;
-	drm_sched_rq_remove_entity(entity->rq, entity);
+	sched = drm_sched_rq_remove_entity(entity->rq, entity);
 	spin_unlock(&entity->lock);
 
-	/* Make sure this entity is not used by the scheduler at the moment */
-	wait_for_completion(&entity->entity_idle);
+	/*
+	 * Make sure this entity is not used by the scheduler at the moment.
+	 *
+	 * Scheduler is guaranteed to be stable after the entity was stopped and
+	 * removed from the run-queue.
+	 */
+	if (sched)
+		drm_sched_flush_run_work(sched);
 
 	spin_lock(&entity->lock);
 	prev = dma_fence_get(entity->last_scheduled);
diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h
index 32d3ddb820be..ca724a8a4433 100644
--- a/drivers/gpu/drm/scheduler/sched_internal.h
+++ b/drivers/gpu/drm/scheduler/sched_internal.h
@@ -42,13 +42,23 @@ bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
 			 struct drm_sched_entity *entity);
 void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
 
+/**
+ * drm_sched_flush_run_work - flush the run-job work
+ * @sched: scheduler instance
+ */
+static inline void drm_sched_flush_run_work(struct drm_gpu_scheduler *sched)
+{
+	flush_work(&sched->work_run_job);
+}
+
 void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
 		       struct drm_sched_rq *rq);
 
 struct drm_gpu_scheduler *
 drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts);
-void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
-				struct drm_sched_entity *entity);
+struct drm_gpu_scheduler *
+drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
+			   struct drm_sched_entity *entity);
 void drm_sched_rq_pop_entity(struct drm_sched_entity *entity);
 
 struct drm_sched_entity *
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 2355acc2b867..338050390fc8 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1033,7 +1033,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
 
 	sched_job = drm_sched_entity_pop_job(entity);
 	if (!sched_job) {
-		complete_all(&entity->entity_idle);
 		drm_sched_run_job_queue(sched);
 		return;
 	}
@@ -1049,7 +1048,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
 	 * refcount has been incremented for the scheduler already.
 	 */
 	fence = sched->ops->run_job(sched_job);
-	complete_all(&entity->entity_idle);
 	drm_sched_fence_scheduled(s_fence, fence);
 
 	if (!IS_ERR_OR_NULL(fence)) {
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index 23f46ec610e7..79f7bdb5b3f0 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -297,23 +297,32 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts)
  * @entity: scheduler entity
  *
  * Removes a scheduler entity from the run queue.
+ *
+ * Return: DRM scheduler selected to handle this entity or NULL if entity has
+ * already been removed.
  */
-void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
-				struct drm_sched_entity *entity)
+struct drm_gpu_scheduler *
+drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
+			   struct drm_sched_entity *entity)
 {
+	struct drm_gpu_scheduler *sched;
+
 	lockdep_assert_held(&entity->lock);
 
 	if (list_empty(&entity->list))
-		return;
+		return NULL;
 
 	spin_lock(&rq->lock);
 
-	atomic_dec(rq->sched->score);
+	sched = rq->sched;
+	atomic_dec(sched->score);
 	list_del_init(&entity->list);
 
 	drm_sched_rq_remove_fifo_locked(entity, rq);
 
 	spin_unlock(&rq->lock);
+
+	return sched;
 }
 
 static ktime_t
@@ -407,8 +416,6 @@ drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
 				spin_unlock(&rq->lock);
 				return ERR_PTR(-ENOSPC);
 			}
-
-			reinit_completion(&entity->entity_idle);
 			break;
 		}
 	}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 3b0978fa8cf0..c89a581f54bf 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -27,7 +27,6 @@
 #include <drm/spsc_queue.h>
 #include <linux/average.h>
 #include <linux/dma-fence.h>
-#include <linux/completion.h>
 #include <linux/xarray.h>
 #include <linux/workqueue.h>
 
@@ -232,14 +231,6 @@ struct drm_sched_entity {
 	 */
 	bool 				stopped;
 
-	/**
-	 * @entity_idle:
-	 *
-	 * Signals when entity is not in use, used to sequence entity cleanup in
-	 * drm_sched_entity_fini().
-	 */
-	struct completion		entity_idle;
-
 	/**
 	 * @oldest_job_waiting:
 	 *
-- 
2.56.0

[PATCH 4/7] drm/sched: Remove the single scheduler special case

[Tvrtko Ursulin]

Long time ago in commit
b3ac17667f11 ("drm/scheduler: rework entity creation")
a change was made which prevented priority changes for entities with only
one assigned scheduler.

That commit reduced drm_sched_entity_set_priority() to simply update the
entities priority, but the run queue selection logic in
drm_sched_entity_select_rq() was never able to actually change the
originally assigned run queue.

In practice that only affected amdgpu, being the only driver which can do
dynamic priority changes, and it appears it was attempted to rectify this
in commit
2316a86bde49 ("drm/amdgpu: change hw sched list on ctx priority override")
.

A few unresolved problems however remained:

The above only fixed drm_sched_entity_set_priority()
if drm_sched_entity_modify_sched() was called first which was not
documented anywhere.

Secondly, that only worked if drm_sched_entity_modify_sched() is actually
called, which in amdgpu's case is true only for gfx and compute. Priority
changes for other engines, which have only one scheduler assigned such as
jpeg and video decode still did not work.

(Note that this was also noticed in 981b04d96856 ("drm/sched: improve docs
around drm_sched_entity").)

Completely different set of non-obvious confusion was that, whereas
drm_sched_entity_init() was not keeping the passed in list of schedulers
(courtesy of commit
8c23056bdc7a ("drm/scheduler: do not keep a copy of sched list")
), drm_sched_entity_modify_sched() was disagreeing with that and
would simply assign the single item list. That incosistency appears to
have been semi-silently fixed in commit
ac4eb83ab255 ("drm/sched: select new rq even if there is only one v3")
.

What was also not documented is why it was important to not keep the list
of schedulers when there is only one scheduler given. This was in fact due
many of the callers passing in a stack container for this case. For the
multiple schedulers on the other hand amdgpu and rocket are the only such
callers and there the container is not on the stack but correctly follows
the lifetime requirement. Keeping a stack backed list in the entity would
obviously be undefined behaviour if the list was kept.

Having said that, amdgpu however did only stop passing in stack backed
container for the more than one scheduler case in commit
977f7e1068be ("drm/amdgpu: allocate entities on demand")
. Until then dereferencing freed stack from drm_sched_entity_select_rq()
was a possibility.

In order to untangle all this lets make this area behave consistently and
store the single scheduler case directly in the entity. We can do that
easily by creating an union between the current pointer to a list of
schedulers, and a new pointer to the single scheduler.

This allows removal of the special cases which artificially cleared
entity->sched_list for a single scheduler case and fixes the amdgpu
dynamic priority changes for the single IP instances.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: b3ac17667f11 ("drm/scheduler: rework entity creation")
References: 8c23056bdc7a ("drm/scheduler: do not keep a copy of sched list")
References: 977f7e1068be ("drm/amdgpu: allocate entities on demand")
References: 2316a86bde49 ("drm/amdgpu: change hw sched list on ctx priority override")
References: ac4eb83ab255 ("drm/sched: select new rq even if there is only one v3")
References: 981b04d96856 ("drm/sched: improve docs around drm_sched_entity")
References: 77a6809f1dc3 ("drm/sched: Remove FIFO and RR and simplify to a single run queue")
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Luben Tuikov <ltuikov89@gmail.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: Cc: Tomeu Vizoso <tomeu@tomeuvizoso.net>
Cc: Sidong Yang <sidong.yang@furiosa.ai>
Cc: amd-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/scheduler/sched_entity.c | 30 ++++++++++-------
 include/drm/gpu_scheduler.h              | 42 ++++++++++++++----------
 2 files changed, 44 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 2a827522e23f..3495153ad4e5 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -102,7 +102,9 @@ ktime_t drm_sched_entity_stats_job_add_gpu_time(struct drm_sched_job *job)
  * @guilty: atomic_t set to 1 when a job on this queue
  *          is found to be guilty causing a timeout
  *
- * Note that the &sched_list must have at least one element to schedule the entity.
+ * Note that the &sched_list must have exactly one element to schedule the
+ * entity. Callers which pass more than one scheduler on the @sched_list must
+ * ensure the list remains valid for the lifetime of the entity.
  *
  * For changing @priority later on at runtime see
  * drm_sched_entity_set_priority(). For changing the set of schedulers
@@ -139,7 +141,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->guilty = guilty;
 	entity->last_user = current->group_leader;
 	entity->num_sched_list = num_sched_list;
-	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+	if (num_sched_list == 1)
+		entity->sched = sched_list[0];
+	else
+		entity->sched_list = sched_list;
 	RB_CLEAR_NODE(&entity->rb_tree_node);
 
 	if (priority >= sched_list[0]->num_user_rqs) {
@@ -178,6 +183,9 @@ EXPORT_SYMBOL(drm_sched_entity_init);
  *		 existing entity->sched_list
  * @num_sched_list: number of drm sched in sched_list
  *
+ * Callers which pass more than one scheduler on the @sched_list must
+ * ensure the list remains valid for the lifetime of the entity.
+ *
  * Note that this must be called under the same common lock for @entity as
  * drm_sched_job_arm() and drm_sched_entity_push_job(), or the driver needs to
  * guarantee through some other means that this is never called while new jobs
@@ -198,7 +206,10 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
 	}
 
 	spin_lock(&entity->lock);
-	entity->sched_list = sched_list;
+	if (num_sched_list == 1)
+		entity->sched = sched_list[0];
+	else
+		entity->sched_list = sched_list;
 	entity->num_sched_list = num_sched_list;
 	spin_unlock(&entity->lock);
 }
@@ -587,10 +598,6 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	struct drm_gpu_scheduler *sched;
 	struct drm_sched_rq *rq;
 
-	/* single possible engine and already selected */
-	if (!entity->sched_list)
-		return;
-
 	/* queue non-empty, stay on the same engine */
 	if (spsc_queue_count(&entity->job_queue))
 		return;
@@ -607,16 +614,17 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	if (fence && !dma_fence_is_signaled(fence))
 		goto out;
 
-	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
+	if (entity->num_sched_list == 1)
+		sched = entity->sched;
+	else
+		sched = drm_sched_pick_best(entity->sched_list,
+					    entity->num_sched_list);
 	rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
 	if (rq != entity->rq) {
 		drm_sched_rq_remove_entity(entity->rq, entity);
 		entity->rq = rq;
 	}
 
-	if (entity->num_sched_list == 1)
-		entity->sched_list = NULL;
-
 out:
 	spin_unlock(&entity->lock);
 }
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index c89a581f54bf..dfa399ace89f 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -120,23 +120,31 @@ struct drm_sched_entity {
 	 */
 	struct drm_sched_entity_stats	*stats;
 
-	/**
-	 * @sched_list:
-	 *
-	 * A list of schedulers (struct drm_gpu_scheduler).  Jobs from this entity can
-	 * be scheduled on any scheduler on this list.
-	 *
-	 * This can be modified by calling drm_sched_entity_modify_sched().
-	 * Locking is entirely up to the driver, see the above function for more
-	 * details.
-	 *
-	 * This will be set to NULL if &num_sched_list equals 1 and @rq has been
-	 * set already.
-	 *
-	 * FIXME: This means priority changes through
-	 * drm_sched_entity_set_priority() will be lost henceforth in this case.
-	 */
-	struct drm_gpu_scheduler        **sched_list;
+	union {
+		/**
+		 * @sched_list:
+		 *
+		 * A list of schedulers (struct drm_gpu_scheduler).  Jobs from
+		 * this entity can be scheduled on any scheduler on this list.
+		 *
+		 * This can be modified by calling
+		 * drm_sched_entity_modify_sched(). Locking is entirely up to
+		 * the driver, see the above function for more details.
+		 */
+		struct drm_gpu_scheduler        **sched_list;
+
+		/**
+		 * @sched:
+		 *
+		 * A single struct drm_gpu_scheduler jobs from this entity will
+		 * be scheduled on.
+		 *
+		 * This can be modified by calling
+		 * drm_sched_entity_modify_sched(). Locking is entirely up to
+		 * the driver, see the above function for more details.
+		 */
+		struct drm_gpu_scheduler        *sched;
+	};
 
 	/**
 	 * @num_sched_list:
-- 
2.56.0

[PATCH 5/7] drm/amdgpu: Rework the amdgpu_job_stop_all_jobs_on_sched() hack

[Tvrtko Ursulin]

amdgpu_job_stop_all_jobs_on_sched is a semi broken hack used on only one
platform which suffers from several problems as mentioned in
https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/.

Until it can be fully removed, given that it directly acceses DRM scheduler
internals which want to be changed, we need to rework it to allow for
replacing the SPSC queue with a standard spin locked list.

Problem at hand is the lock inversion between the run-queue and entity
lock which becomes apparent once locking is added to the job queue
amdgpu_job_stop_all_jobs_on_sched().

To work around it we change the approach of how queued jobs are drained by
using trylock and restarting on locked entities until all queued jobs from
all active entities are removed.

This slightly widens the existing scope for a live lock in case of
userspace actively submitting new jobs. Whereas the current code can keep
processing the single busy entity, this new implementation can retry on the
entities which it previously processed.

Since userspace submitting jobs so rapidly the GPU reset cannot finish and
the problem already exists, plus the plan is to remove this code
altogether, we can say this is acceptable in order to unblock the DRM
scheduler rework.

While at it we add the assert scheduler workqueue has been stopped by the
caller and also correct the wrong order of signalling already submitted
and queued fences.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
References: https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Hawking Zhang <Hawking.Zhang@amd.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 86 ++++++++++++++++++++-----
 1 file changed, 69 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 8689a3c933b2..1f4a4a5a978d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -457,13 +457,6 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 	return fence;
 }
 
-/*
- * This is a duplicate function from DRM scheduler sched_internal.h.
- * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is removed, due
- * latter being incorrect and racy.
- *
- * See https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
- */
 static struct drm_sched_job *
 drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
 {
@@ -476,28 +469,66 @@ drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
 	return container_of(node, struct drm_sched_job, queue_node);
 }
 
-void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
+static struct drm_sched_entity *
+get_entity_with_jobs(struct drm_gpu_scheduler *sched) __acquires(&entity->lock)
 {
-	struct drm_sched_job *s_job;
-	struct drm_sched_entity *s_entity = NULL;
+	struct drm_sched_entity *entity;
 	int i;
 
-	/* Signal all jobs not yet scheduled */
+retry:
 	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
 		struct drm_sched_rq *rq = sched->sched_rq[i];
+
 		spin_lock(&rq->lock);
-		list_for_each_entry(s_entity, &rq->entities, list) {
-			while ((s_job = drm_sched_entity_queue_pop(s_entity))) {
-				struct drm_sched_fence *s_fence = s_job->s_fence;
+		list_for_each_entry(entity, &rq->entities, list) {
+			/*
+			 * This can false negative, but so could the previous
+			 * approach of scanning the run-queues once when
+			 * nothing prevents userspace to queue more jobs.
+			 */
+			if (!spsc_queue_count(&entity->job_queue))
+				continue;
 
-				dma_fence_signal(&s_fence->scheduled);
-				dma_fence_set_error(&s_fence->finished, -EHWPOISON);
-				dma_fence_signal(&s_fence->finished);
+			/*
+			 * We must trylock because normal locking order is
+			 * entity then rq and here we already have rq locked
+			 * in order to be able to walk the entities.
+			 */
+			if (spin_trylock(&entity->lock)) {
+				/* Returns with a locked entity. */
+				spin_unlock(&rq->lock);
+				return entity;
+			} else {
+				/*
+				 * Busy entity - we will try again looking for a
+				 * window where trylock will succeed.
+				 */
+				spin_unlock(&rq->lock);
+				goto retry;
 			}
 		}
 		spin_unlock(&rq->lock);
 	}
 
+	return NULL;
+}
+
+/*
+ * amdgpu_job_stop_all_jobs_on_sched has a single purpose which is an
+ * unreliable and racy, as described in:
+ *  https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/.
+ *
+ * Plan is to remove it, together with the above open coded DRM scheduler
+ * helpers, and replace with a proper solution.
+ */
+void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
+{
+	struct drm_sched_job *s_job, *next;
+	struct drm_sched_entity *s_entity;
+	LIST_HEAD(queued);
+
+	WARN_ON_ONCE(!sched->pause_submit);
+
 	/* Signal all jobs already scheduled to HW */
 	list_for_each_entry(s_job, &sched->pending_list, list) {
 		struct drm_sched_fence *s_fence = s_job->s_fence;
@@ -505,6 +536,27 @@ void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
 		dma_fence_set_error(&s_fence->finished, -EHWPOISON);
 		dma_fence_signal(&s_fence->finished);
 	}
+
+	/*
+	 * Signal all jobs not yet scheduled - note that the entity trylock loop
+	 * can in theory livelock if userspace keeps submitting new jobs more
+	 * rapidly than the loop can do a full pass over all run-queues and
+	 * entities.
+	 */
+	while ((s_entity = get_entity_with_jobs(sched)) != NULL) {
+		while ((s_job = drm_sched_entity_queue_pop(s_entity)))
+			list_add_tail(&s_job->list, &queued);
+		spin_unlock(&s_entity->lock);
+	}
+
+	list_for_each_entry_safe(s_job, next, &queued, list) {
+		struct drm_sched_fence *s_fence = s_job->s_fence;
+
+		list_del(&s_job->list);
+		dma_fence_signal(&s_fence->scheduled);
+		dma_fence_set_error(&s_fence->finished, -EHWPOISON);
+		dma_fence_signal(&s_fence->finished);
+	}
 }
 
 const struct drm_sched_backend_ops amdgpu_sched_ops = {
-- 
2.56.0

[PATCH 6/7] drm/sched: Replace the SPSC queue with a list and a spinlock

[Tvrtko Ursulin]

Migrate the entity job queue from the single user SPSC queue (single
producer, single consumer) to the standard list_head plus a spinlock in
order to make it easier to argue about correctnes and bring the code base
close to the usual kernel patterns.

The main thing which needs to be mentioned is the seeming usage of RCU,
which isn't really so, but only for two trivial reasons:

1.
We need to use the list operations which are safe for unlocked access to
the list head, while parallel (locked) threads can be appending other
jobs.

This so the run job worker is able to process the job dependencies which
includes it calling a potentially sleeping prepare_job() callback. Hence
it is not possible to hold the entity->lock during that stage.

2.
We need to work around the infamous entity<->rq locking inversion which
would now trigger slightly earlier in the run job worker, from the entity
selection stage, where there was previosuly the other "spsc queue peek"
call site.

Both are completely safe since we are guaranteed the head of the queue
cannot disapper until the run job work dequeues it itself later in its
flow.

All the list_*_rcu usage stem from those two requirements and there are no
actual RCU requirements.

Second thing to point out are the simplifications around
drm_sched_entity_is_ready() and drm_sched_can_queue(). In this design we
start tracking the number of job credits of the first job in the queue per
entity.

That allows for both removing one job peek site and the requirement to
access the number of queued jobs (or the queue non empty status).

In the functional respect replacing spsc_queue_count() (atomic_read) in
drm_sched_entity_is_ready() with READ_ONCE(entity->job_credits) are
equivalent but the new flow in drm_sched_rq_select_entity() is more
streamlined and easier to reason about.

The actual number of queued jobs is tracked only when tracing or unit
tests are compiled in and is not used from the regular scheduler logic.

In drm_sched_entity_push_job() because the job now cannot disappear
until we unlock the entity we can drop the respective comment and the
submit_ts caching.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c       |  26 +++-
 .../gpu/drm/scheduler/gpu_scheduler_trace.h   |   2 +-
 drivers/gpu/drm/scheduler/sched_entity.c      |  68 ++++++----
 drivers/gpu/drm/scheduler/sched_internal.h    |  56 +++-----
 drivers/gpu/drm/scheduler/sched_main.c        |  47 +------
 drivers/gpu/drm/scheduler/sched_rq.c          |  58 +++++++-
 .../gpu/drm/scheduler/tests/tests_scheduler.c |   2 +-
 include/drm/gpu_scheduler.h                   |  29 +++-
 include/drm/spsc_queue.h                      | 124 ------------------
 9 files changed, 169 insertions(+), 243 deletions(-)
 delete mode 100644 include/drm/spsc_queue.h

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 1f4a4a5a978d..dd814b5ef367 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -460,13 +460,29 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 static struct drm_sched_job *
 drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
 {
-	struct spsc_node *node;
+	struct drm_sched_job *next_job, *sched_job;
 
-	node = spsc_queue_pop(&entity->job_queue);
-	if (!node)
+	lockdep_assert_held(&entity->lock);
+
+	sched_job = list_first_entry_or_null(&entity->job_queue,
+					     typeof(*sched_job), queue_link);
+	if (!sched_job)
 		return NULL;
 
-	return container_of(node, struct drm_sched_job, queue_node);
+	list_del_rcu(&sched_job->queue_link);
+
+#if IS_ENABLED(CONFIG_TRACEPOINTS) || IS_ENABLED(CONFIG_DRM_SCHED_KUNIT_TEST)
+	WRITE_ONCE(entity->job_count, entity->job_count - 1);
+#endif
+
+	next_job = list_first_entry_or_null(&entity->job_queue,
+					    typeof(*next_job), queue_link);
+	if (next_job)
+		WRITE_ONCE(entity->job_credits, next_job->credits);
+	else
+		WRITE_ONCE(entity->job_credits, 0);
+
+	return sched_job;
 }
 
 static struct drm_sched_entity *
@@ -486,7 +502,7 @@ get_entity_with_jobs(struct drm_gpu_scheduler *sched) __acquires(&entity->lock)
 			 * approach of scanning the run-queues once when
 			 * nothing prevents userspace to queue more jobs.
 			 */
-			if (!spsc_queue_count(&entity->job_queue))
+			if (!READ_ONCE(entity->job_credits))
 				continue;
 
 			/*
diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
index 261713dd7d5a..6a6f7af00089 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
@@ -69,7 +69,7 @@ DECLARE_EVENT_CLASS(drm_sched_job,
 
 	    TP_fast_assign(
 			   __assign_str(name);
-			   __entry->job_count = spsc_queue_count(&entity->job_queue);
+			   __entry->job_count = READ_ONCE(entity->job_count);
 			   __entry->hw_job_count = atomic_read(
 				   &sched_job->sched->credit_count);
 			   __assign_str(dev);
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 3495153ad4e5..9fb5affaab87 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -167,7 +167,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
 
 	spin_lock_init(&entity->lock);
-	spsc_queue_init(&entity->job_queue);
+	INIT_LIST_HEAD(&entity->job_queue);
 
 	atomic_set(&entity->fence_seq, 0);
 	entity->fence_context = dma_fence_context_alloc(2);
@@ -221,8 +221,7 @@ static bool drm_sched_entity_is_idle(struct drm_sched_entity *entity)
 
 	spin_lock(&entity->lock);
 
-	if (list_empty(&entity->list) ||
-	    spsc_queue_count(&entity->job_queue) == 0 ||
+	if (list_empty(&entity->list) || !READ_ONCE(entity->job_credits) ||
 	    entity->stopped)
 		idle = true;
 
@@ -338,7 +337,6 @@ void drm_sched_entity_kill(struct drm_sched_entity *entity)
 
 	spin_lock(&entity->lock);
 	prev = dma_fence_get(entity->last_scheduled);
-	spin_unlock(&entity->lock);
 	while ((job = drm_sched_entity_queue_pop(entity))) {
 		struct drm_sched_fence *s_fence = job->s_fence;
 
@@ -356,6 +354,7 @@ void drm_sched_entity_kill(struct drm_sched_entity *entity)
 
 		prev = &s_fence->finished;
 	}
+	spin_unlock(&entity->lock);
 	dma_fence_put(prev);
 }
 EXPORT_SYMBOL(drm_sched_entity_kill);
@@ -557,10 +556,20 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	/* Helper to avoid dropping the reference while the entity lock is held,
 	 * just to have some more robustness.
 	 */
+	struct drm_sched_job *sched_job, *next_job;
 	struct dma_fence *prev_last_scheduled;
-	struct drm_sched_job *sched_job;
 
-	sched_job = drm_sched_entity_queue_peek(entity);
+	/*
+	 * We cannot lock the entity at this point since processing dependencies
+	 * needs to be able to call a sleeping ops->prepare_job(). Note that RCU
+	 * lock is not stricly required since single workqueue is the only
+	 * caller and that guarantees a single consumer. Ie. the job cannot
+	 * disappear on us.
+	 */
+	rcu_read_lock();
+	sched_job = list_first_or_null_rcu(&entity->job_queue,
+					   typeof(*sched_job), queue_link);
+	rcu_read_unlock();
 	if (!sched_job)
 		return NULL;
 
@@ -577,8 +586,21 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	spin_lock(&entity->lock);
 	prev_last_scheduled = entity->last_scheduled;
 	entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished);
-	spsc_queue_pop(&entity->job_queue);
-	drm_sched_rq_pop_entity(entity);
+
+	list_del_rcu(&sched_job->queue_link);
+#if IS_ENABLED(CONFIG_TRACEPOINTS) || IS_ENABLED(CONFIG_DRM_SCHED_KUNIT_TEST)
+	WRITE_ONCE(entity->job_count, entity->job_count - 1);
+#endif
+	next_job = list_first_entry_or_null(&entity->job_queue,
+					    typeof(*next_job), queue_link);
+	/* WRITE_ONCE just for the annotation to pair up. */
+	if (next_job)
+		WRITE_ONCE(entity->job_credits, next_job->credits);
+	else
+		WRITE_ONCE(entity->job_credits, 0);
+
+	drm_sched_rq_pop_entity(entity, next_job);
+
 	spin_unlock(&entity->lock);
 
 	dma_fence_put(prev_last_scheduled);
@@ -598,11 +620,11 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	struct drm_gpu_scheduler *sched;
 	struct drm_sched_rq *rq;
 
+	spin_lock(&entity->lock);
+
 	/* queue non-empty, stay on the same engine */
-	if (spsc_queue_count(&entity->job_queue))
-		return;
-
-	spin_lock(&entity->lock);
+	if (entity->job_credits)
+		goto out;
 
 	/* Cannot submit to stopped entity anyway so bail out early. */
 	if (entity->stopped)
@@ -642,9 +664,6 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 {
 	struct drm_sched_entity *entity = sched_job->entity;
 	bool first;
-	ktime_t submit_ts;
-
-	trace_drm_sched_job_queue(sched_job, entity);
 
 	if (trace_drm_sched_job_add_dep_enabled()) {
 		struct dma_fence *entry;
@@ -656,26 +675,27 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 	atomic_inc(entity->rq->sched->score);
 	WRITE_ONCE(entity->last_user, current->group_leader);
 
-	/*
-	 * After the sched_job is pushed into the entity queue, it may be
-	 * completed and freed up at any time. We can no longer access it.
-	 * Make sure to set the submit_ts first, to avoid a race.
-	 */
-	sched_job->submit_ts = submit_ts = ktime_get();
+	sched_job->submit_ts = ktime_get();
 
 	spin_lock(&entity->lock);
+	first = list_empty(&entity->job_queue);
+	list_add_tail_rcu(&sched_job->queue_link, &entity->job_queue);
+#if IS_ENABLED(CONFIG_TRACEPOINTS) || IS_ENABLED(CONFIG_DRM_SCHED_KUNIT_TEST)
+	WRITE_ONCE(entity->job_count, entity->job_count + 1);
+#endif
 
-	first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
+	trace_drm_sched_job_queue(sched_job, entity);
 
 	/* first job wakes up scheduler */
 	if (first) {
 		struct drm_gpu_scheduler *sched;
 
-		sched = drm_sched_rq_add_entity(entity, submit_ts);
+		/* WRITE_ONCE just for the annotation to pair up. */
+		WRITE_ONCE(entity->job_credits, sched_job->credits);
+		sched = drm_sched_rq_add_entity(entity, sched_job->submit_ts);
 		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_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h
index ca724a8a4433..7ac988d4d299 100644
--- a/drivers/gpu/drm/scheduler/sched_internal.h
+++ b/drivers/gpu/drm/scheduler/sched_internal.h
@@ -38,8 +38,6 @@ extern int drm_sched_policy;
 #define DRM_SCHED_POLICY_FIFO  1
 #define DRM_SCHED_POLICY_FAIR  2
 
-bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
-			 struct drm_sched_entity *entity);
 void drm_sched_wakeup(struct drm_gpu_scheduler *sched);
 
 /**
@@ -59,7 +57,8 @@ drm_sched_rq_add_entity(struct drm_sched_entity *entity, ktime_t ts);
 struct drm_gpu_scheduler *
 drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 			   struct drm_sched_entity *entity);
-void drm_sched_rq_pop_entity(struct drm_sched_entity *entity);
+void drm_sched_rq_pop_entity(struct drm_sched_entity *entity,
+			     struct drm_sched_job *next_job);
 
 struct drm_sched_entity *
 drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
@@ -89,47 +88,30 @@ void drm_sched_fence_finished(struct drm_sched_fence *fence, int result);
 static inline struct drm_sched_job *
 drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
 {
-	struct spsc_node *node;
+	struct drm_sched_job *next_job, *sched_job;
 
-	node = spsc_queue_pop(&entity->job_queue);
-	if (!node)
-		return NULL;
-
-	return container_of(node, struct drm_sched_job, queue_node);
-}
-
-/**
- * drm_sched_entity_queue_peek - Low level helper for peeking at the job queue
- *
- * @entity: scheduler entity
- *
- * Low level helper for peeking at the job queue
- *
- * Returns: The job at the head of the queue or NULL.
- */
-static inline struct drm_sched_job *
-drm_sched_entity_queue_peek(struct drm_sched_entity *entity)
-{
-	struct spsc_node *node;
+	lockdep_assert_held(&entity->lock);
 
-	node = spsc_queue_peek(&entity->job_queue);
-	if (!node)
+	sched_job = list_first_entry_or_null(&entity->job_queue,
+					     typeof(*sched_job), queue_link);
+	if (!sched_job)
 		return NULL;
 
-	return container_of(node, struct drm_sched_job, queue_node);
-}
+	list_del_rcu(&sched_job->queue_link);
 
-/* Return true if entity could provide a job. */
-static inline bool
-drm_sched_entity_is_ready(struct drm_sched_entity *entity)
-{
-	if (!spsc_queue_count(&entity->job_queue))
-		return false;
+#if IS_ENABLED(CONFIG_TRACEPOINTS) || IS_ENABLED(CONFIG_DRM_SCHED_KUNIT_TEST)
+	WRITE_ONCE(entity->job_count, entity->job_count - 1);
+#endif
 
-	if (READ_ONCE(entity->dependency))
-		return false;
+	next_job = list_first_entry_or_null(&entity->job_queue,
+					    typeof(*next_job), queue_link);
+	/* WRITE_ONCE just for the annotation to pair up. */
+	if (next_job)
+		WRITE_ONCE(entity->job_credits, next_job->credits);
+	else
+		WRITE_ONCE(entity->job_credits, 0);
 
-	return true;
+	return sched_job;
 }
 
 void drm_sched_entity_stats_release(struct kref *kref);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 338050390fc8..85dc570a4816 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -92,46 +92,6 @@ int drm_sched_policy = DRM_SCHED_POLICY_FAIR;
 MODULE_PARM_DESC(sched_policy, "Specify the scheduling policy for entities on a run-queue, " __stringify(DRM_SCHED_POLICY_RR) " = Round Robin, " __stringify(DRM_SCHED_POLICY_FIFO) " = FIFO (default), " __stringify(DRM_SCHED_POLICY_FAIR) " = Fair (experimental).");
 module_param_named(sched_policy, drm_sched_policy, int, 0444);
 
-static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
-{
-	u32 credits;
-
-	WARN_ON(check_sub_overflow(sched->credit_limit,
-				   atomic_read(&sched->credit_count),
-				   &credits));
-
-	return credits;
-}
-
-/**
- * drm_sched_can_queue -- Can we queue more to the hardware?
- * @sched: scheduler instance
- * @entity: the scheduler entity
- *
- * Return true if we can push at least one more job from @entity, false
- * otherwise.
- */
-bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
-			 struct drm_sched_entity *entity)
-{
-	struct drm_sched_job *s_job;
-
-	s_job = drm_sched_entity_queue_peek(entity);
-	if (!s_job)
-		return false;
-
-	/* If a job exceeds the credit limit, truncate it to the credit limit
-	 * itself to guarantee forward progress.
-	 */
-	if (s_job->credits > sched->credit_limit) {
-		dev_WARN(sched->dev,
-			 "Jobs may not exceed the credit limit, truncate.\n");
-		s_job->credits = sched->credit_limit;
-	}
-
-	return drm_sched_available_credits(sched) >= s_job->credits;
-}
-
 /**
  * drm_sched_run_job_queue - enqueue run-job work
  * @sched: scheduler instance
@@ -616,6 +576,7 @@ int drm_sched_job_init(struct drm_sched_job *job,
 		return -ENOMEM;
 
 	INIT_LIST_HEAD(&job->list);
+	INIT_LIST_HEAD(&job->queue_link);
 
 	xa_init_flags(&job->dependencies, XA_FLAGS_ALLOC);
 
@@ -1039,6 +1000,12 @@ static void drm_sched_run_job_work(struct work_struct *w)
 
 	s_fence = sched_job->s_fence;
 
+	if (sched_job->credits > sched->credit_limit) {
+		dev_WARN(sched->dev,
+			 "Jobs may not exceed the credit limit, truncating.\n");
+		sched_job->credits = sched->credit_limit;
+	}
+
 	atomic_add(sched_job->credits, &sched->credit_count);
 	drm_sched_job_begin(sched_job);
 
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index 79f7bdb5b3f0..b6f35fef95f8 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -347,10 +347,11 @@ drm_sched_rq_next_rr_ts(struct drm_sched_rq *rq,
  *
  * To be called every time after a job is popped from the entity.
  */
-void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
+void
+drm_sched_rq_pop_entity(struct drm_sched_entity *entity,
+			struct drm_sched_job *next_job)
 {
 	struct drm_sched_rq *rq = entity->rq;
-	struct drm_sched_job *next_job;
 
 	lockdep_assert_held(&entity->lock);
 
@@ -360,7 +361,6 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
 	 * Update the entity's location in the min heap according to
 	 * the timestamp of the next job, if any.
 	 */
-	next_job = drm_sched_entity_queue_peek(entity);
 	if (next_job) {
 		ktime_t ts;
 
@@ -386,6 +386,52 @@ void drm_sched_rq_pop_entity(struct drm_sched_entity *entity)
 	spin_unlock(&rq->lock);
 }
 
+
+/* Return the number of credits first queued job needs, or zero if none are ready. */
+static inline u32
+drm_sched_entity_ready_credits(struct drm_sched_entity *entity)
+{
+	if (READ_ONCE(entity->dependency))
+		return 0;
+
+	return READ_ONCE(entity->job_credits);
+}
+
+static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
+{
+	u32 credits;
+
+	WARN_ON(check_sub_overflow(sched->credit_limit,
+				   atomic_read(&sched->credit_count),
+				   &credits));
+
+	return credits;
+}
+
+/**
+ * drm_sched_can_queue -- Can we queue more to the hardware?
+ * @sched: scheduler instance
+ * @credits: the number of credits required
+ *
+ * Return true if we can push at least a job worth @credits, false
+ * otherwise.
+ */
+static bool
+drm_sched_can_queue(struct drm_gpu_scheduler *sched, u32 credits)
+{
+	if (!credits)
+		return false;
+
+	/*
+	 * If a job exceeds the credit limit, we will truncate it to guarantee
+	 * forward progress.
+	 */
+	if (credits > sched->credit_limit)
+		credits = sched->credit_limit;
+
+	return drm_sched_available_credits(sched) >= credits;
+}
+
 /**
  * drm_sched_rq_select_entity - Select an entity which provides a job to run
  * @sched: the gpu scheduler
@@ -406,13 +452,15 @@ drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
 	spin_lock(&rq->lock);
 	for (rb = rb_first_cached(&rq->rb_tree_root); rb; rb = rb_next(rb)) {
 		struct drm_sched_entity *entity;
+		u32 credits;
 
 		entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
-		if (drm_sched_entity_is_ready(entity)) {
+		credits = drm_sched_entity_ready_credits(entity);
+		if (credits) {
 			/* If we can't queue yet, preserve the current entity in
 			 * terms of fairness.
 			 */
-			if (!drm_sched_can_queue(sched, entity)) {
+			if (!drm_sched_can_queue(sched, credits)) {
 				spin_unlock(&rq->lock);
 				return ERR_PTR(-ENOSPC);
 			}
diff --git a/drivers/gpu/drm/scheduler/tests/tests_scheduler.c b/drivers/gpu/drm/scheduler/tests/tests_scheduler.c
index 285546a2218f..e68e4fa40688 100644
--- a/drivers/gpu/drm/scheduler/tests/tests_scheduler.c
+++ b/drivers/gpu/drm/scheduler/tests/tests_scheduler.c
@@ -611,7 +611,7 @@ static void drm_sched_scheduler_two_clients_test(struct kunit *test)
 		unsigned int pct[2], qd[2], cycle[2], cps[2];
 
 		for (i = 0; i < ARRAY_SIZE(client); i++) {
-			qd[i] = spsc_queue_count(&client[i].entity->base.job_queue);
+			qd[i] = READ_ONCE(client[i].entity->base.job_count);
 			cycle[i] = READ_ONCE(client[i].cycle);
 			cps[i] = DIV_ROUND_UP(100 * frequency *
 					      (cycle[i] - prev_cycle[i]),
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index dfa399ace89f..0cffb47f151b 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -24,7 +24,6 @@
 #ifndef _DRM_GPU_SCHEDULER_H_
 #define _DRM_GPU_SCHEDULER_H_
 
-#include <drm/spsc_queue.h>
 #include <linux/average.h>
 #include <linux/dma-fence.h>
 #include <linux/xarray.h>
@@ -98,9 +97,9 @@ struct drm_sched_entity {
 	/**
 	 * @lock:
 	 *
-	 * Lock protecting the run-queue (@rq) to which this entity belongs,
-	 * @priority, @last_scheduled and the list of schedulers (@sched_list,
-	 * @num_sched_list), @rr_ts and @stopped.
+	 * Lock protecting the @job_queue, run-queue (@rq) to which this entity
+	 * belongs, @priority, @last_scheduled and the list of schedulers
+	 * (@sched_list, @num_sched_list), @rr_ts and @stopped.
 	 */
 	spinlock_t			lock;
 
@@ -175,8 +174,26 @@ struct drm_sched_entity {
 
 	/**
 	 * @job_queue: the list of jobs of this entity.
+	 *
+	 * It is operated using the RCU list accessors to allow for safe
+	 * initial lockless access from drm_sched_run_job_work(). Since that is
+	 * the only caller, and it is guaranteed to be a singleton, and we
+	 * cannot lock the entity at its top due both lock inversions and the
+	 * need to call sleeping ->prepare_job.
 	 */
-	struct spsc_queue		job_queue;
+	struct list_head		job_queue;
+
+#if IS_ENABLED(CONFIG_TRACEPOINTS) || IS_ENABLED(CONFIG_DRM_SCHED_KUNIT_TEST)
+	/**
+	 * @job_count: number of jobs in the @job_queue.
+	 */
+	unsigned int			job_count;
+#endif
+
+	/**
+	 * @job_credits: credits of the first job in the queue or zero if none.
+	 */
+	u32				job_credits;
 
 	/**
 	 * @fence_seq:
@@ -390,7 +407,7 @@ struct drm_sched_job {
 	unsigned int			last_dependency;
 	atomic_t			karma;
 
-	struct spsc_node		queue_node;
+	struct list_head		queue_link;
 	struct list_head		list;
 
 	/*
diff --git a/include/drm/spsc_queue.h b/include/drm/spsc_queue.h
deleted file mode 100644
index ee9df8cc67b7..000000000000
--- a/include/drm/spsc_queue.h
+++ /dev/null
@@ -1,124 +0,0 @@
-/*
- * Copyright 2017 Advanced Micro Devices, Inc.
- *
- * Permission is hereby granted, free of charge, to any person obtaining a
- * copy of this software and associated documentation files (the "Software"),
- * to deal in the Software without restriction, including without limitation
- * the rights to use, copy, modify, merge, publish, distribute, sublicense,
- * and/or sell copies of the Software, and to permit persons to whom the
- * Software is furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
- * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
- * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
- * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
- * OTHER DEALINGS IN THE SOFTWARE.
- *
- */
-
-#ifndef DRM_SCHEDULER_SPSC_QUEUE_H_
-#define DRM_SCHEDULER_SPSC_QUEUE_H_
-
-#include <linux/atomic.h>
-#include <linux/preempt.h>
-
-/** SPSC lockless queue */
-
-struct spsc_node {
-
-	/* Stores spsc_node* */
-	struct spsc_node *next;
-};
-
-struct spsc_queue {
-
-	 struct spsc_node *head;
-
-	/* atomic pointer to struct spsc_node* */
-	atomic_long_t tail;
-
-	atomic_t job_count;
-};
-
-static inline void spsc_queue_init(struct spsc_queue *queue)
-{
-	queue->head = NULL;
-	atomic_long_set(&queue->tail, (long)&queue->head);
-	atomic_set(&queue->job_count, 0);
-}
-
-static inline struct spsc_node *spsc_queue_peek(struct spsc_queue *queue)
-{
-	return queue->head;
-}
-
-static inline int spsc_queue_count(struct spsc_queue *queue)
-{
-	return atomic_read(&queue->job_count);
-}
-
-static inline bool spsc_queue_push(struct spsc_queue *queue, struct spsc_node *node)
-{
-	struct spsc_node **tail;
-
-	node->next = NULL;
-
-	preempt_disable();
-
-	atomic_inc(&queue->job_count);
-	smp_mb__after_atomic();
-
-	tail = (struct spsc_node **)atomic_long_xchg(&queue->tail, (long)&node->next);
-	WRITE_ONCE(*tail, node);
-
-	/*
-	 * In case of first element verify new node will be visible to the consumer
-	 * thread when we ping the kernel thread that there is new work to do.
-	 */
-	smp_wmb();
-
-	preempt_enable();
-
-	return tail == &queue->head;
-}
-
-
-static inline struct spsc_node *spsc_queue_pop(struct spsc_queue *queue)
-{
-	struct spsc_node *next, *node;
-
-	/* Verify reading from memory and not the cache */
-	smp_rmb();
-
-	node = READ_ONCE(queue->head);
-
-	if (!node)
-		return NULL;
-
-	next = READ_ONCE(node->next);
-	WRITE_ONCE(queue->head, next);
-
-	if (unlikely(!next)) {
-		/* slowpath for the last element in the queue */
-
-		if (atomic_long_cmpxchg(&queue->tail,
-				(long)&node->next, (long) &queue->head) != (long)&node->next) {
-			/* Updating tail failed wait for new next to appear */
-			do {
-				smp_rmb();
-			} while (unlikely(!(queue->head = READ_ONCE(node->next))));
-		}
-	}
-
-	atomic_dec(&queue->job_count);
-	return node;
-}
-
-
-
-#endif /* DRM_SCHEDULER_SPSC_QUEUE_H_ */
-- 
2.56.0

[PATCH 7/7] drm/sched: Address the fence_seq fixme

[Tvrtko Ursulin]

Address a FIXME against struct drm_sched_entity->fence_seq which
documented this field does not need to be atomic_t.

Indeed now that most of the flow is guarded with entity->lock we can pull
out the lock a step up, to drm_sched_job_arm(), which also simplifies
drm_sched_entity_select_rq() and convert the field to a standard unsigned
long.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 13 ++++---------
 drivers/gpu/drm/scheduler/sched_fence.c  |  6 ++++--
 drivers/gpu/drm/scheduler/sched_main.c   |  7 ++++++-
 include/drm/gpu_scheduler.h              |  7 ++-----
 4 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 9fb5affaab87..63f52d1a8c74 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -168,8 +168,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 
 	spin_lock_init(&entity->lock);
 	INIT_LIST_HEAD(&entity->job_queue);
-
-	atomic_set(&entity->fence_seq, 0);
 	entity->fence_context = dma_fence_context_alloc(2);
 
 	return 0;
@@ -620,21 +618,21 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 	struct drm_gpu_scheduler *sched;
 	struct drm_sched_rq *rq;
 
-	spin_lock(&entity->lock);
+	lockdep_assert_held(&entity->lock);
 
 	/* queue non-empty, stay on the same engine */
 	if (entity->job_credits)
-		goto out;
+		return;
 
 	/* Cannot submit to stopped entity anyway so bail out early. */
 	if (entity->stopped)
-		goto out;
+		return;
 
 	fence = entity->last_scheduled;
 
 	/* stay on the same engine if the previous job hasn't finished */
 	if (fence && !dma_fence_is_signaled(fence))
-		goto out;
+		return;
 
 	if (entity->num_sched_list == 1)
 		sched = entity->sched;
@@ -646,9 +644,6 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 		drm_sched_rq_remove_entity(entity->rq, entity);
 		entity->rq = rq;
 	}
-
-out:
-	spin_unlock(&entity->lock);
 }
 
 /**
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index b7a4cb921234..b4c336ff0179 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -224,10 +224,12 @@ struct drm_sched_fence *drm_sched_fence_alloc(void *owner,
 void drm_sched_fence_init(struct drm_sched_fence *fence,
 			  struct drm_sched_entity *entity)
 {
-	unsigned seq;
+	unsigned long seq;
+
+	lockdep_assert_held(&entity->lock);
 
 	fence->sched = entity->rq->sched;
-	seq = atomic_inc_return(&entity->fence_seq);
+	seq = ++entity->fence_seq;
 	dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled,
 		       &fence->lock, entity->fence_context, seq);
 	dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished,
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 85dc570a4816..61f3f1e8520d 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -607,6 +607,9 @@ void drm_sched_job_arm(struct drm_sched_job *job)
 	struct drm_sched_entity *entity = job->entity;
 
 	BUG_ON(!entity);
+
+	spin_lock(&entity->lock);
+
 	drm_sched_entity_select_rq(entity);
 	sched = entity->rq->sched;
 
@@ -614,7 +617,9 @@ void drm_sched_job_arm(struct drm_sched_job *job)
 	job->s_priority = entity->priority;
 	job->entity_stats = drm_sched_entity_stats_get(entity->stats);
 
-	drm_sched_fence_init(job->s_fence, job->entity);
+	drm_sched_fence_init(job->s_fence, entity);
+
+	spin_unlock(&entity->lock);
 }
 EXPORT_SYMBOL(drm_sched_job_arm);
 
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 0cffb47f151b..6a98eb9f0b4b 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -99,7 +99,7 @@ struct drm_sched_entity {
 	 *
 	 * Lock protecting the @job_queue, run-queue (@rq) to which this entity
 	 * belongs, @priority, @last_scheduled and the list of schedulers
-	 * (@sched_list, @num_sched_list), @rr_ts and @stopped.
+	 * (@sched_list, @num_sched_list), @rr_ts, @stopped and @fence_seq.
 	 */
 	spinlock_t			lock;
 
@@ -200,11 +200,8 @@ struct drm_sched_entity {
 	 *
 	 * A linearly increasing seqno incremented with each new
 	 * &drm_sched_fence which is part of the entity.
-	 *
-	 * FIXME: Callers of drm_sched_job_arm() need to ensure correct locking,
-	 * this doesn't need to be atomic.
 	 */
-	atomic_t			fence_seq;
+	unsigned long			fence_seq;
 
 	/**
 	 * @fence_context:
-- 
2.56.0

Re: [PATCH 4/7] drm/sched: Remove the single scheduler special case

[Sidong Yang]

On Fri, Nov 06, 2026 at 02:56:59PM +0000, Tvrtko Ursulin wrote:
> @@ -139,7 +141,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>  	entity->guilty = guilty;
>  	entity->last_user = current->group_leader;
>  	entity->num_sched_list = num_sched_list;
> -	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
> +	if (num_sched_list == 1)
> +		entity->sched = sched_list[0];
> +	else
> +		entity->sched_list = sched_list;

This breaks rocket. rocket_job_open() passes a kmalloc'ed array of
rdev->num_cores schedulers, and rocket_job_close() does
kfree(entity->sched_list). With only one core bound when the file is
opened (e.g. the other cores probe later), the union now holds
&rdev->cores[0].sched, so close kfree()s a pointer into the devm
allocation. Before this patch it was kfree(NULL) (and the array leaked).

I can send a rocket fix that keeps its own pointer to the array and
frees that instead, so it can go in before this patch, unless you'd
rather carry it in this series.

Thanks,
Sidong

Re: [PATCH 4/7] drm/sched: Remove the single scheduler special case

[Sidong Yang]

On Sun, Nov 08, 2026 at 06:12:40PM +0900, Sidong Yang wrote:
> This breaks rocket.

Sorry, please disregard this. I was looking at an older tree. In
drm-misc-next rocket already keeps its own pointer to the array since
91c7da1f42ca ("drm/rocket: Keep scheduler allocation in
rocket_file_priv"), so this patch is fine for rocket.

Thanks,
Sidong

Re: [PATCH 0/7] drm/sched: Replace SPSC queue with a list and spinlock

[Philipp Stanner]

On Fri, 2026-11-06 at 14:56 +0000, Tvrtko Ursulin wrote:
> [First four patches are just my queue to avoid conflicts. I am still hoping I
> can bring them in a state where all is good since all these together simplify
> things.]

I'm confused, so is this an RFC for AMD to take a first look?

> So as requested by Philipp and as the title says - replacing the SPSC queue with
> a list and a spinlock. A first cut which passes unit tests and smoke testing on
> AMD.

Thx for working on this, it's one of the most important chunks of tech
debt!

> But it wasn't easy, since we can't really get away from some of the deeply
> ingrained scheduler design points.

What would those be?



P.

Re: [PATCH 0/7] drm/sched: Replace SPSC queue with a list and spinlock

[Tvrtko Ursulin]

On 09/11/2026 10:21, Philipp Stanner wrote:
> On Fri, 2026-11-06 at 14:56 +0000, Tvrtko Ursulin wrote:
>> [First four patches are just my queue to avoid conflicts. I am still hoping I
>> can bring them in a state where all is good since all these together simplify
>> things.]
> 
> I'm confused, so is this an RFC for AMD to take a first look?

Yes, but as the cover letter later re-iterates, skip to patches 5 and 6 
for spsc removal. Prologue is due my local patch ordering and otherwise 
there would be conflicts.

>> So as requested by Philipp and as the title says - replacing the SPSC queue with
>> a list and a spinlock. A first cut which passes unit tests and smoke testing on
>> AMD.
> 
> Thx for working on this, it's one of the most important chunks of tech
> debt!
> 
>> But it wasn't easy, since we can't really get away from some of the deeply
>> ingrained scheduler design points.
> 
> What would those be?

See patch 6. Unlocked peek from the run job singleton due entity<->rq 
lock inversion and sleeping ->prepare_job. In this RFC peek is 
implemented via list RCU helpers which make it safe because the single 
consumer is still true.

Regards,

Tvrtko

Re: [PATCH 5/7] drm/amdgpu: Rework the amdgpu_job_stop_all_jobs_on_sched() hack

[Philipp Stanner]

On Fri, 2026-11-06 at 14:57 +0000, Tvrtko Ursulin wrote: 
> -/*
> - * This is a duplicate function from DRM scheduler sched_internal.h.
> - * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is removed, due
> - * latter being incorrect and racy.
> - *
> - * See https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
> - */
>  static struct drm_sched_job *
>  drm_sched_entity_queue_pop(struct drm_sched_entity *entity)

Why remove that comment? A reader will still wonder why a drm_sched
function is defined here.

>  {
> @@ -476,28 +469,66 @@ drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>  	return container_of(node, struct drm_sched_job, queue_node);
>  }
>  
> -void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
> +static struct drm_sched_entity *
> +get_entity_with_jobs(struct drm_gpu_scheduler *sched) __acquires(&entity->lock)

[…]

> +retry:
>  	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
>  		struct drm_sched_rq *rq = sched->sched_rq[i];
> +
>  		spin_lock(&rq->lock);
> -		list_for_each_entry(s_entity, &rq->entities, list) {
> -			while ((s_job = drm_sched_entity_queue_pop(s_entity))) {
> -				struct drm_sched_fence *s_fence = s_job->s_fence;
> +		list_for_each_entry(entity, &rq->entities, list) {
> +			/*
> +			 * This can false negative, but so could the previous
> +			 * approach of scanning the run-queues once when
> +			 * nothing prevents userspace to queue more jobs.
> +			 */
> +			if (!spsc_queue_count(&entity->job_queue))
> +				continue;
>  
> -				dma_fence_signal(&s_fence->scheduled);
> -				dma_fence_set_error(&s_fence->finished, -EHWPOISON);
> -				dma_fence_signal(&s_fence->finished);
> +			/*
> +			 * We must trylock because normal locking order is
> +			 * entity then rq and here we already have rq locked
> +			 * in order to be able to walk the entities.
> +			 */
> +			if (spin_trylock(&entity->lock)) {
> +				/* Returns with a locked entity. */
> +				spin_unlock(&rq->lock);
> +				return entity;

Ah, now I understand.

Hmm. I guess considering the circumstances we're in that's the best-ish
option we have?

I hope the AMD folks can't see technical blockers.

> +			} else {
> +				/*
> +				 * Busy entity - we will try again looking for a
> +				 * window where trylock will succeed.
> +				 */
> +				spin_unlock(&rq->lock);
> +				goto retry;

In classic anti-deadlock strategies, wouldn't you want some random
delay here to avoid cycling?



P.

Re: [PATCH 5/7] drm/amdgpu: Rework the amdgpu_job_stop_all_jobs_on_sched() hack

[Tvrtko Ursulin]

On 09/11/2026 12:46, Philipp Stanner wrote:
> On Fri, 2026-11-06 at 14:57 +0000, Tvrtko Ursulin wrote:
>> -/*
>> - * This is a duplicate function from DRM scheduler sched_internal.h.
>> - * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is removed, due
>> - * latter being incorrect and racy.
>> - *
>> - * See https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
>> - */
>>   static struct drm_sched_job *
>>   drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
> 
> Why remove that comment? A reader will still wonder why a drm_sched
> function is defined here.

It effectively moved down in the diff.

>>   {
>> @@ -476,28 +469,66 @@ drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>>   	return container_of(node, struct drm_sched_job, queue_node);
>>   }
>>   
>> -void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
>> +static struct drm_sched_entity *
>> +get_entity_with_jobs(struct drm_gpu_scheduler *sched) __acquires(&entity->lock)
> 
> […]
> 
>> +retry:
>>   	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
>>   		struct drm_sched_rq *rq = sched->sched_rq[i];
>> +
>>   		spin_lock(&rq->lock);
>> -		list_for_each_entry(s_entity, &rq->entities, list) {
>> -			while ((s_job = drm_sched_entity_queue_pop(s_entity))) {
>> -				struct drm_sched_fence *s_fence = s_job->s_fence;
>> +		list_for_each_entry(entity, &rq->entities, list) {
>> +			/*
>> +			 * This can false negative, but so could the previous
>> +			 * approach of scanning the run-queues once when
>> +			 * nothing prevents userspace to queue more jobs.
>> +			 */
>> +			if (!spsc_queue_count(&entity->job_queue))
>> +				continue;
>>   
>> -				dma_fence_signal(&s_fence->scheduled);
>> -				dma_fence_set_error(&s_fence->finished, -EHWPOISON);
>> -				dma_fence_signal(&s_fence->finished);
>> +			/*
>> +			 * We must trylock because normal locking order is
>> +			 * entity then rq and here we already have rq locked
>> +			 * in order to be able to walk the entities.
>> +			 */
>> +			if (spin_trylock(&entity->lock)) {
>> +				/* Returns with a locked entity. */
>> +				spin_unlock(&rq->lock);
>> +				return entity;
> 
> Ah, now I understand.
> 
> Hmm. I guess considering the circumstances we're in that's the best-ish
> option we have?
> 
> I hope the AMD folks can't see technical blockers.

Yeah I think it's the best before AMD follows up with the promised 
complete rework of how this is handled.

I am hoping this hacky rework, assuming it is no more broken by the 
original, and that AMD can test it (I don't have the required hardware), 
can be a motivator. :)

>> +			} else {
>> +				/*
>> +				 * Busy entity - we will try again looking for a
>> +				 * window where trylock will succeed.
>> +				 */
>> +				spin_unlock(&rq->lock);
>> +				goto retry;
> 
> In classic anti-deadlock strategies, wouldn't you want some random
> delay here to avoid cycling?

I did not see a reason to not make this thread the most greedy it can 
be. Because the longer it runs the more potential for livelock has.

Same is true for the existing solution to be clear, but this patch does 
increase the window a bit by adding trylock and restart. I think. Or 
perhaps it is the same potential because all that is required in the 
current code is for an entity to keep submitting and 
amdgpu_job_stop_all_jobs_on_sched() never exits the pop loop. With this 
patch any entity doing a submit can trigger a re-scan of all entities, 
so any submitting anything new will be handled, until the time this 
loops needs to finish the re-scan is lower than the submit frequency. 
Hence I thought the faster it goes the better.

Regards,

Tvrtko

Re: [PATCH 5/7] drm/amdgpu: Rework the amdgpu_job_stop_all_jobs_on_sched() hack

[Christian König]

On 11/6/26 15:57, Tvrtko Ursulin wrote:
> amdgpu_job_stop_all_jobs_on_sched is a semi broken hack used on only one
> platform which suffers from several problems as mentioned in
> https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/.
> 
> Until it can be fully removed, given that it directly acceses DRM scheduler
> internals which want to be changed, we need to rework it to allow for
> replacing the SPSC queue with a standard spin locked list.
> 
> Problem at hand is the lock inversion between the run-queue and entity
> lock which becomes apparent once locking is added to the job queue
> amdgpu_job_stop_all_jobs_on_sched().
> 
> To work around it we change the approach of how queued jobs are drained by
> using trylock and restarting on locked entities until all queued jobs from
> all active entities are removed.
> 
> This slightly widens the existing scope for a live lock in case of
> userspace actively submitting new jobs. Whereas the current code can keep
> processing the single busy entity, this new implementation can retry on the
> entities which it previously processed.
> 
> Since userspace submitting jobs so rapidly the GPU reset cannot finish and
> the problem already exists, plus the plan is to remove this code
> altogether, we can say this is acceptable in order to unblock the DRM
> scheduler rework.
> 
> While at it we add the assert scheduler workqueue has been stopped by the
> caller and also correct the wrong order of signalling already submitted
> and queued fences.

I think that is generally the wrong approach.

See function amdgpu_ras_need_emergency_restart() who triggers the behavior when returning true:

        if (adev->asic_type == CHIP_VEGA20 &&
            adev->pm.fw_version <= 0x283400) {
                return !(amdgpu_asic_reset_method(adev) == AMD_RESET_METHOD_BACO) &&
                                amdgpu_ras_intr_triggered();
        }
        return false;

So only on Vega 20 with an outdated power management FW version we use this hack to clean up the DRM scheduler jobs.

My educated guess is the code wasn't used in at least the last 5-6 years :)

I think we should rather completely remove the amdgpu_job_stop_all_jobs_on_sched() hack and just print a big warning in the system log when we detect an outdated firmware.

Hawking and Alex any objections to that?

Regards,
Christian.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> References: https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 86 ++++++++++++++++++++-----
>  1 file changed, 69 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 8689a3c933b2..1f4a4a5a978d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -457,13 +457,6 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>  	return fence;
>  }
>  
> -/*
> - * This is a duplicate function from DRM scheduler sched_internal.h.
> - * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is removed, due
> - * latter being incorrect and racy.
> - *
> - * See https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
> - */
>  static struct drm_sched_job *
>  drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>  {
> @@ -476,28 +469,66 @@ drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>  	return container_of(node, struct drm_sched_job, queue_node);
>  }
>  
> -void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
> +static struct drm_sched_entity *
> +get_entity_with_jobs(struct drm_gpu_scheduler *sched) __acquires(&entity->lock)
>  {
> -	struct drm_sched_job *s_job;
> -	struct drm_sched_entity *s_entity = NULL;
> +	struct drm_sched_entity *entity;
>  	int i;
>  
> -	/* Signal all jobs not yet scheduled */
> +retry:
>  	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
>  		struct drm_sched_rq *rq = sched->sched_rq[i];
> +
>  		spin_lock(&rq->lock);
> -		list_for_each_entry(s_entity, &rq->entities, list) {
> -			while ((s_job = drm_sched_entity_queue_pop(s_entity))) {
> -				struct drm_sched_fence *s_fence = s_job->s_fence;
> +		list_for_each_entry(entity, &rq->entities, list) {
> +			/*
> +			 * This can false negative, but so could the previous
> +			 * approach of scanning the run-queues once when
> +			 * nothing prevents userspace to queue more jobs.
> +			 */
> +			if (!spsc_queue_count(&entity->job_queue))
> +				continue;
>  
> -				dma_fence_signal(&s_fence->scheduled);
> -				dma_fence_set_error(&s_fence->finished, -EHWPOISON);
> -				dma_fence_signal(&s_fence->finished);
> +			/*
> +			 * We must trylock because normal locking order is
> +			 * entity then rq and here we already have rq locked
> +			 * in order to be able to walk the entities.
> +			 */
> +			if (spin_trylock(&entity->lock)) {
> +				/* Returns with a locked entity. */
> +				spin_unlock(&rq->lock);
> +				return entity;
> +			} else {
> +				/*
> +				 * Busy entity - we will try again looking for a
> +				 * window where trylock will succeed.
> +				 */
> +				spin_unlock(&rq->lock);
> +				goto retry;
>  			}
>  		}
>  		spin_unlock(&rq->lock);
>  	}
>  
> +	return NULL;
> +}
> +
> +/*
> + * amdgpu_job_stop_all_jobs_on_sched has a single purpose which is an
> + * unreliable and racy, as described in:
> + *  https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/.
> + *
> + * Plan is to remove it, together with the above open coded DRM scheduler
> + * helpers, and replace with a proper solution.
> + */
> +void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
> +{
> +	struct drm_sched_job *s_job, *next;
> +	struct drm_sched_entity *s_entity;
> +	LIST_HEAD(queued);
> +
> +	WARN_ON_ONCE(!sched->pause_submit);
> +
>  	/* Signal all jobs already scheduled to HW */
>  	list_for_each_entry(s_job, &sched->pending_list, list) {
>  		struct drm_sched_fence *s_fence = s_job->s_fence;
> @@ -505,6 +536,27 @@ void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
>  		dma_fence_set_error(&s_fence->finished, -EHWPOISON);
>  		dma_fence_signal(&s_fence->finished);
>  	}
> +
> +	/*
> +	 * Signal all jobs not yet scheduled - note that the entity trylock loop
> +	 * can in theory livelock if userspace keeps submitting new jobs more
> +	 * rapidly than the loop can do a full pass over all run-queues and
> +	 * entities.
> +	 */
> +	while ((s_entity = get_entity_with_jobs(sched)) != NULL) {
> +		while ((s_job = drm_sched_entity_queue_pop(s_entity)))
> +			list_add_tail(&s_job->list, &queued);
> +		spin_unlock(&s_entity->lock);
> +	}
> +
> +	list_for_each_entry_safe(s_job, next, &queued, list) {
> +		struct drm_sched_fence *s_fence = s_job->s_fence;
> +
> +		list_del(&s_job->list);
> +		dma_fence_signal(&s_fence->scheduled);
> +		dma_fence_set_error(&s_fence->finished, -EHWPOISON);
> +		dma_fence_signal(&s_fence->finished);
> +	}
>  }
>  
>  const struct drm_sched_backend_ops amdgpu_sched_ops = {

Re: [PATCH 6/7] drm/sched: Replace the SPSC queue with a list and a spinlock

[Philipp Stanner]

Just taking a closer look now since I assumed this is some sort of RFC
for AMD to look first.

On Fri, 2026-11-06 at 14:57 +0000, Tvrtko Ursulin wrote:
> Migrate the entity job queue from the single user SPSC queue (single
> producer, single consumer) to the standard list_head plus a spinlock in
> order to make it easier to argue about correctnes and bring the code base
> close to the usual kernel patterns.
> 
> The main thing which needs to be mentioned is the seeming usage of RCU,
> which isn't really so, but only for two trivial reasons:

If that were really the only way then we would, unfortunately, not at
all end up with "usual kernel patterns" :(

> 
> 1.
> We need to use the list operations which are safe for unlocked access to
> the list head, while parallel (locked) threads can be appending other
> jobs.
> 
> This so the run job worker is able to process the job dependencies which
> includes it calling a potentially sleeping prepare_job() callback. Hence
> it is not possible to hold the entity->lock during that stage.

Why can't it pull a job from the list, release the lock and then do
that? There is only ever 1 worker at a time.

> 2.
> We need to work around the infamous entity<->rq locking inversion which
> would now trigger slightly earlier in the run job worker, from the entity
> selection stage, where there was previosuly the other "spsc queue peek"
> call site.

Is spsc_queue_peek() the only place where an inversion would occur?

> All the list_*_rcu usage stem from those two requirements and there are no
> actual RCU requirements.

IDK, having RCU functions (and all those ACCESS_ONCEs!) in the code
base that is now supposed to be a simple lock + list does not sound
like a good idea for maintainability at all, especially if it's RCU
that is no actual RCU.

[…]


>  static struct drm_sched_job *
>  drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>  {
> -	struct spsc_node *node;
> +	struct drm_sched_job *next_job, *sched_job;
>  
> -	node = spsc_queue_pop(&entity->job_queue);
> -	if (!node)
> +	lockdep_assert_held(&entity->lock);
> +
> +	sched_job = list_first_entry_or_null(&entity->job_queue,
> +					     typeof(*sched_job), queue_link);
> +	if (!sched_job)
>  		return NULL;
>  
> -	return container_of(node, struct drm_sched_job, queue_node);
> +	list_del_rcu(&sched_job->queue_link);
> +
> +#if IS_ENABLED(CONFIG_TRACEPOINTS) || IS_ENABLED(CONFIG_DRM_SCHED_KUNIT_TEST)
> +	WRITE_ONCE(entity->job_count, entity->job_count - 1);

You're literally holding the entity->lock, what do you need read once
for?

> +#endif
> +
> +	next_job = list_first_entry_or_null(&entity->job_queue,
> +					    typeof(*next_job), queue_link);
> +	if (next_job)
> +		WRITE_ONCE(entity->job_credits, next_job->credits);
> +	else
> +		WRITE_ONCE(entity->job_credits, 0);

Same.

> +
> +	return sched_job;
>  }
>  
> 

[…]


>  EXPORT_SYMBOL(drm_sched_entity_kill);
> @@ -557,10 +556,20 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>  	/* Helper to avoid dropping the reference while the entity lock is held,
>  	 * just to have some more robustness.
>  	 */
> +	struct drm_sched_job *sched_job, *next_job;
>  	struct dma_fence *prev_last_scheduled;
> -	struct drm_sched_job *sched_job;
>  
> -	sched_job = drm_sched_entity_queue_peek(entity);
> +	/*
> +	 * We cannot lock the entity at this point since processing dependencies
> +	 * needs to be able to call a sleeping ops->prepare_job(). Note that RCU
> +	 * lock is not stricly required since single workqueue is the only
> +	 * caller and that guarantees a single consumer. Ie. the job cannot
> +	 * disappear on us.
> +	 */
> +	rcu_read_lock();
> +	sched_job = list_first_or_null_rcu(&entity->job_queue,
> +					   typeof(*sched_job), queue_link);
> +	rcu_read_unlock();

You can take the lock, take the job, release the lock, do your sleeping
actions, and then take the lock again, can't you?


P.

Re: [PATCH 6/7] drm/sched: Replace the SPSC queue with a list and a spinlock

[Tvrtko Ursulin]

On 10/11/2026 15:49, Philipp Stanner wrote:
> Just taking a closer look now since I assumed this is some sort of RFC
> for AMD to look first.
> 
> On Fri, 2026-11-06 at 14:57 +0000, Tvrtko Ursulin wrote:
>> Migrate the entity job queue from the single user SPSC queue (single
>> producer, single consumer) to the standard list_head plus a spinlock in
>> order to make it easier to argue about correctnes and bring the code base
>> close to the usual kernel patterns.
>>
>> The main thing which needs to be mentioned is the seeming usage of RCU,
>> which isn't really so, but only for two trivial reasons:
> 
> If that were really the only way then we would, unfortunately, not at
> all end up with "usual kernel patterns" :(
> 
>>
>> 1.
>> We need to use the list operations which are safe for unlocked access to
>> the list head, while parallel (locked) threads can be appending other
>> jobs.
>>
>> This so the run job worker is able to process the job dependencies which
>> includes it calling a potentially sleeping prepare_job() callback. Hence
>> it is not possible to hold the entity->lock during that stage.
> 
> Why can't it pull a job from the list, release the lock and then do
> that? There is only ever 1 worker at a time.
> 
>> 2.
>> We need to work around the infamous entity<->rq locking inversion which
>> would now trigger slightly earlier in the run job worker, from the entity
>> selection stage, where there was previosuly the other "spsc queue peek"
>> call site.
> 
> Is spsc_queue_peek() the only place where an inversion would occur?

Ignoring amdgpu yes.

Regarding spsc_queue_peek(), the current code has three of those:

1. drm_sched_rq_pop_entity()

This call site is not a lock inversion site.

Instead of the peek, the next job now gets passed in by the caller, 
drm_sched_entity_pop_job(), which is correctly holding entity->lock.

2. drm_sched_can_queue()

This is a lock inversion site since it runs under the rq->lock during 
entity selection phase of drm_sched_run_job_work().

The purpose of the peek is to answer "does the first queued job for this 
entity required less or equal to credits than this scheduler instance 
can queue".

Since this is a lock inversion site we cannot lock but truly have to 
have an unlocked solution. In this patch I removed the requirement to 
look at the job queue by tracking the credits requirement of the first 
job in the entity itself (entity->job_credits).

This is effectively replacing one unlocked read (spsc_queue_peek) with a 
different one (READ_ONCE(entity->job_credits)). Something unlocked is 
unavoidable here and I think this is more elegant and simple.

And because it is an unlocked read it has to be annotated with 
READ_ONCE(). Which means the write side, even though it is done under 
the lock, is paired up and marked with WRITE_ONCE().

2. drm_sched_entity_pop_job()

This is not a lock inversion site but needs to call sleepable things 
while considering the head of the job queue (->prepare_job()).

This one is the only remaining reason I used list_add_tail_rcu and 
list_del_rcu, just so from here we can safely touch the first entry 
while elsewhere more are being added.

It is possible to do it, as you suggest, by dropping and re-acquiring 
the lock. Note however that too would need a big fat comment why it is 
safe. At which point, since both cases have a big fat comment explaining 
the same thing apart from the implementation details, I do not see the 
advantage of pointless lock cycling. READ_ONCE and WRITE_ONCE you 
dislike would still remain.

As a side note it would be really good to eliminate the prepare_job 
callback but I haven't look how feasible that would be troughout the 
tree in around two years. Back then I tried to call it earlier, and put 
the fence in the normal dependency array, but that did not work at least 
for amdgpu.

>> All the list_*_rcu usage stem from those two requirements and there are no
>> actual RCU requirements.
> 
> IDK, having RCU functions (and all those ACCESS_ONCEs!) in the code
> base that is now supposed to be a simple lock + list does not sound
> like a good idea for maintainability at all, especially if it's RCU
> that is no actual RCU.

For this and the following questions, I've tried to explain everything 
in the consolidated paragraph just above.

Regards,

Tvrtko

> 
> […]
> 
> 
>>   static struct drm_sched_job *
>>   drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>>   {
>> -	struct spsc_node *node;
>> +	struct drm_sched_job *next_job, *sched_job;
>>   
>> -	node = spsc_queue_pop(&entity->job_queue);
>> -	if (!node)
>> +	lockdep_assert_held(&entity->lock);
>> +
>> +	sched_job = list_first_entry_or_null(&entity->job_queue,
>> +					     typeof(*sched_job), queue_link);
>> +	if (!sched_job)
>>   		return NULL;
>>   
>> -	return container_of(node, struct drm_sched_job, queue_node);
>> +	list_del_rcu(&sched_job->queue_link);
>> +
>> +#if IS_ENABLED(CONFIG_TRACEPOINTS) || IS_ENABLED(CONFIG_DRM_SCHED_KUNIT_TEST)
>> +	WRITE_ONCE(entity->job_count, entity->job_count - 1);
> 
> You're literally holding the entity->lock, what do you need read once
> for?
> 
>> +#endif
>> +
>> +	next_job = list_first_entry_or_null(&entity->job_queue,
>> +					    typeof(*next_job), queue_link);
>> +	if (next_job)
>> +		WRITE_ONCE(entity->job_credits, next_job->credits);
>> +	else
>> +		WRITE_ONCE(entity->job_credits, 0);
> 
> Same.
> 
>> +
>> +	return sched_job;
>>   }
>>   
>>
> 
> […]
> 
> 
>>   EXPORT_SYMBOL(drm_sched_entity_kill);
>> @@ -557,10 +556,20 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>>   	/* Helper to avoid dropping the reference while the entity lock is held,
>>   	 * just to have some more robustness.
>>   	 */
>> +	struct drm_sched_job *sched_job, *next_job;
>>   	struct dma_fence *prev_last_scheduled;
>> -	struct drm_sched_job *sched_job;
>>   
>> -	sched_job = drm_sched_entity_queue_peek(entity);
>> +	/*
>> +	 * We cannot lock the entity at this point since processing dependencies
>> +	 * needs to be able to call a sleeping ops->prepare_job(). Note that RCU
>> +	 * lock is not stricly required since single workqueue is the only
>> +	 * caller and that guarantees a single consumer. Ie. the job cannot
>> +	 * disappear on us.
>> +	 */
>> +	rcu_read_lock();
>> +	sched_job = list_first_or_null_rcu(&entity->job_queue,
>> +					   typeof(*sched_job), queue_link);
>> +	rcu_read_unlock();
> 
> You can take the lock, take the job, release the lock, do your sleeping
> actions, and then take the lock again, can't you?
> 
> 
> P.

Re: [PATCH 6/7] drm/sched: Replace the SPSC queue with a list and a spinlock

[Tvrtko Ursulin]

On 12/11/2026 14:09, Philipp Stanner wrote:
> On Thu, 2026-11-12 at 12:59 +0000, Tvrtko Ursulin wrote:
>> Since this is a lock inversion site we cannot lock
> 
> Which deadlock-circumvention strategies have you tried?
> 
> Pseudocode:
> 
> bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
> 			 struct drm_sched_entity *entity)
> {
> 	struct drm_sched_job *s_job;
> 
> +	lockdep_assert_held(&entity->lock)
> 
> 	s_job = drm_sched_entity_queue_peek(entity);
> 
> 
> drm_sched_rq_select_entity(…)
> {
> retry:
> 	spin_lock(&rq->lock);
> 	
> 	walk_tree(…) {
> 		if (!spin_trylock(&entity->lock) {
> 			spin_unlock(&rq-lock);
> 			udelay(20);
> 			goto retry;
> 		}
> 		ready = !list_empty(&entity->job_list);
> 	}
> }
> 
> 
> It appears very undesirable for someone to re-walk that tree, I get
> that. But do we even know whether that would be a measurable problem in
> practice? How often would it occur?

Problem there is adding trylock+backoff to achieve MPMC which the 
scheduler is designed not to be. Without changing the whole design to 
truly be MPMC, making the locking MPMC is just overhead. Do you see a 
route to achieve full MPMC? Do you plan to pursue it?

> Alternatively, all the other paths that do entity_lock -> rq_lock could
> be tested with trylock-patterns. They might be less impacted.
> 
> In drm_sched_entity_push_job(), for example, only occasionally are both
> locks actually needed, when a job is the first job.
> 
>>   but truly have to have an unlocked solution.
> 
> How do you know that?

Please don't cut too much context. I guess you were in a way alluding 
again to your trylock proposal.

>> This is effectively replacing one unlocked read (spsc_queue_peek) with a
>> different one (READ_ONCE(entity->job_credits)). Something unlocked is
>> unavoidable here and I think this is more elegant and simple.
> 
> Code full of ACCESS_ONCE and RCU that is not RCU is most certainly not
> elegant.
> 
> This is elegant:
> 
> spin_lock()
> list_del()
> spin_unlock()
> 
> 
> With ACCESS_ONCE, manual barriers etc. pp. no one ever knows what's
> accessed where with what ordering and why. It would then be better to
> have the problem remain centralized in spsc_queue.h
> 
> As you say yourself, this here replaces the half-locked hard to reason
> about spsc queue (we don't even know whether that thing is formally
> correct) with a half-locked "locked" linked list.
> 
> It's no improvement in any way regarding our shared goal, which is a
> consistently locked codebase that finally rids us of the race
> conditions which are plagueing this system for >10 years

How it is not an improvement in _any_ way if in your own words we can 
remove the hand rolled single kernel user spsc which we don't even know 
is formally correct?

> There is really no other way than either redesigning drm_sched so that
> the lock inversion disappears altogether, or to address the inversion
> with strategies well established in CS.
> 
>>
>> And because it is an unlocked read it has to be annotated with
>> READ_ONCE(). Which means the write side, even though it is done under
>> the lock, is paired up and marked with WRITE_ONCE().
>>
>> 2. drm_sched_entity_pop_job()
>>
>> This is not a lock inversion site but needs to call sleepable things
>> while considering the head of the job queue (->prepare_job()).
>>
>> This one is the only remaining reason I used list_add_tail_rcu and
>> list_del_rcu, just so from here we can safely touch the first entry
>> while elsewhere more are being added.
>>
>> It is possible to do it, as you suggest, by dropping and re-acquiring
>> the lock. Note however that too would need a big fat comment why it is
>> safe.
> 
> Sure, one can write code comments. No big deal. The job is not in the
> list anymore. The list is lock-protected. No one can race it.
> 
> There's only one worker popping. This must always be that way, the
> entire drm_sched design is founded on that principle.

That was my point. :) Same comment applies to both patterns. The rest is 
just implementation details. Then what remains is the question of what 
is the least bad pattern, is it trylock+back-off & re-locking, or is it 
list_add_tail_rcu & remove the need to access the queue in most places 
to begin with.

Anyway, it seems to two main points of view you expressed is to either 
do it with trylock+backoff or to redesign the whole scheduler to 
eliminate entity and run-queue locking need/order. Latter I thought it 
was agreed is not feasible and that instead a new scheduler will be 
written for new and firmware drivers so they can migrate to it. Equally 
as attempts to split the current one into frontend/backend parts was 
rejected. So I honestly don't know what is the plan any more.

Regards,

Tvrtko

> 
>>   At which point, since both cases have a big fat comment explaining
>> the same thing apart from the implementation details, I do not see the
>> advantage of pointless lock cycling. READ_ONCE and WRITE_ONCE you
>> dislike would still remain.
>>
>> As a side note it would be really good to eliminate the prepare_job
>> callback
> 
> Yes, the user should prepare everything needed before even submitting a
> job. Maybe we can begin by marking it as deprecated.
> 
> 
> 
> 
> P.