[PATCH v2 0/4] Decouple amdgpu from the scheduler, a bit

[PATCH v2 0/4] Decouple amdgpu from the scheduler, a bit

[Tvrtko Ursulin]

General idea is to try and access scheduler data structures less from the
drivers so the series basically adds some helpers to move closer towards that
goal.

Three copies of the same to_drm_sched_job macro get removed and by the end of
the series, as a bonus, we can now re-order members of struct drm_sched_job and
eliminate a hole.

I did not here from folks on which direction we want to take this after v1 so
I went ahead and sketched up a different flavour in v2. The
drm_sched_cancel_all_jobs() helper can be removed if (or when) amdgpu can change
the approach of implementing its permanently wedged state. But until then I see
no harm to clean up that, and even more so the to_drm_sched_job and its hidden
dependency on queue_node being the first element.

v2:
 * Add header file for internal scheduler API.
 * Add helper for peeking too. (Danilo)
 * Add (temporary?) drm_sched_cancel_all_jobs() helper to replace amdgpu
   amdgpu_job_stop_all_jobs_on_sched().

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

Tvrtko Ursulin (4):
  drm/scheduler: Add drm_sched_cancel_all_jobs helper
  drm/amdgpu: Use drm_sched_cancel_all_jobs helper
  drm/sched: Add internal job peek/pop API
  drm/sched: Make the type of drm_sched_job->last_dependency consistent

 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 34 ---------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h    |  2 -
 drivers/gpu/drm/scheduler/sched_entity.c   | 11 +++--
 drivers/gpu/drm/scheduler/sched_internal.h | 43 ++++++++++++++++++
 drivers/gpu/drm/scheduler/sched_main.c     | 51 ++++++++++++++++++++--
 include/drm/gpu_scheduler.h                | 39 +++++++++--------
 7 files changed, 118 insertions(+), 65 deletions(-)
 create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h

-- 
2.48.0

[PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Tvrtko Ursulin]

The helper copies code from the existing amdgpu_job_stop_all_jobs_on_sched
with the purpose of reducing the amount of driver code which directly
touch scheduler internals.

If or when amdgpu manages to change the approach for handling the
permanently wedged state this helper can be removed.

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

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index a48be16ab84f..0363655db22d 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -703,6 +703,50 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, int errno)
 }
 EXPORT_SYMBOL(drm_sched_start);
 
+/**
+ * drm_sched_cancel_all_jobs - Cancel all queued and scheduled jobs
+ *
+ * @sched: scheduler instance
+ * @errno: error value to set on signaled fences
+ *
+ * Signal all queued and scheduled jobs and set them to error state.
+ *
+ * Scheduler must be stopped before calling this.
+ */
+void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int errno)
+{
+	struct drm_sched_entity *entity;
+	struct drm_sched_fence *s_fence;
+	struct drm_sched_job *job;
+	enum drm_sched_priority p;
+
+	drm_WARN_ON_ONCE(sched, !sched->pause_submit);
+
+	/* Signal all jobs not yet scheduled */
+	for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs; p++) {
+		struct drm_sched_rq *rq = sched->sched_rq[p];
+
+		spin_lock(&rq->lock);
+		list_for_each_entry(entity, &rq->entities, list) {
+			while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
+				s_fence = job->s_fence;
+				dma_fence_signal(&s_fence->scheduled);
+				dma_fence_set_error(&s_fence->finished, errno);
+				dma_fence_signal(&s_fence->finished);
+			}
+		}
+		spin_unlock(&rq->lock);
+	}
+
+	/* Signal all jobs already scheduled to HW */
+	list_for_each_entry(job, &sched->pending_list, list) {
+		s_fence = job->s_fence;
+		dma_fence_set_error(&s_fence->finished, errno);
+		dma_fence_signal(&s_fence->finished);
+	}
+}
+EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
+
 /**
  * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
  *
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index a0ff08123f07..298513f8c327 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct drm_gpu_scheduler *sched);
 void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
 void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
 void drm_sched_start(struct drm_gpu_scheduler *sched, int errno);
+void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int errno);
 void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
 void drm_sched_increase_karma(struct drm_sched_job *bad);
 void drm_sched_reset_karma(struct drm_sched_job *bad);
-- 
2.48.0

[PATCH 2/4] drm/amdgpu: Use drm_sched_cancel_all_jobs helper

[Tvrtko Ursulin]

Replace amdgpu_job_stop_all_jobs_on_sched() helper by scheduler common
drm_sched_cancel_all_jobs() and remove one duplicated instance of the
to_drm_sched_job() macro.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 34 ----------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.h    |  2 --
 3 files changed, 2 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index d100bb7a137c..509460f5fe7f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5980,7 +5980,8 @@ int amdgpu_device_gpu_recover(struct amdgpu_device *adev,
 			drm_sched_stop(&ring->sched, job ? &job->base : NULL);
 
 			if (need_emergency_restart)
-				amdgpu_job_stop_all_jobs_on_sched(&ring->sched);
+				drm_sched_cancel_all_jobs(&ring->sched,
+							  -EHWPOISON);
 		}
 		atomic_inc(&tmp_adev->gpu_reset_counter);
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 100f04475943..9e32c504b481 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -411,40 +411,6 @@ static struct dma_fence *amdgpu_job_run(struct drm_sched_job *sched_job)
 	return fence;
 }
 
-#define to_drm_sched_job(sched_job)		\
-		container_of((sched_job), struct drm_sched_job, queue_node)
-
-void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
-{
-	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;
-
-				dma_fence_signal(&s_fence->scheduled);
-				dma_fence_set_error(&s_fence->finished, -EHWPOISON);
-				dma_fence_signal(&s_fence->finished);
-			}
-		}
-		spin_unlock(&rq->lock);
-	}
-
-	/* Signal all jobs already scheduled to HW */
-	list_for_each_entry(s_job, &sched->pending_list, list) {
-		struct drm_sched_fence *s_fence = s_job->s_fence;
-
-		dma_fence_set_error(&s_fence->finished, -EHWPOISON);
-		dma_fence_signal(&s_fence->finished);
-	}
-}
-
 const struct drm_sched_backend_ops amdgpu_sched_ops = {
 	.prepare_job = amdgpu_job_prepare_job,
 	.run_job = amdgpu_job_run,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
index ce6b9ba967ff..5a25c281d98b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.h
@@ -105,6 +105,4 @@ struct dma_fence *amdgpu_job_submit(struct amdgpu_job *job);
 int amdgpu_job_submit_direct(struct amdgpu_job *job, struct amdgpu_ring *ring,
 			     struct dma_fence **fence);
 
-void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched);
-
 #endif
-- 
2.48.0

[PATCH 3/4] drm/sched: Add internal job peek/pop API

[Tvrtko Ursulin]

Idea is to add helpers for peeking and poppling jobs from entities with
the goal of decoupling the hidden assumption in the code that queue_node
is the first element in struct drm_sched_job.

That assumption usually comes in the form of:

  while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue))))

Which breaks if the queue_node is re-positioned due to_drm_sched_job
being implemented with a container_of.

This also allows us to remove duplicate definition of to_drm_sched_job
which the scheduler apparently tried to keep internal, but failed since
in one of the previous patches we also removed a copy from amdgpu.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
 drivers/gpu/drm/scheduler/sched_entity.c   | 11 +++---
 drivers/gpu/drm/scheduler/sched_internal.h | 43 ++++++++++++++++++++++
 drivers/gpu/drm/scheduler/sched_main.c     |  9 ++---
 3 files changed, 52 insertions(+), 11 deletions(-)
 create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c
index 69bcf0e99d57..737feff147a5 100644
--- a/drivers/gpu/drm/scheduler/sched_entity.c
+++ b/drivers/gpu/drm/scheduler/sched_entity.c
@@ -28,11 +28,10 @@
 #include <drm/drm_print.h>
 #include <drm/gpu_scheduler.h>
 
+#include "sched_internal.h"
+
 #include "gpu_scheduler_trace.h"
 
-#define to_drm_sched_job(sched_job)		\
-		container_of((sched_job), struct drm_sched_job, queue_node)
-
 /**
  * drm_sched_entity_init - Init a context entity used by scheduler when
  * submit to HW ring.
@@ -255,7 +254,7 @@ static void drm_sched_entity_kill(struct drm_sched_entity *entity)
 	/* The entity is guaranteed to not be used by the scheduler */
 	prev = rcu_dereference_check(entity->last_scheduled, true);
 	dma_fence_get(prev);
-	while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
+	while ((job = __drm_sched_entity_queue_pop(entity))) {
 		struct drm_sched_fence *s_fence = job->s_fence;
 
 		dma_fence_get(&s_fence->finished);
@@ -477,7 +476,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
 {
 	struct drm_sched_job *sched_job;
 
-	sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
+	sched_job = __drm_sched_entity_queue_peek(entity);
 	if (!sched_job)
 		return NULL;
 
@@ -513,7 +512,7 @@ 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;
 
-		next = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
+		next = __drm_sched_entity_queue_peek(entity);
 		if (next) {
 			struct drm_sched_rq *rq;
 
diff --git a/drivers/gpu/drm/scheduler/sched_internal.h b/drivers/gpu/drm/scheduler/sched_internal.h
new file mode 100644
index 000000000000..565c83e32371
--- /dev/null
+++ b/drivers/gpu/drm/scheduler/sched_internal.h
@@ -0,0 +1,43 @@
+
+
+/**
+ * __drm_sched_entity_queue_pop - Low level helper for popping queued jobs
+ *
+ * @entity: scheduler entity
+ *
+ * Low level helper for popping queued jobs.
+ *
+ * Returns the job dequeued or NULL.
+ */
+static inline struct drm_sched_job *
+__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
+{
+	struct spsc_node *node;
+
+	node = spsc_queue_pop(&entity->job_queue);
+	if (!node)
+		return NULL;
+
+	return container_of(node, struct drm_sched_job, queue_node);
+}
+
+/**
+ * __drm_sched_entity_queue_peek - Low level helper for peeking at the job queue
+ *
+ * @entity: scheduler entity
+ *
+ * Low level helper for peeking at the job queue
+ *
+ * Returns the job at the head of the queue or NULL.
+ */
+static inline struct drm_sched_job *
+__drm_sched_entity_queue_peek(struct drm_sched_entity *entity)
+{
+	struct spsc_node *node;
+
+	node = spsc_queue_peek(&entity->job_queue);
+	if (!node)
+		return NULL;
+
+	return container_of(node, struct drm_sched_job, queue_node);
+}
diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 0363655db22d..41d6f839748e 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -78,6 +78,8 @@
 #include <drm/gpu_scheduler.h>
 #include <drm/spsc_queue.h>
 
+#include "sched_internal.h"
+
 #define CREATE_TRACE_POINTS
 #include "gpu_scheduler_trace.h"
 
@@ -87,9 +89,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;
 
 /**
@@ -123,7 +122,7 @@ static bool drm_sched_can_queue(struct drm_gpu_scheduler *sched,
 {
 	struct drm_sched_job *s_job;
 
-	s_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));
+	s_job = __drm_sched_entity_queue_peek(entity);
 	if (!s_job)
 		return false;
 
@@ -728,7 +727,7 @@ void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int errno)
 
 		spin_lock(&rq->lock);
 		list_for_each_entry(entity, &rq->entities, list) {
-			while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
+			while ((job = __drm_sched_entity_queue_pop(entity))) {
 				s_fence = job->s_fence;
 				dma_fence_signal(&s_fence->scheduled);
 				dma_fence_set_error(&s_fence->finished, errno);
-- 
2.48.0

[PATCH 4/4] drm/sched: Make the type of drm_sched_job->last_dependency consistent

[Tvrtko Ursulin]

Dependency tracking via xarray uses xa_limit_32b so there is not need for
the struct member to be unsigned long.

At the same time re-order some struct members and take u32 credits outside
of the pointer sandwich and avoid a hole.

Pahole report before:
        /* size: 160, cachelines: 3, members: 14 */
        /* sum members: 156, holes: 1, sum holes: 4 */
        /* last cacheline: 32 bytes */

And after:
        /* size: 152, cachelines: 3, members: 14 */
        /* last cacheline: 24 bytes */

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: Philipp Stanner <phasta@kernel.org>
---
 include/drm/gpu_scheduler.h | 38 +++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 298513f8c327..20a6c1b3bab1 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -338,8 +338,14 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f);
  * to schedule the job.
  */
 struct drm_sched_job {
-	struct spsc_node		queue_node;
-	struct list_head		list;
+	u64				id;
+
+	/**
+	 * @submit_ts:
+	 *
+	 * When the job was pushed into the entity queue.
+	 */
+	ktime_t                         submit_ts;
 
 	/**
 	 * @sched:
@@ -349,24 +355,30 @@ struct drm_sched_job {
 	 * has finished.
 	 */
 	struct drm_gpu_scheduler	*sched;
+
 	struct drm_sched_fence		*s_fence;
+	struct drm_sched_entity         *entity;
 
+	enum drm_sched_priority		s_priority;
 	u32				credits;
+	/** @last_dependency: tracks @dependencies as they signal */
+	unsigned int			last_dependency;
+	atomic_t			karma;
+
+	struct spsc_node		queue_node;
+	struct list_head		list;
 
 	/*
 	 * work is used only after finish_cb has been used and will not be
 	 * accessed anymore.
 	 */
 	union {
-		struct dma_fence_cb		finish_cb;
-		struct work_struct		work;
+		struct dma_fence_cb	finish_cb;
+		struct work_struct	work;
 	};
 
-	uint64_t			id;
-	atomic_t			karma;
-	enum drm_sched_priority		s_priority;
-	struct drm_sched_entity         *entity;
 	struct dma_fence_cb		cb;
+
 	/**
 	 * @dependencies:
 	 *
@@ -375,16 +387,6 @@ struct drm_sched_job {
 	 * drm_sched_job_add_implicit_dependencies().
 	 */
 	struct xarray			dependencies;
-
-	/** @last_dependency: tracks @dependencies as they signal */
-	unsigned long			last_dependency;
-
-	/**
-	 * @submit_ts:
-	 *
-	 * When the job was pushed into the entity queue.
-	 */
-	ktime_t                         submit_ts;
 };
 
 static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
-- 
2.48.0

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Christian König]

Adding Hawking for commenting on RAS.

Am 05.02.25 um 16:33 schrieb Tvrtko Ursulin:
> The helper copies code from the existing amdgpu_job_stop_all_jobs_on_sched
> with the purpose of reducing the amount of driver code which directly
> touch scheduler internals.
>
> If or when amdgpu manages to change the approach for handling the
> permanently wedged state this helper can be removed.

When RAS indicates a problem and reset is disabled we shouldn't mess 
with the scheduler internals, but rather mark the device as unplugged 
and clear the PCIe DMA bits.

In other words enter the wedged state which is now well documented.

This way all submissions will run into ENODEV errors and be cleaned up 
immediately on submission by the scheduler. Applications will then just 
wait for their existing submissions and get an error if they try to send 
new ones.

Stopping the scheduler and then messing with the internals is basically 
just a really ugly hack. and never made sense in the first place as far 
as I can see.

See below for more comments.

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 44 ++++++++++++++++++++++++++
>   include/drm/gpu_scheduler.h            |  1 +
>   2 files changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index a48be16ab84f..0363655db22d 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -703,6 +703,50 @@ void drm_sched_start(struct drm_gpu_scheduler *sched, int errno)
>   }
>   EXPORT_SYMBOL(drm_sched_start);
>   
> +/**
> + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled jobs
> + *
> + * @sched: scheduler instance
> + * @errno: error value to set on signaled fences
> + *
> + * Signal all queued and scheduled jobs and set them to error state.
> + *
> + * Scheduler must be stopped before calling this.
> + */
> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int errno)
> +{
> +	struct drm_sched_entity *entity;
> +	struct drm_sched_fence *s_fence;
> +	struct drm_sched_job *job;
> +	enum drm_sched_priority p;
> +
> +	drm_WARN_ON_ONCE(sched, !sched->pause_submit);
> +
> +	/* Signal all jobs not yet scheduled */
> +	for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs; p++) {
> +		struct drm_sched_rq *rq = sched->sched_rq[p];
> +
> +		spin_lock(&rq->lock);
> +		list_for_each_entry(entity, &rq->entities, list) {
> +			while ((job = to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
> +				s_fence = job->s_fence;
> +				dma_fence_signal(&s_fence->scheduled);
> +				dma_fence_set_error(&s_fence->finished, errno);
> +				dma_fence_signal(&s_fence->finished);
> +			}
> +		}
> +		spin_unlock(&rq->lock);
> +	}
> +
> +	/* Signal all jobs already scheduled to HW */
> +	list_for_each_entry(job, &sched->pending_list, list) {
> +		s_fence = job->s_fence;
> +		dma_fence_set_error(&s_fence->finished, errno);
> +		dma_fence_signal(&s_fence->finished);
> +	}

This is in the wrong order, e.g. already scheduled jobs need to signal 
first and then not yet scheduled ones. Otherwise you violate the 
dma_fence ordering rules.

Additional to that this is racy like hell, e.g. even when we had an RAS 
error it is perfectly possible that submissions finish normally.

Regards,
Christian.

> +}
> +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
> +
>   /**
>    * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
>    *
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index a0ff08123f07..298513f8c327 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct drm_gpu_scheduler *sched);
>   void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct drm_sched_job *bad);
>   void drm_sched_start(struct drm_gpu_scheduler *sched, int errno);
> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int errno);
>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
>   void drm_sched_increase_karma(struct drm_sched_job *bad);
>   void drm_sched_reset_karma(struct drm_sched_job *bad);

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Philipp Stanner]

On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
> The helper copies code from the existing
> amdgpu_job_stop_all_jobs_on_sched
> with the purpose of reducing the amount of driver code which directly
> touch scheduler internals.
> 
> If or when amdgpu manages to change the approach for handling the
> permanently wedged state this helper can be removed.

Have you checked how many other drivers might need such a helper?

I have a bit mixed feelings about this, because, AFAICT, in the past
helpers have been added for just 1 driver, such as
drm_sched_wqueue_ready(), and then they have stayed for almost a
decade.

AFAIU this is just code move, and only really "decouples" amdgpu in the
sense of having an official scheduler function that does what amdgpu
used to do.

So my tendency here would be to continue "allowing" amdgpu to touch the
scheduler internals until amdgpu fixes this "permanently wedged
state". And if that's too difficult, couldn't the helper reside in a
amdgpu/sched_helpers.c or similar?

I think that's better than adding 1 helper for just 1 driver and then
supposedly removing it again in the future.

P.

> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 44
> ++++++++++++++++++++++++++
>  include/drm/gpu_scheduler.h            |  1 +
>  2 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index a48be16ab84f..0363655db22d 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -703,6 +703,50 @@ void drm_sched_start(struct drm_gpu_scheduler
> *sched, int errno)
>  }
>  EXPORT_SYMBOL(drm_sched_start);
>  
> +/**
> + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled jobs
> + *
> + * @sched: scheduler instance
> + * @errno: error value to set on signaled fences
> + *
> + * Signal all queued and scheduled jobs and set them to error state.
> + *
> + * Scheduler must be stopped before calling this.
> + */
> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
> errno)
> +{
> +	struct drm_sched_entity *entity;
> +	struct drm_sched_fence *s_fence;
> +	struct drm_sched_job *job;
> +	enum drm_sched_priority p;
> +
> +	drm_WARN_ON_ONCE(sched, !sched->pause_submit);
> +
> +	/* Signal all jobs not yet scheduled */
> +	for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs; p++)
> {
> +		struct drm_sched_rq *rq = sched->sched_rq[p];
> +
> +		spin_lock(&rq->lock);
> +		list_for_each_entry(entity, &rq->entities, list) {
> +			while ((job =
> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
> +				s_fence = job->s_fence;
> +				dma_fence_signal(&s_fence-
> >scheduled);
> +				dma_fence_set_error(&s_fence-
> >finished, errno);
> +				dma_fence_signal(&s_fence-
> >finished);
> +			}
> +		}
> +		spin_unlock(&rq->lock);
> +	}
> +
> +	/* Signal all jobs already scheduled to HW */
> +	list_for_each_entry(job, &sched->pending_list, list) {
> +		s_fence = job->s_fence;
> +		dma_fence_set_error(&s_fence->finished, errno);
> +		dma_fence_signal(&s_fence->finished);
> +	}
> +}
> +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
> +
>  /**
>   * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
>   *
> diff --git a/include/drm/gpu_scheduler.h
> b/include/drm/gpu_scheduler.h
> index a0ff08123f07..298513f8c327 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct
> drm_gpu_scheduler *sched);
>  void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
>  void drm_sched_stop(struct drm_gpu_scheduler *sched, struct
> drm_sched_job *bad);
>  void drm_sched_start(struct drm_gpu_scheduler *sched, int errno);
> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
> errno);
>  void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
>  void drm_sched_increase_karma(struct drm_sched_job *bad);
>  void drm_sched_reset_karma(struct drm_sched_job *bad);

Re: [PATCH 3/4] drm/sched: Add internal job peek/pop API

[Philipp Stanner]

On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
> Idea is to add helpers for peeking and poppling jobs from entities

s/poppling/popping

> with
> the goal of decoupling the hidden assumption in the code that
> queue_node
> is the first element in struct drm_sched_job.
> 
> That assumption usually comes in the form of:
> 
>   while ((job = to_drm_sched_job(spsc_queue_pop(&entity-
> >job_queue))))
> 
> Which breaks if the queue_node is re-positioned due to_drm_sched_job
> being implemented with a container_of.
> 
> This also allows us to remove duplicate definition of
> to_drm_sched_job
> which the scheduler apparently tried to keep internal, but failed
> since
> in one of the previous patches we also removed a copy from amdgpu.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

This one LGTM, +1

> Cc: Christian König <christian.koenig@amd.com>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Philipp Stanner <phasta@kernel.org>
> ---
>  drivers/gpu/drm/scheduler/sched_entity.c   | 11 +++---
>  drivers/gpu/drm/scheduler/sched_internal.h | 43
> ++++++++++++++++++++++
>  drivers/gpu/drm/scheduler/sched_main.c     |  9 ++---
>  3 files changed, 52 insertions(+), 11 deletions(-)
>  create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_entity.c
> b/drivers/gpu/drm/scheduler/sched_entity.c
> index 69bcf0e99d57..737feff147a5 100644
> --- a/drivers/gpu/drm/scheduler/sched_entity.c
> +++ b/drivers/gpu/drm/scheduler/sched_entity.c
> @@ -28,11 +28,10 @@
>  #include <drm/drm_print.h>
>  #include <drm/gpu_scheduler.h>
>  
> +#include "sched_internal.h"
> +
>  #include "gpu_scheduler_trace.h"
>  
> -#define to_drm_sched_job(sched_job)		\
> -		container_of((sched_job), struct drm_sched_job,
> queue_node)
> -
>  /**
>   * drm_sched_entity_init - Init a context entity used by scheduler
> when
>   * submit to HW ring.
> @@ -255,7 +254,7 @@ static void drm_sched_entity_kill(struct
> drm_sched_entity *entity)
>  	/* The entity is guaranteed to not be used by the scheduler
> */
>  	prev = rcu_dereference_check(entity->last_scheduled, true);
>  	dma_fence_get(prev);
> -	while ((job = to_drm_sched_job(spsc_queue_pop(&entity-
> >job_queue)))) {
> +	while ((job = __drm_sched_entity_queue_pop(entity))) {
>  		struct drm_sched_fence *s_fence = job->s_fence;
>  
>  		dma_fence_get(&s_fence->finished);
> @@ -477,7 +476,7 @@ struct drm_sched_job
> *drm_sched_entity_pop_job(struct drm_sched_entity *entity)
>  {
>  	struct drm_sched_job *sched_job;
>  
> -	sched_job = to_drm_sched_job(spsc_queue_peek(&entity-
> >job_queue));
> +	sched_job = __drm_sched_entity_queue_peek(entity);
>  	if (!sched_job)
>  		return NULL;
>  
> @@ -513,7 +512,7 @@ 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;
>  
> -		next = to_drm_sched_job(spsc_queue_peek(&entity-
> >job_queue));
> +		next = __drm_sched_entity_queue_peek(entity);
>  		if (next) {
>  			struct drm_sched_rq *rq;
>  
> diff --git a/drivers/gpu/drm/scheduler/sched_internal.h
> b/drivers/gpu/drm/scheduler/sched_internal.h
> new file mode 100644
> index 000000000000..565c83e32371
> --- /dev/null
> +++ b/drivers/gpu/drm/scheduler/sched_internal.h
> @@ -0,0 +1,43 @@
> +
> +
> +/**
> + * __drm_sched_entity_queue_pop - Low level helper for popping
> queued jobs
> + *
> + * @entity: scheduler entity
> + *
> + * Low level helper for popping queued jobs.
> + *
> + * Returns the job dequeued or NULL.
> + */
> +static inline struct drm_sched_job *
> +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
> +{
> +	struct spsc_node *node;
> +
> +	node = spsc_queue_pop(&entity->job_queue);
> +	if (!node)
> +		return NULL;
> +
> +	return container_of(node, struct drm_sched_job, queue_node);
> +}
> +
> +/**
> + * __drm_sched_entity_queue_peek - Low level helper for peeking at
> the job queue
> + *
> + * @entity: scheduler entity
> + *
> + * Low level helper for peeking at the job queue
> + *
> + * Returns the job at the head of the queue or NULL.
> + */
> +static inline struct drm_sched_job *
> +__drm_sched_entity_queue_peek(struct drm_sched_entity *entity)
> +{
> +	struct spsc_node *node;
> +
> +	node = spsc_queue_peek(&entity->job_queue);
> +	if (!node)
> +		return NULL;
> +
> +	return container_of(node, struct drm_sched_job, queue_node);
> +}
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 0363655db22d..41d6f839748e 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -78,6 +78,8 @@
>  #include <drm/gpu_scheduler.h>
>  #include <drm/spsc_queue.h>
>  
> +#include "sched_internal.h"
> +
>  #define CREATE_TRACE_POINTS
>  #include "gpu_scheduler_trace.h"
>  
> @@ -87,9 +89,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;
>  
>  /**
> @@ -123,7 +122,7 @@ static bool drm_sched_can_queue(struct
> drm_gpu_scheduler *sched,
>  {
>  	struct drm_sched_job *s_job;
>  
> -	s_job = to_drm_sched_job(spsc_queue_peek(&entity-
> >job_queue));
> +	s_job = __drm_sched_entity_queue_peek(entity);
>  	if (!s_job)
>  		return false;
>  
> @@ -728,7 +727,7 @@ void drm_sched_cancel_all_jobs(struct
> drm_gpu_scheduler *sched, int errno)
>  
>  		spin_lock(&rq->lock);
>  		list_for_each_entry(entity, &rq->entities, list) {
> -			while ((job =
> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
> +			while ((job =
> __drm_sched_entity_queue_pop(entity))) {
>  				s_fence = job->s_fence;
>  				dma_fence_signal(&s_fence-
> >scheduled);
>  				dma_fence_set_error(&s_fence-
> >finished, errno);

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Tvrtko Ursulin]

On 06/02/2025 13:35, Philipp Stanner wrote:
> On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
>> The helper copies code from the existing
>> amdgpu_job_stop_all_jobs_on_sched
>> with the purpose of reducing the amount of driver code which directly
>> touch scheduler internals.
>>
>> If or when amdgpu manages to change the approach for handling the
>> permanently wedged state this helper can be removed.
> 
> Have you checked how many other drivers might need such a helper?
> 
> I have a bit mixed feelings about this, because, AFAICT, in the past
> helpers have been added for just 1 driver, such as
> drm_sched_wqueue_ready(), and then they have stayed for almost a
> decade.
> 
> AFAIU this is just code move, and only really "decouples" amdgpu in the
> sense of having an official scheduler function that does what amdgpu
> used to do.
> 
> So my tendency here would be to continue "allowing" amdgpu to touch the
> scheduler internals until amdgpu fixes this "permanently wedged
> state". And if that's too difficult, couldn't the helper reside in a
> amdgpu/sched_helpers.c or similar?
> 
> I think that's better than adding 1 helper for just 1 driver and then
> supposedly removing it again in the future.

I was 50% nudging Christian into providing a more concrete idea on how 
to fix amdgpu ;) and other 50% I want to get rid of three copies of 
to_drm_sched_job and remove the hidden "queue node must be first" 
dependency.

So let it marinate a bit and we will see if a nicer solution shows up.

Regards,

Tvrtko

>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <phasta@kernel.org>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 44
>> ++++++++++++++++++++++++++
>>   include/drm/gpu_scheduler.h            |  1 +
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index a48be16ab84f..0363655db22d 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -703,6 +703,50 @@ void drm_sched_start(struct drm_gpu_scheduler
>> *sched, int errno)
>>   }
>>   EXPORT_SYMBOL(drm_sched_start);
>>   
>> +/**
>> + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled jobs
>> + *
>> + * @sched: scheduler instance
>> + * @errno: error value to set on signaled fences
>> + *
>> + * Signal all queued and scheduled jobs and set them to error state.
>> + *
>> + * Scheduler must be stopped before calling this.
>> + */
>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>> errno)
>> +{
>> +	struct drm_sched_entity *entity;
>> +	struct drm_sched_fence *s_fence;
>> +	struct drm_sched_job *job;
>> +	enum drm_sched_priority p;
>> +
>> +	drm_WARN_ON_ONCE(sched, !sched->pause_submit);
>> +
>> +	/* Signal all jobs not yet scheduled */
>> +	for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs; p++)
>> {
>> +		struct drm_sched_rq *rq = sched->sched_rq[p];
>> +
>> +		spin_lock(&rq->lock);
>> +		list_for_each_entry(entity, &rq->entities, list) {
>> +			while ((job =
>> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>> +				s_fence = job->s_fence;
>> +				dma_fence_signal(&s_fence-
>>> scheduled);
>> +				dma_fence_set_error(&s_fence-
>>> finished, errno);
>> +				dma_fence_signal(&s_fence-
>>> finished);
>> +			}
>> +		}
>> +		spin_unlock(&rq->lock);
>> +	}
>> +
>> +	/* Signal all jobs already scheduled to HW */
>> +	list_for_each_entry(job, &sched->pending_list, list) {
>> +		s_fence = job->s_fence;
>> +		dma_fence_set_error(&s_fence->finished, errno);
>> +		dma_fence_signal(&s_fence->finished);
>> +	}
>> +}
>> +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
>> +
>>   /**
>>    * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
>>    *
>> diff --git a/include/drm/gpu_scheduler.h
>> b/include/drm/gpu_scheduler.h
>> index a0ff08123f07..298513f8c327 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct
>> drm_gpu_scheduler *sched);
>>   void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
>>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct
>> drm_sched_job *bad);
>>   void drm_sched_start(struct drm_gpu_scheduler *sched, int errno);
>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>> errno);
>>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
>>   void drm_sched_increase_karma(struct drm_sched_job *bad);
>>   void drm_sched_reset_karma(struct drm_sched_job *bad);
> 

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Christian König]

Am 06.02.25 um 14:35 schrieb Philipp Stanner:
> On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
>> The helper copies code from the existing
>> amdgpu_job_stop_all_jobs_on_sched
>> with the purpose of reducing the amount of driver code which directly
>> touch scheduler internals.
>>
>> If or when amdgpu manages to change the approach for handling the
>> permanently wedged state this helper can be removed.
> Have you checked how many other drivers might need such a helper?
>
> I have a bit mixed feelings about this, because, AFAICT, in the past
> helpers have been added for just 1 driver, such as
> drm_sched_wqueue_ready(), and then they have stayed for almost a
> decade.
>
> AFAIU this is just code move, and only really "decouples" amdgpu in the
> sense of having an official scheduler function that does what amdgpu
> used to do.
>
> So my tendency here would be to continue "allowing" amdgpu to touch the
> scheduler internals until amdgpu fixes this "permanently wedged
> state". And if that's too difficult, couldn't the helper reside in a
> amdgpu/sched_helpers.c or similar?
>
> I think that's better than adding 1 helper for just 1 driver and then
> supposedly removing it again in the future.

Yeah, agree to that general approach.

What amdgpu does here is kind of nasty and looks unnecessary, but 
changing it means we need time from Hawkings and his people involved on 
RAS for amdgpu.

When we move the code to the scheduler we make it official scheduler 
interface to others to replicate and that is exactly what we should try 
to avoid.

So my suggestion is to add a /* TODO: This is nasty and should be 
avoided */ to the amdgpu code instead.

Regards,
Christian.

>
> P.
>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Danilo Krummrich <dakr@kernel.org>
>> Cc: Matthew Brost <matthew.brost@intel.com>
>> Cc: Philipp Stanner <phasta@kernel.org>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 44
>> ++++++++++++++++++++++++++
>>   include/drm/gpu_scheduler.h            |  1 +
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index a48be16ab84f..0363655db22d 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -703,6 +703,50 @@ void drm_sched_start(struct drm_gpu_scheduler
>> *sched, int errno)
>>   }
>>   EXPORT_SYMBOL(drm_sched_start);
>>   
>> +/**
>> + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled jobs
>> + *
>> + * @sched: scheduler instance
>> + * @errno: error value to set on signaled fences
>> + *
>> + * Signal all queued and scheduled jobs and set them to error state.
>> + *
>> + * Scheduler must be stopped before calling this.
>> + */
>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>> errno)
>> +{
>> +	struct drm_sched_entity *entity;
>> +	struct drm_sched_fence *s_fence;
>> +	struct drm_sched_job *job;
>> +	enum drm_sched_priority p;
>> +
>> +	drm_WARN_ON_ONCE(sched, !sched->pause_submit);
>> +
>> +	/* Signal all jobs not yet scheduled */
>> +	for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs; p++)
>> {
>> +		struct drm_sched_rq *rq = sched->sched_rq[p];
>> +
>> +		spin_lock(&rq->lock);
>> +		list_for_each_entry(entity, &rq->entities, list) {
>> +			while ((job =
>> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>> +				s_fence = job->s_fence;
>> +				dma_fence_signal(&s_fence-
>>> scheduled);
>> +				dma_fence_set_error(&s_fence-
>>> finished, errno);
>> +				dma_fence_signal(&s_fence-
>>> finished);
>> +			}
>> +		}
>> +		spin_unlock(&rq->lock);
>> +	}
>> +
>> +	/* Signal all jobs already scheduled to HW */
>> +	list_for_each_entry(job, &sched->pending_list, list) {
>> +		s_fence = job->s_fence;
>> +		dma_fence_set_error(&s_fence->finished, errno);
>> +		dma_fence_signal(&s_fence->finished);
>> +	}
>> +}
>> +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
>> +
>>   /**
>>    * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
>>    *
>> diff --git a/include/drm/gpu_scheduler.h
>> b/include/drm/gpu_scheduler.h
>> index a0ff08123f07..298513f8c327 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct
>> drm_gpu_scheduler *sched);
>>   void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
>>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct
>> drm_sched_job *bad);
>>   void drm_sched_start(struct drm_gpu_scheduler *sched, int errno);
>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>> errno);
>>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
>>   void drm_sched_increase_karma(struct drm_sched_job *bad);
>>   void drm_sched_reset_karma(struct drm_sched_job *bad);

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Tvrtko Ursulin]

On 06/02/2025 13:46, Christian König wrote:
> Am 06.02.25 um 14:35 schrieb Philipp Stanner:
>> On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
>>> The helper copies code from the existing
>>> amdgpu_job_stop_all_jobs_on_sched
>>> with the purpose of reducing the amount of driver code which directly
>>> touch scheduler internals.
>>>
>>> If or when amdgpu manages to change the approach for handling the
>>> permanently wedged state this helper can be removed.
>> Have you checked how many other drivers might need such a helper?
>>
>> I have a bit mixed feelings about this, because, AFAICT, in the past
>> helpers have been added for just 1 driver, such as
>> drm_sched_wqueue_ready(), and then they have stayed for almost a
>> decade.
>>
>> AFAIU this is just code move, and only really "decouples" amdgpu in the
>> sense of having an official scheduler function that does what amdgpu
>> used to do.
>>
>> So my tendency here would be to continue "allowing" amdgpu to touch the
>> scheduler internals until amdgpu fixes this "permanently wedged
>> state". And if that's too difficult, couldn't the helper reside in a
>> amdgpu/sched_helpers.c or similar?
>>
>> I think that's better than adding 1 helper for just 1 driver and then
>> supposedly removing it again in the future.
> 
> Yeah, agree to that general approach.
> 
> What amdgpu does here is kind of nasty and looks unnecessary, but 
> changing it means we need time from Hawkings and his people involved on 
> RAS for amdgpu.
> 
> When we move the code to the scheduler we make it official scheduler 
> interface to others to replicate and that is exactly what we should try 
> to avoid.
> 
> So my suggestion is to add a /* TODO: This is nasty and should be 
> avoided */ to the amdgpu code instead.

So I got a no go to export a low level queue pop helper, no go to move 
the whole dodgy code to common (reasonable). Any third way to break the 
status quo? What if I respin with just a change local to amdgpu which 
would, instead of duplicating the to_drm_sched_job macro, duplicate 
__drm_sched_entity_queue_pop from 3/4 of this series?

Regards,

Tvrtko

> 
> Regards,
> Christian.
> 
>>
>> P.
>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>> Cc: Philipp Stanner <phasta@kernel.org>
>>> ---
>>>   drivers/gpu/drm/scheduler/sched_main.c | 44
>>> ++++++++++++++++++++++++++
>>>   include/drm/gpu_scheduler.h            |  1 +
>>>   2 files changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>> index a48be16ab84f..0363655db22d 100644
>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>> @@ -703,6 +703,50 @@ void drm_sched_start(struct drm_gpu_scheduler
>>> *sched, int errno)
>>>   }
>>>   EXPORT_SYMBOL(drm_sched_start);
>>> +/**
>>> + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled jobs
>>> + *
>>> + * @sched: scheduler instance
>>> + * @errno: error value to set on signaled fences
>>> + *
>>> + * Signal all queued and scheduled jobs and set them to error state.
>>> + *
>>> + * Scheduler must be stopped before calling this.
>>> + */
>>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>>> errno)
>>> +{
>>> +    struct drm_sched_entity *entity;
>>> +    struct drm_sched_fence *s_fence;
>>> +    struct drm_sched_job *job;
>>> +    enum drm_sched_priority p;
>>> +
>>> +    drm_WARN_ON_ONCE(sched, !sched->pause_submit);
>>> +
>>> +    /* Signal all jobs not yet scheduled */
>>> +    for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs; p++)
>>> {
>>> +        struct drm_sched_rq *rq = sched->sched_rq[p];
>>> +
>>> +        spin_lock(&rq->lock);
>>> +        list_for_each_entry(entity, &rq->entities, list) {
>>> +            while ((job =
>>> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>>> +                s_fence = job->s_fence;
>>> +                dma_fence_signal(&s_fence-
>>>> scheduled);
>>> +                dma_fence_set_error(&s_fence-
>>>> finished, errno);
>>> +                dma_fence_signal(&s_fence-
>>>> finished);
>>> +            }
>>> +        }
>>> +        spin_unlock(&rq->lock);
>>> +    }
>>> +
>>> +    /* Signal all jobs already scheduled to HW */
>>> +    list_for_each_entry(job, &sched->pending_list, list) {
>>> +        s_fence = job->s_fence;
>>> +        dma_fence_set_error(&s_fence->finished, errno);
>>> +        dma_fence_signal(&s_fence->finished);
>>> +    }
>>> +}
>>> +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
>>> +
>>>   /**
>>>    * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
>>>    *
>>> diff --git a/include/drm/gpu_scheduler.h
>>> b/include/drm/gpu_scheduler.h
>>> index a0ff08123f07..298513f8c327 100644
>>> --- a/include/drm/gpu_scheduler.h
>>> +++ b/include/drm/gpu_scheduler.h
>>> @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct
>>> drm_gpu_scheduler *sched);
>>>   void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
>>>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct
>>> drm_sched_job *bad);
>>>   void drm_sched_start(struct drm_gpu_scheduler *sched, int errno);
>>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>>> errno);
>>>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
>>>   void drm_sched_increase_karma(struct drm_sched_job *bad);
>>>   void drm_sched_reset_karma(struct drm_sched_job *bad);
> 

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Philipp Stanner]

On Thu, 2025-02-06 at 14:46 +0100, Christian König wrote:
> Am 06.02.25 um 14:35 schrieb Philipp Stanner:
> > On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
> > > The helper copies code from the existing
> > > amdgpu_job_stop_all_jobs_on_sched
> > > with the purpose of reducing the amount of driver code which
> > > directly
> > > touch scheduler internals.
> > > 
> > > If or when amdgpu manages to change the approach for handling the
> > > permanently wedged state this helper can be removed.
> > Have you checked how many other drivers might need such a helper?
> > 
> > I have a bit mixed feelings about this, because, AFAICT, in the
> > past
> > helpers have been added for just 1 driver, such as
> > drm_sched_wqueue_ready(), and then they have stayed for almost a
> > decade.
> > 
> > AFAIU this is just code move, and only really "decouples" amdgpu in
> > the
> > sense of having an official scheduler function that does what
> > amdgpu
> > used to do.
> > 
> > So my tendency here would be to continue "allowing" amdgpu to touch
> > the
> > scheduler internals until amdgpu fixes this "permanently wedged
> > state". And if that's too difficult, couldn't the helper reside in
> > a
> > amdgpu/sched_helpers.c or similar?
> > 
> > I think that's better than adding 1 helper for just 1 driver and
> > then
> > supposedly removing it again in the future.
> 
> Yeah, agree to that general approach.
> 
> What amdgpu does here is kind of nasty and looks unnecessary, but 
> changing it means we need time from Hawkings and his people involved
> on 
> RAS for amdgpu.
> 
> When we move the code to the scheduler we make it official scheduler 
> interface to others to replicate and that is exactly what we should
> try 
> to avoid.

Yes, I think if we all agree that the scheduler must only contain
infrastructure useful for >= 2 DRM drivers' job queueing related tasks
without any hacks for driver internal issues, that would be a great
thing.


P.

> 
> So my suggestion is to add a /* TODO: This is nasty and should be 
> avoided */ to the amdgpu code instead.
> 
> Regards,
> Christian.
> 
> > 
> > P.
> > 
> > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Philipp Stanner <phasta@kernel.org>
> > > ---
> > >   drivers/gpu/drm/scheduler/sched_main.c | 44
> > > ++++++++++++++++++++++++++
> > >   include/drm/gpu_scheduler.h            |  1 +
> > >   2 files changed, 45 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > index a48be16ab84f..0363655db22d 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > @@ -703,6 +703,50 @@ void drm_sched_start(struct
> > > drm_gpu_scheduler
> > > *sched, int errno)
> > >   }
> > >   EXPORT_SYMBOL(drm_sched_start);
> > >   
> > > +/**
> > > + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled
> > > jobs
> > > + *
> > > + * @sched: scheduler instance
> > > + * @errno: error value to set on signaled fences
> > > + *
> > > + * Signal all queued and scheduled jobs and set them to error
> > > state.
> > > + *
> > > + * Scheduler must be stopped before calling this.
> > > + */
> > > +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched,
> > > int
> > > errno)
> > > +{
> > > +	struct drm_sched_entity *entity;
> > > +	struct drm_sched_fence *s_fence;
> > > +	struct drm_sched_job *job;
> > > +	enum drm_sched_priority p;
> > > +
> > > +	drm_WARN_ON_ONCE(sched, !sched->pause_submit);
> > > +
> > > +	/* Signal all jobs not yet scheduled */
> > > +	for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs;
> > > p++)
> > > {
> > > +		struct drm_sched_rq *rq = sched->sched_rq[p];
> > > +
> > > +		spin_lock(&rq->lock);
> > > +		list_for_each_entry(entity, &rq->entities, list)
> > > {
> > > +			while ((job =
> > > to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
> > > +				s_fence = job->s_fence;
> > > +				dma_fence_signal(&s_fence-
> > > > scheduled);
> > > +				dma_fence_set_error(&s_fence-
> > > > finished, errno);
> > > +				dma_fence_signal(&s_fence-
> > > > finished);
> > > +			}
> > > +		}
> > > +		spin_unlock(&rq->lock);
> > > +	}
> > > +
> > > +	/* Signal all jobs already scheduled to HW */
> > > +	list_for_each_entry(job, &sched->pending_list, list) {
> > > +		s_fence = job->s_fence;
> > > +		dma_fence_set_error(&s_fence->finished, errno);
> > > +		dma_fence_signal(&s_fence->finished);
> > > +	}
> > > +}
> > > +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
> > > +
> > >   /**
> > >    * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
> > >    *
> > > diff --git a/include/drm/gpu_scheduler.h
> > > b/include/drm/gpu_scheduler.h
> > > index a0ff08123f07..298513f8c327 100644
> > > --- a/include/drm/gpu_scheduler.h
> > > +++ b/include/drm/gpu_scheduler.h
> > > @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct
> > > drm_gpu_scheduler *sched);
> > >   void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
> > >   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct
> > > drm_sched_job *bad);
> > >   void drm_sched_start(struct drm_gpu_scheduler *sched, int
> > > errno);
> > > +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched,
> > > int
> > > errno);
> > >   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
> > >   void drm_sched_increase_karma(struct drm_sched_job *bad);
> > >   void drm_sched_reset_karma(struct drm_sched_job *bad);
> 

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Philipp Stanner]

On Thu, 2025-02-06 at 13:53 +0000, Tvrtko Ursulin wrote:
> 
> On 06/02/2025 13:46, Christian König wrote:
> > Am 06.02.25 um 14:35 schrieb Philipp Stanner:
> > > On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
> > > > The helper copies code from the existing
> > > > amdgpu_job_stop_all_jobs_on_sched
> > > > with the purpose of reducing the amount of driver code which
> > > > directly
> > > > touch scheduler internals.
> > > > 
> > > > If or when amdgpu manages to change the approach for handling
> > > > the
> > > > permanently wedged state this helper can be removed.
> > > Have you checked how many other drivers might need such a helper?
> > > 
> > > I have a bit mixed feelings about this, because, AFAICT, in the
> > > past
> > > helpers have been added for just 1 driver, such as
> > > drm_sched_wqueue_ready(), and then they have stayed for almost a
> > > decade.
> > > 
> > > AFAIU this is just code move, and only really "decouples" amdgpu
> > > in the
> > > sense of having an official scheduler function that does what
> > > amdgpu
> > > used to do.
> > > 
> > > So my tendency here would be to continue "allowing" amdgpu to
> > > touch the
> > > scheduler internals until amdgpu fixes this "permanently wedged
> > > state". And if that's too difficult, couldn't the helper reside
> > > in a
> > > amdgpu/sched_helpers.c or similar?
> > > 
> > > I think that's better than adding 1 helper for just 1 driver and
> > > then
> > > supposedly removing it again in the future.
> > 
> > Yeah, agree to that general approach.
> > 
> > What amdgpu does here is kind of nasty and looks unnecessary, but 
> > changing it means we need time from Hawkings and his people
> > involved on 
> > RAS for amdgpu.
> > 
> > When we move the code to the scheduler we make it official
> > scheduler 
> > interface to others to replicate and that is exactly what we should
> > try 
> > to avoid.
> > 
> > So my suggestion is to add a /* TODO: This is nasty and should be 
> > avoided */ to the amdgpu code instead.
> 
> So I got a no go to export a low level queue pop helper

The spsc_queue helper in patch 3 is totally alright. Patch 3 only
depends on patch 1 in the sense of it adding the new helper to the
cancel_all function of patch 1, or am I missing something obvious?

> , no go to move 
> the whole dodgy code to common (reasonable). Any third way to break
> the 
> status quo? What if I respin with just a change local to amdgpu which
> would, instead of duplicating the to_drm_sched_job macro, duplicate 
> __drm_sched_entity_queue_pop from 3/4 of this series?

I'm willing to take patch 3 if it's independent. That would then mean
that to_drm_sched_job() is only necessary anmyore in amdgpu, wouldn't
it?

That's independent from the cancel_all() function as far as the
scheduler is concerned.


P.


> 
> Regards,
> 
> Tvrtko
> 
> > 
> > Regards,
> > Christian.
> > 
> > > 
> > > P.
> > > 
> > > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> > > > Cc: Christian König <christian.koenig@amd.com>
> > > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > > Cc: Philipp Stanner <phasta@kernel.org>
> > > > ---
> > > >   drivers/gpu/drm/scheduler/sched_main.c | 44
> > > > ++++++++++++++++++++++++++
> > > >   include/drm/gpu_scheduler.h            |  1 +
> > > >   2 files changed, 45 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index a48be16ab84f..0363655db22d 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -703,6 +703,50 @@ void drm_sched_start(struct
> > > > drm_gpu_scheduler
> > > > *sched, int errno)
> > > >   }
> > > >   EXPORT_SYMBOL(drm_sched_start);
> > > > +/**
> > > > + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled
> > > > jobs
> > > > + *
> > > > + * @sched: scheduler instance
> > > > + * @errno: error value to set on signaled fences
> > > > + *
> > > > + * Signal all queued and scheduled jobs and set them to error
> > > > state.
> > > > + *
> > > > + * Scheduler must be stopped before calling this.
> > > > + */
> > > > +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler
> > > > *sched, int
> > > > errno)
> > > > +{
> > > > +    struct drm_sched_entity *entity;
> > > > +    struct drm_sched_fence *s_fence;
> > > > +    struct drm_sched_job *job;
> > > > +    enum drm_sched_priority p;
> > > > +
> > > > +    drm_WARN_ON_ONCE(sched, !sched->pause_submit);
> > > > +
> > > > +    /* Signal all jobs not yet scheduled */
> > > > +    for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs;
> > > > p++)
> > > > {
> > > > +        struct drm_sched_rq *rq = sched->sched_rq[p];
> > > > +
> > > > +        spin_lock(&rq->lock);
> > > > +        list_for_each_entry(entity, &rq->entities, list) {
> > > > +            while ((job =
> > > > to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
> > > > +                s_fence = job->s_fence;
> > > > +                dma_fence_signal(&s_fence-
> > > > > scheduled);
> > > > +                dma_fence_set_error(&s_fence-
> > > > > finished, errno);
> > > > +                dma_fence_signal(&s_fence-
> > > > > finished);
> > > > +            }
> > > > +        }
> > > > +        spin_unlock(&rq->lock);
> > > > +    }
> > > > +
> > > > +    /* Signal all jobs already scheduled to HW */
> > > > +    list_for_each_entry(job, &sched->pending_list, list) {
> > > > +        s_fence = job->s_fence;
> > > > +        dma_fence_set_error(&s_fence->finished, errno);
> > > > +        dma_fence_signal(&s_fence->finished);
> > > > +    }
> > > > +}
> > > > +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
> > > > +
> > > >   /**
> > > >    * drm_sched_resubmit_jobs - Deprecated, don't use in new
> > > > code!
> > > >    *
> > > > diff --git a/include/drm/gpu_scheduler.h
> > > > b/include/drm/gpu_scheduler.h
> > > > index a0ff08123f07..298513f8c327 100644
> > > > --- a/include/drm/gpu_scheduler.h
> > > > +++ b/include/drm/gpu_scheduler.h
> > > > @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct
> > > > drm_gpu_scheduler *sched);
> > > >   void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
> > > >   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct
> > > > drm_sched_job *bad);
> > > >   void drm_sched_start(struct drm_gpu_scheduler *sched, int
> > > > errno);
> > > > +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler
> > > > *sched, int
> > > > errno);
> > > >   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler
> > > > *sched);
> > > >   void drm_sched_increase_karma(struct drm_sched_job *bad);
> > > >   void drm_sched_reset_karma(struct drm_sched_job *bad);
> > 

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Christian König]

Am 06.02.25 um 14:53 schrieb Tvrtko Ursulin:
>
> On 06/02/2025 13:46, Christian König wrote:
>> Am 06.02.25 um 14:35 schrieb Philipp Stanner:
>>> On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
>>>> The helper copies code from the existing
>>>> amdgpu_job_stop_all_jobs_on_sched
>>>> with the purpose of reducing the amount of driver code which directly
>>>> touch scheduler internals.
>>>>
>>>> If or when amdgpu manages to change the approach for handling the
>>>> permanently wedged state this helper can be removed.
>>> Have you checked how many other drivers might need such a helper?
>>>
>>> I have a bit mixed feelings about this, because, AFAICT, in the past
>>> helpers have been added for just 1 driver, such as
>>> drm_sched_wqueue_ready(), and then they have stayed for almost a
>>> decade.
>>>
>>> AFAIU this is just code move, and only really "decouples" amdgpu in the
>>> sense of having an official scheduler function that does what amdgpu
>>> used to do.
>>>
>>> So my tendency here would be to continue "allowing" amdgpu to touch the
>>> scheduler internals until amdgpu fixes this "permanently wedged
>>> state". And if that's too difficult, couldn't the helper reside in a
>>> amdgpu/sched_helpers.c or similar?
>>>
>>> I think that's better than adding 1 helper for just 1 driver and then
>>> supposedly removing it again in the future.
>>
>> Yeah, agree to that general approach.
>>
>> What amdgpu does here is kind of nasty and looks unnecessary, but 
>> changing it means we need time from Hawkings and his people involved 
>> on RAS for amdgpu.
>>
>> When we move the code to the scheduler we make it official scheduler 
>> interface to others to replicate and that is exactly what we should 
>> try to avoid.
>>
>> So my suggestion is to add a /* TODO: This is nasty and should be 
>> avoided */ to the amdgpu code instead.
>
> So I got a no go to export a low level queue pop helper, no go to move 
> the whole dodgy code to common (reasonable). Any third way to break 
> the status quo? What if I respin with just a change local to amdgpu 
> which would, instead of duplicating the to_drm_sched_job macro, 
> duplicate __drm_sched_entity_queue_pop from 3/4 of this series?

Removing the necessity for queue to be the first memory is still a good 
idea.

I would add internal container_of helpers to the scheduler and then use 
explicit container_of in amdgpu. E.g. don't expose the scheduler 
helpers, but rather manually code them up.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>
>> Regards,
>> Christian.
>>
>>>
>>> P.
>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>>> Cc: Christian König <christian.koenig@amd.com>
>>>> Cc: Danilo Krummrich <dakr@kernel.org>
>>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>>> Cc: Philipp Stanner <phasta@kernel.org>
>>>> ---
>>>>   drivers/gpu/drm/scheduler/sched_main.c | 44
>>>> ++++++++++++++++++++++++++
>>>>   include/drm/gpu_scheduler.h            |  1 +
>>>>   2 files changed, 45 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>>>> b/drivers/gpu/drm/scheduler/sched_main.c
>>>> index a48be16ab84f..0363655db22d 100644
>>>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>>>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>>>> @@ -703,6 +703,50 @@ void drm_sched_start(struct drm_gpu_scheduler
>>>> *sched, int errno)
>>>>   }
>>>>   EXPORT_SYMBOL(drm_sched_start);
>>>> +/**
>>>> + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled jobs
>>>> + *
>>>> + * @sched: scheduler instance
>>>> + * @errno: error value to set on signaled fences
>>>> + *
>>>> + * Signal all queued and scheduled jobs and set them to error state.
>>>> + *
>>>> + * Scheduler must be stopped before calling this.
>>>> + */
>>>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>>>> errno)
>>>> +{
>>>> +    struct drm_sched_entity *entity;
>>>> +    struct drm_sched_fence *s_fence;
>>>> +    struct drm_sched_job *job;
>>>> +    enum drm_sched_priority p;
>>>> +
>>>> +    drm_WARN_ON_ONCE(sched, !sched->pause_submit);
>>>> +
>>>> +    /* Signal all jobs not yet scheduled */
>>>> +    for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs; p++)
>>>> {
>>>> +        struct drm_sched_rq *rq = sched->sched_rq[p];
>>>> +
>>>> +        spin_lock(&rq->lock);
>>>> +        list_for_each_entry(entity, &rq->entities, list) {
>>>> +            while ((job =
>>>> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>>>> +                s_fence = job->s_fence;
>>>> +                dma_fence_signal(&s_fence-
>>>>> scheduled);
>>>> +                dma_fence_set_error(&s_fence-
>>>>> finished, errno);
>>>> +                dma_fence_signal(&s_fence-
>>>>> finished);
>>>> +            }
>>>> +        }
>>>> +        spin_unlock(&rq->lock);
>>>> +    }
>>>> +
>>>> +    /* Signal all jobs already scheduled to HW */
>>>> +    list_for_each_entry(job, &sched->pending_list, list) {
>>>> +        s_fence = job->s_fence;
>>>> +        dma_fence_set_error(&s_fence->finished, errno);
>>>> +        dma_fence_signal(&s_fence->finished);
>>>> +    }
>>>> +}
>>>> +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
>>>> +
>>>>   /**
>>>>    * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
>>>>    *
>>>> diff --git a/include/drm/gpu_scheduler.h
>>>> b/include/drm/gpu_scheduler.h
>>>> index a0ff08123f07..298513f8c327 100644
>>>> --- a/include/drm/gpu_scheduler.h
>>>> +++ b/include/drm/gpu_scheduler.h
>>>> @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct
>>>> drm_gpu_scheduler *sched);
>>>>   void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
>>>>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct
>>>> drm_sched_job *bad);
>>>>   void drm_sched_start(struct drm_gpu_scheduler *sched, int errno);
>>>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>>>> errno);
>>>>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
>>>>   void drm_sched_increase_karma(struct drm_sched_job *bad);
>>>>   void drm_sched_reset_karma(struct drm_sched_job *bad);
>>

Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Danilo Krummrich]

On Thu, Feb 06, 2025 at 02:46:40PM +0100, Christian König wrote:
> Am 06.02.25 um 14:35 schrieb Philipp Stanner:
> > On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
> > > The helper copies code from the existing
> > > amdgpu_job_stop_all_jobs_on_sched
> > > with the purpose of reducing the amount of driver code which directly
> > > touch scheduler internals.
> > > 
> > > If or when amdgpu manages to change the approach for handling the
> > > permanently wedged state this helper can be removed.
> > Have you checked how many other drivers might need such a helper?
> > 
> > I have a bit mixed feelings about this, because, AFAICT, in the past
> > helpers have been added for just 1 driver, such as
> > drm_sched_wqueue_ready(), and then they have stayed for almost a
> > decade.
> > 
> > AFAIU this is just code move, and only really "decouples" amdgpu in the
> > sense of having an official scheduler function that does what amdgpu
> > used to do.
> > 
> > So my tendency here would be to continue "allowing" amdgpu to touch the
> > scheduler internals until amdgpu fixes this "permanently wedged
> > state". And if that's too difficult, couldn't the helper reside in a
> > amdgpu/sched_helpers.c or similar?
> > 
> > I think that's better than adding 1 helper for just 1 driver and then
> > supposedly removing it again in the future.
> 
> Yeah, agree to that general approach.
> 
> What amdgpu does here is kind of nasty and looks unnecessary, but changing
> it means we need time from Hawkings and his people involved on RAS for
> amdgpu.
> 
> When we move the code to the scheduler we make it official scheduler
> interface to others to replicate and that is exactly what we should try to
> avoid.

It'd be even worse. It would mean that we create an example for drivers to be
"rewarded" with a free driver specific API by abusing the general API.

Clearly (also) a NACK from my end.

RE: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

[Zhang, Hawking]

[-- Attachment #1: Type: text/plain, Size: 6325 bytes --]

[AMD Official Use Only - AMD Internal Distribution Only]


I agree with the overall approach and support Chris's suggestion. The function amdgpu_job_stop_all_jobs_on_sched is now only applicable to a few older AMD hardware models when they encounter uncorrectable hardware errors. We have discontinued this method for several generations of newer hardware.

CC @Christian König<mailto:christian.koenig@amd.com>/@Deucher, Alexander<mailto:Alexander.Deucher@amd.com>

Regards,
Hawking

-----Original Message-----
From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Christian König
Sent: Thursday, February 6, 2025 21:47
To: phasta@kernel.org; Tvrtko Ursulin <tvrtko.ursulin@igalia.com>; amd-gfx@lists.freedesktop.org
Cc: kernel-dev@igalia.com; Danilo Krummrich <dakr@kernel.org>; Matthew Brost <matthew.brost@intel.com>
Subject: Re: [PATCH 1/4] drm/scheduler: Add drm_sched_cancel_all_jobs helper

Am 06.02.25 um 14:35 schrieb Philipp Stanner:
> On Wed, 2025-02-05 at 15:33 +0000, Tvrtko Ursulin wrote:
>> The helper copies code from the existing
>> amdgpu_job_stop_all_jobs_on_sched with the purpose of reducing the
>> amount of driver code which directly touch scheduler internals.
>>
>> If or when amdgpu manages to change the approach for handling the
>> permanently wedged state this helper can be removed.
> Have you checked how many other drivers might need such a helper?
>
> I have a bit mixed feelings about this, because, AFAICT, in the past
> helpers have been added for just 1 driver, such as
> drm_sched_wqueue_ready(), and then they have stayed for almost a
> decade.
>
> AFAIU this is just code move, and only really "decouples" amdgpu in
> the sense of having an official scheduler function that does what
> amdgpu used to do.
>
> So my tendency here would be to continue "allowing" amdgpu to touch
> the scheduler internals until amdgpu fixes this "permanently wedged
> state". And if that's too difficult, couldn't the helper reside in a
> amdgpu/sched_helpers.c or similar?
>
> I think that's better than adding 1 helper for just 1 driver and then
> supposedly removing it again in the future.

Yeah, agree to that general approach.

What amdgpu does here is kind of nasty and looks unnecessary, but changing it means we need time from Hawkings and his people involved on RAS for amdgpu.

When we move the code to the scheduler we make it official scheduler interface to others to replicate and that is exactly what we should try to avoid.

So my suggestion is to add a /* TODO: This is nasty and should be avoided */ to the amdgpu code instead.

Regards,
Christian.

>
> P.
>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com<mailto:tvrtko.ursulin@igalia.com>>
>> Cc: Christian König <christian.koenig@amd.com<mailto:christian.koenig@amd.com>>
>> Cc: Danilo Krummrich <dakr@kernel.org<mailto:dakr@kernel.org>>
>> Cc: Matthew Brost <matthew.brost@intel.com<mailto:matthew.brost@intel.com>>
>> Cc: Philipp Stanner <phasta@kernel.org<mailto:phasta@kernel.org>>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 44
>> ++++++++++++++++++++++++++
>>   include/drm/gpu_scheduler.h            |  1 +
>>   2 files changed, 45 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index a48be16ab84f..0363655db22d 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -703,6 +703,50 @@ void drm_sched_start(struct drm_gpu_scheduler
>> *sched, int errno)
>>   }
>>   EXPORT_SYMBOL(drm_sched_start);
>>
>> +/**
>> + * drm_sched_cancel_all_jobs - Cancel all queued and scheduled jobs
>> + *
>> + * @sched: scheduler instance
>> + * @errno: error value to set on signaled fences
>> + *
>> + * Signal all queued and scheduled jobs and set them to error state.
>> + *
>> + * Scheduler must be stopped before calling this.
>> + */
>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>> errno)
>> +{
>> +    struct drm_sched_entity *entity;
>> +    struct drm_sched_fence *s_fence;
>> +    struct drm_sched_job *job;
>> +    enum drm_sched_priority p;
>> +
>> +    drm_WARN_ON_ONCE(sched, !sched->pause_submit);
>> +
>> +    /* Signal all jobs not yet scheduled */
>> +    for (p = DRM_SCHED_PRIORITY_KERNEL; p < sched->num_rqs; p++)
>> {
>> +            struct drm_sched_rq *rq = sched->sched_rq[p];
>> +
>> +            spin_lock(&rq->lock);
>> +            list_for_each_entry(entity, &rq->entities, list) {
>> +                    while ((job =
>> to_drm_sched_job(spsc_queue_pop(&entity->job_queue)))) {
>> +                            s_fence = job->s_fence;
>> +                            dma_fence_signal(&s_fence-
>>> scheduled);
>> +                            dma_fence_set_error(&s_fence-
>>> finished, errno);
>> +                            dma_fence_signal(&s_fence-
>>> finished);
>> +                    }
>> +            }
>> +            spin_unlock(&rq->lock);
>> +    }
>> +
>> +    /* Signal all jobs already scheduled to HW */
>> +    list_for_each_entry(job, &sched->pending_list, list) {
>> +            s_fence = job->s_fence;
>> +            dma_fence_set_error(&s_fence->finished, errno);
>> +            dma_fence_signal(&s_fence->finished);
>> +    }
>> +}
>> +EXPORT_SYMBOL(drm_sched_cancel_all_jobs);
>> +
>>   /**
>>    * drm_sched_resubmit_jobs - Deprecated, don't use in new code!
>>    *
>> diff --git a/include/drm/gpu_scheduler.h
>> b/include/drm/gpu_scheduler.h index a0ff08123f07..298513f8c327 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -579,6 +579,7 @@ void drm_sched_wqueue_stop(struct
>> drm_gpu_scheduler *sched);
>>   void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched);
>>   void drm_sched_stop(struct drm_gpu_scheduler *sched, struct
>> drm_sched_job *bad);
>>   void drm_sched_start(struct drm_gpu_scheduler *sched, int errno);
>> +void drm_sched_cancel_all_jobs(struct drm_gpu_scheduler *sched, int
>> errno);
>>   void drm_sched_resubmit_jobs(struct drm_gpu_scheduler *sched);
>>   void drm_sched_increase_karma(struct drm_sched_job *bad);
>>   void drm_sched_reset_karma(struct drm_sched_job *bad);


[-- Attachment #2: Type: text/html, Size: 21474 bytes --]