[RFC v2 00/18] Deadline scheduler and other ideas

[RFC v2 00/18] Deadline scheduler and other ideas

[Tvrtko Ursulin]

<tldr>
Replacing FIFO with a flavour of deadline driven scheduling and removing round-
robin. Connecting the scheduler with dma-fence deadlines. Second draft and
testing by different drivers and feedback would be nice. I was only able to test
it with amdgpu. Other drivers may not even compile.
</tldr>

If I remember correctly Christian mentioned recently (give or take) that maybe
round-robin could be removed. That got me thinking how and what could be
improved and simplified. So I played a bit in the scheduler code and came up
with something which appears to not crash at least. Whether or not there are
significant advantages apart from maybe code consolidation and reduction is the
main thing to be determined.

One big question is whether round-robin can really be removed. Does anyone use
it, rely on it, or what are even use cases where it is much better than FIFO.

See "drm/sched: Add deadline policy" commit message for a short description on
what flavour of deadline scheduling it is. But in essence it should a more fair
FIFO where higher priority can not forever starve lower priorities.

"drm/sched: Connect with dma-fence deadlines" wires up dma-fence deadlines to
the scheduler because it is easy and makes logical sense with this. And I
noticed userspace already uses it so why not wire it up fully.

Otherwise the series is a bit of progression from trivial cleanups to
consolidating RR into FIFO code paths and going from there to deadline and then
some code simplification to 1:1 run queue to scheduler relationship, because
deadline does not need per priority run queues.

There is quite a bit of code to go throught here so I think it could be even
better if other drivers could give it a spin as is and see if some improvements
can be detected. Or at least no regressions.

v2:
 * Fixed many rebase errors.
 * Added some new patches.
 * Dropped single shot dependecy handling.

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

Tvrtko Ursulin (18):
  drm/amdgpu: Use DRM scheduler API in amdgpu_xcp_release_sched
  drm/sched: Delete unused update_job_credits
  drm/sched: Remove one local variable
  drm/sched: Remove weak paused submission checks
  drm/sched: Avoid double re-lock on the job free path
  drm/sched: Add helper to check job dependencies
  drm/imagination: Use the drm_sched_job_has_dependency helper
  drm/sched: Clarify locked section in drm_sched_rq_select_entity_fifo
  drm/sched: Remove idle entity from tree
  drm/sched: Implement RR via FIFO
  drm/sched: Consolidate entity run queue management
  drm/sched: Move run queue related code into a separate file
  drm/sched: Add deadline policy
  drm/sched: Remove FIFO and RR and simplify to a single run queue
  drm/sched: Queue all free credits in one worker invocation
  drm/sched: Connect with dma-fence deadlines
  drm/sched: Embed run queue singleton into the scheduler
  drm/sched: Scale deadlines depending on queue depth

 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |   6 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c     |  27 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h     |   5 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |   8 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c |   8 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c     |  10 +-
 drivers/gpu/drm/imagination/pvr_job.c       |  12 +-
 drivers/gpu/drm/scheduler/Makefile          |   2 +-
 drivers/gpu/drm/scheduler/sched_entity.c    | 147 +++---
 drivers/gpu/drm/scheduler/sched_fence.c     |   2 +-
 drivers/gpu/drm/scheduler/sched_main.c      | 541 ++++----------------
 drivers/gpu/drm/scheduler/sched_rq.c        | 177 +++++++
 include/drm/gpu_scheduler.h                 |  55 +-
 13 files changed, 424 insertions(+), 576 deletions(-)
 create mode 100644 drivers/gpu/drm/scheduler/sched_rq.c

-- 
2.47.1

[RFC 01/18] drm/amdgpu: Use DRM scheduler API in amdgpu_xcp_release_sched

[Tvrtko Ursulin]

Lets use the existing helper instead of peeking into the structure
directly.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
index e209b5e101df..23b6f7a4aa4a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
@@ -427,7 +427,7 @@ void amdgpu_xcp_release_sched(struct amdgpu_device *adev,
 		return;
 
 	sched = entity->entity.rq->sched;
-	if (sched->ready) {
+	if (drm_sched_wqueue_ready(sched)) {
 		ring = to_amdgpu_ring(entity->entity.rq->sched);
 		atomic_dec(&adev->xcp_mgr->xcp[ring->xcp_id].ref_cnt);
 	}
-- 
2.47.1

[RFC 02/18] drm/sched: Delete unused update_job_credits

[Tvrtko Ursulin]

No driver is using the update_job_credits() schduler vfunc
so lets remove it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
Acked-by: Danilo Krummrich <dakr@kernel.org>
Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
Acked-by: Matt Coster <matt.coster@imgtec.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 13 -------------
 include/drm/gpu_scheduler.h            | 13 -------------
 2 files changed, 26 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 7ce25281c74c..1734c17aeea5 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -64,12 +64,6 @@
  * credit limit, the job won't be executed. Instead, the scheduler will wait
  * until the credit count has decreased enough to not overflow its credit limit.
  * This implies waiting for previously executed jobs.
- *
- * Optionally, drivers may register a callback (update_job_credits) provided by
- * struct drm_sched_backend_ops to update the job's credits dynamically. The
- * scheduler executes this callback every time the scheduler considers a job for
- * execution and subsequently checks whether the job fits the scheduler's credit
- * limit.
  */
 
 #include <linux/wait.h>
@@ -133,13 +127,6 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
 	if (!s_job)
 		return false;
 
-	if (sched->ops->update_job_credits) {
-		s_job->credits = sched->ops->update_job_credits(s_job);
-
-		drm_WARN(sched, !s_job->credits,
-			 "Jobs with zero credits bypass job-flow control.\n");
-	}
-
 	/* If a job exceeds the credit limit, truncate it to the credit limit
 	 * itself to guarantee forward progress.
 	 */
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 95e17504e46a..e2e6af8849c6 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -476,19 +476,6 @@ struct drm_sched_backend_ops {
          * and it's time to clean it up.
 	 */
 	void (*free_job)(struct drm_sched_job *sched_job);
-
-	/**
-	 * @update_job_credits: Called when the scheduler is considering this
-	 * job for execution.
-	 *
-	 * This callback returns the number of credits the job would take if
-	 * pushed to the hardware. Drivers may use this to dynamically update
-	 * the job's credit count. For instance, deduct the number of credits
-	 * for already signalled native fences.
-	 *
-	 * This callback is optional.
-	 */
-	u32 (*update_job_credits)(struct drm_sched_job *sched_job);
 };
 
 /**
-- 
2.47.1

[RFC 03/18] drm/sched: Remove one local variable

[Tvrtko Ursulin]

It is not helping readability nor it is required to keep the line length
in check.

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

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 1734c17aeea5..01e0d6e686d1 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1175,7 +1175,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
 		container_of(w, struct drm_gpu_scheduler, work_run_job);
 	struct drm_sched_entity *entity;
 	struct dma_fence *fence;
-	struct drm_sched_fence *s_fence;
 	struct drm_sched_job *sched_job;
 	int r;
 
@@ -1194,15 +1193,13 @@ static void drm_sched_run_job_work(struct work_struct *w)
 		return;
 	}
 
-	s_fence = sched_job->s_fence;
-
 	atomic_add(sched_job->credits, &sched->credit_count);
 	drm_sched_job_begin(sched_job);
 
 	trace_drm_run_job(sched_job, entity);
 	fence = sched->ops->run_job(sched_job);
 	complete_all(&entity->entity_idle);
-	drm_sched_fence_scheduled(s_fence, fence);
+	drm_sched_fence_scheduled(sched_job->s_fence, fence);
 
 	if (!IS_ERR_OR_NULL(fence)) {
 		/* Drop for original kref_init of the fence */
-- 
2.47.1

[RFC 04/18] drm/sched: Remove weak paused submission checks

[Tvrtko Ursulin]

There is no need to check the boolean in the work item's prologues since
the boolean can be set at any later time anyway.

The helper which pauses submission sets it and synchronously cancels the
work and helpers which queue the work check for the flag so all should be
good.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 01e0d6e686d1..4c96d459a0c9 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -1153,9 +1153,6 @@ static void drm_sched_free_job_work(struct work_struct *w)
 		container_of(w, struct drm_gpu_scheduler, work_free_job);
 	struct drm_sched_job *job;
 
-	if (READ_ONCE(sched->pause_submit))
-		return;
-
 	job = drm_sched_get_finished_job(sched);
 	if (job)
 		sched->ops->free_job(job);
@@ -1178,9 +1175,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
 	struct drm_sched_job *sched_job;
 	int r;
 
-	if (READ_ONCE(sched->pause_submit))
-		return;
-
 	/* Find entity with a ready job */
 	entity = drm_sched_select_entity(sched);
 	if (!entity)
-- 
2.47.1

[RFC 05/18] drm/sched: Avoid double re-lock on the job free path

[Tvrtko Ursulin]

Currently the job free work item will lock sched->job_list_lock first time
to see if there are any jobs, free a single job, and then lock again to
decide whether to re-queue itself if there are more finished jobs.

Since drm_sched_get_finished_job() even already looks at the second job in
the queue we can simply make it return its presence to the caller.

That way the caller does not need to lock the list again only to peek into
the same job.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 32 +++++++++-----------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 4c96d459a0c9..64cd79e1793a 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -370,22 +370,6 @@ static void __drm_sched_run_free_queue(struct drm_gpu_scheduler *sched)
 		queue_work(sched->submit_wq, &sched->work_free_job);
 }
 
-/**
- * drm_sched_run_free_queue - enqueue free-job work if ready
- * @sched: scheduler instance
- */
-static void drm_sched_run_free_queue(struct drm_gpu_scheduler *sched)
-{
-	struct drm_sched_job *job;
-
-	spin_lock(&sched->job_list_lock);
-	job = list_first_entry_or_null(&sched->pending_list,
-				       struct drm_sched_job, list);
-	if (job && dma_fence_is_signaled(&job->s_fence->finished))
-		__drm_sched_run_free_queue(sched);
-	spin_unlock(&sched->job_list_lock);
-}
-
 /**
  * drm_sched_job_done - complete a job
  * @s_job: pointer to the job which is done
@@ -1065,12 +1049,13 @@ drm_sched_select_entity(struct drm_gpu_scheduler *sched)
  * drm_sched_get_finished_job - fetch the next finished job to be destroyed
  *
  * @sched: scheduler instance
+ * @have_more: are there more finished jobs on the list
  *
  * Returns the next finished job from the pending list (if there is one)
  * ready for it to be destroyed.
  */
 static struct drm_sched_job *
-drm_sched_get_finished_job(struct drm_gpu_scheduler *sched)
+drm_sched_get_finished_job(struct drm_gpu_scheduler *sched, bool *have_more)
 {
 	struct drm_sched_job *job, *next;
 
@@ -1088,14 +1073,16 @@ drm_sched_get_finished_job(struct drm_gpu_scheduler *sched)
 		/* make the scheduled timestamp more accurate */
 		next = list_first_entry_or_null(&sched->pending_list,
 						typeof(*next), list);
-
 		if (next) {
+			*have_more = dma_fence_is_signaled(&next->s_fence->finished);
 			if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT,
 				     &next->s_fence->scheduled.flags))
 				next->s_fence->scheduled.timestamp =
 					dma_fence_timestamp(&job->s_fence->finished);
 			/* start TO timer for next job */
 			drm_sched_start_timeout(sched);
+		} else {
+			*have_more = false;
 		}
 	} else {
 		job = NULL;
@@ -1152,12 +1139,15 @@ static void drm_sched_free_job_work(struct work_struct *w)
 	struct drm_gpu_scheduler *sched =
 		container_of(w, struct drm_gpu_scheduler, work_free_job);
 	struct drm_sched_job *job;
+	bool have_more;
 
-	job = drm_sched_get_finished_job(sched);
-	if (job)
+	job = drm_sched_get_finished_job(sched, &have_more);
+	if (job) {
 		sched->ops->free_job(job);
+		if (have_more)
+			__drm_sched_run_free_queue(sched);
+	}
 
-	drm_sched_run_free_queue(sched);
 	drm_sched_run_job_queue(sched);
 }
 
-- 
2.47.1

[RFC 06/18] drm/sched: Add helper to check job dependencies

[Tvrtko Ursulin]

Lets isolate scheduler internals from drivers such as pvr which currently
walks the dependency array to look for fences.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 23 +++++++++++++++++++++++
 include/drm/gpu_scheduler.h            |  3 ++-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 64cd79e1793a..f07b19c97d0f 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -968,6 +968,29 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
 }
 EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies);
 
+/**
+ * drm_sched_job_has_dependency - check whether fence is the job's dependency
+ * @job: scheduler job to check
+ * @fence: fence to look for
+ *
+ * Returns:
+ * True if @fence is found within the job's dependencies, or otherwise false.
+ */
+bool drm_sched_job_has_dependency(struct drm_sched_job *job,
+				  struct dma_fence *fence)
+{
+	struct dma_fence *f;
+	unsigned long index;
+
+	xa_for_each(&job->dependencies, index, f) {
+		if (f == fence)
+			return true;
+	}
+
+	return false;
+}
+EXPORT_SYMBOL(drm_sched_job_has_dependency);
+
 /**
  * drm_sched_job_cleanup - clean up scheduler job resources
  * @job: scheduler job to clean up
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index e2e6af8849c6..a0ff08123f07 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -564,7 +564,8 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
 int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
 					    struct drm_gem_object *obj,
 					    bool write);
-
+bool drm_sched_job_has_dependency(struct drm_sched_job *job,
+				  struct dma_fence *fence);
 
 void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
 				    struct drm_gpu_scheduler **sched_list,
-- 
2.47.1

[RFC 07/18] drm/imagination: Use the drm_sched_job_has_dependency helper

[Tvrtko Ursulin]

Instead of manually peeking into the DRM scheduler implementation details
lets use the previously added helper.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
Cc: Frank Binns <frank.binns@imgtec.com>
Cc: Matt Coster <matt.coster@imgtec.com>
---
 drivers/gpu/drm/imagination/pvr_job.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/imagination/pvr_job.c b/drivers/gpu/drm/imagination/pvr_job.c
index 618503a212a7..1cdb3cfd058d 100644
--- a/drivers/gpu/drm/imagination/pvr_job.c
+++ b/drivers/gpu/drm/imagination/pvr_job.c
@@ -597,8 +597,6 @@ update_job_resvs_for_each(struct pvr_job_data *job_data, u32 job_count)
 static bool can_combine_jobs(struct pvr_job *a, struct pvr_job *b)
 {
 	struct pvr_job *geom_job = a, *frag_job = b;
-	struct dma_fence *fence;
-	unsigned long index;
 
 	/* Geometry and fragment jobs can be combined if they are queued to the
 	 * same context and targeting the same HWRT.
@@ -609,13 +607,9 @@ static bool can_combine_jobs(struct pvr_job *a, struct pvr_job *b)
 	    a->hwrt != b->hwrt)
 		return false;
 
-	xa_for_each(&frag_job->base.dependencies, index, fence) {
-		/* We combine when we see an explicit geom -> frag dep. */
-		if (&geom_job->base.s_fence->scheduled == fence)
-			return true;
-	}
-
-	return false;
+	/* We combine when we see an explicit geom -> frag dep. */
+	return drm_sched_job_has_dependency(&frag_job->base,
+					    &geom_job->base.s_fence->scheduled);
 }
 
 static struct dma_fence *
-- 
2.47.1

[RFC 08/18] drm/sched: Clarify locked section in drm_sched_rq_select_entity_fifo

[Tvrtko Ursulin]

Rq->lock only protects the tree walk so lets move the rest out.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 31 ++++++++++++++------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index f07b19c97d0f..9a3d31c33fa9 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -325,29 +325,32 @@ static struct drm_sched_entity *
 drm_sched_rq_select_entity_fifo(struct drm_gpu_scheduler *sched,
 				struct drm_sched_rq *rq)
 {
+	struct drm_sched_entity *entity = NULL;
 	struct rb_node *rb;
 
 	spin_lock(&rq->lock);
 	for (rb = rb_first_cached(&rq->rb_tree_root); rb; rb = rb_next(rb)) {
-		struct drm_sched_entity *entity;
-
 		entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
-		if (drm_sched_entity_is_ready(entity)) {
-			/* If we can't queue yet, preserve the current entity in
-			 * terms of fairness.
-			 */
-			if (!drm_sched_can_queue(sched, entity)) {
-				spin_unlock(&rq->lock);
-				return ERR_PTR(-ENOSPC);
-			}
-
-			reinit_completion(&entity->entity_idle);
+		if (drm_sched_entity_is_ready(entity))
 			break;
-		}
+		else
+			entity = NULL;
 	}
 	spin_unlock(&rq->lock);
 
-	return rb ? rb_entry(rb, struct drm_sched_entity, rb_tree_node) : NULL;
+	if (!entity)
+		return NULL;
+
+	/*
+	 * If scheduler cannot take more jobs signal the caller to not consider
+	 * lower priority queues.
+	 */
+	if (!drm_sched_can_queue(sched, entity))
+		return ERR_PTR(-ENOSPC);
+
+	reinit_completion(&entity->entity_idle);
+
+	return entity;
 }
 
 /**
-- 
2.47.1

[RFC 09/18] drm/sched: Remove idle entity from tree

[Tvrtko Ursulin]

There is no need to keep entities with no jobs in the tree so lets remove
it once the last job is consumed. This keeps the tree smaller which is
nicer and more efficient.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 15 ++++++++-------
 drivers/gpu/drm/scheduler/sched_main.c   |  4 ++--
 include/drm/gpu_scheduler.h              |  2 ++
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 69bcf0e99d57..8e910586979e 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -512,19 +512,20 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	 */
 	if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) {
 		struct drm_sched_job *next;
+		struct drm_sched_rq *rq;
 
+		spin_lock(&entity->lock);
+		rq = entity->rq;
+		spin_lock(&rq->lock);
 		next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
 		if (next) {
-			struct drm_sched_rq *rq;
-
-			spin_lock(&entity->lock);
-			rq = entity->rq;
-			spin_lock(&rq->lock);
 			drm_sched_rq_update_fifo_locked(entity, rq,
 							next->submit_ts);
-			spin_unlock(&rq->lock);
-			spin_unlock(&entity->lock);
+		} else {
+			drm_sched_rq_remove_fifo_locked(entity, rq);
 		}
+		spin_unlock(&rq->lock);
+		spin_unlock(&entity->lock);
 	}
 
 	/* Jobs and entities might have different lifecycles. Since we're
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 9a3d31c33fa9..bcfc2abe349a 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -146,8 +146,8 @@ static __always_inline bool drm_sched_entity_compare_before(struct rb_node *a,
 	return ktime_before(ent_a->oldest_job_waiting, ent_b->oldest_job_waiting);
 }
 
-static void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
-					    struct drm_sched_rq *rq)
+void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
+				     struct drm_sched_rq *rq)
 {
 	if (!RB_EMPTY_NODE(&entity->rb_tree_node)) {
 		rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index a0ff08123f07..005db1e35fad 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -592,6 +592,8 @@ void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
 void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 				struct drm_sched_entity *entity);
 
+void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
+				     struct drm_sched_rq *rq);
 void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
 				     struct drm_sched_rq *rq, ktime_t ts);
 
-- 
2.47.1

[RFC 10/18] drm/sched: Implement RR via FIFO

[Tvrtko Ursulin]

Round-robin being the non-default policy and unclear how much it is used,
we can notice that it can be implemented using the FIFO data structures if
we only invent a fake submit timestamp which is monotonically increasing
inside drm_sched_rq instances.

So instead of remembering which was the last entity the scheduler worker
picked, we can bump the picked one to the bottom of the tree, achieving
the same round-robin behaviour.

Advantage is that we can consolidate to a single code path and remove a
bunch of code. Downside is round-robin mode now needs to lock on the job
pop path but that should not be visible.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 50 ++++++++------
 drivers/gpu/drm/scheduler/sched_main.c   | 84 ++----------------------
 include/drm/gpu_scheduler.h              |  5 +-
 3 files changed, 39 insertions(+), 100 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 8e910586979e..2b1bc4d00b57 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -473,9 +473,20 @@ drm_sched_job_dependency(struct drm_sched_job *job,
 	return NULL;
 }
 
+static ktime_t
+drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
+{
+	lockdep_assert_held(&rq->lock);
+
+	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
+
+	return rq->rr_deadline;
+}
+
 struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 {
-	struct drm_sched_job *sched_job;
+	struct drm_sched_job *sched_job, *next_job;
+	struct drm_sched_rq *rq;
 
 	sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
 	if (!sched_job)
@@ -510,23 +521,24 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	 * Update the entity's location in the min heap according to
 	 * the timestamp of the next job, if any.
 	 */
-	if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) {
-		struct drm_sched_job *next;
-		struct drm_sched_rq *rq;
+	spin_lock(&entity->lock);
+	rq = entity->rq;
+	spin_lock(&rq->lock);
+	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
+	if (next_job) {
+		ktime_t ts;
 
-		spin_lock(&entity->lock);
-		rq = entity->rq;
-		spin_lock(&rq->lock);
-		next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
-		if (next) {
-			drm_sched_rq_update_fifo_locked(entity, rq,
-							next->submit_ts);
-		} else {
-			drm_sched_rq_remove_fifo_locked(entity, rq);
-		}
-		spin_unlock(&rq->lock);
-		spin_unlock(&entity->lock);
+		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
+			ts = next_job->submit_ts;
+		else
+			ts = drm_sched_rq_get_rr_deadline(rq);
+
+		drm_sched_rq_update_fifo_locked(entity, rq, ts);
+	} else {
+		drm_sched_rq_remove_fifo_locked(entity, rq);
 	}
+	spin_unlock(&rq->lock);
+	spin_unlock(&entity->lock);
 
 	/* Jobs and entities might have different lifecycles. Since we're
 	 * removing the job from the entities queue, set the jobs entity pointer
@@ -624,9 +636,9 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 
 		spin_lock(&rq->lock);
 		drm_sched_rq_add_entity(rq, entity);
-
-		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
-			drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
+		if (drm_sched_policy == DRM_SCHED_POLICY_RR)
+			submit_ts = drm_sched_rq_get_rr_deadline(rq);
+		drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
 
 		spin_unlock(&rq->lock);
 		spin_unlock(&entity->lock);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index bcfc2abe349a..31cab7bb5428 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -189,7 +189,6 @@ static void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
 	spin_lock_init(&rq->lock);
 	INIT_LIST_HEAD(&rq->entities);
 	rq->rb_tree_root = RB_ROOT_CACHED;
-	rq->current_entity = NULL;
 	rq->sched = sched;
 }
 
@@ -235,82 +234,13 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 	atomic_dec(rq->sched->score);
 	list_del_init(&entity->list);
 
-	if (rq->current_entity == entity)
-		rq->current_entity = NULL;
-
-	if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
-		drm_sched_rq_remove_fifo_locked(entity, rq);
+	drm_sched_rq_remove_fifo_locked(entity, rq);
 
 	spin_unlock(&rq->lock);
 }
 
 /**
- * drm_sched_rq_select_entity_rr - Select an entity which could provide a job to run
- *
- * @sched: the gpu scheduler
- * @rq: scheduler run queue to check.
- *
- * Try to find the next ready entity.
- *
- * Return an entity if one is found; return an error-pointer (!NULL) if an
- * entity was ready, but the scheduler had insufficient credits to accommodate
- * its job; return NULL, if no ready entity was found.
- */
-static struct drm_sched_entity *
-drm_sched_rq_select_entity_rr(struct drm_gpu_scheduler *sched,
-			      struct drm_sched_rq *rq)
-{
-	struct drm_sched_entity *entity;
-
-	spin_lock(&rq->lock);
-
-	entity = rq->current_entity;
-	if (entity) {
-		list_for_each_entry_continue(entity, &rq->entities, list) {
-			if (drm_sched_entity_is_ready(entity)) {
-				/* If we can't queue yet, preserve the current
-				 * entity in terms of fairness.
-				 */
-				if (!drm_sched_can_queue(sched, entity)) {
-					spin_unlock(&rq->lock);
-					return ERR_PTR(-ENOSPC);
-				}
-
-				rq->current_entity = entity;
-				reinit_completion(&entity->entity_idle);
-				spin_unlock(&rq->lock);
-				return entity;
-			}
-		}
-	}
-
-	list_for_each_entry(entity, &rq->entities, list) {
-		if (drm_sched_entity_is_ready(entity)) {
-			/* If we can't queue yet, preserve the current entity in
-			 * terms of fairness.
-			 */
-			if (!drm_sched_can_queue(sched, entity)) {
-				spin_unlock(&rq->lock);
-				return ERR_PTR(-ENOSPC);
-			}
-
-			rq->current_entity = entity;
-			reinit_completion(&entity->entity_idle);
-			spin_unlock(&rq->lock);
-			return entity;
-		}
-
-		if (entity == rq->current_entity)
-			break;
-	}
-
-	spin_unlock(&rq->lock);
-
-	return NULL;
-}
-
-/**
- * drm_sched_rq_select_entity_fifo - Select an entity which provides a job to run
+ * drm_sched_rq_select_entity - Select an entity which provides a job to run
  *
  * @sched: the gpu scheduler
  * @rq: scheduler run queue to check.
@@ -322,8 +252,8 @@ drm_sched_rq_select_entity_rr(struct drm_gpu_scheduler *sched,
  * its job; return NULL, if no ready entity was found.
  */
 static struct drm_sched_entity *
-drm_sched_rq_select_entity_fifo(struct drm_gpu_scheduler *sched,
-				struct drm_sched_rq *rq)
+drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
+			   struct drm_sched_rq *rq)
 {
 	struct drm_sched_entity *entity = NULL;
 	struct rb_node *rb;
@@ -1055,15 +985,13 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
 static struct drm_sched_entity *
 drm_sched_select_entity(struct drm_gpu_scheduler *sched)
 {
-	struct drm_sched_entity *entity;
+	struct drm_sched_entity *entity = NULL;
 	int i;
 
 	/* Start with the highest priority.
 	 */
 	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
-		entity = drm_sched_policy == DRM_SCHED_POLICY_FIFO ?
-			drm_sched_rq_select_entity_fifo(sched, sched->sched_rq[i]) :
-			drm_sched_rq_select_entity_rr(sched, sched->sched_rq[i]);
+		entity = drm_sched_rq_select_entity(sched, sched->sched_rq[i]);
 		if (entity)
 			break;
 	}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 005db1e35fad..a0164de08f5b 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -245,8 +245,7 @@ struct drm_sched_entity {
  * struct drm_sched_rq - queue of entities to be scheduled.
  *
  * @sched: the scheduler to which this rq belongs to.
- * @lock: protects @entities, @rb_tree_root and @current_entity.
- * @current_entity: the entity which is to be scheduled.
+ * @lock: protects @entities, @rb_tree_root and @rr_deadline.
  * @entities: list of the entities to be scheduled.
  * @rb_tree_root: root of time based priority queue of entities for FIFO scheduling
  *
@@ -259,7 +258,7 @@ struct drm_sched_rq {
 
 	spinlock_t			lock;
 	/* Following members are protected by the @lock: */
-	struct drm_sched_entity		*current_entity;
+	ktime_t				rr_deadline;
 	struct list_head		entities;
 	struct rb_root_cached		rb_tree_root;
 };
-- 
2.47.1

[RFC 11/18] drm/sched: Consolidate entity run queue management

[Tvrtko Ursulin]

Move the code dealing with entities entering and exiting run queues to
helpers to logically separate it from jobs entering and exiting entities.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 63 ++-------------
 drivers/gpu/drm/scheduler/sched_main.c   | 97 +++++++++++++++++++-----
 include/drm/gpu_scheduler.h              | 12 +--
 3 files changed, 88 insertions(+), 84 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 2b1bc4d00b57..b93da068585e 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -473,20 +473,9 @@ drm_sched_job_dependency(struct drm_sched_job *job,
 	return NULL;
 }
 
-static ktime_t
-drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
-{
-	lockdep_assert_held(&rq->lock);
-
-	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
-
-	return rq->rr_deadline;
-}
-
 struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 {
-	struct drm_sched_job *sched_job, *next_job;
-	struct drm_sched_rq *rq;
+	struct drm_sched_job *sched_job;
 
 	sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
 	if (!sched_job)
@@ -516,29 +505,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	smp_wmb();
 
 	spsc_queue_pop(&entity->job_queue);
-
-	/*
-	 * Update the entity's location in the min heap according to
-	 * the timestamp of the next job, if any.
-	 */
-	spin_lock(&entity->lock);
-	rq = entity->rq;
-	spin_lock(&rq->lock);
-	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
-	if (next_job) {
-		ktime_t ts;
-
-		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
-			ts = next_job->submit_ts;
-		else
-			ts = drm_sched_rq_get_rr_deadline(rq);
-
-		drm_sched_rq_update_fifo_locked(entity, rq, ts);
-	} else {
-		drm_sched_rq_remove_fifo_locked(entity, rq);
-	}
-	spin_unlock(&rq->lock);
-	spin_unlock(&entity->lock);
+	drm_sched_rq_pop_entity(entity->rq, entity);
 
 	/* Jobs and entities might have different lifecycles. Since we're
 	 * removing the job from the entities queue, set the jobs entity pointer
@@ -620,30 +587,10 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 	/* first job wakes up scheduler */
 	if (first) {
 		struct drm_gpu_scheduler *sched;
-		struct drm_sched_rq *rq;
 
-		/* Add the entity to the run queue */
-		spin_lock(&entity->lock);
-		if (entity->stopped) {
-			spin_unlock(&entity->lock);
-
-			DRM_ERROR("Trying to push to a killed entity\n");
-			return;
-		}
-
-		rq = entity->rq;
-		sched = rq->sched;
-
-		spin_lock(&rq->lock);
-		drm_sched_rq_add_entity(rq, entity);
-		if (drm_sched_policy == DRM_SCHED_POLICY_RR)
-			submit_ts = drm_sched_rq_get_rr_deadline(rq);
-		drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
-
-		spin_unlock(&rq->lock);
-		spin_unlock(&entity->lock);
-
-		drm_sched_wakeup(sched);
+		sched = drm_sched_rq_add_entity(entity->rq, entity, submit_ts);
+		if (sched)
+			drm_sched_wakeup(sched);
 	}
 }
 EXPORT_SYMBOL(drm_sched_entity_push_job);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 31cab7bb5428..a21376ce859f 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -146,18 +146,19 @@ static __always_inline bool drm_sched_entity_compare_before(struct rb_node *a,
 	return ktime_before(ent_a->oldest_job_waiting, ent_b->oldest_job_waiting);
 }
 
-void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
-				     struct drm_sched_rq *rq)
+static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
+					      struct drm_sched_rq *rq)
 {
-	if (!RB_EMPTY_NODE(&entity->rb_tree_node)) {
-		rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
-		RB_CLEAR_NODE(&entity->rb_tree_node);
-	}
+	lockdep_assert_held(&entity->lock);
+	lockdep_assert_held(&rq->lock);
+
+	rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
+	RB_CLEAR_NODE(&entity->rb_tree_node);
 }
 
-void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
-				     struct drm_sched_rq *rq,
-				     ktime_t ts)
+static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity *entity,
+					   struct drm_sched_rq *rq,
+					   ktime_t ts)
 {
 	/*
 	 * Both locks need to be grabbed, one to protect from entity->rq change
@@ -167,8 +168,6 @@ void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
 	lockdep_assert_held(&entity->lock);
 	lockdep_assert_held(&rq->lock);
 
-	drm_sched_rq_remove_fifo_locked(entity, rq);
-
 	entity->oldest_job_waiting = ts;
 
 	rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
@@ -192,6 +191,16 @@ static void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
 	rq->sched = sched;
 }
 
+static ktime_t
+drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
+{
+	lockdep_assert_held(&rq->lock);
+
+	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
+
+	return rq->rr_deadline;
+}
+
 /**
  * drm_sched_rq_add_entity - add an entity
  *
@@ -199,18 +208,42 @@ static void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
  * @entity: scheduler entity
  *
  * Adds a scheduler entity to the run queue.
+ *
+ * Returns a DRM scheduler pre-selected to handle this entity.
  */
-void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
-			     struct drm_sched_entity *entity)
+struct drm_gpu_scheduler *
+drm_sched_rq_add_entity(struct drm_sched_rq *rq,
+			struct drm_sched_entity *entity,
+			ktime_t ts)
 {
-	lockdep_assert_held(&entity->lock);
-	lockdep_assert_held(&rq->lock);
+	struct drm_gpu_scheduler *sched;
 
-	if (!list_empty(&entity->list))
-		return;
+	if (entity->stopped) {
+		DRM_ERROR("Trying to push to a killed entity\n");
+		return NULL;
+	}
 
-	atomic_inc(rq->sched->score);
-	list_add_tail(&entity->list, &rq->entities);
+	spin_lock(&entity->lock);
+	spin_lock(&rq->lock);
+
+	sched = rq->sched;
+
+	if (!list_empty(&entity->list)) {
+		atomic_inc(sched->score);
+		list_add_tail(&entity->list, &rq->entities);
+	}
+
+	if (drm_sched_policy == DRM_SCHED_POLICY_RR)
+		ts = drm_sched_rq_get_rr_deadline(rq);
+
+	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
+		__drm_sched_rq_remove_fifo_locked(entity, rq);
+	__drm_sched_rq_add_fifo_locked(entity, rq, ts);
+
+	spin_unlock(&rq->lock);
+	spin_unlock(&entity->lock);
+
+	return sched;
 }
 
 /**
@@ -234,11 +267,35 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 	atomic_dec(rq->sched->score);
 	list_del_init(&entity->list);
 
-	drm_sched_rq_remove_fifo_locked(entity, rq);
+	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
+		__drm_sched_rq_remove_fifo_locked(entity, rq);
 
 	spin_unlock(&rq->lock);
 }
 
+void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
+			     struct drm_sched_entity *entity)
+{
+	struct drm_sched_job *next_job;
+
+	spin_lock(&entity->lock);
+	spin_lock(&rq->lock);
+	__drm_sched_rq_remove_fifo_locked(entity, rq);
+	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
+	if (next_job) {
+		ktime_t ts;
+
+		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
+			ts = next_job->submit_ts;
+		else
+			ts = drm_sched_rq_get_rr_deadline(rq);
+
+		__drm_sched_rq_add_fifo_locked(entity, rq, ts);
+	}
+	spin_unlock(&rq->lock);
+	spin_unlock(&entity->lock);
+}
+
 /**
  * drm_sched_rq_select_entity - Select an entity which provides a job to run
  *
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index a0164de08f5b..daf4665f37fa 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -586,15 +586,15 @@ bool drm_sched_dependency_optimized(struct dma_fence* fence,
 				    struct drm_sched_entity *entity);
 void drm_sched_fault(struct drm_gpu_scheduler *sched);
 
-void drm_sched_rq_add_entity(struct drm_sched_rq *rq,
-			     struct drm_sched_entity *entity);
+struct drm_gpu_scheduler *
+drm_sched_rq_add_entity(struct drm_sched_rq *rq,
+			struct drm_sched_entity *entity,
+			ktime_t ts);
 void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 				struct drm_sched_entity *entity);
 
-void drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
-				     struct drm_sched_rq *rq);
-void drm_sched_rq_update_fifo_locked(struct drm_sched_entity *entity,
-				     struct drm_sched_rq *rq, ktime_t ts);
+void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
+			     struct drm_sched_entity *entity);
 
 int drm_sched_entity_init(struct drm_sched_entity *entity,
 			  enum drm_sched_priority priority,
-- 
2.47.1

[RFC 12/18] drm/sched: Move run queue related code into a separate file

[Tvrtko Ursulin]

Lets move all the code dealing with struct drm_sched_rq into a separate
compilation unit. Advantage being sched_main.c is left with a clearer set
of responsibilities.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/Makefile     |   2 +-
 drivers/gpu/drm/scheduler/sched_main.c | 210 +------------------------
 drivers/gpu/drm/scheduler/sched_rq.c   | 207 ++++++++++++++++++++++++
 include/drm/gpu_scheduler.h            |  12 ++
 4 files changed, 222 insertions(+), 209 deletions(-)
 create mode 100644 drivers/gpu/drm/scheduler/sched_rq.c

diff --git a/drivers/gpu/drm/scheduler/Makefile b/drivers/gpu/drm/scheduler/Makefile
index 53863621829f..d11d83e285e7 100644
--- a/drivers/gpu/drm/scheduler/Makefile
+++ b/drivers/gpu/drm/scheduler/Makefile
@@ -20,6 +20,6 @@
 # OTHER DEALINGS IN THE SOFTWARE.
 #
 #
-gpu-sched-y := sched_main.o sched_fence.o sched_entity.o
+gpu-sched-y := sched_main.o sched_fence.o sched_entity.o sched_rq.o
 
 obj-$(CONFIG_DRM_SCHED) += gpu-sched.o
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index a21376ce859f..a556ee736f9f 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -87,9 +87,6 @@ static struct lockdep_map drm_sched_lockdep_map = {
 };
 #endif
 
-#define to_drm_sched_job(sched_job)		\
-		container_of((sched_job), struct drm_sched_job, queue_node)
-
 int drm_sched_policy = DRM_SCHED_POLICY_FIFO;
 
 /**
@@ -118,8 +115,8 @@ static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
  * Return true if we can push at least one more job from @entity, false
  * otherwise.
  */
-static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
-				struct drm_sched_entity *entity)
+bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
+			 struct drm_sched_entity *entity)
 {
 	struct drm_sched_job *s_job;
 
@@ -137,209 +134,6 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
 	return drm_sched_available_credits(sched) >= s_job->credits;
 }
 
-static __always_inline bool drm_sched_entity_compare_before(struct rb_node *a,
-							    const struct rb_node *b)
-{
-	struct drm_sched_entity *ent_a =  rb_entry((a), struct drm_sched_entity, rb_tree_node);
-	struct drm_sched_entity *ent_b =  rb_entry((b), struct drm_sched_entity, rb_tree_node);
-
-	return ktime_before(ent_a->oldest_job_waiting, ent_b->oldest_job_waiting);
-}
-
-static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
-					      struct drm_sched_rq *rq)
-{
-	lockdep_assert_held(&entity->lock);
-	lockdep_assert_held(&rq->lock);
-
-	rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
-	RB_CLEAR_NODE(&entity->rb_tree_node);
-}
-
-static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity *entity,
-					   struct drm_sched_rq *rq,
-					   ktime_t ts)
-{
-	/*
-	 * Both locks need to be grabbed, one to protect from entity->rq change
-	 * for entity from within concurrent drm_sched_entity_select_rq and the
-	 * other to update the rb tree structure.
-	 */
-	lockdep_assert_held(&entity->lock);
-	lockdep_assert_held(&rq->lock);
-
-	entity->oldest_job_waiting = ts;
-
-	rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
-		      drm_sched_entity_compare_before);
-}
-
-/**
- * drm_sched_rq_init - initialize a given run queue struct
- *
- * @sched: scheduler instance to associate with this run queue
- * @rq: scheduler run queue
- *
- * Initializes a scheduler runqueue.
- */
-static void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
-			      struct drm_sched_rq *rq)
-{
-	spin_lock_init(&rq->lock);
-	INIT_LIST_HEAD(&rq->entities);
-	rq->rb_tree_root = RB_ROOT_CACHED;
-	rq->sched = sched;
-}
-
-static ktime_t
-drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
-{
-	lockdep_assert_held(&rq->lock);
-
-	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
-
-	return rq->rr_deadline;
-}
-
-/**
- * drm_sched_rq_add_entity - add an entity
- *
- * @rq: scheduler run queue
- * @entity: scheduler entity
- *
- * Adds a scheduler entity to the run queue.
- *
- * Returns a DRM scheduler pre-selected to handle this entity.
- */
-struct drm_gpu_scheduler *
-drm_sched_rq_add_entity(struct drm_sched_rq *rq,
-			struct drm_sched_entity *entity,
-			ktime_t ts)
-{
-	struct drm_gpu_scheduler *sched;
-
-	if (entity->stopped) {
-		DRM_ERROR("Trying to push to a killed entity\n");
-		return NULL;
-	}
-
-	spin_lock(&entity->lock);
-	spin_lock(&rq->lock);
-
-	sched = rq->sched;
-
-	if (!list_empty(&entity->list)) {
-		atomic_inc(sched->score);
-		list_add_tail(&entity->list, &rq->entities);
-	}
-
-	if (drm_sched_policy == DRM_SCHED_POLICY_RR)
-		ts = drm_sched_rq_get_rr_deadline(rq);
-
-	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
-		__drm_sched_rq_remove_fifo_locked(entity, rq);
-	__drm_sched_rq_add_fifo_locked(entity, rq, ts);
-
-	spin_unlock(&rq->lock);
-	spin_unlock(&entity->lock);
-
-	return sched;
-}
-
-/**
- * drm_sched_rq_remove_entity - remove an entity
- *
- * @rq: scheduler run queue
- * @entity: scheduler entity
- *
- * Removes a scheduler entity from the run queue.
- */
-void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
-				struct drm_sched_entity *entity)
-{
-	lockdep_assert_held(&entity->lock);
-
-	if (list_empty(&entity->list))
-		return;
-
-	spin_lock(&rq->lock);
-
-	atomic_dec(rq->sched->score);
-	list_del_init(&entity->list);
-
-	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
-		__drm_sched_rq_remove_fifo_locked(entity, rq);
-
-	spin_unlock(&rq->lock);
-}
-
-void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
-			     struct drm_sched_entity *entity)
-{
-	struct drm_sched_job *next_job;
-
-	spin_lock(&entity->lock);
-	spin_lock(&rq->lock);
-	__drm_sched_rq_remove_fifo_locked(entity, rq);
-	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
-	if (next_job) {
-		ktime_t ts;
-
-		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
-			ts = next_job->submit_ts;
-		else
-			ts = drm_sched_rq_get_rr_deadline(rq);
-
-		__drm_sched_rq_add_fifo_locked(entity, rq, ts);
-	}
-	spin_unlock(&rq->lock);
-	spin_unlock(&entity->lock);
-}
-
-/**
- * drm_sched_rq_select_entity - Select an entity which provides a job to run
- *
- * @sched: the gpu scheduler
- * @rq: scheduler run queue to check.
- *
- * Find oldest waiting ready entity.
- *
- * Return an entity if one is found; return an error-pointer (!NULL) if an
- * entity was ready, but the scheduler had insufficient credits to accommodate
- * its job; return NULL, if no ready entity was found.
- */
-static struct drm_sched_entity *
-drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
-			   struct drm_sched_rq *rq)
-{
-	struct drm_sched_entity *entity = NULL;
-	struct rb_node *rb;
-
-	spin_lock(&rq->lock);
-	for (rb = rb_first_cached(&rq->rb_tree_root); rb; rb = rb_next(rb)) {
-		entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
-		if (drm_sched_entity_is_ready(entity))
-			break;
-		else
-			entity = NULL;
-	}
-	spin_unlock(&rq->lock);
-
-	if (!entity)
-		return NULL;
-
-	/*
-	 * If scheduler cannot take more jobs signal the caller to not consider
-	 * lower priority queues.
-	 */
-	if (!drm_sched_can_queue(sched, entity))
-		return ERR_PTR(-ENOSPC);
-
-	reinit_completion(&entity->entity_idle);
-
-	return entity;
-}
-
 /**
  * drm_sched_run_job_queue - enqueue run-job work
  * @sched: scheduler instance
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
new file mode 100644
index 000000000000..40f5b770f21a
--- /dev/null
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -0,0 +1,207 @@
+#include <linux/rbtree.h>
+
+#include <drm/drm_print.h>
+#include <drm/gpu_scheduler.h>
+
+static __always_inline bool drm_sched_entity_compare_before(struct rb_node *a,
+							    const struct rb_node *b)
+{
+	struct drm_sched_entity *ent_a =  rb_entry((a), struct drm_sched_entity, rb_tree_node);
+	struct drm_sched_entity *ent_b =  rb_entry((b), struct drm_sched_entity, rb_tree_node);
+
+	return ktime_before(ent_a->oldest_job_waiting, ent_b->oldest_job_waiting);
+}
+
+static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
+					      struct drm_sched_rq *rq)
+{
+	lockdep_assert_held(&entity->lock);
+	lockdep_assert_held(&rq->lock);
+
+	rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
+	RB_CLEAR_NODE(&entity->rb_tree_node);
+}
+
+static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity *entity,
+					   struct drm_sched_rq *rq,
+					   ktime_t ts)
+{
+	/*
+	 * Both locks need to be grabbed, one to protect from entity->rq change
+	 * for entity from within concurrent drm_sched_entity_select_rq and the
+	 * other to update the rb tree structure.
+	 */
+	lockdep_assert_held(&entity->lock);
+	lockdep_assert_held(&rq->lock);
+
+	entity->oldest_job_waiting = ts;
+	rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
+		      drm_sched_entity_compare_before);
+}
+
+/**
+ * drm_sched_rq_init - initialize a given run queue struct
+ *
+ * @sched: scheduler instance to associate with this run queue
+ * @rq: scheduler run queue
+ *
+ * Initializes a scheduler runqueue.
+ */
+void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
+		       struct drm_sched_rq *rq)
+{
+	spin_lock_init(&rq->lock);
+	INIT_LIST_HEAD(&rq->entities);
+	rq->rb_tree_root = RB_ROOT_CACHED;
+	rq->sched = sched;
+}
+
+static ktime_t
+drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
+{
+	lockdep_assert_held(&rq->lock);
+
+	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
+
+	return rq->rr_deadline;
+}
+
+/**
+ * drm_sched_rq_add_entity - add an entity
+ *
+ * @rq: scheduler run queue
+ * @entity: scheduler entity
+ * @ts: submission timestamp
+ *
+ * Adds a scheduler entity to the run queue.
+ *
+ * Returns a DRM scheduler pre-selected to handle this entity.
+ */
+struct drm_gpu_scheduler *
+drm_sched_rq_add_entity(struct drm_sched_rq *rq,
+			struct drm_sched_entity *entity,
+			ktime_t ts)
+{
+	struct drm_gpu_scheduler *sched;
+
+	if (entity->stopped) {
+		DRM_ERROR("Trying to push to a killed entity\n");
+		return NULL;
+	}
+
+	spin_lock(&entity->lock);
+	spin_lock(&rq->lock);
+
+	sched = rq->sched;
+
+	if (!list_empty(&entity->list)) {
+		atomic_inc(sched->score);
+		list_add_tail(&entity->list, &rq->entities);
+	}
+
+	if (drm_sched_policy == DRM_SCHED_POLICY_RR)
+		ts = drm_sched_rq_get_rr_deadline(rq);
+
+	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
+		__drm_sched_rq_remove_fifo_locked(entity, rq);
+	__drm_sched_rq_add_fifo_locked(entity, rq, ts);
+
+	spin_unlock(&rq->lock);
+	spin_unlock(&entity->lock);
+
+	return sched;
+}
+
+/**
+ * drm_sched_rq_remove_entity - remove an entity
+ *
+ * @rq: scheduler run queue
+ * @entity: scheduler entity
+ *
+ * Removes a scheduler entity from the run queue.
+ */
+void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
+				struct drm_sched_entity *entity)
+{
+	lockdep_assert_held(&entity->lock);
+
+	if (list_empty(&entity->list))
+		return;
+
+	spin_lock(&rq->lock);
+
+	atomic_dec(rq->sched->score);
+	list_del_init(&entity->list);
+
+	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
+		__drm_sched_rq_remove_fifo_locked(entity, rq);
+
+	spin_unlock(&rq->lock);
+}
+
+void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
+			     struct drm_sched_entity *entity)
+{
+	struct drm_sched_job *next_job;
+
+	spin_lock(&entity->lock);
+	spin_lock(&rq->lock);
+	__drm_sched_rq_remove_fifo_locked(entity, rq);
+	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
+	if (next_job) {
+		ktime_t ts;
+
+		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
+			ts = next_job->submit_ts;
+		else
+			ts = drm_sched_rq_get_rr_deadline(rq);
+
+		__drm_sched_rq_add_fifo_locked(entity, rq, ts);
+	}
+	spin_unlock(&rq->lock);
+	spin_unlock(&entity->lock);
+}
+
+/**
+ * drm_sched_rq_select_entity - Select an entity which provides a job to run
+ *
+ * @sched: the gpu scheduler
+ * @rq: scheduler run queue to check.
+ *
+ * Find oldest waiting ready entity.
+ *
+ * Return an entity if one is found; return an error-pointer (!NULL) if an
+ * entity was ready, but the scheduler had insufficient credits to accommodate
+ * its job; return NULL, if no ready entity was found.
+ */
+struct drm_sched_entity *
+drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
+			   struct drm_sched_rq *rq)
+{
+	struct drm_sched_entity *entity = NULL;
+	struct rb_node *rb;
+
+	spin_lock(&rq->lock);
+	for (rb = rb_first_cached(&rq->rb_tree_root); rb; rb = rb_next(rb)) {
+		entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
+		if (drm_sched_entity_is_ready(entity))
+			break;
+		else
+			entity = NULL;
+	}
+	spin_unlock(&rq->lock);
+
+	if (!entity)
+		return NULL;
+
+	/*
+	 * If scheduler cannot take more jobs signal the caller to not consider
+	 * lower priority queues.
+	 */
+	if (!drm_sched_can_queue(sched, entity))
+		return ERR_PTR(-ENOSPC);
+
+	reinit_completion(&entity->entity_idle);
+
+	return entity;
+}
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index daf4665f37fa..ccb39e7bf384 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -386,6 +386,9 @@ struct drm_sched_job {
 	ktime_t                         submit_ts;
 };
 
+#define to_drm_sched_job(sched_job)		\
+		container_of((sched_job), struct drm_sched_job, queue_node)
+
 static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
 					    int threshold)
 {
@@ -547,6 +550,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		   atomic_t *score, const char *name, struct device *dev);
 
 void drm_sched_fini(struct drm_gpu_scheduler *sched);
+bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
+			 struct drm_sched_entity *entity);
+
 int drm_sched_job_init(struct drm_sched_job *job,
 		       struct drm_sched_entity *entity,
 		       u32 credits, void *owner);
@@ -586,6 +592,9 @@ bool drm_sched_dependency_optimized(struct dma_fence* fence,
 				    struct drm_sched_entity *entity);
 void drm_sched_fault(struct drm_gpu_scheduler *sched);
 
+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_rq *rq,
 			struct drm_sched_entity *entity,
@@ -595,6 +604,9 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 
 void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
 			     struct drm_sched_entity *entity);
+struct drm_sched_entity *
+drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
+			   struct drm_sched_rq *rq);
 
 int drm_sched_entity_init(struct drm_sched_entity *entity,
 			  enum drm_sched_priority priority,
-- 
2.47.1

[RFC 13/18] drm/sched: Add deadline policy

[Tvrtko Ursulin]

Deadline scheduling policy should be a fairer flavour of FIFO with two
main advantages being that it can naturally connect with the dma-fence
deadlines, and secondly that it can get away with multiple run queues per
scheduler.

From the latter comes the fairness advantage. Where the current FIFO
policy will always starve low priority entities by normal, and normal
by high etc, deadline tracks all runnable entities in a single run queue
and assigns them deadlines based on priority. Instead of being ordered
strictly by priority, jobs and entities become ordered by deadlines.

This means that a later higher priority submission can still overtake an
earlier lower priority one, but eventually the lower priority will get its
turn even if high priority is constantly feeding new work.

Current mapping of priority to deadlines is somewhat arbitrary and looks
like this (submit timestamp plus constant offset in micro-seconds):

	static const unsigned int d_us[] = {
		[DRM_SCHED_PRIORITY_KERNEL] =    100,
		[DRM_SCHED_PRIORITY_HIGH]   =   1000,
		[DRM_SCHED_PRIORITY_NORMAL] =   5000,
		[DRM_SCHED_PRIORITY_LOW]    = 100000,
	};

Assuming simultaneous submission of one normal and one low prioriy job at
a time of "t", they will get respective deadlines of t+5ms and t+100ms.
Hence normal will run first and low will run after it, or at the latest
100ms after it was submitted in case other higher priority submissions
overtake it in the meantime.

Because deadline policy does not need run queues, if the FIFO and RR
polices are later removed, that would allow for a significant
simplification of the code base by reducing the 1:N to 1:1 scheduler
to run queue relationship.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 53 +++++++++++++++++++-----
 drivers/gpu/drm/scheduler/sched_main.c   | 14 ++++---
 drivers/gpu/drm/scheduler/sched_rq.c     |  5 ++-
 include/drm/gpu_scheduler.h              | 10 ++++-
 4 files changed, 64 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index b93da068585e..e3c875b0afe8 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -71,6 +71,8 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->guilty = guilty;
 	entity->num_sched_list = num_sched_list;
 	entity->priority = priority;
+	entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_DEADLINE ?
+			      DRM_SCHED_PRIORITY_KERNEL : priority;
 	/*
 	 * It's perfectly valid to initialize an entity without having a valid
 	 * scheduler attached. It's just not valid to use the scheduler before it
@@ -87,17 +89,23 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 		 */
 		pr_warn("%s: called with uninitialized scheduler\n", __func__);
 	} else if (num_sched_list) {
-		/* 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.
+		enum drm_sched_priority p = entity->priority;
+
+		/*
+		 * 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 (entity->priority >= sched_list[0]->num_rqs) {
-			drm_err(sched_list[0], "entity with out-of-bounds priority:%u num_rqs:%u\n",
-				entity->priority, sched_list[0]->num_rqs);
-			entity->priority = max_t(s32, (s32) sched_list[0]->num_rqs - 1,
-						 (s32) DRM_SCHED_PRIORITY_KERNEL);
+		if (p >= sched_list[0]->num_user_rqs) {
+			drm_err(sched_list[0], "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;
 		}
-		entity->rq = sched_list[0]->sched_rq[entity->priority];
+		entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
 	}
 
 	init_completion(&entity->entity_idle);
@@ -402,6 +410,27 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity,
 }
 EXPORT_SYMBOL(drm_sched_entity_set_priority);
 
+static ktime_t
+__drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
+				    ktime_t submit_ts)
+{
+	static const unsigned int d_us[] = {
+		[DRM_SCHED_PRIORITY_KERNEL] =    100,
+		[DRM_SCHED_PRIORITY_HIGH]   =   1000,
+		[DRM_SCHED_PRIORITY_NORMAL] =   5000,
+		[DRM_SCHED_PRIORITY_LOW]    = 100000,
+	};
+
+	return ktime_add_us(submit_ts, d_us[entity->priority]);
+}
+
+ktime_t
+drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
+				  struct drm_sched_job *job)
+{
+	return __drm_sched_entity_get_job_deadline(entity, job->submit_ts);
+}
+
 /*
  * Add a callback to the current dependency of the entity to wake up the
  * scheduler when the entity becomes available.
@@ -546,7 +575,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 
 	spin_lock(&entity->lock);
 	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
-	rq = sched ? sched->sched_rq[entity->priority] : NULL;
+	rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
 	if (rq != entity->rq) {
 		drm_sched_rq_remove_entity(entity->rq, entity);
 		entity->rq = rq;
@@ -588,6 +617,10 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 	if (first) {
 		struct drm_gpu_scheduler *sched;
 
+		if (drm_sched_policy == DRM_SCHED_POLICY_DEADLINE)
+			submit_ts = __drm_sched_entity_get_job_deadline(entity,
+									submit_ts);
+
 		sched = drm_sched_rq_add_entity(entity->rq, entity, submit_ts);
 		if (sched)
 			drm_sched_wakeup(sched);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index a556ee736f9f..e2a67c7fe2d8 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -87,13 +87,13 @@ static struct lockdep_map drm_sched_lockdep_map = {
 };
 #endif
 
-int drm_sched_policy = DRM_SCHED_POLICY_FIFO;
+int drm_sched_policy = DRM_SCHED_POLICY_DEADLINE;
 
 /**
  * DOC: sched_policy (int)
  * Used to override default entities scheduling policy in a run queue.
  */
-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).");
+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, " __stringify(DRM_SCHED_POLICY_DEADLINE) " = Virtual deadline (default).");
 module_param_named(sched_policy, drm_sched_policy, int, 0444);
 
 static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
@@ -1078,11 +1078,15 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		sched->own_submit_wq = true;
 	}
 
-	sched->sched_rq = kmalloc_array(num_rqs, sizeof(*sched->sched_rq),
+	sched->num_user_rqs = num_rqs;
+	sched->num_rqs = drm_sched_policy != DRM_SCHED_POLICY_DEADLINE ?
+			 num_rqs : 1;
+	sched->sched_rq = kmalloc_array(sched->num_rqs,
+					sizeof(*sched->sched_rq),
 					GFP_KERNEL | __GFP_ZERO);
 	if (!sched->sched_rq)
 		goto Out_check_own;
-	sched->num_rqs = num_rqs;
+
 	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
 		sched->sched_rq[i] = kzalloc(sizeof(*sched->sched_rq[i]), GFP_KERNEL);
 		if (!sched->sched_rq[i])
@@ -1196,7 +1200,7 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
 	if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
 		atomic_inc(&bad->karma);
 
-		for (i = DRM_SCHED_PRIORITY_HIGH; i < sched->num_rqs; i++) {
+		for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
 			struct drm_sched_rq *rq = sched->sched_rq[i];
 
 			spin_lock(&rq->lock);
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index 40f5b770f21a..ac28e2877585 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -151,7 +151,10 @@ void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
 	if (next_job) {
 		ktime_t ts;
 
-		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
+		if (drm_sched_policy == DRM_SCHED_POLICY_DEADLINE)
+			ts = drm_sched_entity_get_job_deadline(entity,
+							       next_job);
+		else if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
 			ts = next_job->submit_ts;
 		else
 			ts = drm_sched_rq_get_rr_deadline(rq);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index ccb39e7bf384..a722c5d6e5f9 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -74,8 +74,9 @@ enum drm_sched_priority {
 /* Used to choose between FIFO and RR job-scheduling */
 extern int drm_sched_policy;
 
-#define DRM_SCHED_POLICY_RR    0
-#define DRM_SCHED_POLICY_FIFO  1
+#define DRM_SCHED_POLICY_RR	  0
+#define DRM_SCHED_POLICY_FIFO	  1
+#define DRM_SCHED_POLICY_DEADLINE 2
 
 /**
  * struct drm_sched_entity - A wrapper around a job queue (typically
@@ -153,6 +154,8 @@ struct drm_sched_entity {
 	 */
 	struct spsc_queue		job_queue;
 
+	enum drm_sched_priority         rq_priority;
+
 	/**
 	 * @fence_seq:
 	 *
@@ -522,6 +525,7 @@ struct drm_gpu_scheduler {
 	long				timeout;
 	const char			*name;
 	u32                             num_rqs;
+	u32                             num_user_rqs;
 	struct drm_sched_rq             **sched_rq;
 	wait_queue_head_t		job_scheduled;
 	atomic64_t			job_id_count;
@@ -623,6 +627,8 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity,
 				   enum drm_sched_priority priority);
 bool drm_sched_entity_is_ready(struct drm_sched_entity *entity);
 int drm_sched_entity_error(struct drm_sched_entity *entity);
+ktime_t drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
+					  struct drm_sched_job *job);
 
 struct drm_sched_fence *drm_sched_fence_alloc(
 	struct drm_sched_entity *s_entity, void *owner);
-- 
2.47.1

[RFC 14/18] drm/sched: Remove FIFO and RR and simplify to a single run queue

[Tvrtko Ursulin]

If the new deadline policy is at least as good as FIFO and we can afford
to remove round-robin, we can simplify the scheduler code by making the
scheduler to run queue relationship always 1:1 and remove some code.

Also, now that the FIFO policy is gone the tree of entities is not a FIFO
tree any more so rename it to just the tree.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  23 ++--
 drivers/gpu/drm/scheduler/sched_entity.c |  30 +----
 drivers/gpu/drm/scheduler/sched_main.c   | 136 ++++++-----------------
 drivers/gpu/drm/scheduler/sched_rq.c     |  36 ++----
 include/drm/gpu_scheduler.h              |  13 +--
 5 files changed, 56 insertions(+), 182 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index b9d08bc96581..918b6d4919e1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -418,25 +418,22 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 
 void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
 {
+	struct drm_sched_rq *rq = sched->rq;
+	struct drm_sched_entity *s_entity;
 	struct drm_sched_job *s_job;
-	struct drm_sched_entity *s_entity = NULL;
-	int i;
 
 	/* Signal all jobs not yet scheduled */
-	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 = to_drm_sched_job(spsc_queue_pop(&s_entity->job_queue)))) {
-				struct drm_sched_fence *s_fence = s_job->s_fence;
+	spin_lock(&rq->lock);
+	list_for_each_entry(s_entity, &rq->entities, list) {
+		while ((s_job = to_drm_sched_job(spsc_queue_pop(&s_entity->job_queue)))) {
+			struct drm_sched_fence *s_fence = s_job->s_fence;
 
-				dma_fence_signal(&s_fence->scheduled);
-				dma_fence_set_error(&s_fence->finished, -EHWPOISON);
-				dma_fence_signal(&s_fence->finished);
-			}
+			dma_fence_signal(&s_fence->scheduled);
+			dma_fence_set_error(&s_fence->finished, -EHWPOISON);
+			dma_fence_signal(&s_fence->finished);
 		}
-		spin_unlock(&rq->lock);
 	}
+	spin_unlock(&rq->lock);
 
 	/* Signal all jobs already scheduled to HW */
 	list_for_each_entry(s_job, &sched->pending_list, list) {
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index e3c875b0afe8..98c78d1373d8 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -71,8 +71,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	entity->guilty = guilty;
 	entity->num_sched_list = num_sched_list;
 	entity->priority = priority;
-	entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_DEADLINE ?
-			      DRM_SCHED_PRIORITY_KERNEL : priority;
 	/*
 	 * It's perfectly valid to initialize an entity without having a valid
 	 * scheduler attached. It's just not valid to use the scheduler before it
@@ -82,30 +80,14 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	RCU_INIT_POINTER(entity->last_scheduled, NULL);
 	RB_CLEAR_NODE(&entity->rb_tree_node);
 
-	if (num_sched_list && !sched_list[0]->sched_rq) {
+	if (num_sched_list && !sched_list[0]->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 (num_sched_list) {
-		enum drm_sched_priority p = entity->priority;
-
-		/*
-		 * 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) {
-			drm_err(sched_list[0], "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;
-		}
-		entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
+		entity->rq = sched_list[0]->rq;
 	}
 
 	init_completion(&entity->entity_idle);
@@ -575,7 +557,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 
 	spin_lock(&entity->lock);
 	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
-	rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
+	rq = sched ? sched->rq : NULL;
 	if (rq != entity->rq) {
 		drm_sched_rq_remove_entity(entity->rq, entity);
 		entity->rq = rq;
@@ -617,10 +599,8 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 	if (first) {
 		struct drm_gpu_scheduler *sched;
 
-		if (drm_sched_policy == DRM_SCHED_POLICY_DEADLINE)
-			submit_ts = __drm_sched_entity_get_job_deadline(entity,
-									submit_ts);
-
+		submit_ts = __drm_sched_entity_get_job_deadline(entity,
+								submit_ts);
 		sched = drm_sched_rq_add_entity(entity->rq, entity, submit_ts);
 		if (sched)
 			drm_sched_wakeup(sched);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index e2a67c7fe2d8..f748dcd06774 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -87,15 +87,6 @@ static struct lockdep_map drm_sched_lockdep_map = {
 };
 #endif
 
-int drm_sched_policy = DRM_SCHED_POLICY_DEADLINE;
-
-/**
- * DOC: sched_policy (int)
- * Used to override default entities scheduling policy in a run queue.
- */
-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, " __stringify(DRM_SCHED_POLICY_DEADLINE) " = Virtual deadline (default).");
-module_param_named(sched_policy, drm_sched_policy, int, 0444);
-
 static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
 {
 	u32 credits;
@@ -822,34 +813,6 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
 	drm_sched_run_job_queue(sched);
 }
 
-/**
- * drm_sched_select_entity - Select next entity to process
- *
- * @sched: scheduler instance
- *
- * Return an entity to process or NULL if none are found.
- *
- * Note, that we break out of the for-loop when "entity" is non-null, which can
- * also be an error-pointer--this assures we don't process lower priority
- * run-queues. See comments in the respectively called functions.
- */
-static struct drm_sched_entity *
-drm_sched_select_entity(struct drm_gpu_scheduler *sched)
-{
-	struct drm_sched_entity *entity = NULL;
-	int i;
-
-	/* Start with the highest priority.
-	 */
-	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
-		entity = drm_sched_rq_select_entity(sched, sched->sched_rq[i]);
-		if (entity)
-			break;
-	}
-
-	return IS_ERR(entity) ? NULL : entity;
-}
-
 /**
  * drm_sched_get_finished_job - fetch the next finished job to be destroyed
  *
@@ -971,8 +934,8 @@ static void drm_sched_run_job_work(struct work_struct *w)
 	int r;
 
 	/* Find entity with a ready job */
-	entity = drm_sched_select_entity(sched);
-	if (!entity)
+	entity = drm_sched_rq_select_entity(sched, sched->rq);
+	if (IS_ERR_OR_NULL(entity))
 		return;	/* No more work */
 
 	sched_job = drm_sched_entity_pop_job(entity);
@@ -1016,7 +979,7 @@ static void drm_sched_run_job_work(struct work_struct *w)
  * @ops: backend operations for this scheduler
  * @submit_wq: workqueue to use for submission. If NULL, an ordered wq is
  *	       allocated and used
- * @num_rqs: number of runqueues, one for each priority, up to DRM_SCHED_PRIORITY_COUNT
+ * @num_rqs: deprecated and ignored
  * @credit_limit: the number of credits this scheduler can hold from all jobs
  * @hang_limit: number of times to allow a job to hang before dropping it
  * @timeout: timeout value in jiffies for the scheduler
@@ -1035,8 +998,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		   long timeout, struct workqueue_struct *timeout_wq,
 		   atomic_t *score, const char *name, struct device *dev)
 {
-	int i;
-
 	sched->ops = ops;
 	sched->credit_limit = credit_limit;
 	sched->name = name;
@@ -1046,13 +1007,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 	sched->score = score ? score : &sched->_score;
 	sched->dev = dev;
 
-	if (num_rqs > DRM_SCHED_PRIORITY_COUNT) {
-		/* This is a gross violation--tell drivers what the  problem is.
-		 */
-		drm_err(sched, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
-			__func__);
-		return -EINVAL;
-	} else if (sched->sched_rq) {
+	if (sched->rq) {
 		/* Not an error, but warn anyway so drivers can
 		 * fine-tune their DRM calling order, and return all
 		 * is good.
@@ -1078,21 +1033,11 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		sched->own_submit_wq = true;
 	}
 
-	sched->num_user_rqs = num_rqs;
-	sched->num_rqs = drm_sched_policy != DRM_SCHED_POLICY_DEADLINE ?
-			 num_rqs : 1;
-	sched->sched_rq = kmalloc_array(sched->num_rqs,
-					sizeof(*sched->sched_rq),
-					GFP_KERNEL | __GFP_ZERO);
-	if (!sched->sched_rq)
+	sched->rq = kmalloc(sizeof(*sched->rq), GFP_KERNEL | __GFP_ZERO);
+	if (!sched->rq)
 		goto Out_check_own;
 
-	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
-		sched->sched_rq[i] = kzalloc(sizeof(*sched->sched_rq[i]), GFP_KERNEL);
-		if (!sched->sched_rq[i])
-			goto Out_unroll;
-		drm_sched_rq_init(sched, sched->sched_rq[i]);
-	}
+	drm_sched_rq_init(sched, sched->rq);
 
 	init_waitqueue_head(&sched->job_scheduled);
 	INIT_LIST_HEAD(&sched->pending_list);
@@ -1104,15 +1049,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 	atomic_set(&sched->_score, 0);
 	atomic64_set(&sched->job_id_count, 0);
 	sched->pause_submit = false;
-
 	sched->ready = true;
 	return 0;
-Out_unroll:
-	for (--i ; i >= DRM_SCHED_PRIORITY_KERNEL; i--)
-		kfree(sched->sched_rq[i]);
 
-	kfree(sched->sched_rq);
-	sched->sched_rq = NULL;
 Out_check_own:
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
@@ -1143,25 +1082,21 @@ EXPORT_SYMBOL(drm_sched_init);
  */
 void drm_sched_fini(struct drm_gpu_scheduler *sched)
 {
+
+	struct drm_sched_rq *rq = sched->rq;
 	struct drm_sched_entity *s_entity;
-	int i;
 
 	drm_sched_wqueue_stop(sched);
 
-	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)
-			/*
-			 * Prevents reinsertion and marks job_queue as idle,
-			 * it will be removed from the rq in drm_sched_entity_fini()
-			 * eventually
-			 */
-			s_entity->stopped = true;
-		spin_unlock(&rq->lock);
-		kfree(sched->sched_rq[i]);
-	}
+	spin_lock(&rq->lock);
+	list_for_each_entry(s_entity, &rq->entities, list)
+		/*
+		 * Prevents reinsertion and marks job_queue as idle,
+		 * it will be removed from the rq in drm_sched_entity_fini()
+		 * eventually
+		 */
+		s_entity->stopped = true;
+	spin_unlock(&rq->lock);
 
 	/* Wakeup everyone stuck in drm_sched_entity_flush for this scheduler */
 	wake_up_all(&sched->job_scheduled);
@@ -1172,8 +1107,8 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
 	sched->ready = false;
-	kfree(sched->sched_rq);
-	sched->sched_rq = NULL;
+	kfree(sched->rq);
+	sched->rq = NULL;
 }
 EXPORT_SYMBOL(drm_sched_fini);
 
@@ -1188,35 +1123,28 @@ EXPORT_SYMBOL(drm_sched_fini);
  */
 void drm_sched_increase_karma(struct drm_sched_job *bad)
 {
-	int i;
-	struct drm_sched_entity *tmp;
-	struct drm_sched_entity *entity;
 	struct drm_gpu_scheduler *sched = bad->sched;
+	struct drm_sched_entity *entity, *tmp;
+	struct drm_sched_rq *rq = sched->rq;
 
 	/* don't change @bad's karma if it's from KERNEL RQ,
 	 * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
 	 * corrupt but keep in mind that kernel jobs always considered good.
 	 */
-	if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
-		atomic_inc(&bad->karma);
+	if (bad->s_priority == DRM_SCHED_PRIORITY_KERNEL)
+		return;
 
-		for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
-			struct drm_sched_rq *rq = sched->sched_rq[i];
+	atomic_inc(&bad->karma);
 
-			spin_lock(&rq->lock);
-			list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
-				if (bad->s_fence->scheduled.context ==
-				    entity->fence_context) {
-					if (entity->guilty)
-						atomic_set(entity->guilty, 1);
-					break;
-				}
-			}
-			spin_unlock(&rq->lock);
-			if (&entity->list != &rq->entities)
-				break;
+	spin_lock(&rq->lock);
+	list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
+		if (bad->s_fence->scheduled.context == entity->fence_context) {
+			if (entity->guilty)
+				atomic_set(entity->guilty, 1);
+			break;
 		}
 	}
+	spin_unlock(&rq->lock);
 }
 EXPORT_SYMBOL(drm_sched_increase_karma);
 
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index ac28e2877585..d9c854b2e495 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -12,7 +12,7 @@ static __always_inline bool drm_sched_entity_compare_before(struct rb_node *a,
 	return ktime_before(ent_a->oldest_job_waiting, ent_b->oldest_job_waiting);
 }
 
-static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
+static void __drm_sched_rq_remove_tree_locked(struct drm_sched_entity *entity,
 					      struct drm_sched_rq *rq)
 {
 	lockdep_assert_held(&entity->lock);
@@ -22,7 +22,7 @@ static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
 	RB_CLEAR_NODE(&entity->rb_tree_node);
 }
 
-static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity *entity,
+static void __drm_sched_rq_add_tree_locked(struct drm_sched_entity *entity,
 					   struct drm_sched_rq *rq,
 					   ktime_t ts)
 {
@@ -56,16 +56,6 @@ void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
 	rq->sched = sched;
 }
 
-static ktime_t
-drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
-{
-	lockdep_assert_held(&rq->lock);
-
-	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
-
-	return rq->rr_deadline;
-}
-
 /**
  * drm_sched_rq_add_entity - add an entity
  *
@@ -99,12 +89,9 @@ drm_sched_rq_add_entity(struct drm_sched_rq *rq,
 		list_add_tail(&entity->list, &rq->entities);
 	}
 
-	if (drm_sched_policy == DRM_SCHED_POLICY_RR)
-		ts = drm_sched_rq_get_rr_deadline(rq);
-
 	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
-		__drm_sched_rq_remove_fifo_locked(entity, rq);
-	__drm_sched_rq_add_fifo_locked(entity, rq, ts);
+		__drm_sched_rq_remove_tree_locked(entity, rq);
+	__drm_sched_rq_add_tree_locked(entity, rq, ts);
 
 	spin_unlock(&rq->lock);
 	spin_unlock(&entity->lock);
@@ -134,7 +121,7 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 	list_del_init(&entity->list);
 
 	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
-		__drm_sched_rq_remove_fifo_locked(entity, rq);
+		__drm_sched_rq_remove_tree_locked(entity, rq);
 
 	spin_unlock(&rq->lock);
 }
@@ -146,20 +133,13 @@ void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
 
 	spin_lock(&entity->lock);
 	spin_lock(&rq->lock);
-	__drm_sched_rq_remove_fifo_locked(entity, rq);
+	__drm_sched_rq_remove_tree_locked(entity, rq);
 	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
 	if (next_job) {
 		ktime_t ts;
 
-		if (drm_sched_policy == DRM_SCHED_POLICY_DEADLINE)
-			ts = drm_sched_entity_get_job_deadline(entity,
-							       next_job);
-		else if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
-			ts = next_job->submit_ts;
-		else
-			ts = drm_sched_rq_get_rr_deadline(rq);
-
-		__drm_sched_rq_add_fifo_locked(entity, rq, ts);
+		ts = drm_sched_entity_get_job_deadline(entity, next_job);
+		__drm_sched_rq_add_tree_locked(entity, rq, ts);
 	}
 	spin_unlock(&rq->lock);
 	spin_unlock(&entity->lock);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index a722c5d6e5f9..9f952574efe2 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -71,13 +71,6 @@ enum drm_sched_priority {
 	DRM_SCHED_PRIORITY_COUNT
 };
 
-/* Used to choose between FIFO and RR job-scheduling */
-extern int drm_sched_policy;
-
-#define DRM_SCHED_POLICY_RR	  0
-#define DRM_SCHED_POLICY_FIFO	  1
-#define DRM_SCHED_POLICY_DEADLINE 2
-
 /**
  * struct drm_sched_entity - A wrapper around a job queue (typically
  * attached to the DRM file_priv).
@@ -154,8 +147,6 @@ struct drm_sched_entity {
 	 */
 	struct spsc_queue		job_queue;
 
-	enum drm_sched_priority         rq_priority;
-
 	/**
 	 * @fence_seq:
 	 *
@@ -524,9 +515,7 @@ struct drm_gpu_scheduler {
 	atomic_t			credit_count;
 	long				timeout;
 	const char			*name;
-	u32                             num_rqs;
-	u32                             num_user_rqs;
-	struct drm_sched_rq             **sched_rq;
+	struct drm_sched_rq             *rq;
 	wait_queue_head_t		job_scheduled;
 	atomic64_t			job_id_count;
 	struct workqueue_struct		*submit_wq;
-- 
2.47.1

[RFC 15/18] drm/sched: Queue all free credits in one worker invocation

[Tvrtko Ursulin]

There is no reason to queue just a single job if scheduler can take more
and re-queue the worker to queue more. We can simply feed the hardware
with as much as it can take in one go and hopefully win some latency.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 109 ++++++++++++-------------
 drivers/gpu/drm/scheduler/sched_rq.c   |  17 +---
 include/drm/gpu_scheduler.h            |   2 -
 3 files changed, 54 insertions(+), 74 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index f748dcd06774..e3d885678b9b 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -98,33 +98,6 @@ static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
 	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 = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
-	if (!s_job)
-		return false;
-
-	/* If a job exceeds the credit limit, truncate it to the credit limit
-	 * itself to guarantee forward progress.
-	 */
-	if (drm_WARN(sched, s_job->credits > sched->credit_limit,
-		     "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
@@ -928,48 +901,68 @@ static void drm_sched_run_job_work(struct work_struct *w)
 {
 	struct drm_gpu_scheduler *sched =
 		container_of(w, struct drm_gpu_scheduler, work_run_job);
+	u32 job_credits, submitted_credits = 0;
 	struct drm_sched_entity *entity;
-	struct dma_fence *fence;
 	struct drm_sched_job *sched_job;
-	int r;
+	struct dma_fence *fence;
 
-	/* Find entity with a ready job */
-	entity = drm_sched_rq_select_entity(sched, sched->rq);
-	if (IS_ERR_OR_NULL(entity))
-		return;	/* No more work */
+	for (;;) {
+		/* Find entity with a ready job */
+		entity = drm_sched_rq_select_entity(sched, sched->rq);
+		if (!entity)
+			break;	/* No more work */
 
-	sched_job = drm_sched_entity_pop_job(entity);
-	if (!sched_job) {
+		/*
+		 * If a job exceeds the credit limit truncate it to guarantee
+		 * forward progress.
+		 */
+		sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
+		job_credits = sched_job->credits;
+		if (drm_WARN_ONCE(sched, job_credits > sched->credit_limit,
+				  "Jobs may not exceed the credit limit, truncating.\n"))
+			job_credits = sched_job->credits = sched->credit_limit;
+
+		if (job_credits > drm_sched_available_credits(sched)) {
+			complete_all(&entity->entity_idle);
+			break;
+		}
+
+		sched_job = drm_sched_entity_pop_job(entity);
 		complete_all(&entity->entity_idle);
-		drm_sched_run_job_queue(sched);
-		return;
-	}
+		if (!sched_job) {
+			/* Top entity is not yet runnable after all */
+			continue;
+		}
+
+		drm_sched_job_begin(sched_job);
+		trace_drm_run_job(sched_job, entity);
+		submitted_credits += job_credits;
+		atomic_add(job_credits, &sched->credit_count);
 
-	atomic_add(sched_job->credits, &sched->credit_count);
-	drm_sched_job_begin(sched_job);
+		fence = sched->ops->run_job(sched_job);
+		drm_sched_fence_scheduled(sched_job->s_fence, fence);
 
-	trace_drm_run_job(sched_job, entity);
-	fence = sched->ops->run_job(sched_job);
-	complete_all(&entity->entity_idle);
-	drm_sched_fence_scheduled(sched_job->s_fence, fence);
+		if (!IS_ERR_OR_NULL(fence)) {
+			int r;
 
-	if (!IS_ERR_OR_NULL(fence)) {
-		/* Drop for original kref_init of the fence */
-		dma_fence_put(fence);
+			/* Drop for original kref_init of the fence */
+			dma_fence_put(fence);
 
-		r = dma_fence_add_callback(fence, &sched_job->cb,
-					   drm_sched_job_done_cb);
-		if (r == -ENOENT)
-			drm_sched_job_done(sched_job, fence->error);
-		else if (r)
-			DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n", r);
-	} else {
-		drm_sched_job_done(sched_job, IS_ERR(fence) ?
-				   PTR_ERR(fence) : 0);
+			r = dma_fence_add_callback(fence, &sched_job->cb,
+						   drm_sched_job_done_cb);
+			if (r == -ENOENT)
+				drm_sched_job_done(sched_job, fence->error);
+			else if (r)
+				DRM_DEV_ERROR(sched->dev,
+					      "fence add callback failed (%d)\n", r);
+		} else {
+			drm_sched_job_done(sched_job, IS_ERR(fence) ?
+						      PTR_ERR(fence) : 0);
+		}
 	}
 
-	wake_up(&sched->job_scheduled);
-	drm_sched_run_job_queue(sched);
+	if (submitted_credits)
+		wake_up(&sched->job_scheduled);
 }
 
 /**
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index d9c854b2e495..647734d3ed73 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -153,9 +153,7 @@ void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
  *
  * Find oldest waiting ready entity.
  *
- * Return an entity if one is found; return an error-pointer (!NULL) if an
- * entity was ready, but the scheduler had insufficient credits to accommodate
- * its job; return NULL, if no ready entity was found.
+ * Return an entity if one is found or NULL if no ready entity was found.
  */
 struct drm_sched_entity *
 drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
@@ -174,17 +172,8 @@ drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
 	}
 	spin_unlock(&rq->lock);
 
-	if (!entity)
-		return NULL;
-
-	/*
-	 * If scheduler cannot take more jobs signal the caller to not consider
-	 * lower priority queues.
-	 */
-	if (!drm_sched_can_queue(sched, entity))
-		return ERR_PTR(-ENOSPC);
-
-	reinit_completion(&entity->entity_idle);
+	if (entity)
+		reinit_completion(&entity->entity_idle);
 
 	return entity;
 }
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 9f952574efe2..6d3a38772e72 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -543,8 +543,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		   atomic_t *score, const char *name, struct device *dev);
 
 void drm_sched_fini(struct drm_gpu_scheduler *sched);
-bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
-			 struct drm_sched_entity *entity);
 
 int drm_sched_job_init(struct drm_sched_job *job,
 		       struct drm_sched_entity *entity,
-- 
2.47.1

[RFC 16/18] drm/sched: Connect with dma-fence deadlines

[Tvrtko Ursulin]

Now that the scheduling policy is deadline based it feels completely
natural to allow propagating externaly set deadlines to the scheduler.

Scheduler deadlines are not a guarantee but as the dma-fence facility is
already in use by userspace lets wire it up.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
Cc: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 98c78d1373d8..db5d34310b18 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -410,7 +410,16 @@ ktime_t
 drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
 				  struct drm_sched_job *job)
 {
-	return __drm_sched_entity_get_job_deadline(entity, job->submit_ts);
+	struct drm_sched_fence *s_fence = job->s_fence;
+	struct dma_fence *fence = &s_fence->finished;
+	ktime_t deadline;
+
+	deadline = __drm_sched_entity_get_job_deadline(entity, job->submit_ts);
+	if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &fence->flags) &&
+	    ktime_before(s_fence->deadline, deadline))
+		deadline = s_fence->deadline;
+
+	return deadline;
 }
 
 /*
@@ -579,9 +588,12 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
  */
 void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 {
+	struct drm_sched_fence *s_fence = sched_job->s_fence;
 	struct drm_sched_entity *entity = sched_job->entity;
-	bool first;
+	struct dma_fence *fence = &s_fence->finished;
+	ktime_t fence_deadline;
 	ktime_t submit_ts;
+	bool first;
 
 	trace_drm_sched_job(sched_job, entity);
 	atomic_inc(entity->rq->sched->score);
@@ -593,6 +605,11 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 	 * Make sure to set the submit_ts first, to avoid a race.
 	 */
 	sched_job->submit_ts = submit_ts = ktime_get();
+	if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &fence->flags))
+		fence_deadline = s_fence->deadline;
+	else
+		fence_deadline = KTIME_MAX;
+
 	first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
 
 	/* first job wakes up scheduler */
@@ -601,6 +618,9 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 
 		submit_ts = __drm_sched_entity_get_job_deadline(entity,
 								submit_ts);
+		if (ktime_before(fence_deadline, submit_ts))
+			submit_ts = fence_deadline;
+
 		sched = drm_sched_rq_add_entity(entity->rq, entity, submit_ts);
 		if (sched)
 			drm_sched_wakeup(sched);
-- 
2.47.1

[RFC 17/18] drm/sched: Embed run queue singleton into the scheduler

[Tvrtko Ursulin]

Now that the run queue to scheduler relationship is always 1:1 we can
embed it (the run queue) directly in the scheduler struct and save on
some allocation error handling code and such.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |  6 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c     |  6 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h     |  5 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h   |  8 ++++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c |  8 +++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c     |  8 +++---
 drivers/gpu/drm/scheduler/sched_entity.c    | 32 +++++++++------------
 drivers/gpu/drm/scheduler/sched_fence.c     |  2 +-
 drivers/gpu/drm/scheduler/sched_main.c      | 31 ++++----------------
 drivers/gpu/drm/scheduler/sched_rq.c        | 20 ++++++-------
 include/drm/gpu_scheduler.h                 | 11 ++-----
 11 files changed, 58 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index d891ab779ca7..25028ac48844 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1108,7 +1108,8 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	if (p->gang_size > 1 && !p->adev->vm_manager.concurrent_flush) {
 		for (i = 0; i < p->gang_size; ++i) {
 			struct drm_sched_entity *entity = p->entities[i];
-			struct drm_gpu_scheduler *sched = entity->rq->sched;
+			struct drm_gpu_scheduler *sched =
+				container_of(entity->rq, typeof(*sched), rq);
 			struct amdgpu_ring *ring = to_amdgpu_ring(sched);
 
 			if (amdgpu_vmid_uses_reserved(adev, vm, ring->vm_hub))
@@ -1233,7 +1234,8 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 			return r;
 	}
 
-	sched = p->gang_leader->base.entity->rq->sched;
+	sched = container_of(p->gang_leader->base.entity->rq, typeof(*sched),
+			     rq);
 	while ((fence = amdgpu_sync_get_fence(&p->sync))) {
 		struct drm_sched_fence *s_fence = to_drm_sched_fence(fence);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 918b6d4919e1..f7abe413044e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -349,7 +349,9 @@ static struct dma_fence *
 amdgpu_job_prepare_job(struct drm_sched_job *sched_job,
 		      struct drm_sched_entity *s_entity)
 {
-	struct amdgpu_ring *ring = to_amdgpu_ring(s_entity->rq->sched);
+	struct drm_gpu_scheduler *sched =
+		container_of(s_entity->rq, typeof(*sched), rq);
+	struct amdgpu_ring *ring = to_amdgpu_ring(sched);
 	struct amdgpu_job *job = to_amdgpu_job(sched_job);
 	struct dma_fence *fence = NULL;
 	int r;
@@ -418,7 +420,7 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 
 void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
 {
-	struct drm_sched_rq *rq = sched->rq;
+	struct drm_sched_rq *rq = &sched->rq;
 	struct drm_sched_entity *s_entity;
 	struct drm_sched_job *s_job;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
index ce6b9ba967ff..d6872baeba1e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
@@ -85,7 +85,10 @@ struct amdgpu_job {
 
 static inline struct amdgpu_ring *amdgpu_job_ring(struct amdgpu_job *job)
 {
-	return to_amdgpu_ring(job->base.entity->rq->sched);
+	struct drm_gpu_scheduler *sched =
+		container_of(job->base.entity->rq, typeof(*sched), rq);
+
+	return to_amdgpu_ring(sched);
 }
 
 int amdgpu_job_alloc(struct amdgpu_device *adev, struct amdgpu_vm *vm,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 383fce40d4dd..a3819ed20d27 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -145,6 +145,7 @@ TRACE_EVENT(amdgpu_cs,
 		     struct amdgpu_ib *ib),
 	    TP_ARGS(p, job, ib),
 	    TP_STRUCT__entry(
+			     __field(struct drm_gpu_scheduler *, sched)
 			     __field(struct amdgpu_bo_list *, bo_list)
 			     __field(u32, ring)
 			     __field(u32, dw)
@@ -152,11 +153,14 @@ TRACE_EVENT(amdgpu_cs,
 			     ),
 
 	    TP_fast_assign(
+			   __entry->sched = container_of(job->base.entity->rq,
+							 typeof(*__entry->sched),
+							 rq);
 			   __entry->bo_list = p->bo_list;
-			   __entry->ring = to_amdgpu_ring(job->base.entity->rq->sched)->idx;
+			   __entry->ring = to_amdgpu_ring(__entry->sched)->idx;
 			   __entry->dw = ib->length_dw;
 			   __entry->fences = amdgpu_fence_count_emitted(
-				to_amdgpu_ring(job->base.entity->rq->sched));
+				to_amdgpu_ring(__entry->sched));
 			   ),
 	    TP_printk("bo_list=%p, ring=%u, dw=%u, fences=%u",
 		      __entry->bo_list, __entry->ring, __entry->dw,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index 46d9fb433ab2..42f2bfb30af1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -105,13 +105,13 @@ static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
 static int amdgpu_vm_sdma_commit(struct amdgpu_vm_update_params *p,
 				 struct dma_fence **fence)
 {
+	struct drm_gpu_scheduler *sched =
+		container_of(p->vm->delayed.rq, typeof(*sched), rq);
+	struct amdgpu_ring *ring =
+		container_of(sched, struct amdgpu_ring, sched);
 	struct amdgpu_ib *ib = p->job->ibs;
-	struct amdgpu_ring *ring;
 	struct dma_fence *f;
 
-	ring = container_of(p->vm->delayed.rq->sched, struct amdgpu_ring,
-			    sched);
-
 	WARN_ON(ib->length_dw == 0);
 	amdgpu_ring_pad_ib(ring, ib);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
index 23b6f7a4aa4a..ab132dae8183 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
@@ -420,15 +420,15 @@ int amdgpu_xcp_open_device(struct amdgpu_device *adev,
 void amdgpu_xcp_release_sched(struct amdgpu_device *adev,
 				  struct amdgpu_ctx_entity *entity)
 {
-	struct drm_gpu_scheduler *sched;
-	struct amdgpu_ring *ring;
+	struct drm_gpu_scheduler *sched =
+		container_of(entity->entity.rq, typeof(*sched), rq);
 
 	if (!adev->xcp_mgr)
 		return;
 
-	sched = entity->entity.rq->sched;
 	if (drm_sched_wqueue_ready(sched)) {
-		ring = to_amdgpu_ring(entity->entity.rq->sched);
+		struct amdgpu_ring *ring = to_amdgpu_ring(sched);
+
 		atomic_dec(&adev->xcp_mgr->xcp[ring->xcp_id].ref_cnt);
 	}
 }
diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index db5d34310b18..29daee6b06e5 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -77,19 +77,12 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
 	 * is initialized itself.
 	 */
 	entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+	if (num_sched_list) {
+		entity->sched_list = num_sched_list > 1 ? sched_list : NULL;
+		entity->rq = &sched_list[0]->rq;
+	}
 	RCU_INIT_POINTER(entity->last_scheduled, NULL);
 	RB_CLEAR_NODE(&entity->rb_tree_node);
-
-	if (num_sched_list && !sched_list[0]->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 (num_sched_list) {
-		entity->rq = sched_list[0]->rq;
-	}
-
 	init_completion(&entity->entity_idle);
 
 	/* We start in an idle state. */
@@ -279,7 +272,7 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout)
 	if (!entity->rq)
 		return 0;
 
-	sched = entity->rq->sched;
+	sched = container_of(entity->rq, typeof(*sched), rq);
 	/**
 	 * The client will not queue more IBs during this fini, consume existing
 	 * queued IBs or discard them on SIGKILL
@@ -370,9 +363,11 @@ static void drm_sched_entity_wakeup(struct dma_fence *f,
 {
 	struct drm_sched_entity *entity =
 		container_of(cb, struct drm_sched_entity, cb);
+	struct drm_gpu_scheduler *sched =
+		container_of(entity->rq, typeof(*sched), rq);
 
 	drm_sched_entity_clear_dep(f, cb);
-	drm_sched_wakeup(entity->rq->sched);
+	drm_sched_wakeup(sched);
 }
 
 /**
@@ -428,7 +423,8 @@ drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
  */
 static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity)
 {
-	struct drm_gpu_scheduler *sched = entity->rq->sched;
+	struct drm_gpu_scheduler *sched =
+		container_of(entity->rq, typeof(*sched), rq);
 	struct dma_fence *fence = entity->dependency;
 	struct drm_sched_fence *s_fence;
 
@@ -566,7 +562,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
 
 	spin_lock(&entity->lock);
 	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
-	rq = sched ? sched->rq : NULL;
+	rq = sched ? &sched->rq : NULL;
 	if (rq != entity->rq) {
 		drm_sched_rq_remove_entity(entity->rq, entity);
 		entity->rq = rq;
@@ -590,13 +586,15 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 {
 	struct drm_sched_fence *s_fence = sched_job->s_fence;
 	struct drm_sched_entity *entity = sched_job->entity;
+	struct drm_gpu_scheduler *sched =
+		container_of(entity->rq, typeof(*sched), rq);
 	struct dma_fence *fence = &s_fence->finished;
 	ktime_t fence_deadline;
 	ktime_t submit_ts;
 	bool first;
 
 	trace_drm_sched_job(sched_job, entity);
-	atomic_inc(entity->rq->sched->score);
+	atomic_inc(sched->score);
 	WRITE_ONCE(entity->last_user, current->group_leader);
 
 	/*
@@ -614,8 +612,6 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 
 	/* first job wakes up scheduler */
 	if (first) {
-		struct drm_gpu_scheduler *sched;
-
 		submit_ts = __drm_sched_entity_get_job_deadline(entity,
 								submit_ts);
 		if (ktime_before(fence_deadline, submit_ts))
diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c
index 0f35f009b9d3..d3bc067cc70b 100644
--- a/drivers/gpu/drm/scheduler/sched_fence.c
+++ b/drivers/gpu/drm/scheduler/sched_fence.c
@@ -223,7 +223,7 @@ void drm_sched_fence_init(struct drm_sched_fence *fence,
 {
 	unsigned seq;
 
-	fence->sched = entity->rq->sched;
+	fence->sched = container_of(entity->rq, typeof(*fence->sched), rq);
 	seq = atomic_inc_return(&entity->fence_seq);
 	dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled,
 		       &fence->lock, entity->fence_context, seq);
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index e3d885678b9b..c078fb7f76db 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -573,7 +573,7 @@ void drm_sched_job_arm(struct drm_sched_job *job)
 
 	BUG_ON(!entity);
 	drm_sched_entity_select_rq(entity);
-	sched = entity->rq->sched;
+	sched = container_of(entity->rq, typeof(*sched), rq);
 
 	job->sched = sched;
 	job->s_priority = entity->priority;
@@ -908,7 +908,7 @@ static void drm_sched_run_job_work(struct work_struct *w)
 
 	for (;;) {
 		/* Find entity with a ready job */
-		entity = drm_sched_rq_select_entity(sched, sched->rq);
+		entity = drm_sched_rq_select_entity(sched);
 		if (!entity)
 			break;	/* No more work */
 
@@ -1000,15 +1000,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 	sched->score = score ? score : &sched->_score;
 	sched->dev = dev;
 
-	if (sched->rq) {
-		/* Not an error, but warn anyway so drivers can
-		 * fine-tune their DRM calling order, and return all
-		 * is good.
-		 */
-		drm_warn(sched, "%s: scheduler already initialized!\n", __func__);
-		return 0;
-	}
-
 	if (submit_wq) {
 		sched->submit_wq = submit_wq;
 		sched->own_submit_wq = false;
@@ -1026,11 +1017,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 		sched->own_submit_wq = true;
 	}
 
-	sched->rq = kmalloc(sizeof(*sched->rq), GFP_KERNEL | __GFP_ZERO);
-	if (!sched->rq)
-		goto Out_check_own;
-
-	drm_sched_rq_init(sched, sched->rq);
+	drm_sched_rq_init(sched);
 
 	init_waitqueue_head(&sched->job_scheduled);
 	INIT_LIST_HEAD(&sched->pending_list);
@@ -1044,12 +1031,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
 	sched->pause_submit = false;
 	sched->ready = true;
 	return 0;
-
-Out_check_own:
-	if (sched->own_submit_wq)
-		destroy_workqueue(sched->submit_wq);
-	drm_err(sched, "%s: Failed to setup GPU scheduler--out of memory\n", __func__);
-	return -ENOMEM;
 }
 EXPORT_SYMBOL(drm_sched_init);
 
@@ -1076,7 +1057,7 @@ EXPORT_SYMBOL(drm_sched_init);
 void drm_sched_fini(struct drm_gpu_scheduler *sched)
 {
 
-	struct drm_sched_rq *rq = sched->rq;
+	struct drm_sched_rq *rq = &sched->rq;
 	struct drm_sched_entity *s_entity;
 
 	drm_sched_wqueue_stop(sched);
@@ -1100,8 +1081,6 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
 	if (sched->own_submit_wq)
 		destroy_workqueue(sched->submit_wq);
 	sched->ready = false;
-	kfree(sched->rq);
-	sched->rq = NULL;
 }
 EXPORT_SYMBOL(drm_sched_fini);
 
@@ -1118,7 +1097,7 @@ void drm_sched_increase_karma(struct drm_sched_job *bad)
 {
 	struct drm_gpu_scheduler *sched = bad->sched;
 	struct drm_sched_entity *entity, *tmp;
-	struct drm_sched_rq *rq = sched->rq;
+	struct drm_sched_rq *rq = &sched->rq;
 
 	/* don't change @bad's karma if it's from KERNEL RQ,
 	 * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
index 647734d3ed73..08c61ea00b93 100644
--- a/drivers/gpu/drm/scheduler/sched_rq.c
+++ b/drivers/gpu/drm/scheduler/sched_rq.c
@@ -43,17 +43,16 @@ static void __drm_sched_rq_add_tree_locked(struct drm_sched_entity *entity,
  * drm_sched_rq_init - initialize a given run queue struct
  *
  * @sched: scheduler instance to associate with this run queue
- * @rq: scheduler run queue
  *
  * Initializes a scheduler runqueue.
  */
-void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
-		       struct drm_sched_rq *rq)
+void drm_sched_rq_init(struct drm_gpu_scheduler *sched)
 {
+	struct drm_sched_rq *rq = &sched->rq;
+
 	spin_lock_init(&rq->lock);
 	INIT_LIST_HEAD(&rq->entities);
 	rq->rb_tree_root = RB_ROOT_CACHED;
-	rq->sched = sched;
 }
 
 /**
@@ -72,7 +71,7 @@ drm_sched_rq_add_entity(struct drm_sched_rq *rq,
 			struct drm_sched_entity *entity,
 			ktime_t ts)
 {
-	struct drm_gpu_scheduler *sched;
+	struct drm_gpu_scheduler *sched = container_of(rq, typeof(*sched), rq);
 
 	if (entity->stopped) {
 		DRM_ERROR("Trying to push to a killed entity\n");
@@ -82,8 +81,6 @@ drm_sched_rq_add_entity(struct drm_sched_rq *rq,
 	spin_lock(&entity->lock);
 	spin_lock(&rq->lock);
 
-	sched = rq->sched;
-
 	if (!list_empty(&entity->list)) {
 		atomic_inc(sched->score);
 		list_add_tail(&entity->list, &rq->entities);
@@ -110,6 +107,8 @@ drm_sched_rq_add_entity(struct drm_sched_rq *rq,
 void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 				struct drm_sched_entity *entity)
 {
+	struct drm_gpu_scheduler *sched = container_of(rq, typeof(*sched), rq);
+
 	lockdep_assert_held(&entity->lock);
 
 	if (list_empty(&entity->list))
@@ -117,7 +116,7 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 
 	spin_lock(&rq->lock);
 
-	atomic_dec(rq->sched->score);
+	atomic_dec(sched->score);
 	list_del_init(&entity->list);
 
 	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
@@ -149,17 +148,16 @@ void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
  * drm_sched_rq_select_entity - Select an entity which provides a job to run
  *
  * @sched: the gpu scheduler
- * @rq: scheduler run queue to check.
  *
  * Find oldest waiting ready entity.
  *
  * Return an entity if one is found or NULL if no ready entity was found.
  */
 struct drm_sched_entity *
-drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
-			   struct drm_sched_rq *rq)
+drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched)
 {
 	struct drm_sched_entity *entity = NULL;
+	struct drm_sched_rq *rq = &sched->rq;
 	struct rb_node *rb;
 
 	spin_lock(&rq->lock);
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 6d3a38772e72..25786fb941d8 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -238,7 +238,6 @@ struct drm_sched_entity {
 /**
  * struct drm_sched_rq - queue of entities to be scheduled.
  *
- * @sched: the scheduler to which this rq belongs to.
  * @lock: protects @entities, @rb_tree_root and @rr_deadline.
  * @entities: list of the entities to be scheduled.
  * @rb_tree_root: root of time based priority queue of entities for FIFO scheduling
@@ -248,8 +247,6 @@ struct drm_sched_entity {
  * the next entity to emit commands from.
  */
 struct drm_sched_rq {
-	struct drm_gpu_scheduler	*sched;
-
 	spinlock_t			lock;
 	/* Following members are protected by the @lock: */
 	ktime_t				rr_deadline;
@@ -515,7 +512,7 @@ struct drm_gpu_scheduler {
 	atomic_t			credit_count;
 	long				timeout;
 	const char			*name;
-	struct drm_sched_rq             *rq;
+	struct drm_sched_rq             rq;
 	wait_queue_head_t		job_scheduled;
 	atomic64_t			job_id_count;
 	struct workqueue_struct		*submit_wq;
@@ -583,8 +580,7 @@ bool drm_sched_dependency_optimized(struct dma_fence* fence,
 				    struct drm_sched_entity *entity);
 void drm_sched_fault(struct drm_gpu_scheduler *sched);
 
-void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
-		       struct drm_sched_rq *rq);
+void drm_sched_rq_init(struct drm_gpu_scheduler *sched);
 
 struct drm_gpu_scheduler *
 drm_sched_rq_add_entity(struct drm_sched_rq *rq,
@@ -596,8 +592,7 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
 void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
 			     struct drm_sched_entity *entity);
 struct drm_sched_entity *
-drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
-			   struct drm_sched_rq *rq);
+drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched);
 
 int drm_sched_entity_init(struct drm_sched_entity *entity,
 			  enum drm_sched_priority priority,
-- 
2.47.1

[RFC 18/18] drm/sched: Scale deadlines depending on queue depth

[Tvrtko Ursulin]

Speculative idea for the concern of how to somewhat prioritise clients who
submit small amount of work infrequently is to scale their deadline
calculation based on their queue depth and priority. Kernel context we can
pull in more aggressively into the past.

On the other hand queue depth may not be representative on the GPU
utilisation so it can also incorrectly penalise short deep(-ish) queues
versus single large jobs.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@redhat.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <pstanner@redhat.com>
---
 drivers/gpu/drm/scheduler/sched_entity.c | 24 ++++++++++++++++++------
 include/drm/gpu_scheduler.h              |  2 ++
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 29daee6b06e5..d17980af85fc 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -391,14 +391,24 @@ static ktime_t
 __drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
 				    ktime_t submit_ts)
 {
-	static const unsigned int d_us[] = {
-		[DRM_SCHED_PRIORITY_KERNEL] =    100,
-		[DRM_SCHED_PRIORITY_HIGH]   =   1000,
-		[DRM_SCHED_PRIORITY_NORMAL] =   5000,
-		[DRM_SCHED_PRIORITY_LOW]    = 100000,
+	static const long d_us[] = {
+		[DRM_SCHED_PRIORITY_KERNEL] = -1000,
+		[DRM_SCHED_PRIORITY_HIGH]   =   334,
+		[DRM_SCHED_PRIORITY_NORMAL] =  1000,
+		[DRM_SCHED_PRIORITY_LOW]    =  6667,
 	};
+	static const unsigned int shift[] = {
+		[DRM_SCHED_PRIORITY_KERNEL] = 4,
+		[DRM_SCHED_PRIORITY_HIGH]   = 1,
+		[DRM_SCHED_PRIORITY_NORMAL] = 2,
+		[DRM_SCHED_PRIORITY_LOW]    = 3,
+	};
+	const unsigned int prio = entity->priority;
+	long d;
+
+	d = d_us[prio] * (atomic_read(&entity->qd) << shift[prio]);
 
-	return ktime_add_us(submit_ts, d_us[entity->priority]);
+	return ktime_add_us(submit_ts, d);
 }
 
 ktime_t
@@ -520,6 +530,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 	 */
 	smp_wmb();
 
+	atomic_dec(&entity->qd);
 	spsc_queue_pop(&entity->job_queue);
 	drm_sched_rq_pop_entity(entity->rq, entity);
 
@@ -608,6 +619,7 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
 	else
 		fence_deadline = KTIME_MAX;
 
+	atomic_inc(&entity->qd);
 	first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
 
 	/* first job wakes up scheduler */
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 25786fb941d8..bce88c9b30c1 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -147,6 +147,8 @@ struct drm_sched_entity {
 	 */
 	struct spsc_queue		job_queue;
 
+	atomic_t			qd;
+
 	/**
 	 * @fence_seq:
 	 *
-- 
2.47.1

Re: [RFC 01/18] drm/amdgpu: Use DRM scheduler API in amdgpu_xcp_release_sched

[Christian König]

Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
> Lets use the existing helper instead of peeking into the structure
> directly.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>

Grr, I can't count of how many of those I already fixed :(

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> index e209b5e101df..23b6f7a4aa4a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> @@ -427,7 +427,7 @@ void amdgpu_xcp_release_sched(struct amdgpu_device *adev,
>   		return;
>   
>   	sched = entity->entity.rq->sched;
> -	if (sched->ready) {
> +	if (drm_sched_wqueue_ready(sched)) {
>   		ring = to_amdgpu_ring(entity->entity.rq->sched);
>   		atomic_dec(&adev->xcp_mgr->xcp[ring->xcp_id].ref_cnt);
>   	}

Re: [RFC 03/18] drm/sched: Remove one local variable

[Christian König]

Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
> It is not helping readability nor it is required to keep the line length
> in check.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 1734c17aeea5..01e0d6e686d1 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -1175,7 +1175,6 @@ static void drm_sched_run_job_work(struct work_struct *w)
>   		container_of(w, struct drm_gpu_scheduler, work_run_job);
>   	struct drm_sched_entity *entity;
>   	struct dma_fence *fence;
> -	struct drm_sched_fence *s_fence;
>   	struct drm_sched_job *sched_job;
>   	int r;
>   
> @@ -1194,15 +1193,13 @@ static void drm_sched_run_job_work(struct work_struct *w)
>   		return;
>   	}
>   
> -	s_fence = sched_job->s_fence;
> -
>   	atomic_add(sched_job->credits, &sched->credit_count);
>   	drm_sched_job_begin(sched_job);
>   
>   	trace_drm_run_job(sched_job, entity);
>   	fence = sched->ops->run_job(sched_job);
>   	complete_all(&entity->entity_idle);
> -	drm_sched_fence_scheduled(s_fence, fence);
> +	drm_sched_fence_scheduled(sched_job->s_fence, fence);

Originally that was not for readability but for correctness.

As soon as complete_all(&entity->entity_idle); was called the sched_job 
could have been released.

But we changed that so that the sched_job is released from a separate 
worker instead, so that is most likely not necessary any more.

Regards,
Christian.

>   
>   	if (!IS_ERR_OR_NULL(fence)) {
>   		/* Drop for original kref_init of the fence */

Re: [RFC 10/18] drm/sched: Implement RR via FIFO

[Christian König]

Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
> Round-robin being the non-default policy and unclear how much it is used,
> we can notice that it can be implemented using the FIFO data structures if
> we only invent a fake submit timestamp which is monotonically increasing
> inside drm_sched_rq instances.
>
> So instead of remembering which was the last entity the scheduler worker
> picked, we can bump the picked one to the bottom of the tree, achieving
> the same round-robin behaviour.
>
> Advantage is that we can consolidate to a single code path and remove a
> bunch of code. Downside is round-robin mode now needs to lock on the job
> pop path but that should not be visible.

Oh that's a really nice one. One comment at the end of the patch.

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c | 50 ++++++++------
>   drivers/gpu/drm/scheduler/sched_main.c   | 84 ++----------------------
>   include/drm/gpu_scheduler.h              |  5 +-
>   3 files changed, 39 insertions(+), 100 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 8e910586979e..2b1bc4d00b57 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -473,9 +473,20 @@ drm_sched_job_dependency(struct drm_sched_job *job,
>   	return NULL;
>   }
>   
> +static ktime_t
> +drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
> +{
> +	lockdep_assert_held(&rq->lock);
> +
> +	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
> +
> +	return rq->rr_deadline;
> +}
> +
>   struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>   {
> -	struct drm_sched_job *sched_job;
> +	struct drm_sched_job *sched_job, *next_job;
> +	struct drm_sched_rq *rq;
>   
>   	sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>   	if (!sched_job)
> @@ -510,23 +521,24 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>   	 * Update the entity's location in the min heap according to
>   	 * the timestamp of the next job, if any.
>   	 */
> -	if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) {
> -		struct drm_sched_job *next;
> -		struct drm_sched_rq *rq;
> +	spin_lock(&entity->lock);
> +	rq = entity->rq;
> +	spin_lock(&rq->lock);
> +	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
> +	if (next_job) {
> +		ktime_t ts;
>   
> -		spin_lock(&entity->lock);
> -		rq = entity->rq;
> -		spin_lock(&rq->lock);
> -		next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
> -		if (next) {
> -			drm_sched_rq_update_fifo_locked(entity, rq,
> -							next->submit_ts);
> -		} else {
> -			drm_sched_rq_remove_fifo_locked(entity, rq);
> -		}
> -		spin_unlock(&rq->lock);
> -		spin_unlock(&entity->lock);
> +		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
> +			ts = next_job->submit_ts;
> +		else
> +			ts = drm_sched_rq_get_rr_deadline(rq);
> +
> +		drm_sched_rq_update_fifo_locked(entity, rq, ts);
> +	} else {
> +		drm_sched_rq_remove_fifo_locked(entity, rq);
>   	}
> +	spin_unlock(&rq->lock);
> +	spin_unlock(&entity->lock);
>   
>   	/* Jobs and entities might have different lifecycles. Since we're
>   	 * removing the job from the entities queue, set the jobs entity pointer
> @@ -624,9 +636,9 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>   
>   		spin_lock(&rq->lock);
>   		drm_sched_rq_add_entity(rq, entity);
> -
> -		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
> -			drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
> +		if (drm_sched_policy == DRM_SCHED_POLICY_RR)
> +			submit_ts = drm_sched_rq_get_rr_deadline(rq);
> +		drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
>   
>   		spin_unlock(&rq->lock);
>   		spin_unlock(&entity->lock);
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index bcfc2abe349a..31cab7bb5428 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -189,7 +189,6 @@ static void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
>   	spin_lock_init(&rq->lock);
>   	INIT_LIST_HEAD(&rq->entities);
>   	rq->rb_tree_root = RB_ROOT_CACHED;
> -	rq->current_entity = NULL;
>   	rq->sched = sched;
>   }
>   
> @@ -235,82 +234,13 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>   	atomic_dec(rq->sched->score);
>   	list_del_init(&entity->list);
>   
> -	if (rq->current_entity == entity)
> -		rq->current_entity = NULL;
> -
> -	if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
> -		drm_sched_rq_remove_fifo_locked(entity, rq);
> +	drm_sched_rq_remove_fifo_locked(entity, rq);
>   
>   	spin_unlock(&rq->lock);
>   }
>   
>   /**
> - * drm_sched_rq_select_entity_rr - Select an entity which could provide a job to run
> - *
> - * @sched: the gpu scheduler
> - * @rq: scheduler run queue to check.
> - *
> - * Try to find the next ready entity.
> - *
> - * Return an entity if one is found; return an error-pointer (!NULL) if an
> - * entity was ready, but the scheduler had insufficient credits to accommodate
> - * its job; return NULL, if no ready entity was found.
> - */
> -static struct drm_sched_entity *
> -drm_sched_rq_select_entity_rr(struct drm_gpu_scheduler *sched,
> -			      struct drm_sched_rq *rq)
> -{
> -	struct drm_sched_entity *entity;
> -
> -	spin_lock(&rq->lock);
> -
> -	entity = rq->current_entity;
> -	if (entity) {
> -		list_for_each_entry_continue(entity, &rq->entities, list) {
> -			if (drm_sched_entity_is_ready(entity)) {
> -				/* If we can't queue yet, preserve the current
> -				 * entity in terms of fairness.
> -				 */
> -				if (!drm_sched_can_queue(sched, entity)) {
> -					spin_unlock(&rq->lock);
> -					return ERR_PTR(-ENOSPC);
> -				}
> -
> -				rq->current_entity = entity;
> -				reinit_completion(&entity->entity_idle);
> -				spin_unlock(&rq->lock);
> -				return entity;
> -			}
> -		}
> -	}
> -
> -	list_for_each_entry(entity, &rq->entities, list) {
> -		if (drm_sched_entity_is_ready(entity)) {
> -			/* If we can't queue yet, preserve the current entity in
> -			 * terms of fairness.
> -			 */
> -			if (!drm_sched_can_queue(sched, entity)) {
> -				spin_unlock(&rq->lock);
> -				return ERR_PTR(-ENOSPC);
> -			}
> -
> -			rq->current_entity = entity;
> -			reinit_completion(&entity->entity_idle);
> -			spin_unlock(&rq->lock);
> -			return entity;
> -		}
> -
> -		if (entity == rq->current_entity)
> -			break;
> -	}
> -
> -	spin_unlock(&rq->lock);
> -
> -	return NULL;
> -}
> -
> -/**
> - * drm_sched_rq_select_entity_fifo - Select an entity which provides a job to run
> + * drm_sched_rq_select_entity - Select an entity which provides a job to run
>    *
>    * @sched: the gpu scheduler
>    * @rq: scheduler run queue to check.
> @@ -322,8 +252,8 @@ drm_sched_rq_select_entity_rr(struct drm_gpu_scheduler *sched,
>    * its job; return NULL, if no ready entity was found.
>    */
>   static struct drm_sched_entity *
> -drm_sched_rq_select_entity_fifo(struct drm_gpu_scheduler *sched,
> -				struct drm_sched_rq *rq)
> +drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
> +			   struct drm_sched_rq *rq)
>   {
>   	struct drm_sched_entity *entity = NULL;
>   	struct rb_node *rb;
> @@ -1055,15 +985,13 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
>   static struct drm_sched_entity *
>   drm_sched_select_entity(struct drm_gpu_scheduler *sched)
>   {
> -	struct drm_sched_entity *entity;
> +	struct drm_sched_entity *entity = NULL;
>   	int i;
>   
>   	/* Start with the highest priority.
>   	 */
>   	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
> -		entity = drm_sched_policy == DRM_SCHED_POLICY_FIFO ?
> -			drm_sched_rq_select_entity_fifo(sched, sched->sched_rq[i]) :
> -			drm_sched_rq_select_entity_rr(sched, sched->sched_rq[i]);
> +		entity = drm_sched_rq_select_entity(sched, sched->sched_rq[i]);
>   		if (entity)
>   			break;
>   	}
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 005db1e35fad..a0164de08f5b 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -245,8 +245,7 @@ struct drm_sched_entity {
>    * struct drm_sched_rq - queue of entities to be scheduled.
>    *
>    * @sched: the scheduler to which this rq belongs to.
> - * @lock: protects @entities, @rb_tree_root and @current_entity.
> - * @current_entity: the entity which is to be scheduled.
> + * @lock: protects @entities, @rb_tree_root and @rr_deadline.
>    * @entities: list of the entities to be scheduled.
>    * @rb_tree_root: root of time based priority queue of entities for FIFO scheduling
>    *
> @@ -259,7 +258,7 @@ struct drm_sched_rq {
>   
>   	spinlock_t			lock;
>   	/* Following members are protected by the @lock: */
> -	struct drm_sched_entity		*current_entity;
> +	ktime_t				rr_deadline;
>   	struct list_head		entities;

If I'm not completely mistaken you can now also nuke this entities list 
if you haven't already removed all users.

Regards,
Christian.

>   	struct rb_root_cached		rb_tree_root;
>   };

Re: [RFC 12/18] drm/sched: Move run queue related code into a separate file

[Christian König]

Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
> Lets move all the code dealing with struct drm_sched_rq into a separate
> compilation unit. Advantage being sched_main.c is left with a clearer set
> of responsibilities.

Looks like a good idea to me in general, but I would also push to 
completely remove run queues or at least rename them.

We only came up with the run queue object to handle different job 
priorities.

But with the FIFO approach now used everywhere you can nuke that and 
just give individual jobs with higher priorities a boost in their FIFO 
score.

Regards,
Christian.

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>
> ---
>   drivers/gpu/drm/scheduler/Makefile     |   2 +-
>   drivers/gpu/drm/scheduler/sched_main.c | 210 +------------------------
>   drivers/gpu/drm/scheduler/sched_rq.c   | 207 ++++++++++++++++++++++++
>   include/drm/gpu_scheduler.h            |  12 ++
>   4 files changed, 222 insertions(+), 209 deletions(-)
>   create mode 100644 drivers/gpu/drm/scheduler/sched_rq.c
>
> diff --git a/drivers/gpu/drm/scheduler/Makefile b/drivers/gpu/drm/scheduler/Makefile
> index 53863621829f..d11d83e285e7 100644
> --- a/drivers/gpu/drm/scheduler/Makefile
> +++ b/drivers/gpu/drm/scheduler/Makefile
> @@ -20,6 +20,6 @@
>   # OTHER DEALINGS IN THE SOFTWARE.
>   #
>   #
> -gpu-sched-y := sched_main.o sched_fence.o sched_entity.o
> +gpu-sched-y := sched_main.o sched_fence.o sched_entity.o sched_rq.o
>   
>   obj-$(CONFIG_DRM_SCHED) += gpu-sched.o
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index a21376ce859f..a556ee736f9f 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -87,9 +87,6 @@ static struct lockdep_map drm_sched_lockdep_map = {
>   };
>   #endif
>   
> -#define to_drm_sched_job(sched_job)		\
> -		container_of((sched_job), struct drm_sched_job, queue_node)
> -
>   int drm_sched_policy = DRM_SCHED_POLICY_FIFO;
>   
>   /**
> @@ -118,8 +115,8 @@ static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
>    * Return true if we can push at least one more job from @entity, false
>    * otherwise.
>    */
> -static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
> -				struct drm_sched_entity *entity)
> +bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
> +			 struct drm_sched_entity *entity)
>   {
>   	struct drm_sched_job *s_job;
>   
> @@ -137,209 +134,6 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
>   	return drm_sched_available_credits(sched) >= s_job->credits;
>   }
>   
> -static __always_inline bool drm_sched_entity_compare_before(struct rb_node *a,
> -							    const struct rb_node *b)
> -{
> -	struct drm_sched_entity *ent_a =  rb_entry((a), struct drm_sched_entity, rb_tree_node);
> -	struct drm_sched_entity *ent_b =  rb_entry((b), struct drm_sched_entity, rb_tree_node);
> -
> -	return ktime_before(ent_a->oldest_job_waiting, ent_b->oldest_job_waiting);
> -}
> -
> -static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
> -					      struct drm_sched_rq *rq)
> -{
> -	lockdep_assert_held(&entity->lock);
> -	lockdep_assert_held(&rq->lock);
> -
> -	rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
> -	RB_CLEAR_NODE(&entity->rb_tree_node);
> -}
> -
> -static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity *entity,
> -					   struct drm_sched_rq *rq,
> -					   ktime_t ts)
> -{
> -	/*
> -	 * Both locks need to be grabbed, one to protect from entity->rq change
> -	 * for entity from within concurrent drm_sched_entity_select_rq and the
> -	 * other to update the rb tree structure.
> -	 */
> -	lockdep_assert_held(&entity->lock);
> -	lockdep_assert_held(&rq->lock);
> -
> -	entity->oldest_job_waiting = ts;
> -
> -	rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
> -		      drm_sched_entity_compare_before);
> -}
> -
> -/**
> - * drm_sched_rq_init - initialize a given run queue struct
> - *
> - * @sched: scheduler instance to associate with this run queue
> - * @rq: scheduler run queue
> - *
> - * Initializes a scheduler runqueue.
> - */
> -static void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
> -			      struct drm_sched_rq *rq)
> -{
> -	spin_lock_init(&rq->lock);
> -	INIT_LIST_HEAD(&rq->entities);
> -	rq->rb_tree_root = RB_ROOT_CACHED;
> -	rq->sched = sched;
> -}
> -
> -static ktime_t
> -drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
> -{
> -	lockdep_assert_held(&rq->lock);
> -
> -	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
> -
> -	return rq->rr_deadline;
> -}
> -
> -/**
> - * drm_sched_rq_add_entity - add an entity
> - *
> - * @rq: scheduler run queue
> - * @entity: scheduler entity
> - *
> - * Adds a scheduler entity to the run queue.
> - *
> - * Returns a DRM scheduler pre-selected to handle this entity.
> - */
> -struct drm_gpu_scheduler *
> -drm_sched_rq_add_entity(struct drm_sched_rq *rq,
> -			struct drm_sched_entity *entity,
> -			ktime_t ts)
> -{
> -	struct drm_gpu_scheduler *sched;
> -
> -	if (entity->stopped) {
> -		DRM_ERROR("Trying to push to a killed entity\n");
> -		return NULL;
> -	}
> -
> -	spin_lock(&entity->lock);
> -	spin_lock(&rq->lock);
> -
> -	sched = rq->sched;
> -
> -	if (!list_empty(&entity->list)) {
> -		atomic_inc(sched->score);
> -		list_add_tail(&entity->list, &rq->entities);
> -	}
> -
> -	if (drm_sched_policy == DRM_SCHED_POLICY_RR)
> -		ts = drm_sched_rq_get_rr_deadline(rq);
> -
> -	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
> -		__drm_sched_rq_remove_fifo_locked(entity, rq);
> -	__drm_sched_rq_add_fifo_locked(entity, rq, ts);
> -
> -	spin_unlock(&rq->lock);
> -	spin_unlock(&entity->lock);
> -
> -	return sched;
> -}
> -
> -/**
> - * drm_sched_rq_remove_entity - remove an entity
> - *
> - * @rq: scheduler run queue
> - * @entity: scheduler entity
> - *
> - * Removes a scheduler entity from the run queue.
> - */
> -void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> -				struct drm_sched_entity *entity)
> -{
> -	lockdep_assert_held(&entity->lock);
> -
> -	if (list_empty(&entity->list))
> -		return;
> -
> -	spin_lock(&rq->lock);
> -
> -	atomic_dec(rq->sched->score);
> -	list_del_init(&entity->list);
> -
> -	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
> -		__drm_sched_rq_remove_fifo_locked(entity, rq);
> -
> -	spin_unlock(&rq->lock);
> -}
> -
> -void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
> -			     struct drm_sched_entity *entity)
> -{
> -	struct drm_sched_job *next_job;
> -
> -	spin_lock(&entity->lock);
> -	spin_lock(&rq->lock);
> -	__drm_sched_rq_remove_fifo_locked(entity, rq);
> -	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
> -	if (next_job) {
> -		ktime_t ts;
> -
> -		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
> -			ts = next_job->submit_ts;
> -		else
> -			ts = drm_sched_rq_get_rr_deadline(rq);
> -
> -		__drm_sched_rq_add_fifo_locked(entity, rq, ts);
> -	}
> -	spin_unlock(&rq->lock);
> -	spin_unlock(&entity->lock);
> -}
> -
> -/**
> - * drm_sched_rq_select_entity - Select an entity which provides a job to run
> - *
> - * @sched: the gpu scheduler
> - * @rq: scheduler run queue to check.
> - *
> - * Find oldest waiting ready entity.
> - *
> - * Return an entity if one is found; return an error-pointer (!NULL) if an
> - * entity was ready, but the scheduler had insufficient credits to accommodate
> - * its job; return NULL, if no ready entity was found.
> - */
> -static struct drm_sched_entity *
> -drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
> -			   struct drm_sched_rq *rq)
> -{
> -	struct drm_sched_entity *entity = NULL;
> -	struct rb_node *rb;
> -
> -	spin_lock(&rq->lock);
> -	for (rb = rb_first_cached(&rq->rb_tree_root); rb; rb = rb_next(rb)) {
> -		entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
> -		if (drm_sched_entity_is_ready(entity))
> -			break;
> -		else
> -			entity = NULL;
> -	}
> -	spin_unlock(&rq->lock);
> -
> -	if (!entity)
> -		return NULL;
> -
> -	/*
> -	 * If scheduler cannot take more jobs signal the caller to not consider
> -	 * lower priority queues.
> -	 */
> -	if (!drm_sched_can_queue(sched, entity))
> -		return ERR_PTR(-ENOSPC);
> -
> -	reinit_completion(&entity->entity_idle);
> -
> -	return entity;
> -}
> -
>   /**
>    * drm_sched_run_job_queue - enqueue run-job work
>    * @sched: scheduler instance
> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
> new file mode 100644
> index 000000000000..40f5b770f21a
> --- /dev/null
> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
> @@ -0,0 +1,207 @@
> +#include <linux/rbtree.h>
> +
> +#include <drm/drm_print.h>
> +#include <drm/gpu_scheduler.h>
> +
> +static __always_inline bool drm_sched_entity_compare_before(struct rb_node *a,
> +							    const struct rb_node *b)
> +{
> +	struct drm_sched_entity *ent_a =  rb_entry((a), struct drm_sched_entity, rb_tree_node);
> +	struct drm_sched_entity *ent_b =  rb_entry((b), struct drm_sched_entity, rb_tree_node);
> +
> +	return ktime_before(ent_a->oldest_job_waiting, ent_b->oldest_job_waiting);
> +}
> +
> +static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
> +					      struct drm_sched_rq *rq)
> +{
> +	lockdep_assert_held(&entity->lock);
> +	lockdep_assert_held(&rq->lock);
> +
> +	rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
> +	RB_CLEAR_NODE(&entity->rb_tree_node);
> +}
> +
> +static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity *entity,
> +					   struct drm_sched_rq *rq,
> +					   ktime_t ts)
> +{
> +	/*
> +	 * Both locks need to be grabbed, one to protect from entity->rq change
> +	 * for entity from within concurrent drm_sched_entity_select_rq and the
> +	 * other to update the rb tree structure.
> +	 */
> +	lockdep_assert_held(&entity->lock);
> +	lockdep_assert_held(&rq->lock);
> +
> +	entity->oldest_job_waiting = ts;
> +	rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
> +		      drm_sched_entity_compare_before);
> +}
> +
> +/**
> + * drm_sched_rq_init - initialize a given run queue struct
> + *
> + * @sched: scheduler instance to associate with this run queue
> + * @rq: scheduler run queue
> + *
> + * Initializes a scheduler runqueue.
> + */
> +void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
> +		       struct drm_sched_rq *rq)
> +{
> +	spin_lock_init(&rq->lock);
> +	INIT_LIST_HEAD(&rq->entities);
> +	rq->rb_tree_root = RB_ROOT_CACHED;
> +	rq->sched = sched;
> +}
> +
> +static ktime_t
> +drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
> +{
> +	lockdep_assert_held(&rq->lock);
> +
> +	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
> +
> +	return rq->rr_deadline;
> +}
> +
> +/**
> + * drm_sched_rq_add_entity - add an entity
> + *
> + * @rq: scheduler run queue
> + * @entity: scheduler entity
> + * @ts: submission timestamp
> + *
> + * Adds a scheduler entity to the run queue.
> + *
> + * Returns a DRM scheduler pre-selected to handle this entity.
> + */
> +struct drm_gpu_scheduler *
> +drm_sched_rq_add_entity(struct drm_sched_rq *rq,
> +			struct drm_sched_entity *entity,
> +			ktime_t ts)
> +{
> +	struct drm_gpu_scheduler *sched;
> +
> +	if (entity->stopped) {
> +		DRM_ERROR("Trying to push to a killed entity\n");
> +		return NULL;
> +	}
> +
> +	spin_lock(&entity->lock);
> +	spin_lock(&rq->lock);
> +
> +	sched = rq->sched;
> +
> +	if (!list_empty(&entity->list)) {
> +		atomic_inc(sched->score);
> +		list_add_tail(&entity->list, &rq->entities);
> +	}
> +
> +	if (drm_sched_policy == DRM_SCHED_POLICY_RR)
> +		ts = drm_sched_rq_get_rr_deadline(rq);
> +
> +	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
> +		__drm_sched_rq_remove_fifo_locked(entity, rq);
> +	__drm_sched_rq_add_fifo_locked(entity, rq, ts);
> +
> +	spin_unlock(&rq->lock);
> +	spin_unlock(&entity->lock);
> +
> +	return sched;
> +}
> +
> +/**
> + * drm_sched_rq_remove_entity - remove an entity
> + *
> + * @rq: scheduler run queue
> + * @entity: scheduler entity
> + *
> + * Removes a scheduler entity from the run queue.
> + */
> +void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
> +				struct drm_sched_entity *entity)
> +{
> +	lockdep_assert_held(&entity->lock);
> +
> +	if (list_empty(&entity->list))
> +		return;
> +
> +	spin_lock(&rq->lock);
> +
> +	atomic_dec(rq->sched->score);
> +	list_del_init(&entity->list);
> +
> +	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
> +		__drm_sched_rq_remove_fifo_locked(entity, rq);
> +
> +	spin_unlock(&rq->lock);
> +}
> +
> +void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
> +			     struct drm_sched_entity *entity)
> +{
> +	struct drm_sched_job *next_job;
> +
> +	spin_lock(&entity->lock);
> +	spin_lock(&rq->lock);
> +	__drm_sched_rq_remove_fifo_locked(entity, rq);
> +	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
> +	if (next_job) {
> +		ktime_t ts;
> +
> +		if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
> +			ts = next_job->submit_ts;
> +		else
> +			ts = drm_sched_rq_get_rr_deadline(rq);
> +
> +		__drm_sched_rq_add_fifo_locked(entity, rq, ts);
> +	}
> +	spin_unlock(&rq->lock);
> +	spin_unlock(&entity->lock);
> +}
> +
> +/**
> + * drm_sched_rq_select_entity - Select an entity which provides a job to run
> + *
> + * @sched: the gpu scheduler
> + * @rq: scheduler run queue to check.
> + *
> + * Find oldest waiting ready entity.
> + *
> + * Return an entity if one is found; return an error-pointer (!NULL) if an
> + * entity was ready, but the scheduler had insufficient credits to accommodate
> + * its job; return NULL, if no ready entity was found.
> + */
> +struct drm_sched_entity *
> +drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
> +			   struct drm_sched_rq *rq)
> +{
> +	struct drm_sched_entity *entity = NULL;
> +	struct rb_node *rb;
> +
> +	spin_lock(&rq->lock);
> +	for (rb = rb_first_cached(&rq->rb_tree_root); rb; rb = rb_next(rb)) {
> +		entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
> +		if (drm_sched_entity_is_ready(entity))
> +			break;
> +		else
> +			entity = NULL;
> +	}
> +	spin_unlock(&rq->lock);
> +
> +	if (!entity)
> +		return NULL;
> +
> +	/*
> +	 * If scheduler cannot take more jobs signal the caller to not consider
> +	 * lower priority queues.
> +	 */
> +	if (!drm_sched_can_queue(sched, entity))
> +		return ERR_PTR(-ENOSPC);
> +
> +	reinit_completion(&entity->entity_idle);
> +
> +	return entity;
> +}
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index daf4665f37fa..ccb39e7bf384 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -386,6 +386,9 @@ struct drm_sched_job {
>   	ktime_t                         submit_ts;
>   };
>   
> +#define to_drm_sched_job(sched_job)		\
> +		container_of((sched_job), struct drm_sched_job, queue_node)
> +
>   static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
>   					    int threshold)
>   {
> @@ -547,6 +550,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>   		   atomic_t *score, const char *name, struct device *dev);
>   
>   void drm_sched_fini(struct drm_gpu_scheduler *sched);
> +bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
> +			 struct drm_sched_entity *entity);
> +
>   int drm_sched_job_init(struct drm_sched_job *job,
>   		       struct drm_sched_entity *entity,
>   		       u32 credits, void *owner);
> @@ -586,6 +592,9 @@ bool drm_sched_dependency_optimized(struct dma_fence* fence,
>   				    struct drm_sched_entity *entity);
>   void drm_sched_fault(struct drm_gpu_scheduler *sched);
>   
> +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_rq *rq,
>   			struct drm_sched_entity *entity,
> @@ -595,6 +604,9 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>   
>   void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
>   			     struct drm_sched_entity *entity);
> +struct drm_sched_entity *
> +drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
> +			   struct drm_sched_rq *rq);
>   
>   int drm_sched_entity_init(struct drm_sched_entity *entity,
>   			  enum drm_sched_priority priority,

Re: [RFC 14/18] drm/sched: Remove FIFO and RR and simplify to a single run queue

[Christian König]

Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
> If the new deadline policy is at least as good as FIFO and we can afford
> to remove round-robin, we can simplify the scheduler code by making the
> scheduler to run queue relationship always 1:1 and remove some code.
>
> Also, now that the FIFO policy is gone the tree of entities is not a FIFO
> tree any more so rename it to just the tree.

Yeah that's exactly what I mean with my comment on the other patch.

I'm totally in favor of that, feel free to add my acked by to this approach.

I will try to find some time to give it some in deep review.

Thanks,
Christian.

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c  |  23 ++--
>   drivers/gpu/drm/scheduler/sched_entity.c |  30 +----
>   drivers/gpu/drm/scheduler/sched_main.c   | 136 ++++++-----------------
>   drivers/gpu/drm/scheduler/sched_rq.c     |  36 ++----
>   include/drm/gpu_scheduler.h              |  13 +--
>   5 files changed, 56 insertions(+), 182 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index b9d08bc96581..918b6d4919e1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -418,25 +418,22 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
>   
>   void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
>   {
> +	struct drm_sched_rq *rq = sched->rq;
> +	struct drm_sched_entity *s_entity;
>   	struct drm_sched_job *s_job;
> -	struct drm_sched_entity *s_entity = NULL;
> -	int i;
>   
>   	/* Signal all jobs not yet scheduled */
> -	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 = to_drm_sched_job(spsc_queue_pop(&s_entity->job_queue)))) {
> -				struct drm_sched_fence *s_fence = s_job->s_fence;
> +	spin_lock(&rq->lock);
> +	list_for_each_entry(s_entity, &rq->entities, list) {
> +		while ((s_job = to_drm_sched_job(spsc_queue_pop(&s_entity->job_queue)))) {
> +			struct drm_sched_fence *s_fence = s_job->s_fence;
>   
> -				dma_fence_signal(&s_fence->scheduled);
> -				dma_fence_set_error(&s_fence->finished, -EHWPOISON);
> -				dma_fence_signal(&s_fence->finished);
> -			}
> +			dma_fence_signal(&s_fence->scheduled);
> +			dma_fence_set_error(&s_fence->finished, -EHWPOISON);
> +			dma_fence_signal(&s_fence->finished);
>   		}
> -		spin_unlock(&rq->lock);
>   	}
> +	spin_unlock(&rq->lock);
>   
>   	/* Signal all jobs already scheduled to HW */
>   	list_for_each_entry(s_job, &sched->pending_list, list) {
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index e3c875b0afe8..98c78d1373d8 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -71,8 +71,6 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>   	entity->guilty = guilty;
>   	entity->num_sched_list = num_sched_list;
>   	entity->priority = priority;
> -	entity->rq_priority = drm_sched_policy == DRM_SCHED_POLICY_DEADLINE ?
> -			      DRM_SCHED_PRIORITY_KERNEL : priority;
>   	/*
>   	 * It's perfectly valid to initialize an entity without having a valid
>   	 * scheduler attached. It's just not valid to use the scheduler before it
> @@ -82,30 +80,14 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,
>   	RCU_INIT_POINTER(entity->last_scheduled, NULL);
>   	RB_CLEAR_NODE(&entity->rb_tree_node);
>   
> -	if (num_sched_list && !sched_list[0]->sched_rq) {
> +	if (num_sched_list && !sched_list[0]->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 (num_sched_list) {
> -		enum drm_sched_priority p = entity->priority;
> -
> -		/*
> -		 * 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) {
> -			drm_err(sched_list[0], "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;
> -		}
> -		entity->rq = sched_list[0]->sched_rq[entity->rq_priority];
> +		entity->rq = sched_list[0]->rq;
>   	}
>   
>   	init_completion(&entity->entity_idle);
> @@ -575,7 +557,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
>   
>   	spin_lock(&entity->lock);
>   	sched = drm_sched_pick_best(entity->sched_list, entity->num_sched_list);
> -	rq = sched ? sched->sched_rq[entity->rq_priority] : NULL;
> +	rq = sched ? sched->rq : NULL;
>   	if (rq != entity->rq) {
>   		drm_sched_rq_remove_entity(entity->rq, entity);
>   		entity->rq = rq;
> @@ -617,10 +599,8 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>   	if (first) {
>   		struct drm_gpu_scheduler *sched;
>   
> -		if (drm_sched_policy == DRM_SCHED_POLICY_DEADLINE)
> -			submit_ts = __drm_sched_entity_get_job_deadline(entity,
> -									submit_ts);
> -
> +		submit_ts = __drm_sched_entity_get_job_deadline(entity,
> +								submit_ts);
>   		sched = drm_sched_rq_add_entity(entity->rq, entity, submit_ts);
>   		if (sched)
>   			drm_sched_wakeup(sched);
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index e2a67c7fe2d8..f748dcd06774 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -87,15 +87,6 @@ static struct lockdep_map drm_sched_lockdep_map = {
>   };
>   #endif
>   
> -int drm_sched_policy = DRM_SCHED_POLICY_DEADLINE;
> -
> -/**
> - * DOC: sched_policy (int)
> - * Used to override default entities scheduling policy in a run queue.
> - */
> -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, " __stringify(DRM_SCHED_POLICY_DEADLINE) " = Virtual deadline (default).");
> -module_param_named(sched_policy, drm_sched_policy, int, 0444);
> -
>   static u32 drm_sched_available_credits(struct drm_gpu_scheduler *sched)
>   {
>   	u32 credits;
> @@ -822,34 +813,6 @@ void drm_sched_wakeup(struct drm_gpu_scheduler *sched)
>   	drm_sched_run_job_queue(sched);
>   }
>   
> -/**
> - * drm_sched_select_entity - Select next entity to process
> - *
> - * @sched: scheduler instance
> - *
> - * Return an entity to process or NULL if none are found.
> - *
> - * Note, that we break out of the for-loop when "entity" is non-null, which can
> - * also be an error-pointer--this assures we don't process lower priority
> - * run-queues. See comments in the respectively called functions.
> - */
> -static struct drm_sched_entity *
> -drm_sched_select_entity(struct drm_gpu_scheduler *sched)
> -{
> -	struct drm_sched_entity *entity = NULL;
> -	int i;
> -
> -	/* Start with the highest priority.
> -	 */
> -	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
> -		entity = drm_sched_rq_select_entity(sched, sched->sched_rq[i]);
> -		if (entity)
> -			break;
> -	}
> -
> -	return IS_ERR(entity) ? NULL : entity;
> -}
> -
>   /**
>    * drm_sched_get_finished_job - fetch the next finished job to be destroyed
>    *
> @@ -971,8 +934,8 @@ static void drm_sched_run_job_work(struct work_struct *w)
>   	int r;
>   
>   	/* Find entity with a ready job */
> -	entity = drm_sched_select_entity(sched);
> -	if (!entity)
> +	entity = drm_sched_rq_select_entity(sched, sched->rq);
> +	if (IS_ERR_OR_NULL(entity))
>   		return;	/* No more work */
>   
>   	sched_job = drm_sched_entity_pop_job(entity);
> @@ -1016,7 +979,7 @@ static void drm_sched_run_job_work(struct work_struct *w)
>    * @ops: backend operations for this scheduler
>    * @submit_wq: workqueue to use for submission. If NULL, an ordered wq is
>    *	       allocated and used
> - * @num_rqs: number of runqueues, one for each priority, up to DRM_SCHED_PRIORITY_COUNT
> + * @num_rqs: deprecated and ignored
>    * @credit_limit: the number of credits this scheduler can hold from all jobs
>    * @hang_limit: number of times to allow a job to hang before dropping it
>    * @timeout: timeout value in jiffies for the scheduler
> @@ -1035,8 +998,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>   		   long timeout, struct workqueue_struct *timeout_wq,
>   		   atomic_t *score, const char *name, struct device *dev)
>   {
> -	int i;
> -
>   	sched->ops = ops;
>   	sched->credit_limit = credit_limit;
>   	sched->name = name;
> @@ -1046,13 +1007,7 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>   	sched->score = score ? score : &sched->_score;
>   	sched->dev = dev;
>   
> -	if (num_rqs > DRM_SCHED_PRIORITY_COUNT) {
> -		/* This is a gross violation--tell drivers what the  problem is.
> -		 */
> -		drm_err(sched, "%s: num_rqs cannot be greater than DRM_SCHED_PRIORITY_COUNT\n",
> -			__func__);
> -		return -EINVAL;
> -	} else if (sched->sched_rq) {
> +	if (sched->rq) {
>   		/* Not an error, but warn anyway so drivers can
>   		 * fine-tune their DRM calling order, and return all
>   		 * is good.
> @@ -1078,21 +1033,11 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>   		sched->own_submit_wq = true;
>   	}
>   
> -	sched->num_user_rqs = num_rqs;
> -	sched->num_rqs = drm_sched_policy != DRM_SCHED_POLICY_DEADLINE ?
> -			 num_rqs : 1;
> -	sched->sched_rq = kmalloc_array(sched->num_rqs,
> -					sizeof(*sched->sched_rq),
> -					GFP_KERNEL | __GFP_ZERO);
> -	if (!sched->sched_rq)
> +	sched->rq = kmalloc(sizeof(*sched->rq), GFP_KERNEL | __GFP_ZERO);
> +	if (!sched->rq)
>   		goto Out_check_own;
>   
> -	for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
> -		sched->sched_rq[i] = kzalloc(sizeof(*sched->sched_rq[i]), GFP_KERNEL);
> -		if (!sched->sched_rq[i])
> -			goto Out_unroll;
> -		drm_sched_rq_init(sched, sched->sched_rq[i]);
> -	}
> +	drm_sched_rq_init(sched, sched->rq);
>   
>   	init_waitqueue_head(&sched->job_scheduled);
>   	INIT_LIST_HEAD(&sched->pending_list);
> @@ -1104,15 +1049,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>   	atomic_set(&sched->_score, 0);
>   	atomic64_set(&sched->job_id_count, 0);
>   	sched->pause_submit = false;
> -
>   	sched->ready = true;
>   	return 0;
> -Out_unroll:
> -	for (--i ; i >= DRM_SCHED_PRIORITY_KERNEL; i--)
> -		kfree(sched->sched_rq[i]);
>   
> -	kfree(sched->sched_rq);
> -	sched->sched_rq = NULL;
>   Out_check_own:
>   	if (sched->own_submit_wq)
>   		destroy_workqueue(sched->submit_wq);
> @@ -1143,25 +1082,21 @@ EXPORT_SYMBOL(drm_sched_init);
>    */
>   void drm_sched_fini(struct drm_gpu_scheduler *sched)
>   {
> +
> +	struct drm_sched_rq *rq = sched->rq;
>   	struct drm_sched_entity *s_entity;
> -	int i;
>   
>   	drm_sched_wqueue_stop(sched);
>   
> -	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)
> -			/*
> -			 * Prevents reinsertion and marks job_queue as idle,
> -			 * it will be removed from the rq in drm_sched_entity_fini()
> -			 * eventually
> -			 */
> -			s_entity->stopped = true;
> -		spin_unlock(&rq->lock);
> -		kfree(sched->sched_rq[i]);
> -	}
> +	spin_lock(&rq->lock);
> +	list_for_each_entry(s_entity, &rq->entities, list)
> +		/*
> +		 * Prevents reinsertion and marks job_queue as idle,
> +		 * it will be removed from the rq in drm_sched_entity_fini()
> +		 * eventually
> +		 */
> +		s_entity->stopped = true;
> +	spin_unlock(&rq->lock);
>   
>   	/* Wakeup everyone stuck in drm_sched_entity_flush for this scheduler */
>   	wake_up_all(&sched->job_scheduled);
> @@ -1172,8 +1107,8 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched)
>   	if (sched->own_submit_wq)
>   		destroy_workqueue(sched->submit_wq);
>   	sched->ready = false;
> -	kfree(sched->sched_rq);
> -	sched->sched_rq = NULL;
> +	kfree(sched->rq);
> +	sched->rq = NULL;
>   }
>   EXPORT_SYMBOL(drm_sched_fini);
>   
> @@ -1188,35 +1123,28 @@ EXPORT_SYMBOL(drm_sched_fini);
>    */
>   void drm_sched_increase_karma(struct drm_sched_job *bad)
>   {
> -	int i;
> -	struct drm_sched_entity *tmp;
> -	struct drm_sched_entity *entity;
>   	struct drm_gpu_scheduler *sched = bad->sched;
> +	struct drm_sched_entity *entity, *tmp;
> +	struct drm_sched_rq *rq = sched->rq;
>   
>   	/* don't change @bad's karma if it's from KERNEL RQ,
>   	 * because sometimes GPU hang would cause kernel jobs (like VM updating jobs)
>   	 * corrupt but keep in mind that kernel jobs always considered good.
>   	 */
> -	if (bad->s_priority != DRM_SCHED_PRIORITY_KERNEL) {
> -		atomic_inc(&bad->karma);
> +	if (bad->s_priority == DRM_SCHED_PRIORITY_KERNEL)
> +		return;
>   
> -		for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
> -			struct drm_sched_rq *rq = sched->sched_rq[i];
> +	atomic_inc(&bad->karma);
>   
> -			spin_lock(&rq->lock);
> -			list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
> -				if (bad->s_fence->scheduled.context ==
> -				    entity->fence_context) {
> -					if (entity->guilty)
> -						atomic_set(entity->guilty, 1);
> -					break;
> -				}
> -			}
> -			spin_unlock(&rq->lock);
> -			if (&entity->list != &rq->entities)
> -				break;
> +	spin_lock(&rq->lock);
> +	list_for_each_entry_safe(entity, tmp, &rq->entities, list) {
> +		if (bad->s_fence->scheduled.context == entity->fence_context) {
> +			if (entity->guilty)
> +				atomic_set(entity->guilty, 1);
> +			break;
>   		}
>   	}
> +	spin_unlock(&rq->lock);
>   }
>   EXPORT_SYMBOL(drm_sched_increase_karma);
>   
> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c b/drivers/gpu/drm/scheduler/sched_rq.c
> index ac28e2877585..d9c854b2e495 100644
> --- a/drivers/gpu/drm/scheduler/sched_rq.c
> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
> @@ -12,7 +12,7 @@ static __always_inline bool drm_sched_entity_compare_before(struct rb_node *a,
>   	return ktime_before(ent_a->oldest_job_waiting, ent_b->oldest_job_waiting);
>   }
>   
> -static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
> +static void __drm_sched_rq_remove_tree_locked(struct drm_sched_entity *entity,
>   					      struct drm_sched_rq *rq)
>   {
>   	lockdep_assert_held(&entity->lock);
> @@ -22,7 +22,7 @@ static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity *entity,
>   	RB_CLEAR_NODE(&entity->rb_tree_node);
>   }
>   
> -static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity *entity,
> +static void __drm_sched_rq_add_tree_locked(struct drm_sched_entity *entity,
>   					   struct drm_sched_rq *rq,
>   					   ktime_t ts)
>   {
> @@ -56,16 +56,6 @@ void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
>   	rq->sched = sched;
>   }
>   
> -static ktime_t
> -drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
> -{
> -	lockdep_assert_held(&rq->lock);
> -
> -	rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
> -
> -	return rq->rr_deadline;
> -}
> -
>   /**
>    * drm_sched_rq_add_entity - add an entity
>    *
> @@ -99,12 +89,9 @@ drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>   		list_add_tail(&entity->list, &rq->entities);
>   	}
>   
> -	if (drm_sched_policy == DRM_SCHED_POLICY_RR)
> -		ts = drm_sched_rq_get_rr_deadline(rq);
> -
>   	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
> -		__drm_sched_rq_remove_fifo_locked(entity, rq);
> -	__drm_sched_rq_add_fifo_locked(entity, rq, ts);
> +		__drm_sched_rq_remove_tree_locked(entity, rq);
> +	__drm_sched_rq_add_tree_locked(entity, rq, ts);
>   
>   	spin_unlock(&rq->lock);
>   	spin_unlock(&entity->lock);
> @@ -134,7 +121,7 @@ void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>   	list_del_init(&entity->list);
>   
>   	if (!RB_EMPTY_NODE(&entity->rb_tree_node))
> -		__drm_sched_rq_remove_fifo_locked(entity, rq);
> +		__drm_sched_rq_remove_tree_locked(entity, rq);
>   
>   	spin_unlock(&rq->lock);
>   }
> @@ -146,20 +133,13 @@ void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
>   
>   	spin_lock(&entity->lock);
>   	spin_lock(&rq->lock);
> -	__drm_sched_rq_remove_fifo_locked(entity, rq);
> +	__drm_sched_rq_remove_tree_locked(entity, rq);
>   	next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>   	if (next_job) {
>   		ktime_t ts;
>   
> -		if (drm_sched_policy == DRM_SCHED_POLICY_DEADLINE)
> -			ts = drm_sched_entity_get_job_deadline(entity,
> -							       next_job);
> -		else if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
> -			ts = next_job->submit_ts;
> -		else
> -			ts = drm_sched_rq_get_rr_deadline(rq);
> -
> -		__drm_sched_rq_add_fifo_locked(entity, rq, ts);
> +		ts = drm_sched_entity_get_job_deadline(entity, next_job);
> +		__drm_sched_rq_add_tree_locked(entity, rq, ts);
>   	}
>   	spin_unlock(&rq->lock);
>   	spin_unlock(&entity->lock);
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index a722c5d6e5f9..9f952574efe2 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -71,13 +71,6 @@ enum drm_sched_priority {
>   	DRM_SCHED_PRIORITY_COUNT
>   };
>   
> -/* Used to choose between FIFO and RR job-scheduling */
> -extern int drm_sched_policy;
> -
> -#define DRM_SCHED_POLICY_RR	  0
> -#define DRM_SCHED_POLICY_FIFO	  1
> -#define DRM_SCHED_POLICY_DEADLINE 2
> -
>   /**
>    * struct drm_sched_entity - A wrapper around a job queue (typically
>    * attached to the DRM file_priv).
> @@ -154,8 +147,6 @@ struct drm_sched_entity {
>   	 */
>   	struct spsc_queue		job_queue;
>   
> -	enum drm_sched_priority         rq_priority;
> -
>   	/**
>   	 * @fence_seq:
>   	 *
> @@ -524,9 +515,7 @@ struct drm_gpu_scheduler {
>   	atomic_t			credit_count;
>   	long				timeout;
>   	const char			*name;
> -	u32                             num_rqs;
> -	u32                             num_user_rqs;
> -	struct drm_sched_rq             **sched_rq;
> +	struct drm_sched_rq             *rq;
>   	wait_queue_head_t		job_scheduled;
>   	atomic64_t			job_id_count;
>   	struct workqueue_struct		*submit_wq;

Re: [RFC 16/18] drm/sched: Connect with dma-fence deadlines

[Christian König]

Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
> Now that the scheduling policy is deadline based it feels completely
> natural to allow propagating externaly set deadlines to the scheduler.
>
> Scheduler deadlines are not a guarantee but as the dma-fence facility is
> already in use by userspace lets wire it up.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>
> Cc: Rob Clark <robdclark@gmail.com>
> ---
>   drivers/gpu/drm/scheduler/sched_entity.c | 24 ++++++++++++++++++++++--
>   1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
> index 98c78d1373d8..db5d34310b18 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -410,7 +410,16 @@ ktime_t
>   drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
>   				  struct drm_sched_job *job)
>   {
> -	return __drm_sched_entity_get_job_deadline(entity, job->submit_ts);
> +	struct drm_sched_fence *s_fence = job->s_fence;
> +	struct dma_fence *fence = &s_fence->finished;
> +	ktime_t deadline;
> +
> +	deadline = __drm_sched_entity_get_job_deadline(entity, job->submit_ts);
> +	if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &fence->flags) &&
> +	    ktime_before(s_fence->deadline, deadline))
> +		deadline = s_fence->deadline;
> +
> +	return deadline;
>   }
>   
>   /*
> @@ -579,9 +588,12 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)
>    */
>   void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>   {
> +	struct drm_sched_fence *s_fence = sched_job->s_fence;
>   	struct drm_sched_entity *entity = sched_job->entity;
> -	bool first;
> +	struct dma_fence *fence = &s_fence->finished;
> +	ktime_t fence_deadline;
>   	ktime_t submit_ts;
> +	bool first;
>   
>   	trace_drm_sched_job(sched_job, entity);
>   	atomic_inc(entity->rq->sched->score);
> @@ -593,6 +605,11 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>   	 * Make sure to set the submit_ts first, to avoid a race.
>   	 */
>   	sched_job->submit_ts = submit_ts = ktime_get();
> +	if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &fence->flags))
> +		fence_deadline = s_fence->deadline;
> +	else
> +		fence_deadline = KTIME_MAX;
> +

That makes no sense. When the job is pushed the fence is not made public 
yet.

So no deadline can be set on the fence.

>   	first = spsc_queue_push(&entity->job_queue, &sched_job->queue_node);
>   
>   	/* first job wakes up scheduler */
> @@ -601,6 +618,9 @@ void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>   
>   		submit_ts = __drm_sched_entity_get_job_deadline(entity,
>   								submit_ts);
> +		if (ktime_before(fence_deadline, submit_ts))
> +			submit_ts = fence_deadline;
> +

Yeah, that won't work at all as far as I can see.

Regards,
Christian.

>   		sched = drm_sched_rq_add_entity(entity->rq, entity, submit_ts);
>   		if (sched)
>   			drm_sched_wakeup(sched);

Re: [RFC 03/18] drm/sched: Remove one local variable

[Tvrtko Ursulin]

On 09/01/2025 12:49, Christian König wrote:
> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>> It is not helping readability nor it is required to keep the line length
>> in check.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 5 +----
>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index 1734c17aeea5..01e0d6e686d1 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -1175,7 +1175,6 @@ static void drm_sched_run_job_work(struct 
>> work_struct *w)
>>           container_of(w, struct drm_gpu_scheduler, work_run_job);
>>       struct drm_sched_entity *entity;
>>       struct dma_fence *fence;
>> -    struct drm_sched_fence *s_fence;
>>       struct drm_sched_job *sched_job;
>>       int r;
>> @@ -1194,15 +1193,13 @@ static void drm_sched_run_job_work(struct 
>> work_struct *w)
>>           return;
>>       }
>> -    s_fence = sched_job->s_fence;
>> -
>>       atomic_add(sched_job->credits, &sched->credit_count);
>>       drm_sched_job_begin(sched_job);
>>       trace_drm_run_job(sched_job, entity);
>>       fence = sched->ops->run_job(sched_job);
>>       complete_all(&entity->entity_idle);
>> -    drm_sched_fence_scheduled(s_fence, fence);
>> +    drm_sched_fence_scheduled(sched_job->s_fence, fence);
> 
> Originally that was not for readability but for correctness.
> 
> As soon as complete_all(&entity->entity_idle); was called the sched_job 
> could have been released.

And without a comment ouch.

> But we changed that so that the sched_job is released from a separate 
> worker instead, so that is most likely not necessary any more.

Very subtle. Especially given some drivers use unordered queue.

And for them sched_job is dereferenced a few more times in the block 
below so not sure how it is safe.

Move complete_all() to the end of it all?

Regards,

Tvrtko

>>       if (!IS_ERR_OR_NULL(fence)) {
>>           /* Drop for original kref_init of the fence */
> 

Re: [RFC 10/18] drm/sched: Implement RR via FIFO

[Tvrtko Ursulin]

On 09/01/2025 12:59, Christian König wrote:
> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>> Round-robin being the non-default policy and unclear how much it is used,
>> we can notice that it can be implemented using the FIFO data 
>> structures if
>> we only invent a fake submit timestamp which is monotonically increasing
>> inside drm_sched_rq instances.
>>
>> So instead of remembering which was the last entity the scheduler worker
>> picked, we can bump the picked one to the bottom of the tree, achieving
>> the same round-robin behaviour.
>>
>> Advantage is that we can consolidate to a single code path and remove a
>> bunch of code. Downside is round-robin mode now needs to lock on the job
>> pop path but that should not be visible.
> 
> Oh that's a really nice one. One comment at the end of the patch.

Thanks!

> 
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_entity.c | 50 ++++++++------
>>   drivers/gpu/drm/scheduler/sched_main.c   | 84 ++----------------------
>>   include/drm/gpu_scheduler.h              |  5 +-
>>   3 files changed, 39 insertions(+), 100 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>> b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 8e910586979e..2b1bc4d00b57 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -473,9 +473,20 @@ drm_sched_job_dependency(struct drm_sched_job *job,
>>       return NULL;
>>   }
>> +static ktime_t
>> +drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
>> +{
>> +    lockdep_assert_held(&rq->lock);
>> +
>> +    rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
>> +
>> +    return rq->rr_deadline;
>> +}
>> +
>>   struct drm_sched_job *drm_sched_entity_pop_job(struct 
>> drm_sched_entity *entity)
>>   {
>> -    struct drm_sched_job *sched_job;
>> +    struct drm_sched_job *sched_job, *next_job;
>> +    struct drm_sched_rq *rq;
>>       sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>>       if (!sched_job)
>> @@ -510,23 +521,24 @@ struct drm_sched_job 
>> *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>>        * Update the entity's location in the min heap according to
>>        * the timestamp of the next job, if any.
>>        */
>> -    if (drm_sched_policy == DRM_SCHED_POLICY_FIFO) {
>> -        struct drm_sched_job *next;
>> -        struct drm_sched_rq *rq;
>> +    spin_lock(&entity->lock);
>> +    rq = entity->rq;
>> +    spin_lock(&rq->lock);
>> +    next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>> +    if (next_job) {
>> +        ktime_t ts;
>> -        spin_lock(&entity->lock);
>> -        rq = entity->rq;
>> -        spin_lock(&rq->lock);
>> -        next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>> -        if (next) {
>> -            drm_sched_rq_update_fifo_locked(entity, rq,
>> -                            next->submit_ts);
>> -        } else {
>> -            drm_sched_rq_remove_fifo_locked(entity, rq);
>> -        }
>> -        spin_unlock(&rq->lock);
>> -        spin_unlock(&entity->lock);
>> +        if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> +            ts = next_job->submit_ts;
>> +        else
>> +            ts = drm_sched_rq_get_rr_deadline(rq);
>> +
>> +        drm_sched_rq_update_fifo_locked(entity, rq, ts);
>> +    } else {
>> +        drm_sched_rq_remove_fifo_locked(entity, rq);
>>       }
>> +    spin_unlock(&rq->lock);
>> +    spin_unlock(&entity->lock);
>>       /* Jobs and entities might have different lifecycles. Since we're
>>        * removing the job from the entities queue, set the jobs entity 
>> pointer
>> @@ -624,9 +636,9 @@ void drm_sched_entity_push_job(struct 
>> drm_sched_job *sched_job)
>>           spin_lock(&rq->lock);
>>           drm_sched_rq_add_entity(rq, entity);
>> -
>> -        if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> -            drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
>> +        if (drm_sched_policy == DRM_SCHED_POLICY_RR)
>> +            submit_ts = drm_sched_rq_get_rr_deadline(rq);
>> +        drm_sched_rq_update_fifo_locked(entity, rq, submit_ts);
>>           spin_unlock(&rq->lock);
>>           spin_unlock(&entity->lock);
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index bcfc2abe349a..31cab7bb5428 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -189,7 +189,6 @@ static void drm_sched_rq_init(struct 
>> drm_gpu_scheduler *sched,
>>       spin_lock_init(&rq->lock);
>>       INIT_LIST_HEAD(&rq->entities);
>>       rq->rb_tree_root = RB_ROOT_CACHED;
>> -    rq->current_entity = NULL;
>>       rq->sched = sched;
>>   }
>> @@ -235,82 +234,13 @@ void drm_sched_rq_remove_entity(struct 
>> drm_sched_rq *rq,
>>       atomic_dec(rq->sched->score);
>>       list_del_init(&entity->list);
>> -    if (rq->current_entity == entity)
>> -        rq->current_entity = NULL;
>> -
>> -    if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> -        drm_sched_rq_remove_fifo_locked(entity, rq);
>> +    drm_sched_rq_remove_fifo_locked(entity, rq);
>>       spin_unlock(&rq->lock);
>>   }
>>   /**
>> - * drm_sched_rq_select_entity_rr - Select an entity which could 
>> provide a job to run
>> - *
>> - * @sched: the gpu scheduler
>> - * @rq: scheduler run queue to check.
>> - *
>> - * Try to find the next ready entity.
>> - *
>> - * Return an entity if one is found; return an error-pointer (!NULL) 
>> if an
>> - * entity was ready, but the scheduler had insufficient credits to 
>> accommodate
>> - * its job; return NULL, if no ready entity was found.
>> - */
>> -static struct drm_sched_entity *
>> -drm_sched_rq_select_entity_rr(struct drm_gpu_scheduler *sched,
>> -                  struct drm_sched_rq *rq)
>> -{
>> -    struct drm_sched_entity *entity;
>> -
>> -    spin_lock(&rq->lock);
>> -
>> -    entity = rq->current_entity;
>> -    if (entity) {
>> -        list_for_each_entry_continue(entity, &rq->entities, list) {
>> -            if (drm_sched_entity_is_ready(entity)) {
>> -                /* If we can't queue yet, preserve the current
>> -                 * entity in terms of fairness.
>> -                 */
>> -                if (!drm_sched_can_queue(sched, entity)) {
>> -                    spin_unlock(&rq->lock);
>> -                    return ERR_PTR(-ENOSPC);
>> -                }
>> -
>> -                rq->current_entity = entity;
>> -                reinit_completion(&entity->entity_idle);
>> -                spin_unlock(&rq->lock);
>> -                return entity;
>> -            }
>> -        }
>> -    }
>> -
>> -    list_for_each_entry(entity, &rq->entities, list) {
>> -        if (drm_sched_entity_is_ready(entity)) {
>> -            /* If we can't queue yet, preserve the current entity in
>> -             * terms of fairness.
>> -             */
>> -            if (!drm_sched_can_queue(sched, entity)) {
>> -                spin_unlock(&rq->lock);
>> -                return ERR_PTR(-ENOSPC);
>> -            }
>> -
>> -            rq->current_entity = entity;
>> -            reinit_completion(&entity->entity_idle);
>> -            spin_unlock(&rq->lock);
>> -            return entity;
>> -        }
>> -
>> -        if (entity == rq->current_entity)
>> -            break;
>> -    }
>> -
>> -    spin_unlock(&rq->lock);
>> -
>> -    return NULL;
>> -}
>> -
>> -/**
>> - * drm_sched_rq_select_entity_fifo - Select an entity which provides 
>> a job to run
>> + * drm_sched_rq_select_entity - Select an entity which provides a job 
>> to run
>>    *
>>    * @sched: the gpu scheduler
>>    * @rq: scheduler run queue to check.
>> @@ -322,8 +252,8 @@ drm_sched_rq_select_entity_rr(struct 
>> drm_gpu_scheduler *sched,
>>    * its job; return NULL, if no ready entity was found.
>>    */
>>   static struct drm_sched_entity *
>> -drm_sched_rq_select_entity_fifo(struct drm_gpu_scheduler *sched,
>> -                struct drm_sched_rq *rq)
>> +drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
>> +               struct drm_sched_rq *rq)
>>   {
>>       struct drm_sched_entity *entity = NULL;
>>       struct rb_node *rb;
>> @@ -1055,15 +985,13 @@ void drm_sched_wakeup(struct drm_gpu_scheduler 
>> *sched)
>>   static struct drm_sched_entity *
>>   drm_sched_select_entity(struct drm_gpu_scheduler *sched)
>>   {
>> -    struct drm_sched_entity *entity;
>> +    struct drm_sched_entity *entity = NULL;
>>       int i;
>>       /* Start with the highest priority.
>>        */
>>       for (i = DRM_SCHED_PRIORITY_KERNEL; i < sched->num_rqs; i++) {
>> -        entity = drm_sched_policy == DRM_SCHED_POLICY_FIFO ?
>> -            drm_sched_rq_select_entity_fifo(sched, sched->sched_rq[i]) :
>> -            drm_sched_rq_select_entity_rr(sched, sched->sched_rq[i]);
>> +        entity = drm_sched_rq_select_entity(sched, sched->sched_rq[i]);
>>           if (entity)
>>               break;
>>       }
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 005db1e35fad..a0164de08f5b 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -245,8 +245,7 @@ struct drm_sched_entity {
>>    * struct drm_sched_rq - queue of entities to be scheduled.
>>    *
>>    * @sched: the scheduler to which this rq belongs to.
>> - * @lock: protects @entities, @rb_tree_root and @current_entity.
>> - * @current_entity: the entity which is to be scheduled.
>> + * @lock: protects @entities, @rb_tree_root and @rr_deadline.
>>    * @entities: list of the entities to be scheduled.
>>    * @rb_tree_root: root of time based priority queue of entities for 
>> FIFO scheduling
>>    *
>> @@ -259,7 +258,7 @@ struct drm_sched_rq {
>>       spinlock_t            lock;
>>       /* Following members are protected by the @lock: */
>> -    struct drm_sched_entity        *current_entity;
>> +    ktime_t                rr_deadline;
>>       struct list_head        entities;
> 
> If I'm not completely mistaken you can now also nuke this entities list 
> if you haven't already removed all users.

I had a version which did that too. But then I dropped it in favour of 
only keeping entities with queued jobs in the tree. (Before both the 
list and the tree contained entities which submitted at least one job in 
the past.)

I kind of like keeping the tree trimmed (would it lower the rb tree 
re-balancing costs?) in which case the full list is needed for that 
karma processing business.

Regards,

Tvrtko

> 
> Regards,
> Christian.
> 
>>       struct rb_root_cached        rb_tree_root;
>>   };
> 

Re: [RFC 12/18] drm/sched: Move run queue related code into a separate file

[Tvrtko Ursulin]

On 09/01/2025 13:02, Christian König wrote:
> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>> Lets move all the code dealing with struct drm_sched_rq into a separate
>> compilation unit. Advantage being sched_main.c is left with a clearer set
>> of responsibilities.
> 
> Looks like a good idea to me in general, but I would also push to 
> completely remove run queues or at least rename them.

Glad you noticed how this lead to more possbile simplifications and
yes, that was next in my list of goals.

Key is to find test cases to establish more confidence in the absence of 
regressions with the single rq approach.

Regards,

Tvrtko

> We only came up with the run queue object to handle different job 
> priorities.
> 
> But with the FIFO approach now used everywhere you can nuke that and 
> just give individual jobs with higher priorities a boost in their FIFO 
> score.
> 
> Regards,
> Christian.
> 
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> ---
>>   drivers/gpu/drm/scheduler/Makefile     |   2 +-
>>   drivers/gpu/drm/scheduler/sched_main.c | 210 +------------------------
>>   drivers/gpu/drm/scheduler/sched_rq.c   | 207 ++++++++++++++++++++++++
>>   include/drm/gpu_scheduler.h            |  12 ++
>>   4 files changed, 222 insertions(+), 209 deletions(-)
>>   create mode 100644 drivers/gpu/drm/scheduler/sched_rq.c
>>
>> diff --git a/drivers/gpu/drm/scheduler/Makefile 
>> b/drivers/gpu/drm/scheduler/Makefile
>> index 53863621829f..d11d83e285e7 100644
>> --- a/drivers/gpu/drm/scheduler/Makefile
>> +++ b/drivers/gpu/drm/scheduler/Makefile
>> @@ -20,6 +20,6 @@
>>   # OTHER DEALINGS IN THE SOFTWARE.
>>   #
>>   #
>> -gpu-sched-y := sched_main.o sched_fence.o sched_entity.o
>> +gpu-sched-y := sched_main.o sched_fence.o sched_entity.o sched_rq.o
>>   obj-$(CONFIG_DRM_SCHED) += gpu-sched.o
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index a21376ce859f..a556ee736f9f 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -87,9 +87,6 @@ static struct lockdep_map drm_sched_lockdep_map = {
>>   };
>>   #endif
>> -#define to_drm_sched_job(sched_job)        \
>> -        container_of((sched_job), struct drm_sched_job, queue_node)
>> -
>>   int drm_sched_policy = DRM_SCHED_POLICY_FIFO;
>>   /**
>> @@ -118,8 +115,8 @@ static u32 drm_sched_available_credits(struct 
>> drm_gpu_scheduler *sched)
>>    * Return true if we can push at least one more job from @entity, false
>>    * otherwise.
>>    */
>> -static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
>> -                struct drm_sched_entity *entity)
>> +bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
>> +             struct drm_sched_entity *entity)
>>   {
>>       struct drm_sched_job *s_job;
>> @@ -137,209 +134,6 @@ static bool drm_sched_can_queue(struct 
>> drm_gpu_scheduler *sched,
>>       return drm_sched_available_credits(sched) >= s_job->credits;
>>   }
>> -static __always_inline bool drm_sched_entity_compare_before(struct 
>> rb_node *a,
>> -                                const struct rb_node *b)
>> -{
>> -    struct drm_sched_entity *ent_a =  rb_entry((a), struct 
>> drm_sched_entity, rb_tree_node);
>> -    struct drm_sched_entity *ent_b =  rb_entry((b), struct 
>> drm_sched_entity, rb_tree_node);
>> -
>> -    return ktime_before(ent_a->oldest_job_waiting, 
>> ent_b->oldest_job_waiting);
>> -}
>> -
>> -static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity 
>> *entity,
>> -                          struct drm_sched_rq *rq)
>> -{
>> -    lockdep_assert_held(&entity->lock);
>> -    lockdep_assert_held(&rq->lock);
>> -
>> -    rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
>> -    RB_CLEAR_NODE(&entity->rb_tree_node);
>> -}
>> -
>> -static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity 
>> *entity,
>> -                       struct drm_sched_rq *rq,
>> -                       ktime_t ts)
>> -{
>> -    /*
>> -     * Both locks need to be grabbed, one to protect from entity->rq 
>> change
>> -     * for entity from within concurrent drm_sched_entity_select_rq 
>> and the
>> -     * other to update the rb tree structure.
>> -     */
>> -    lockdep_assert_held(&entity->lock);
>> -    lockdep_assert_held(&rq->lock);
>> -
>> -    entity->oldest_job_waiting = ts;
>> -
>> -    rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
>> -              drm_sched_entity_compare_before);
>> -}
>> -
>> -/**
>> - * drm_sched_rq_init - initialize a given run queue struct
>> - *
>> - * @sched: scheduler instance to associate with this run queue
>> - * @rq: scheduler run queue
>> - *
>> - * Initializes a scheduler runqueue.
>> - */
>> -static void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
>> -                  struct drm_sched_rq *rq)
>> -{
>> -    spin_lock_init(&rq->lock);
>> -    INIT_LIST_HEAD(&rq->entities);
>> -    rq->rb_tree_root = RB_ROOT_CACHED;
>> -    rq->sched = sched;
>> -}
>> -
>> -static ktime_t
>> -drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
>> -{
>> -    lockdep_assert_held(&rq->lock);
>> -
>> -    rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
>> -
>> -    return rq->rr_deadline;
>> -}
>> -
>> -/**
>> - * drm_sched_rq_add_entity - add an entity
>> - *
>> - * @rq: scheduler run queue
>> - * @entity: scheduler entity
>> - *
>> - * Adds a scheduler entity to the run queue.
>> - *
>> - * Returns a DRM scheduler pre-selected to handle this entity.
>> - */
>> -struct drm_gpu_scheduler *
>> -drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>> -            struct drm_sched_entity *entity,
>> -            ktime_t ts)
>> -{
>> -    struct drm_gpu_scheduler *sched;
>> -
>> -    if (entity->stopped) {
>> -        DRM_ERROR("Trying to push to a killed entity\n");
>> -        return NULL;
>> -    }
>> -
>> -    spin_lock(&entity->lock);
>> -    spin_lock(&rq->lock);
>> -
>> -    sched = rq->sched;
>> -
>> -    if (!list_empty(&entity->list)) {
>> -        atomic_inc(sched->score);
>> -        list_add_tail(&entity->list, &rq->entities);
>> -    }
>> -
>> -    if (drm_sched_policy == DRM_SCHED_POLICY_RR)
>> -        ts = drm_sched_rq_get_rr_deadline(rq);
>> -
>> -    if (!RB_EMPTY_NODE(&entity->rb_tree_node))
>> -        __drm_sched_rq_remove_fifo_locked(entity, rq);
>> -    __drm_sched_rq_add_fifo_locked(entity, rq, ts);
>> -
>> -    spin_unlock(&rq->lock);
>> -    spin_unlock(&entity->lock);
>> -
>> -    return sched;
>> -}
>> -
>> -/**
>> - * drm_sched_rq_remove_entity - remove an entity
>> - *
>> - * @rq: scheduler run queue
>> - * @entity: scheduler entity
>> - *
>> - * Removes a scheduler entity from the run queue.
>> - */
>> -void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>> -                struct drm_sched_entity *entity)
>> -{
>> -    lockdep_assert_held(&entity->lock);
>> -
>> -    if (list_empty(&entity->list))
>> -        return;
>> -
>> -    spin_lock(&rq->lock);
>> -
>> -    atomic_dec(rq->sched->score);
>> -    list_del_init(&entity->list);
>> -
>> -    if (!RB_EMPTY_NODE(&entity->rb_tree_node))
>> -        __drm_sched_rq_remove_fifo_locked(entity, rq);
>> -
>> -    spin_unlock(&rq->lock);
>> -}
>> -
>> -void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
>> -                 struct drm_sched_entity *entity)
>> -{
>> -    struct drm_sched_job *next_job;
>> -
>> -    spin_lock(&entity->lock);
>> -    spin_lock(&rq->lock);
>> -    __drm_sched_rq_remove_fifo_locked(entity, rq);
>> -    next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>> -    if (next_job) {
>> -        ktime_t ts;
>> -
>> -        if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> -            ts = next_job->submit_ts;
>> -        else
>> -            ts = drm_sched_rq_get_rr_deadline(rq);
>> -
>> -        __drm_sched_rq_add_fifo_locked(entity, rq, ts);
>> -    }
>> -    spin_unlock(&rq->lock);
>> -    spin_unlock(&entity->lock);
>> -}
>> -
>> -/**
>> - * drm_sched_rq_select_entity - Select an entity which provides a job 
>> to run
>> - *
>> - * @sched: the gpu scheduler
>> - * @rq: scheduler run queue to check.
>> - *
>> - * Find oldest waiting ready entity.
>> - *
>> - * Return an entity if one is found; return an error-pointer (!NULL) 
>> if an
>> - * entity was ready, but the scheduler had insufficient credits to 
>> accommodate
>> - * its job; return NULL, if no ready entity was found.
>> - */
>> -static struct drm_sched_entity *
>> -drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
>> -               struct drm_sched_rq *rq)
>> -{
>> -    struct drm_sched_entity *entity = NULL;
>> -    struct rb_node *rb;
>> -
>> -    spin_lock(&rq->lock);
>> -    for (rb = rb_first_cached(&rq->rb_tree_root); rb; rb = 
>> rb_next(rb)) {
>> -        entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
>> -        if (drm_sched_entity_is_ready(entity))
>> -            break;
>> -        else
>> -            entity = NULL;
>> -    }
>> -    spin_unlock(&rq->lock);
>> -
>> -    if (!entity)
>> -        return NULL;
>> -
>> -    /*
>> -     * If scheduler cannot take more jobs signal the caller to not 
>> consider
>> -     * lower priority queues.
>> -     */
>> -    if (!drm_sched_can_queue(sched, entity))
>> -        return ERR_PTR(-ENOSPC);
>> -
>> -    reinit_completion(&entity->entity_idle);
>> -
>> -    return entity;
>> -}
>> -
>>   /**
>>    * drm_sched_run_job_queue - enqueue run-job work
>>    * @sched: scheduler instance
>> diff --git a/drivers/gpu/drm/scheduler/sched_rq.c 
>> b/drivers/gpu/drm/scheduler/sched_rq.c
>> new file mode 100644
>> index 000000000000..40f5b770f21a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/scheduler/sched_rq.c
>> @@ -0,0 +1,207 @@
>> +#include <linux/rbtree.h>
>> +
>> +#include <drm/drm_print.h>
>> +#include <drm/gpu_scheduler.h>
>> +
>> +static __always_inline bool drm_sched_entity_compare_before(struct 
>> rb_node *a,
>> +                                const struct rb_node *b)
>> +{
>> +    struct drm_sched_entity *ent_a =  rb_entry((a), struct 
>> drm_sched_entity, rb_tree_node);
>> +    struct drm_sched_entity *ent_b =  rb_entry((b), struct 
>> drm_sched_entity, rb_tree_node);
>> +
>> +    return ktime_before(ent_a->oldest_job_waiting, 
>> ent_b->oldest_job_waiting);
>> +}
>> +
>> +static void __drm_sched_rq_remove_fifo_locked(struct drm_sched_entity 
>> *entity,
>> +                          struct drm_sched_rq *rq)
>> +{
>> +    lockdep_assert_held(&entity->lock);
>> +    lockdep_assert_held(&rq->lock);
>> +
>> +    rb_erase_cached(&entity->rb_tree_node, &rq->rb_tree_root);
>> +    RB_CLEAR_NODE(&entity->rb_tree_node);
>> +}
>> +
>> +static void __drm_sched_rq_add_fifo_locked(struct drm_sched_entity 
>> *entity,
>> +                       struct drm_sched_rq *rq,
>> +                       ktime_t ts)
>> +{
>> +    /*
>> +     * Both locks need to be grabbed, one to protect from entity->rq 
>> change
>> +     * for entity from within concurrent drm_sched_entity_select_rq 
>> and the
>> +     * other to update the rb tree structure.
>> +     */
>> +    lockdep_assert_held(&entity->lock);
>> +    lockdep_assert_held(&rq->lock);
>> +
>> +    entity->oldest_job_waiting = ts;
>> +    rb_add_cached(&entity->rb_tree_node, &rq->rb_tree_root,
>> +              drm_sched_entity_compare_before);
>> +}
>> +
>> +/**
>> + * drm_sched_rq_init - initialize a given run queue struct
>> + *
>> + * @sched: scheduler instance to associate with this run queue
>> + * @rq: scheduler run queue
>> + *
>> + * Initializes a scheduler runqueue.
>> + */
>> +void drm_sched_rq_init(struct drm_gpu_scheduler *sched,
>> +               struct drm_sched_rq *rq)
>> +{
>> +    spin_lock_init(&rq->lock);
>> +    INIT_LIST_HEAD(&rq->entities);
>> +    rq->rb_tree_root = RB_ROOT_CACHED;
>> +    rq->sched = sched;
>> +}
>> +
>> +static ktime_t
>> +drm_sched_rq_get_rr_deadline(struct drm_sched_rq *rq)
>> +{
>> +    lockdep_assert_held(&rq->lock);
>> +
>> +    rq->rr_deadline = ktime_add_ns(rq->rr_deadline, 1);
>> +
>> +    return rq->rr_deadline;
>> +}
>> +
>> +/**
>> + * drm_sched_rq_add_entity - add an entity
>> + *
>> + * @rq: scheduler run queue
>> + * @entity: scheduler entity
>> + * @ts: submission timestamp
>> + *
>> + * Adds a scheduler entity to the run queue.
>> + *
>> + * Returns a DRM scheduler pre-selected to handle this entity.
>> + */
>> +struct drm_gpu_scheduler *
>> +drm_sched_rq_add_entity(struct drm_sched_rq *rq,
>> +            struct drm_sched_entity *entity,
>> +            ktime_t ts)
>> +{
>> +    struct drm_gpu_scheduler *sched;
>> +
>> +    if (entity->stopped) {
>> +        DRM_ERROR("Trying to push to a killed entity\n");
>> +        return NULL;
>> +    }
>> +
>> +    spin_lock(&entity->lock);
>> +    spin_lock(&rq->lock);
>> +
>> +    sched = rq->sched;
>> +
>> +    if (!list_empty(&entity->list)) {
>> +        atomic_inc(sched->score);
>> +        list_add_tail(&entity->list, &rq->entities);
>> +    }
>> +
>> +    if (drm_sched_policy == DRM_SCHED_POLICY_RR)
>> +        ts = drm_sched_rq_get_rr_deadline(rq);
>> +
>> +    if (!RB_EMPTY_NODE(&entity->rb_tree_node))
>> +        __drm_sched_rq_remove_fifo_locked(entity, rq);
>> +    __drm_sched_rq_add_fifo_locked(entity, rq, ts);
>> +
>> +    spin_unlock(&rq->lock);
>> +    spin_unlock(&entity->lock);
>> +
>> +    return sched;
>> +}
>> +
>> +/**
>> + * drm_sched_rq_remove_entity - remove an entity
>> + *
>> + * @rq: scheduler run queue
>> + * @entity: scheduler entity
>> + *
>> + * Removes a scheduler entity from the run queue.
>> + */
>> +void drm_sched_rq_remove_entity(struct drm_sched_rq *rq,
>> +                struct drm_sched_entity *entity)
>> +{
>> +    lockdep_assert_held(&entity->lock);
>> +
>> +    if (list_empty(&entity->list))
>> +        return;
>> +
>> +    spin_lock(&rq->lock);
>> +
>> +    atomic_dec(rq->sched->score);
>> +    list_del_init(&entity->list);
>> +
>> +    if (!RB_EMPTY_NODE(&entity->rb_tree_node))
>> +        __drm_sched_rq_remove_fifo_locked(entity, rq);
>> +
>> +    spin_unlock(&rq->lock);
>> +}
>> +
>> +void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
>> +                 struct drm_sched_entity *entity)
>> +{
>> +    struct drm_sched_job *next_job;
>> +
>> +    spin_lock(&entity->lock);
>> +    spin_lock(&rq->lock);
>> +    __drm_sched_rq_remove_fifo_locked(entity, rq);
>> +    next_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
>> +    if (next_job) {
>> +        ktime_t ts;
>> +
>> +        if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
>> +            ts = next_job->submit_ts;
>> +        else
>> +            ts = drm_sched_rq_get_rr_deadline(rq);
>> +
>> +        __drm_sched_rq_add_fifo_locked(entity, rq, ts);
>> +    }
>> +    spin_unlock(&rq->lock);
>> +    spin_unlock(&entity->lock);
>> +}
>> +
>> +/**
>> + * drm_sched_rq_select_entity - Select an entity which provides a job 
>> to run
>> + *
>> + * @sched: the gpu scheduler
>> + * @rq: scheduler run queue to check.
>> + *
>> + * Find oldest waiting ready entity.
>> + *
>> + * Return an entity if one is found; return an error-pointer (!NULL) 
>> if an
>> + * entity was ready, but the scheduler had insufficient credits to 
>> accommodate
>> + * its job; return NULL, if no ready entity was found.
>> + */
>> +struct drm_sched_entity *
>> +drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
>> +               struct drm_sched_rq *rq)
>> +{
>> +    struct drm_sched_entity *entity = NULL;
>> +    struct rb_node *rb;
>> +
>> +    spin_lock(&rq->lock);
>> +    for (rb = rb_first_cached(&rq->rb_tree_root); rb; rb = 
>> rb_next(rb)) {
>> +        entity = rb_entry(rb, struct drm_sched_entity, rb_tree_node);
>> +        if (drm_sched_entity_is_ready(entity))
>> +            break;
>> +        else
>> +            entity = NULL;
>> +    }
>> +    spin_unlock(&rq->lock);
>> +
>> +    if (!entity)
>> +        return NULL;
>> +
>> +    /*
>> +     * If scheduler cannot take more jobs signal the caller to not 
>> consider
>> +     * lower priority queues.
>> +     */
>> +    if (!drm_sched_can_queue(sched, entity))
>> +        return ERR_PTR(-ENOSPC);
>> +
>> +    reinit_completion(&entity->entity_idle);
>> +
>> +    return entity;
>> +}
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index daf4665f37fa..ccb39e7bf384 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -386,6 +386,9 @@ struct drm_sched_job {
>>       ktime_t                         submit_ts;
>>   };
>> +#define to_drm_sched_job(sched_job)        \
>> +        container_of((sched_job), struct drm_sched_job, queue_node)
>> +
>>   static inline bool drm_sched_invalidate_job(struct drm_sched_job 
>> *s_job,
>>                           int threshold)
>>   {
>> @@ -547,6 +550,9 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>>              atomic_t *score, const char *name, struct device *dev);
>>   void drm_sched_fini(struct drm_gpu_scheduler *sched);
>> +bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
>> +             struct drm_sched_entity *entity);
>> +
>>   int drm_sched_job_init(struct drm_sched_job *job,
>>                  struct drm_sched_entity *entity,
>>                  u32 credits, void *owner);
>> @@ -586,6 +592,9 @@ bool drm_sched_dependency_optimized(struct 
>> dma_fence* fence,
>>                       struct drm_sched_entity *entity);
>>   void drm_sched_fault(struct drm_gpu_scheduler *sched);
>> +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_rq *rq,
>>               struct drm_sched_entity *entity,
>> @@ -595,6 +604,9 @@ void drm_sched_rq_remove_entity(struct 
>> drm_sched_rq *rq,
>>   void drm_sched_rq_pop_entity(struct drm_sched_rq *rq,
>>                    struct drm_sched_entity *entity);
>> +struct drm_sched_entity *
>> +drm_sched_rq_select_entity(struct drm_gpu_scheduler *sched,
>> +               struct drm_sched_rq *rq);
>>   int drm_sched_entity_init(struct drm_sched_entity *entity,
>>                 enum drm_sched_priority priority,
> 

Re: [RFC 16/18] drm/sched: Connect with dma-fence deadlines

[Tvrtko Ursulin]

On 09/01/2025 13:07, Christian König wrote:
> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>> Now that the scheduling policy is deadline based it feels completely
>> natural to allow propagating externaly set deadlines to the scheduler.
>>
>> Scheduler deadlines are not a guarantee but as the dma-fence facility is
>> already in use by userspace lets wire it up.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> Cc: Rob Clark <robdclark@gmail.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_entity.c | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>> b/drivers/gpu/drm/scheduler/sched_entity.c
>> index 98c78d1373d8..db5d34310b18 100644
>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>> @@ -410,7 +410,16 @@ ktime_t
>>   drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
>>                     struct drm_sched_job *job)
>>   {
>> -    return __drm_sched_entity_get_job_deadline(entity, job->submit_ts);
>> +    struct drm_sched_fence *s_fence = job->s_fence;
>> +    struct dma_fence *fence = &s_fence->finished;
>> +    ktime_t deadline;
>> +
>> +    deadline = __drm_sched_entity_get_job_deadline(entity, 
>> job->submit_ts);
>> +    if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, 
>> &fence->flags) &&
>> +        ktime_before(s_fence->deadline, deadline))
>> +        deadline = s_fence->deadline;
>> +
>> +    return deadline;
>>   }
>>   /*
>> @@ -579,9 +588,12 @@ void drm_sched_entity_select_rq(struct 
>> drm_sched_entity *entity)
>>    */
>>   void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>>   {
>> +    struct drm_sched_fence *s_fence = sched_job->s_fence;
>>       struct drm_sched_entity *entity = sched_job->entity;
>> -    bool first;
>> +    struct dma_fence *fence = &s_fence->finished;
>> +    ktime_t fence_deadline;
>>       ktime_t submit_ts;
>> +    bool first;
>>       trace_drm_sched_job(sched_job, entity);
>>       atomic_inc(entity->rq->sched->score);
>> @@ -593,6 +605,11 @@ void drm_sched_entity_push_job(struct 
>> drm_sched_job *sched_job)
>>        * Make sure to set the submit_ts first, to avoid a race.
>>        */
>>       sched_job->submit_ts = submit_ts = ktime_get();
>> +    if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, &fence->flags))
>> +        fence_deadline = s_fence->deadline;
>> +    else
>> +        fence_deadline = KTIME_MAX;
>> +
> 
> That makes no sense. When the job is pushed the fence is not made public 
> yet.
> 
> So no deadline can be set on the fence.

You are correct, the push side of things was a mistake a laziness that I 
did not remove it from the RFC.

>>       first = spsc_queue_push(&entity->job_queue, 
>> &sched_job->queue_node);
>>       /* first job wakes up scheduler */
>> @@ -601,6 +618,9 @@ void drm_sched_entity_push_job(struct 
>> drm_sched_job *sched_job)
>>           submit_ts = __drm_sched_entity_get_job_deadline(entity,
>>                                   submit_ts);
>> +        if (ktime_before(fence_deadline, submit_ts))
>> +            submit_ts = fence_deadline;
>> +
> 
> Yeah, that won't work at all as far as I can see.

It works from the pop side though.

When job N is scheduled, deadline is taken from N+1 and tree 
re-balanced. At the point of N scheduling N+1 can definitely have a real 
deadline set.

What does not work is for queue depth of one. No way at the moment to 
"bump" the entity in the tree while N is waiting for submission because 
we cannot dereference the entity from the job. (I had that in v1 of the 
series and realized it was unsafe.)

I (very) briefly though about reference counting entities but quickly 
had a feeling it would be annoying. So for now this patch only offers a 
partial solution.

Regards,

Tvrtko

>>           sched = drm_sched_rq_add_entity(entity->rq, entity, submit_ts);
>>           if (sched)
>>               drm_sched_wakeup(sched);
> 

Re: [RFC 16/18] drm/sched: Connect with dma-fence deadlines

[Christian König]

Am 09.01.25 um 14:41 schrieb Tvrtko Ursulin:
>
> On 09/01/2025 13:07, Christian König wrote:
>> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>>> Now that the scheduling policy is deadline based it feels completely
>>> natural to allow propagating externaly set deadlines to the scheduler.
>>>
>>> Scheduler deadlines are not a guarantee but as the dma-fence 
>>> facility is
>>> already in use by userspace lets wire it up.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Danilo Krummrich <dakr@redhat.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> Cc: Philipp Stanner <pstanner@redhat.com>
>>> Cc: Rob Clark <robdclark@gmail.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_entity.c | 24 
>>> ++++++++++++++++++++++--
>>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>> index 98c78d1373d8..db5d34310b18 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>> @@ -410,7 +410,16 @@ ktime_t
>>>   drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
>>>                     struct drm_sched_job *job)
>>>   {
>>> -    return __drm_sched_entity_get_job_deadline(entity, 
>>> job->submit_ts);
>>> +    struct drm_sched_fence *s_fence = job->s_fence;
>>> +    struct dma_fence *fence = &s_fence->finished;
>>> +    ktime_t deadline;
>>> +
>>> +    deadline = __drm_sched_entity_get_job_deadline(entity, 
>>> job->submit_ts);
>>> +    if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, 
>>> &fence->flags) &&
>>> +        ktime_before(s_fence->deadline, deadline))
>>> +        deadline = s_fence->deadline;
>>> +
>>> +    return deadline;
>>>   }
>>>   /*
>>> @@ -579,9 +588,12 @@ void drm_sched_entity_select_rq(struct 
>>> drm_sched_entity *entity)
>>>    */
>>>   void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>>>   {
>>> +    struct drm_sched_fence *s_fence = sched_job->s_fence;
>>>       struct drm_sched_entity *entity = sched_job->entity;
>>> -    bool first;
>>> +    struct dma_fence *fence = &s_fence->finished;
>>> +    ktime_t fence_deadline;
>>>       ktime_t submit_ts;
>>> +    bool first;
>>>       trace_drm_sched_job(sched_job, entity);
>>>       atomic_inc(entity->rq->sched->score);
>>> @@ -593,6 +605,11 @@ void drm_sched_entity_push_job(struct 
>>> drm_sched_job *sched_job)
>>>        * Make sure to set the submit_ts first, to avoid a race.
>>>        */
>>>       sched_job->submit_ts = submit_ts = ktime_get();
>>> +    if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, 
>>> &fence->flags))
>>> +        fence_deadline = s_fence->deadline;
>>> +    else
>>> +        fence_deadline = KTIME_MAX;
>>> +
>>
>> That makes no sense. When the job is pushed the fence is not made 
>> public yet.
>>
>> So no deadline can be set on the fence.
>
> You are correct, the push side of things was a mistake a laziness that 
> I did not remove it from the RFC.
>
>>>       first = spsc_queue_push(&entity->job_queue, 
>>> &sched_job->queue_node);
>>>       /* first job wakes up scheduler */
>>> @@ -601,6 +618,9 @@ void drm_sched_entity_push_job(struct 
>>> drm_sched_job *sched_job)
>>>           submit_ts = __drm_sched_entity_get_job_deadline(entity,
>>>                                   submit_ts);
>>> +        if (ktime_before(fence_deadline, submit_ts))
>>> +            submit_ts = fence_deadline;
>>> +
>>
>> Yeah, that won't work at all as far as I can see.
>
> It works from the pop side though.

Yeah, but only partially.

>
> When job N is scheduled, deadline is taken from N+1 and tree 
> re-balanced. At the point of N scheduling N+1 can definitely have a 
> real deadline set.

The fundamental design problem with the fence deadline approach is that 
it sets the deadline only on the last submission instead of the first one.

E.g. unigine heaven for example uses 3 submissions on a typical system.

We would somehow need to propagate a deadline to previous submissions 
for this to work halve way correctly.

> What does not work is for queue depth of one. No way at the moment to 
> "bump" the entity in the tree while N is waiting for submission 
> because we cannot dereference the entity from the job. (I had that in 
> v1 of the series and realized it was unsafe.)
>
> I (very) briefly though about reference counting entities but quickly 
> had a feeling it would be annoying. So for now this patch only offers 
> a partial solution.

Nah, please not.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>>           sched = drm_sched_rq_add_entity(entity->rq, entity, 
>>> submit_ts);
>>>           if (sched)
>>>               drm_sched_wakeup(sched);
>>

Re: [RFC 10/18] drm/sched: Implement RR via FIFO

[Christian König]

Am 09.01.25 um 14:27 schrieb Tvrtko Ursulin:
> [SNIP]
>>> @@ -259,7 +258,7 @@ struct drm_sched_rq {
>>>       spinlock_t            lock;
>>>       /* Following members are protected by the @lock: */
>>> -    struct drm_sched_entity        *current_entity;
>>> +    ktime_t                rr_deadline;
>>>       struct list_head        entities;
>>
>> If I'm not completely mistaken you can now also nuke this entities 
>> list if you haven't already removed all users.
>
> I had a version which did that too. But then I dropped it in favour of 
> only keeping entities with queued jobs in the tree. (Before both the 
> list and the tree contained entities which submitted at least one job 
> in the past.)
>
> I kind of like keeping the tree trimmed (would it lower the rb tree 
> re-balancing costs?) in which case the full list is needed for that 
> karma processing business.

Well, is anybody still using this karma stuff (maybe amdgpu, but we 
could drop that)? That as well turned out to be a quite broken approach.

Basically the idea behind karma was that on a crash you re-submit the 
same job over and over again until it either doesn't crash any more or 
your karma became to bad.

And when you now think of what Einstein once said about insanity then 
yeah that was also my first thought when I learned about that :)

Cheers,
Christian.

>
> Regards,
>
> Tvrtko
>
>>
>> Regards,
>> Christian.
>>
>>>       struct rb_root_cached rb_tree_root;
>>>   };
>>

Re: [RFC 03/18] drm/sched: Remove one local variable

[Christian König]

Am 09.01.25 um 14:20 schrieb Tvrtko Ursulin:
>
> On 09/01/2025 12:49, Christian König wrote:
>> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>>> It is not helping readability nor it is required to keep the line 
>>> length
>>> in check.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Danilo Krummrich <dakr@redhat.com>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> Cc: Philipp Stanner <pstanner@redhat.com>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_main.c | 5 +----
>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index 1734c17aeea5..01e0d6e686d1 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -1175,7 +1175,6 @@ static void drm_sched_run_job_work(struct 
>>> work_struct *w)
>>>           container_of(w, struct drm_gpu_scheduler, work_run_job);
>>>       struct drm_sched_entity *entity;
>>>       struct dma_fence *fence;
>>> -    struct drm_sched_fence *s_fence;
>>>       struct drm_sched_job *sched_job;
>>>       int r;
>>> @@ -1194,15 +1193,13 @@ static void drm_sched_run_job_work(struct 
>>> work_struct *w)
>>>           return;
>>>       }
>>> -    s_fence = sched_job->s_fence;
>>> -
>>>       atomic_add(sched_job->credits, &sched->credit_count);
>>>       drm_sched_job_begin(sched_job);
>>>       trace_drm_run_job(sched_job, entity);
>>>       fence = sched->ops->run_job(sched_job);
>>>       complete_all(&entity->entity_idle);
>>> -    drm_sched_fence_scheduled(s_fence, fence);
>>> +    drm_sched_fence_scheduled(sched_job->s_fence, fence);
>>
>> Originally that was not for readability but for correctness.
>>
>> As soon as complete_all(&entity->entity_idle); was called the 
>> sched_job could have been released.
>
> And without a comment ouch.

That changed long long time ago and IIRC we did had a comment for that.

>
>> But we changed that so that the sched_job is released from a separate 
>> worker instead, so that is most likely not necessary any more.
>
> Very subtle. Especially given some drivers use unordered queue.

Hui? unordered queue? How should that work?

Job submission ordering is a mandatory requirement of the dma_fence.

>
> And for them sched_job is dereferenced a few more times in the block 
> below so not sure how it is safe.
>
> Move complete_all() to the end of it all?

Most likely good idea, yes.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>>       if (!IS_ERR_OR_NULL(fence)) {
>>>           /* Drop for original kref_init of the fence */
>>

Re: [RFC 16/18] drm/sched: Connect with dma-fence deadlines

[Tvrtko Ursulin]

On 09/01/2025 13:51, Christian König wrote:
> Am 09.01.25 um 14:41 schrieb Tvrtko Ursulin:
>>
>> On 09/01/2025 13:07, Christian König wrote:
>>> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>>>> Now that the scheduling policy is deadline based it feels completely
>>>> natural to allow propagating externaly set deadlines to the scheduler.
>>>>
>>>> Scheduler deadlines are not a guarantee but as the dma-fence 
>>>> facility is
>>>> already in use by userspace lets wire it up.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Danilo Krummrich <dakr@redhat.com>
>>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>>> Cc: Philipp Stanner <pstanner@redhat.com>
>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/sched_entity.c | 24 
>>>> ++++++++++++++++++++++--
>>>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c 
>>>> b/drivers/gpu/drm/scheduler/sched_entity.c
>>>> index 98c78d1373d8..db5d34310b18 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_entity.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
>>>> @@ -410,7 +410,16 @@ ktime_t
>>>>   drm_sched_entity_get_job_deadline(struct drm_sched_entity *entity,
>>>>                     struct drm_sched_job *job)
>>>>   {
>>>> -    return __drm_sched_entity_get_job_deadline(entity, 
>>>> job->submit_ts);
>>>> +    struct drm_sched_fence *s_fence = job->s_fence;
>>>> +    struct dma_fence *fence = &s_fence->finished;
>>>> +    ktime_t deadline;
>>>> +
>>>> +    deadline = __drm_sched_entity_get_job_deadline(entity, 
>>>> job->submit_ts);
>>>> +    if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, 
>>>> &fence->flags) &&
>>>> +        ktime_before(s_fence->deadline, deadline))
>>>> +        deadline = s_fence->deadline;
>>>> +
>>>> +    return deadline;
>>>>   }
>>>>   /*
>>>> @@ -579,9 +588,12 @@ void drm_sched_entity_select_rq(struct 
>>>> drm_sched_entity *entity)
>>>>    */
>>>>   void drm_sched_entity_push_job(struct drm_sched_job *sched_job)
>>>>   {
>>>> +    struct drm_sched_fence *s_fence = sched_job->s_fence;
>>>>       struct drm_sched_entity *entity = sched_job->entity;
>>>> -    bool first;
>>>> +    struct dma_fence *fence = &s_fence->finished;
>>>> +    ktime_t fence_deadline;
>>>>       ktime_t submit_ts;
>>>> +    bool first;
>>>>       trace_drm_sched_job(sched_job, entity);
>>>>       atomic_inc(entity->rq->sched->score);
>>>> @@ -593,6 +605,11 @@ void drm_sched_entity_push_job(struct 
>>>> drm_sched_job *sched_job)
>>>>        * Make sure to set the submit_ts first, to avoid a race.
>>>>        */
>>>>       sched_job->submit_ts = submit_ts = ktime_get();
>>>> +    if (test_bit(DRM_SCHED_FENCE_FLAG_HAS_DEADLINE_BIT, 
>>>> &fence->flags))
>>>> +        fence_deadline = s_fence->deadline;
>>>> +    else
>>>> +        fence_deadline = KTIME_MAX;
>>>> +
>>>
>>> That makes no sense. When the job is pushed the fence is not made 
>>> public yet.
>>>
>>> So no deadline can be set on the fence.
>>
>> You are correct, the push side of things was a mistake a laziness that 
>> I did not remove it from the RFC.
>>
>>>>       first = spsc_queue_push(&entity->job_queue, 
>>>> &sched_job->queue_node);
>>>>       /* first job wakes up scheduler */
>>>> @@ -601,6 +618,9 @@ void drm_sched_entity_push_job(struct 
>>>> drm_sched_job *sched_job)
>>>>           submit_ts = __drm_sched_entity_get_job_deadline(entity,
>>>>                                   submit_ts);
>>>> +        if (ktime_before(fence_deadline, submit_ts))
>>>> +            submit_ts = fence_deadline;
>>>> +
>>>
>>> Yeah, that won't work at all as far as I can see.
>>
>> It works from the pop side though.
> 
> Yeah, but only partially.
> 
>>
>> When job N is scheduled, deadline is taken from N+1 and tree 
>> re-balanced. At the point of N scheduling N+1 can definitely have a 
>> real deadline set.
> 
> The fundamental design problem with the fence deadline approach is that 
> it sets the deadline only on the last submission instead of the first one.
> 
> E.g. unigine heaven for example uses 3 submissions on a typical system.
> 
> We would somehow need to propagate a deadline to previous submissions 
> for this to work halve way correctly.

I played with this in one of my branches but was lacking a good test 
case. In any case clean solution(s) would not be easy with the current 
scheduler design.

Regards,

Tvrtko

>> What does not work is for queue depth of one. No way at the moment to 
>> "bump" the entity in the tree while N is waiting for submission 
>> because we cannot dereference the entity from the job. (I had that in 
>> v1 of the series and realized it was unsafe.)
>>
>> I (very) briefly though about reference counting entities but quickly 
>> had a feeling it would be annoying. So for now this patch only offers 
>> a partial solution.
> 
> Nah, please not.
> 
> Regards,
> Christian.
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>>           sched = drm_sched_rq_add_entity(entity->rq, entity, 
>>>> submit_ts);
>>>>           if (sched)
>>>>               drm_sched_wakeup(sched);
>>>
> 

Re: [RFC 03/18] drm/sched: Remove one local variable

[Tvrtko Ursulin]

On 09/01/2025 14:17, Christian König wrote:
> Am 09.01.25 um 14:20 schrieb Tvrtko Ursulin:
>>
>> On 09/01/2025 12:49, Christian König wrote:
>>> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>>>> It is not helping readability nor it is required to keep the line 
>>>> length
>>>> in check.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Danilo Krummrich <dakr@redhat.com>
>>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>>> Cc: Philipp Stanner <pstanner@redhat.com>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/sched_main.c | 5 +----
>>>>   1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c 
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index 1734c17aeea5..01e0d6e686d1 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -1175,7 +1175,6 @@ static void drm_sched_run_job_work(struct 
>>>> work_struct *w)
>>>>           container_of(w, struct drm_gpu_scheduler, work_run_job);
>>>>       struct drm_sched_entity *entity;
>>>>       struct dma_fence *fence;
>>>> -    struct drm_sched_fence *s_fence;
>>>>       struct drm_sched_job *sched_job;
>>>>       int r;
>>>> @@ -1194,15 +1193,13 @@ static void drm_sched_run_job_work(struct 
>>>> work_struct *w)
>>>>           return;
>>>>       }
>>>> -    s_fence = sched_job->s_fence;
>>>> -
>>>>       atomic_add(sched_job->credits, &sched->credit_count);
>>>>       drm_sched_job_begin(sched_job);
>>>>       trace_drm_run_job(sched_job, entity);
>>>>       fence = sched->ops->run_job(sched_job);
>>>>       complete_all(&entity->entity_idle);
>>>> -    drm_sched_fence_scheduled(s_fence, fence);
>>>> +    drm_sched_fence_scheduled(sched_job->s_fence, fence);
>>>
>>> Originally that was not for readability but for correctness.
>>>
>>> As soon as complete_all(&entity->entity_idle); was called the 
>>> sched_job could have been released.
>>
>> And without a comment ouch.
> 
> That changed long long time ago and IIRC we did had a comment for that.
> 
>>
>>> But we changed that so that the sched_job is released from a separate 
>>> worker instead, so that is most likely not necessary any more.
>>
>> Very subtle. Especially given some drivers use unordered queue.
> 
> Hui? unordered queue? How should that work?
> 
> Job submission ordering is a mandatory requirement of the dma_fence.

I think it is fine for submission since it is a single work item which 
still runs serialized to itself. But free work can the overtake it on 
drivers who pass in unordered queue.

I think Matt promised to document the ordered vs unordered 
criteria/requirements some time ago and maybe forgot*.

In any case seems like moving the complete_all() to be last is the 
safest option. I'll rework this patch to that effect for v3.

Regards,

Tvrtko

*)
https://lore.kernel.org/all/ZjlmZHBMfK9fld9c@DUT025-TGLU.fm.intel.com/T/

>> And for them sched_job is dereferenced a few more times in the block 
>> below so not sure how it is safe.
>>
>> Move complete_all() to the end of it all?
> 
> Most likely good idea, yes.
> 
> Regards,
> Christian.
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>>>       if (!IS_ERR_OR_NULL(fence)) {
>>>>           /* Drop for original kref_init of the fence */
>>>
> 

Re: [RFC 01/18] drm/amdgpu: Use DRM scheduler API in amdgpu_xcp_release_sched

[Tvrtko Ursulin]

On 09/01/2025 12:30, Christian König wrote:
> Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
>> Lets use the existing helper instead of peeking into the structure
>> directly.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
> 
> Grr, I can't count of how many of those I already fixed :(
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>

Thanks!

+ Alex

I forgot to cc amd-gfx for this one. Can you take it from here or I can 
send it standalone?

Regards,

Tvrtko

> 
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> index e209b5e101df..23b6f7a4aa4a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
>> @@ -427,7 +427,7 @@ void amdgpu_xcp_release_sched(struct amdgpu_device 
>> *adev,
>>           return;
>>       sched = entity->entity.rq->sched;
>> -    if (sched->ready) {
>> +    if (drm_sched_wqueue_ready(sched)) {
>>           ring = to_amdgpu_ring(entity->entity.rq->sched);
>>           atomic_dec(&adev->xcp_mgr->xcp[ring->xcp_id].ref_cnt);
>>       }
> 

Re: [RFC 10/18] drm/sched: Implement RR via FIFO

[Tvrtko Ursulin]

On 09/01/2025 14:12, Christian König wrote:
> Am 09.01.25 um 14:27 schrieb Tvrtko Ursulin:
>> [SNIP]
>>>> @@ -259,7 +258,7 @@ struct drm_sched_rq {
>>>>       spinlock_t            lock;
>>>>       /* Following members are protected by the @lock: */
>>>> -    struct drm_sched_entity        *current_entity;
>>>> +    ktime_t                rr_deadline;
>>>>       struct list_head        entities;
>>>
>>> If I'm not completely mistaken you can now also nuke this entities 
>>> list if you haven't already removed all users.
>>
>> I had a version which did that too. But then I dropped it in favour of 
>> only keeping entities with queued jobs in the tree. (Before both the 
>> list and the tree contained entities which submitted at least one job 
>> in the past.)
>>
>> I kind of like keeping the tree trimmed (would it lower the rb tree 
>> re-balancing costs?) in which case the full list is needed for that 
>> karma processing business.
> 
> Well, is anybody still using this karma stuff (maybe amdgpu, but we 
> could drop that)? That as well turned out to be a quite broken approach.

Git grep says etnaviv, lima, panfrost, v3d.

> Basically the idea behind karma was that on a crash you re-submit the 
> same job over and over again until it either doesn't crash any more or 
> your karma became to bad.

Well I don't know, TBD I guess. In any case probably best to leave that 
work for a different series to keep this one reasonably focused.

I also still like the idea of keeping the tree pruned of idle entities 
so for me rq->entities list is not a big deal.

> And when you now think of what Einstein once said about insanity then 
> yeah that was also my first thought when I learned about that :)

Right, resubmitting hanging jobs sounds highly questionable indeed.

But it is also the entity->guilty business.

Now if entities were reference counted... ;) The list walk 
hack/workaround in drm_sched_increase_karma could also go away.

Regards,

Tvrtko

>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>>       struct rb_root_cached rb_tree_root;
>>>>   };
>>>
> 

Re: [RFC 01/18] drm/amdgpu: Use DRM scheduler API in amdgpu_xcp_release_sched

[Alex Deucher]

On Fri, Jan 10, 2025 at 5:52 AM Tvrtko Ursulin
<tvrtko.ursulin@igalia.com> wrote:
>
>
> On 09/01/2025 12:30, Christian König wrote:
> > Am 08.01.25 um 19:35 schrieb Tvrtko Ursulin:
> >> Lets use the existing helper instead of peeking into the structure
> >> directly.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> >> Cc: Christian König <christian.koenig@amd.com>
> >> Cc: Danilo Krummrich <dakr@redhat.com>
> >> Cc: Matthew Brost <matthew.brost@intel.com>
> >> Cc: Philipp Stanner <pstanner@redhat.com>
> >
> > Grr, I can't count of how many of those I already fixed :(
> >
> > Reviewed-by: Christian König <christian.koenig@amd.com>
>
> Thanks!
>
> + Alex
>
> I forgot to cc amd-gfx for this one. Can you take it from here or I can
> send it standalone?

Will pick it up today.  Thanks,

Alex

>
> Regards,
>
> Tvrtko
>
> >
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> >> index e209b5e101df..23b6f7a4aa4a 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_xcp.c
> >> @@ -427,7 +427,7 @@ void amdgpu_xcp_release_sched(struct amdgpu_device
> >> *adev,
> >>           return;
> >>       sched = entity->entity.rq->sched;
> >> -    if (sched->ready) {
> >> +    if (drm_sched_wqueue_ready(sched)) {
> >>           ring = to_amdgpu_ring(entity->entity.rq->sched);
> >>           atomic_dec(&adev->xcp_mgr->xcp[ring->xcp_id].ref_cnt);
> >>       }
> >

Re: [RFC 06/18] drm/sched: Add helper to check job dependencies

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 2546 bytes --]

On 08/01/2025 18:35, Tvrtko Ursulin wrote:
> Lets isolate scheduler internals from drivers such as pvr which currently
> walks the dependency array to look for fences.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>

Reviewed-by: Matt Coster <matt.coster@imgtec.com>

-- 
Matt Coster
E: matt.coster@imgtec.com

> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 23 +++++++++++++++++++++++
>  include/drm/gpu_scheduler.h            |  3 ++-
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 64cd79e1793a..f07b19c97d0f 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -968,6 +968,29 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>  }
>  EXPORT_SYMBOL(drm_sched_job_add_implicit_dependencies);
>  
> +/**
> + * drm_sched_job_has_dependency - check whether fence is the job's dependency
> + * @job: scheduler job to check
> + * @fence: fence to look for
> + *
> + * Returns:
> + * True if @fence is found within the job's dependencies, or otherwise false.
> + */
> +bool drm_sched_job_has_dependency(struct drm_sched_job *job,
> +				  struct dma_fence *fence)
> +{
> +	struct dma_fence *f;
> +	unsigned long index;
> +
> +	xa_for_each(&job->dependencies, index, f) {
> +		if (f == fence)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(drm_sched_job_has_dependency);
> +
>  /**
>   * drm_sched_job_cleanup - clean up scheduler job resources
>   * @job: scheduler job to clean up
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index e2e6af8849c6..a0ff08123f07 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -564,7 +564,8 @@ int drm_sched_job_add_resv_dependencies(struct drm_sched_job *job,
>  int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job,
>  					    struct drm_gem_object *obj,
>  					    bool write);
> -
> +bool drm_sched_job_has_dependency(struct drm_sched_job *job,
> +				  struct dma_fence *fence);
>  
>  void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>  				    struct drm_gpu_scheduler **sched_list,


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [RFC 07/18] drm/imagination: Use the drm_sched_job_has_dependency helper

[Matt Coster]

[-- Attachment #1.1: Type: text/plain, Size: 2007 bytes --]

On 08/01/2025 18:35, Tvrtko Ursulin wrote:
> Instead of manually peeking into the DRM scheduler implementation details
> lets use the previously added helper.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@redhat.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <pstanner@redhat.com>
> Cc: Frank Binns <frank.binns@imgtec.com>
> Cc: Matt Coster <matt.coster@imgtec.com>

Reviewed-by: Matt Coster <matt.coster@imgtec.com>

-- 
Matt Coster
E: matt.coster@imgtec.com

> ---
>  drivers/gpu/drm/imagination/pvr_job.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imagination/pvr_job.c b/drivers/gpu/drm/imagination/pvr_job.c
> index 618503a212a7..1cdb3cfd058d 100644
> --- a/drivers/gpu/drm/imagination/pvr_job.c
> +++ b/drivers/gpu/drm/imagination/pvr_job.c
> @@ -597,8 +597,6 @@ update_job_resvs_for_each(struct pvr_job_data *job_data, u32 job_count)
>  static bool can_combine_jobs(struct pvr_job *a, struct pvr_job *b)
>  {
>  	struct pvr_job *geom_job = a, *frag_job = b;
> -	struct dma_fence *fence;
> -	unsigned long index;
>  
>  	/* Geometry and fragment jobs can be combined if they are queued to the
>  	 * same context and targeting the same HWRT.
> @@ -609,13 +607,9 @@ static bool can_combine_jobs(struct pvr_job *a, struct pvr_job *b)
>  	    a->hwrt != b->hwrt)
>  		return false;
>  
> -	xa_for_each(&frag_job->base.dependencies, index, fence) {
> -		/* We combine when we see an explicit geom -> frag dep. */
> -		if (&geom_job->base.s_fence->scheduled == fence)
> -			return true;
> -	}
> -
> -	return false;
> +	/* We combine when we see an explicit geom -> frag dep. */
> +	return drm_sched_job_has_dependency(&frag_job->base,
> +					    &geom_job->base.s_fence->scheduled);
>  }
>  
>  static struct dma_fence *


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

Re: [RFC 07/18] drm/imagination: Use the drm_sched_job_has_dependency helper

[Tvrtko Ursulin]

Hi Matt,

On 10/01/2025 16:39, Matt Coster wrote:
> On 08/01/2025 18:35, Tvrtko Ursulin wrote:
>> Instead of manually peeking into the DRM scheduler implementation details
>> lets use the previously added helper.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@redhat.com>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <pstanner@redhat.com>
>> Cc: Frank Binns <frank.binns@imgtec.com>
>> Cc: Matt Coster <matt.coster@imgtec.com>
> 
> Reviewed-by: Matt Coster <matt.coster@imgtec.com>

Thanks - let me resend these two standalone.

Regards,

Tvrtko