[PATCH 0/9] DRM scheduler assorted cleanups

[PATCH 0/9] DRM scheduler assorted cleanups

[Tvrtko Ursulin]

A little bit of a dump of my patch queue leading towards the SPSC queue removal
(which is still WIP). Primarily because Philipp asked me to rebase and re-post
"drm/sched: Replace completion with a flush".

Then I realised we agreed to split out one part of that in a separate patch.
Specifically "drm/sched: Ignore stopped entity attempt to change its run-queue".
And I also realized that one is easier to argue for if we also try to tighten
the rules around entity initialization. Which is "drm/sched: Disallow
initializing entities with un-initialized schedulers".

Then there are some trivial cleanups in the same area which should be easy
reviews.

The first two patches of the series feel free to ignore. I have sent those
separately already but need to include them so the series actually applies, both
for CI, and should someone want to apply the series.

Last patch in the series is something I would like to get accepted as well since
I think the current list ownership rules are quite bad. As the patch touches the
same functions as the SPSC removal it would be good to get it in first.

Immediate questions are whether patches 3 & 4 are the simplifications which can
indeed be made without any drivers crying foul.

And of course what our AI overlord will have to say on it all.

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 (9):
  drm/sched: Fix out of bounds entity priority check
  drm/sched: Document entity->stopped is protect by entity->lock
  drm/sched: Disallow initializing entities with un-initialized
    schedulers
  drm/sched: Reject double scheduler initialization
  drm/sched: Remove log message of questionable use
  drm/sched: Remove useless log message on memory exhaustion
  drm/sched: Ignore stopped entity attempt to change its run-queue
  drm/sched: Replace completion with a flush
  drm/sched: Untangle the scheduler list mess

 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c    |  31 +++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h    |   2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c  |  13 +-
 drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c      |   3 +-
 drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c      |   3 +-
 drivers/gpu/drm/scheduler/sched_entity.c   | 138 +++++++++++++--------
 drivers/gpu/drm/scheduler/sched_internal.h |  14 ++-
 drivers/gpu/drm/scheduler/sched_main.c     |  24 +---
 drivers/gpu/drm/scheduler/sched_rq.c       |  19 ++-
 include/drm/gpu_scheduler.h                |  59 +++++----
 10 files changed, 179 insertions(+), 127 deletions(-)

-- 
2.55.0

[PATCH 1/9] drm/sched: Fix out of bounds entity priority check

[Tvrtko Ursulin]

When the out of range priority at entity init time is clamped to the
number of run queues in the supplied scheduler the internal run-queue
priority needs to be adjusted as well.

There is no impact since none of the drivers hit the clamping path, but
we still need to address it by moving the entity->rq_priority assignment
to after the clamping is done.

Note that a lot of this will hopefully get removed once again when the
single run-queue simplification lands.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Fixes: 2fa4d8e2c109 ("drm/sched: Add fair scheduling policy")
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: <stable@vger.kernel.org> # v7.2+
---
 drivers/gpu/drm/scheduler/sched_entity.c | 27 +++++++++++-------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 7be0387bac3c..e12709b7bd0c 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -130,10 +130,7 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 
 	INIT_LIST_HEAD(&entity->list);
 	entity->guilty = guilty;
-	entity->priority = priority;
 	entity->last_user = current->group_leader;
-	entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_FAIR ?
-			      DRM_SCHED_PRIORITY_KERNEL : priority;
 	entity->num_sched_list = num_sched_list;
 	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
 	RB_CLEAR_NODE(&entity->rb_tree_node);
@@ -144,25 +141,25 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 		 * not to do this and to fix their DRM calling order.
 		 */
 		pr_warn("%s: called with uninitialized scheduler\n", __func__);
-	} else {
-		enum drm_sched_priority p = entity->priority;
-
+	} else 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,
 		 * by converting to signed. Choose the lowest priority
 		 * available.
 		 */
-		if (p >= sched_list[0]->num_user_rqs) {
-			dev_err(sched_list[0]->dev, "entity with out-of-bounds priority:%u num_user_rqs:%u\n",
-				p, sched_list[0]->num_user_rqs);
-			p = max_t(s32,
-				  (s32)sched_list[0]->num_user_rqs - 1,
-				  (s32)DRM_SCHED_PRIORITY_KERNEL);
-			entity->priority = p;
-		}
+		dev_err(sched_list[0]->dev, "entity with out-of-bounds priority:%u num_user_rqs:%u\n",
+			priority, sched_list[0]->num_user_rqs);
+		priority = max_t(s32,
+				 (s32)sched_list[0]->num_user_rqs - 1,
+				 (s32)DRM_SCHED_PRIORITY_KERNEL);
+	}
+
+	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];
-	}
 
 	init_completion(&entity->entity_idle);
 
-- 
2.55.0

[PATCH 2/9] drm/sched: Document entity->stopped is protect by entity->lock

[Tvrtko Ursulin]

Commit
0e118b936dc5 ("drm/sched: Lock drm_sched_entity_is_idle()")
has removed the last unlocked entity->stopped access so lets officially
document it in kerneldoc as locked with the entity lock.

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>
---
 include/drm/gpu_scheduler.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 563d7fb88a6d..3b0978fa8cf0 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -101,7 +101,7 @@ struct drm_sched_entity {
 	 *
 	 * Lock protecting the run-queue (@rq) to which this entity belongs,
 	 * @priority, @last_scheduled and the list of schedulers (@sched_list,
-	 * @num_sched_list) and @rr_ts.
+	 * @num_sched_list), @rr_ts and @stopped.
 	 */
 	spinlock_t			lock;
 
-- 
2.55.0

[PATCH 3/9] 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 | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index e12709b7bd0c..95bb79b3d003 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);
 
-- 
2.55.0

[PATCH 4/9] drm/sched: Reject double scheduler initialization

[Tvrtko Ursulin]

There is no point in having code and logging whose only purpose is to
notify developers of incorrect usage of the API while bringing up a new
driver. Lets simply return an error and let the developers figure it out.

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_main.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index aba5545033ac..bb89c169d553 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1102,6 +1102,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
 {
 	int i;
 
+	if (sched->sched_rq)
+		return -EINVAL;
+
 	sched->ops = args->ops;
 	sched->credit_limit = args->credit_limit;
 	sched->name = args->name;
@@ -1117,13 +1120,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
 		dev_err(sched->dev, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
 			__func__);
 		return -EINVAL;
-	} else if (sched->sched_rq) {
-		/* Not an error, but warn anyway so drivers can
-		 * fine-tune their DRM calling order, and return all
-		 * is good.
-		 */
-		dev_warn(sched->dev, "%s: scheduler already initialized!\n", __func__);
-		return 0;
 	}
 
 	if (args->submit_wq) {
-- 
2.55.0

[PATCH 5/9] drm/sched: Remove log message of questionable use

[Tvrtko Ursulin]

There is no point in having a log message whose only purpose is to
notify developers of incorrect usage of the API while bringing up a new
driver. Lets simply return an error and let the developers figure it out.

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_main.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index bb89c169d553..76acd0e4339c 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1105,6 +1105,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
 	if (sched->sched_rq)
 		return -EINVAL;
 
+	if (args->num_rqs > DRM_SCHED_PRIORITY_COUNT)
+		return -EINVAL;
+
 	sched->ops = args->ops;
 	sched->credit_limit = args->credit_limit;
 	sched->name = args->name;
@@ -1114,14 +1117,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
 	sched->score = args->score ? args->score : &sched->_score;
 	sched->dev = args->dev;
 
-	if (args->num_rqs > DRM_SCHED_PRIORITY_COUNT) {
-		/* This is a gross violation--tell drivers what the  problem is.
-		 */
-		dev_err(sched->dev, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
-			__func__);
-		return -EINVAL;
-	}
-
 	if (args->submit_wq) {
 		sched->submit_wq = args->submit_wq;
 		sched->own_submit_wq = false;
-- 
2.55.0

[PATCH 6/9] drm/sched: Remove useless log message on memory exhaustion

[Tvrtko Ursulin]

If scheduler initialization has failed due failure to allocate a handful
of bytes for the run-queue backing data structure, there will be plenty of
loud messages in the kernel log. We can safely remove the one
drm_sched_init() logs.

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_main.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 76acd0e4339c..00eb017fbcec 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1165,7 +1165,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched, const struct drm_sched_init_
 Out_check_own:
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
-	dev_err(sched->dev, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
 	return -ENOMEM;
 }
 EXPORT_SYMBOL(drm_sched_init);
-- 
2.55.0

[PATCH 7/9] 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 95bb79b3d003..3ca671af02ed 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -584,13 +584,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;
@@ -602,6 +605,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.55.0

[PATCH 8/9] 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 3ca671af02ed..79ea20838328 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);
 
@@ -304,16 +299,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 00eb017fbcec..182d4546ac9c 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1034,7 +1034,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;
 	}
@@ -1050,7 +1049,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.55.0

[PATCH 9/9] drm/sched: Untangle the scheduler list mess

[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 remained - this 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").)

The last problem was ultimately indirectly fixed by the removal of multiple
run-queues in commit
77a6809f1dc3 ("drm/sched: Remove FIFO and RR and simplify to a single run queue")
. After that commit all drivers and all engines (queue types) can now do
dynamic priority changes if they so wish.

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 is 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 is the only such caller,
and there the container is not on the stack. Keeping a stack backed list
in the entity would obviously be undefined behaviour if the list was kept.

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, document how the scheduler behaves, and
make it behave consistently and sensibly, lets make the entity own its own
copy of the scheduler list. This allows removal of the special cases which
needed to artificially clear entity->sched_list for a single scheduler
case. Individual scheduler pointers are still owned by the driver with
unchanged lifetime rules.

To enable maintaining its own container some API calls needed to grow a
capability for returning success/failure, which is a change which
percolates mostly through amdgpu.

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: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Philipp Stanner <phasta@kernel.org>
Cc: amd-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c   | 31 +++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h   |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 13 ++--
 drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c     |  3 +-
 drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c     |  3 +-
 drivers/gpu/drm/scheduler/sched_entity.c  | 85 ++++++++++++++++-------
 include/drm/gpu_scheduler.h               | 48 +++++++------
 7 files changed, 119 insertions(+), 66 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
index acafa8c59cc9..6500e7f69f5e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
@@ -791,15 +791,15 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
 	return fence;
 }
 
-static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
-					   struct amdgpu_ctx_entity *aentity,
-					   int hw_ip,
-					   int32_t priority)
+static int amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
+					  struct amdgpu_ctx_entity *aentity,
+					  int hw_ip,
+					  int32_t priority)
 {
 	struct amdgpu_device *adev = ctx->mgr->adev;
-	unsigned int hw_prio;
 	struct drm_gpu_scheduler **scheds = NULL;
-	unsigned num_scheds;
+	unsigned int hw_prio, num_scheds;
+	int ret = 0;
 
 	/* set sw priority */
 	drm_sched_entity_set_priority(&aentity->entity,
@@ -811,16 +811,18 @@ static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
 		hw_prio = array_index_nospec(hw_prio, AMDGPU_RING_PRIO_MAX);
 		scheds = adev->gpu_sched[hw_ip][hw_prio].sched;
 		num_scheds = adev->gpu_sched[hw_ip][hw_prio].num_scheds;
-		drm_sched_entity_modify_sched(&aentity->entity, scheds,
-					      num_scheds);
+		ret = drm_sched_entity_modify_sched(&aentity->entity, scheds,
+						    num_scheds);
 	}
+
+	return ret;
 }
 
-void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
-				  int32_t priority)
+int amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, int32_t priority)
 {
 	int32_t ctx_prio;
 	unsigned i, j;
+	int ret;
 
 	ctx->override_priority = priority;
 
@@ -831,10 +833,15 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
 			if (!ctx->entities[i][j])
 				continue;
 
-			amdgpu_ctx_set_entity_priority(ctx, ctx->entities[i][j],
-						       i, ctx_prio);
+			ret = amdgpu_ctx_set_entity_priority(ctx,
+							     ctx->entities[i][j],
+							     i, ctx_prio);
+			if (ret)
+				return ret;
 		}
 	}
+
+	return 0;
 }
 
 int amdgpu_ctx_wait_prev_fence(struct amdgpu_ctx *ctx,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
index a4b89eca4169..c44349f310f4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.h
@@ -86,7 +86,7 @@ struct dma_fence *amdgpu_ctx_get_fence(struct amdgpu_ctx *ctx,
 				       struct drm_sched_entity *entity,
 				       uint64_t seq);
 bool amdgpu_ctx_priority_is_valid(int32_t ctx_prio);
-void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, int32_t ctx_prio);
+int amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, int32_t ctx_prio);
 
 int amdgpu_ctx_ioctl(struct drm_device *dev, void *data,
 		     struct drm_file *filp);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
index 8effb1158430..0efe90767bfb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
@@ -51,11 +51,14 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev,
 
 	mgr = &fpriv->ctx_mgr;
 	xa_lock(&mgr->ctx_handles);
-	xa_for_each(&mgr->ctx_handles, id, ctx)
-		amdgpu_ctx_priority_override(ctx, priority);
+	xa_for_each(&mgr->ctx_handles, id, ctx) {
+		r = amdgpu_ctx_priority_override(ctx, priority);
+		if (r)
+			break;
+	}
 	xa_unlock(&mgr->ctx_handles);
 
-	return 0;
+	return r;
 }
 
 static int amdgpu_sched_context_priority_override(struct amdgpu_device *adev,
@@ -80,9 +83,9 @@ static int amdgpu_sched_context_priority_override(struct amdgpu_device *adev,
 	if (!ctx)
 		return -EINVAL;
 
-	amdgpu_ctx_priority_override(ctx, priority);
+	r = amdgpu_ctx_priority_override(ctx, priority);
 	amdgpu_ctx_put(ctx);
-	return 0;
+	return r;
 }
 
 int amdgpu_sched_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
index 0f4f4d66e2e4..40ea94e31fc1 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v3_0.c
@@ -1901,8 +1901,7 @@ static int vcn_v3_0_limit_sched(struct amdgpu_cs_parser *p,
 
 	scheds = p->adev->gpu_sched[AMDGPU_HW_IP_VCN_DEC]
 		[AMDGPU_RING_PRIO_DEFAULT].sched;
-	drm_sched_entity_modify_sched(job->base.entity, scheds, 1);
-	return 0;
+	return drm_sched_entity_modify_sched(job->base.entity, scheds, 1);
 }
 
 static int vcn_v3_0_dec_msg(struct amdgpu_cs_parser *p, struct amdgpu_job *job,
diff --git a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
index 4b7f7bbe18da..6ca8430fa426 100644
--- a/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/vcn_v4_0.c
@@ -1817,8 +1817,7 @@ static int vcn_v4_0_limit_sched(struct amdgpu_cs_parser *p,
 
 	scheds = p->adev->gpu_sched[AMDGPU_HW_IP_VCN_ENC]
 		[AMDGPU_RING_PRIO_0].sched;
-	drm_sched_entity_modify_sched(job->base.entity, scheds, 1);
-	return 0;
+	return drm_sched_entity_modify_sched(job->base.entity, scheds, 1);
 }
 
 static int vcn_v4_0_dec_msg(struct amdgpu_cs_parser *p, struct amdgpu_job *job,
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 79ea20838328..a2e043df1061 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -102,7 +102,12 @@ 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 at least one element to schedule the
+ * entity.
+ *
+ * The individual drm_gpu_scheduler pointers have borrow semantics, ie.
+ * they must remain valid during entities lifetime, while the containing
+ * array can be freed after this call returns.
  *
  * For changing @priority later on at runtime see
  * drm_sched_entity_set_priority(). For changing the set of schedulers
@@ -119,16 +124,11 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 			  unsigned int num_sched_list,
 			  atomic_t *guilty)
 {
-	unsigned int i;
+	int ret;
 
-	if (!entity || !sched_list || !num_sched_list)
+	if (!entity)
 		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();
@@ -138,8 +138,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	INIT_LIST_HEAD(&entity->list);
 	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;
 	RB_CLEAR_NODE(&entity->rb_tree_node);
 
 	if (priority >= sched_list[0]->num_user_rqs) {
@@ -159,11 +157,18 @@ 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;
-	entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
 
 	spin_lock_init(&entity->lock);
 	spsc_queue_init(&entity->job_queue);
 
+	ret = drm_sched_entity_modify_sched(entity, sched_list, num_sched_list);
+	if (ret) {
+		drm_sched_entity_stats_put(entity->stats);
+		return ret;
+	}
+
+	drm_sched_entity_select_rq(entity);
+
 	atomic_set(&entity->fence_seq, 0);
 	entity->fence_context = dma_fence_context_alloc(2);
 
@@ -178,21 +183,53 @@ EXPORT_SYMBOL(drm_sched_entity_init);
  *		 existing entity->sched_list
  * @num_sched_list: number of drm sched in sched_list
  *
+ * The individual drm_gpu_scheduler pointers have borrow semantics, ie.
+ * they must remain valid during entities lifetime, while the containing
+ * array can be freed after this call returns.
+ *
  * 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
  * can be pushed to @entity.
+ *
+ * Returns zero on success and a negative error code on failure.
  */
-void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
-				    struct drm_gpu_scheduler **sched_list,
-				    unsigned int num_sched_list)
+int 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);
+	struct drm_gpu_scheduler **newscheds, **oldscheds;
+	unsigned int i;
+
+	if (!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;
+	}
+
+	if (num_sched_list > 1) {
+		newscheds = kmemdup_array(sched_list, num_sched_list,
+					  sizeof(*sched_list), GFP_KERNEL);
+		if (!newscheds)
+			return -ENOMEM;
+	}
 
 	spin_lock(&entity->lock);
-	entity->sched_list = sched_list;
+	if (num_sched_list == 1) {
+		entity->sched = sched_list[0];
+		oldscheds = NULL;
+	} else {
+		oldscheds = entity->sched_list;
+		entity->sched_list = newscheds;
+	}
 	entity->num_sched_list = num_sched_list;
 	spin_unlock(&entity->lock);
+
+	kfree(oldscheds);
+
+	return 0;
 }
 EXPORT_SYMBOL(drm_sched_entity_modify_sched);
 
@@ -413,6 +450,9 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity)
 
 	dma_fence_put(entity->last_scheduled);
 	drm_sched_entity_stats_put(entity->stats);
+
+	if (entity->num_sched_list > 1)
+		kfree(entity->sched_list);
 }
 EXPORT_SYMBOL(drm_sched_entity_fini);
 
@@ -577,10 +617,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;
@@ -597,16 +633,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..97b72bcbfe0b 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:
@@ -721,9 +729,9 @@ void drm_sched_entity_destroy(struct drm_sched_entity *entity);
 void drm_sched_entity_set_priority(struct drm_sched_entity *entity,
 				   enum drm_sched_priority priority);
 int drm_sched_entity_error(struct drm_sched_entity *entity);
-void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
-				   struct drm_gpu_scheduler **sched_list,
-				   unsigned int num_sched_list);
+int drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
+				  struct drm_gpu_scheduler **sched_list,
+				  unsigned int num_sched_list);
 
 /**
  * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
-- 
2.55.0

Re: [PATCH 8/9] drm/sched: Replace completion with a flush

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/sched: cross-entity deadlock via flush_work() in drm_sched_entity_kill()
--

commit 3786a3d89362d00183a97f456e4eaeb1dff144f4
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

drm/sched: Replace completion with a flush

This commit removes the per-entity completion used to synchronize entity
teardown, replacing it with a direct flush of the scheduler's run job
worker. The goal is to achieve the same synchronization guarantee without
requiring the scheduler to signal a completion when an entity goes idle.

> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
[ ... ]
> @@ -304,16 +299,23 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f,
>  void drm_sched_entity_kill(struct drm_sched_entity *entity)
>  {
[ ... ]
> -	/* 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);

[Severity: High]
Could this introduce a cross-entity deadlock regression during context teardown?

Because drm_sched_flush_run_work calls flush_work on the scheduler's shared
worker, it synchronously waits for the worker to finish its current execution.
This might cause it to wait on a job belonging to a completely different
entity.

If a process destroys its device context and triggers drm_sched_entity_kill
while holding a driver-specific lock, and the shared worker is concurrently
running drm_sched_run_job_work for an unrelated entity, that driver-provided
run_job callback might attempt to acquire the same driver lock.

The original code used a per-entity completion (entity_idle) which explicitly
avoided waiting on other entities. Does this flush create a new cross-entity
dependency that could lead to a deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002154713.77591-1-tvrtko.ursulin@igalia.com?part=8

Re: [PATCH 9/9] drm/sched: Untangle the scheduler list mess

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] drm/sched: memory corruption and leak due to union aliasing in drm_sched_entity_modify_sched
- [High] drm/amdgpu: sleeping in atomic context during priority override
- [High] drm/sched: NULL pointer dereference in drm_sched_entity_init due to missing checks
- [Medium] drm/amdgpu: incomplete state rollback on priority update failure
--

commit ab2f90841362b0f1b7727691f75f0960403af244
Author: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Date:   Fri Oct 2 15:47:13 2026 +0000

    drm/sched: Untangle the scheduler list mess

This commit documents how the scheduler behaves and makes the entity own
its own copy of the scheduler list to untangle the API. Some API calls
are updated to return success/failure, which percolates mostly through
amdgpu priority overrides.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> index acafa8c59cc90..6500e7f69f5e0 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c
[ ... ]
> @@ -811,16 +811,18 @@ static void amdgpu_ctx_set_entity_priority(struct amdgpu_ctx *ctx,
>  		hw_prio = array_index_nospec(hw_prio, AMDGPU_RING_PRIO_MAX);
>  		scheds = adev->gpu_sched[hw_ip][hw_prio].sched;
>  		num_scheds = adev->gpu_sched[hw_ip][hw_prio].num_scheds;
> -		drm_sched_entity_modify_sched(&aentity->entity, scheds,
> -					      num_scheds);
> +		ret = drm_sched_entity_modify_sched(&aentity->entity, scheds,
> +						    num_scheds);
>  	}
> +
> +	return ret;
>  }
>  
> -void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
> -				  int32_t priority)
> +int amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, int32_t priority)
>  {
>  	int32_t ctx_prio;
>  	unsigned i, j;
> +	int ret;
>  
>  	ctx->override_priority = priority;
>  
> @@ -831,10 +833,15 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx,
>  			if (!ctx->entities[i][j])
>  				continue;
>  
> -			amdgpu_ctx_set_entity_priority(ctx, ctx->entities[i][j],
> -						       i, ctx_prio);
> +			ret = amdgpu_ctx_set_entity_priority(ctx,
> +							     ctx->entities[i][j],
> +							     i, ctx_prio);
> +			if (ret)
> +				return ret;
>  		}
>  	}
> +
> +	return 0;
>  }

[Severity: Medium]
If drm_sched_entity_modify_sched() fails (e.g. from an -ENOMEM during its
allocation), isn't the software priority already modified unconditionally
by the earlier call to drm_sched_entity_set_priority()?

Additionally, when an error is returned to amdgpu_ctx_priority_override(),
the loop exits early, returning the error. Does this leave the context
partially updated without rolling back the priorities of the previous
entities?

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> index 8effb1158430d..0efe90767bfba 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c
> @@ -51,11 +51,14 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev,
>  
>  	mgr = &fpriv->ctx_mgr;
>  	xa_lock(&mgr->ctx_handles);
> -	xa_for_each(&mgr->ctx_handles, id, ctx)
> -		amdgpu_ctx_priority_override(ctx, priority);
> +	xa_for_each(&mgr->ctx_handles, id, ctx) {
> +		r = amdgpu_ctx_priority_override(ctx, priority);
> +		if (r)
> +			break;
> +	}
>  	xa_unlock(&mgr->ctx_handles);
>  
> -	return 0;
> +	return r;
>  }

[Severity: High]
Does this sequence lead to sleeping in atomic context?

amdgpu_sched_process_priority_override() acquires the mgr->ctx_handles
spinlock via xa_lock(), entering an atomic context, and calls:

  amdgpu_ctx_priority_override() ->
    amdgpu_ctx_set_entity_priority() ->
      drm_sched_entity_modify_sched()

If the engine has multiple schedulers, drm_sched_entity_modify_sched()
dynamically allocates memory using GFP_KERNEL, which implies
__GFP_DIRECT_RECLAIM and can sleep. Can this trigger a Scheduling while
atomic BUG?

> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 79ea20838328d..a2e043df10619 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
[ ... ]
> @@ -119,16 +124,11 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>  			  unsigned int num_sched_list,
>  			  atomic_t *guilty)
>  {
> -	unsigned int i;
> +	int ret;
>  
> -	if (!entity || !sched_list || !num_sched_list)
> +	if (!entity)
>  		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();
> @@ -138,8 +138,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>  	INIT_LIST_HEAD(&entity->list);
>  	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;
>  	RB_CLEAR_NODE(&entity->rb_tree_node);
>  
>  	if (priority >= sched_list[0]->num_user_rqs) {

[Severity: High]
The fail-safe checks for sched_list and num_sched_list were moved down to
drm_sched_entity_modify_sched(), but sched_list[0] is still dereferenced
here before that new validation takes place.

If a driver initializes an entity with an empty scheduler list, won't this
immediately result in a NULL pointer or out-of-bounds dereference instead
of a graceful -EINVAL?

> @@ -178,21 +183,53 @@ EXPORT_SYMBOL(drm_sched_entity_init);
[ ... ]
> +int 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);
> +	struct drm_gpu_scheduler **newscheds, **oldscheds;
> +	unsigned int i;
> +
> +	if (!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;
> +	}
> +
> +	if (num_sched_list > 1) {
> +		newscheds = kmemdup_array(sched_list, num_sched_list,
> +					  sizeof(*sched_list), GFP_KERNEL);
> +		if (!newscheds)
> +			return -ENOMEM;
> +	}
>  
>  	spin_lock(&entity->lock);
> -	entity->sched_list = sched_list;
> +	if (num_sched_list == 1) {
> +		entity->sched = sched_list[0];
> +		oldscheds = NULL;
> +	} else {
> +		oldscheds = entity->sched_list;
> +		entity->sched_list = newscheds;
> +	}
>  	entity->num_sched_list = num_sched_list;
>  	spin_unlock(&entity->lock);
> +
> +	kfree(oldscheds);
> +
> +	return 0;
>  }

[Severity: Critical]
The struct drm_sched_entity uses a union for sched_list and sched. Does
this logic incorrectly use the new num_sched_list count to determine how
to extract the old pointer?

When transitioning from 1 scheduler to >1, the else branch executes, and
oldscheds is assigned entity->sched_list. Since they share a union, doesn't
this alias with the old entity->sched pointer, causing kfree(oldscheds) to
incorrectly free a driver-owned scheduler object instead of an array?

Conversely, when going from >1 to 1, the if branch sets oldscheds to NULL,
which seems to leak the previously allocated entity->sched_list array.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261002154713.77591-1-tvrtko.ursulin@igalia.com?part=9