[PATCH v3 0/3] drm/sched: Job queue peek/pop helpers and struct job re-order

[PATCH v3 0/3] drm/sched: Job queue peek/pop helpers and struct job re-order

[Tvrtko Ursulin]

Lets add some helpers for peeking and popping from the job queue which allows us
to re-order the fields in struct drm_sched_job and remove one hole.

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().

v3:
 * Settle for a copy of __drm_sched_entity_queue_pop in amdgpu for now.

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 (3):
  drm/sched: Add internal job peek/pop API
  drm/amdgpu: Pop jobs from the queue more robustly
  drm/sched: Remove a hole from struct drm_sched_job

 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 22 +++++++++--
 drivers/gpu/drm/scheduler/sched_entity.c   | 11 +++---
 drivers/gpu/drm/scheduler/sched_internal.h | 43 ++++++++++++++++++++++
 drivers/gpu/drm/scheduler/sched_main.c     |  7 ++--
 include/drm/gpu_scheduler.h                | 38 ++++++++++---------
 5 files changed, 90 insertions(+), 31 deletions(-)
 create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h

-- 
2.48.0

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

[Tvrtko Ursulin]

Idea is to add helpers for peeking and popping 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 definitions of to_drm_sched_job.

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     |  7 ++--
 3 files changed, 51 insertions(+), 10 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 a48be16ab84f..43ca98e8db5f 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;
 
-- 
2.48.0

[PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Tvrtko Ursulin]

Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a newly
added __drm_sched_entity_queue_pop.

This allows breaking the hidden dependency that queue_node has to be the
first element in struct drm_sched_job.

A comment is also added with a reference to the mailing list discussion
explaining the copied helper will be removed when the whole broken
amdgpu_job_stop_all_jobs_on_sched is 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>
Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 100f04475943..22cb48bab24d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -411,8 +411,24 @@ 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)
+/*
+ * This is a duplicate function from DRM scheduler sched_internal.h.
+ * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is removed, due
+ * latter being incorrect and racy.
+ *
+ * See https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
+ */
+static struct drm_sched_job *
+__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
+{
+	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);
+}
 
 void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
 {
@@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
 		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)))) {
+			while ((s_job = __drm_sched_entity_queue_pop(s_entity))) {
 				struct drm_sched_fence *s_fence = s_job->s_fence;
 
 				dma_fence_signal(&s_fence->scheduled);
-- 
2.48.0

[PATCH 3/3] drm/sched: Remove a hole from struct drm_sched_job

[Tvrtko Ursulin]

We can re-order some struct members and take u32 credits outside of the
pointer sandwich and also for the last_dependency member we can get away
with an unsigned int since for dependency we use xa_limit_32b.

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 a0ff08123f07..68da3dec8dba 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/3] drm/sched: Add internal job peek/pop API

[Danilo Krummrich]

On Thu, Feb 06, 2025 at 04:40:29PM +0000, Tvrtko Ursulin wrote:
> Idea is to add helpers for peeking and popping 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 definitions of to_drm_sched_job.
> 
> 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     |  7 ++--
>  3 files changed, 51 insertions(+), 10 deletions(-)
>  create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h
> 
> 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

Why the '__' prefix?

Could also use this to replace spsc_queue_pop() in drm_sched_entity_pop_job().

> + *
> + * @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

Same here.

Could also use this for drm_sched_entity_is_ready().

> + *
> + * @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 a48be16ab84f..43ca98e8db5f 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;
>  
> -- 
> 2.48.0
> 

Re: [PATCH v3 0/3] drm/sched: Job queue peek/pop helpers and struct job re-order

[Danilo Krummrich]

On 2/6/25 5:40 PM, Tvrtko Ursulin wrote:
> Lets add some helpers for peeking and popping from the job queue which allows us
> to re-order the fields in struct drm_sched_job and remove one hole.

I think you forgot to add the dri-devel list.

Can't fetch patches with b4. :(

> 
> 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().
> 
> v3:
>   * Settle for a copy of __drm_sched_entity_queue_pop in amdgpu for now.
> 
> 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 (3):
>    drm/sched: Add internal job peek/pop API
>    drm/amdgpu: Pop jobs from the queue more robustly
>    drm/sched: Remove a hole from struct drm_sched_job
> 
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 22 +++++++++--
>   drivers/gpu/drm/scheduler/sched_entity.c   | 11 +++---
>   drivers/gpu/drm/scheduler/sched_internal.h | 43 ++++++++++++++++++++++
>   drivers/gpu/drm/scheduler/sched_main.c     |  7 ++--
>   include/drm/gpu_scheduler.h                | 38 ++++++++++---------
>   5 files changed, 90 insertions(+), 31 deletions(-)
>   create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h
> 

Re: [PATCH 3/3] drm/sched: Remove a hole from struct drm_sched_job

[Danilo Krummrich]

On Thu, Feb 06, 2025 at 04:40:31PM +0000, Tvrtko Ursulin wrote:
> We can re-order some struct members and take u32 credits outside of the
> pointer sandwich and also for the last_dependency member we can get away
> with an unsigned int since for dependency we use xa_limit_32b.
> 
> 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 a0ff08123f07..68da3dec8dba 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;

I usually prefer to leave those things alone, but since you change most of this
struct anyways...

With or without this diff:

Acked-by: Danilo Krummrich <dakr@kernel.org>

>  	};
>  
> -	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/3] drm/sched: Add internal job peek/pop API

[Tvrtko Ursulin]

On 06/02/2025 16:54, Danilo Krummrich wrote:
> On Thu, Feb 06, 2025 at 04:40:29PM +0000, Tvrtko Ursulin wrote:
>> Idea is to add helpers for peeking and popping 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 definitions of to_drm_sched_job.
>>
>> 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     |  7 ++--
>>   3 files changed, 51 insertions(+), 10 deletions(-)
>>   create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h
>>
>> 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
> 
> Why the '__' prefix?

True, it is a remnant from v1 when I had it in gpu_scheduler.h because 
then double underscores were supposed to signal this is "special" 
(internal) API. After adding sched_internal.h it makes no more sense. I 
will drop them.

Speaking of sched_internal.h, as a follow up we could also move a bunch 
of other prototypes from gpu_scheduler.h in there.

> Could also use this to replace spsc_queue_pop() in drm_sched_entity_pop_job().

We could but as that caller does not want/need the job returned maybe 
okay not to. Open to change if you insist.

>> + *
>> + * @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
> 
> Same here.
> 
> Could also use this for drm_sched_entity_is_ready().

Again we could but that one does not want a job but just a does a "queue 
not empty" check. Could replace also with spsc_queue_count() to make 
that clearer.

Regards,

Tvrtko

>> + *
>> + * @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 a48be16ab84f..43ca98e8db5f 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;
>>   
>> -- 
>> 2.48.0
>>

Re: [PATCH v3 0/3] drm/sched: Job queue peek/pop helpers and struct job re-order

[Tvrtko Ursulin]

On 06/02/2025 16:58, Danilo Krummrich wrote:
> On 2/6/25 5:40 PM, Tvrtko Ursulin wrote:
>> Lets add some helpers for peeking and popping from the job queue which 
>> allows us
>> to re-order the fields in struct drm_sched_job and remove one hole.
> 
> I think you forgot to add the dri-devel list.
> 
> Can't fetch patches with b4. :(

Hmm I probably blundered with git send-email --identity. Will re-send 
after we close on opens from 1/3.

Regards,

Tvrtko

>> 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().
>>
>> v3:
>>   * Settle for a copy of __drm_sched_entity_queue_pop in amdgpu for now.
>>
>> 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 (3):
>>    drm/sched: Add internal job peek/pop API
>>    drm/amdgpu: Pop jobs from the queue more robustly
>>    drm/sched: Remove a hole from struct drm_sched_job
>>
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 22 +++++++++--
>>   drivers/gpu/drm/scheduler/sched_entity.c   | 11 +++---
>>   drivers/gpu/drm/scheduler/sched_internal.h | 43 ++++++++++++++++++++++
>>   drivers/gpu/drm/scheduler/sched_main.c     |  7 ++--
>>   include/drm/gpu_scheduler.h                | 38 ++++++++++---------
>>   5 files changed, 90 insertions(+), 31 deletions(-)
>>   create mode 100644 drivers/gpu/drm/scheduler/sched_internal.h
>>
> 

Re: [PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Christian König]

Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
> Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a newly
> added __drm_sched_entity_queue_pop.
>
> This allows breaking the hidden dependency that queue_node has to be the
> first element in struct drm_sched_job.
>
> A comment is also added with a reference to the mailing list discussion
> explaining the copied helper will be removed when the whole broken
> amdgpu_job_stop_all_jobs_on_sched is 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>
> Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>

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

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 100f04475943..22cb48bab24d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -411,8 +411,24 @@ 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)
> +/*
> + * This is a duplicate function from DRM scheduler sched_internal.h.
> + * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is removed, due
> + * latter being incorrect and racy.
> + *
> + * See https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
> + */
> +static struct drm_sched_job *
> +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
> +{
> +	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);
> +}
>   
>   void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
>   {
> @@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler *sched)
>   		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)))) {
> +			while ((s_job = __drm_sched_entity_queue_pop(s_entity))) {
>   				struct drm_sched_fence *s_fence = s_job->s_fence;
>   
>   				dma_fence_signal(&s_fence->scheduled);

Re: [PATCH 3/3] drm/sched: Remove a hole from struct drm_sched_job

[Christian König]

Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
> We can re-order some struct members and take u32 credits outside of the
> pointer sandwich and also for the last_dependency member we can get away
> with an unsigned int since for dependency we use xa_limit_32b.
>
> 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 a0ff08123f07..68da3dec8dba 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;

Not actually part of this patch here, but I think we should remove this 
id field and rather always use the context/sequence number pair of the 
scheduler fence to identify a submission.

The patch itself is Acked-by: Christian König <christian.koenig@amd.com>.

Regards,
Christian.

> +
> +	/**
> +	 * @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,

Re: [PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Philipp Stanner]

On Tue, 2025-02-11 at 09:22 +0100, Christian König wrote:
> Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
> > Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a
> > newly
> > added __drm_sched_entity_queue_pop.
> > 
> > This allows breaking the hidden dependency that queue_node has to
> > be the
> > first element in struct drm_sched_job.
> > 
> > A comment is also added with a reference to the mailing list
> > discussion
> > explaining the copied helper will be removed when the whole broken
> > amdgpu_job_stop_all_jobs_on_sched is 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>
> > Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>
> 
> Reviewed-by: Christian König <christian.koenig@amd.com>

I think this v3 has been supplanted by a v4 by now.

@Tvrtko: btw, do you create patches with
git format-patch -v4 ?

That way the v4 label will be included in all patch titles, too, not
just the cover letter. That makes searching etc. easier in large
inboxes

P.

> 
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++-
> > --
> >   1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > index 100f04475943..22cb48bab24d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> > @@ -411,8 +411,24 @@ 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)
> > +/*
> > + * This is a duplicate function from DRM scheduler
> > sched_internal.h.
> > + * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is
> > removed, due
> > + * latter being incorrect and racy.
> > + *
> > + * See
> > https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
> > + */
> > +static struct drm_sched_job *
> > +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
> > +{
> > +	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);
> > +}
> >   
> >   void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler
> > *sched)
> >   {
> > @@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct
> > drm_gpu_scheduler *sched)
> >   		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)))) {
> > +			while ((s_job =
> > __drm_sched_entity_queue_pop(s_entity))) {
> >   				struct drm_sched_fence *s_fence =
> > s_job->s_fence;
> >   
> >   				dma_fence_signal(&s_fence-
> > >scheduled);
> 

Re: [PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Christian König]

Am 11.02.25 um 11:08 schrieb Philipp Stanner:
> On Tue, 2025-02-11 at 09:22 +0100, Christian König wrote:
>> Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
>>> Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a
>>> newly
>>> added __drm_sched_entity_queue_pop.
>>>
>>> This allows breaking the hidden dependency that queue_node has to
>>> be the
>>> first element in struct drm_sched_job.
>>>
>>> A comment is also added with a reference to the mailing list
>>> discussion
>>> explaining the copied helper will be removed when the whole broken
>>> amdgpu_job_stop_all_jobs_on_sched is 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>
>>> Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
> I think this v3 has been supplanted by a v4 by now.

I've seen the larger v4 series as well, but at least that patch here 
looks identical on first glance. So my rb still counts.

Christian.

>
> @Tvrtko: btw, do you create patches with
> git format-patch -v4 ?
>
> That way the v4 label will be included in all patch titles, too, not
> just the cover letter. That makes searching etc. easier in large
> inboxes
>
> P.
>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++-
>>> --
>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> index 100f04475943..22cb48bab24d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>> @@ -411,8 +411,24 @@ 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)
>>> +/*
>>> + * This is a duplicate function from DRM scheduler
>>> sched_internal.h.
>>> + * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is
>>> removed, due
>>> + * latter being incorrect and racy.
>>> + *
>>> + * See
>>> https://lore.kernel.org/amd-gfx/44edde63-7181-44fb-a4f7-94e50514f539@amd.com/
>>> + */
>>> +static struct drm_sched_job *
>>> +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>>> +{
>>> +	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);
>>> +}
>>>    
>>>    void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler
>>> *sched)
>>>    {
>>> @@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct
>>> drm_gpu_scheduler *sched)
>>>    		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)))) {
>>> +			while ((s_job =
>>> __drm_sched_entity_queue_pop(s_entity))) {
>>>    				struct drm_sched_fence *s_fence =
>>> s_job->s_fence;
>>>    
>>>    				dma_fence_signal(&s_fence-
>>>> scheduled);

Re: [PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Tvrtko Ursulin]

On 11/02/2025 10:21, Christian König wrote:
> Am 11.02.25 um 11:08 schrieb Philipp Stanner:
>> On Tue, 2025-02-11 at 09:22 +0100, Christian König wrote:
>>> Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
>>>> Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a
>>>> newly
>>>> added __drm_sched_entity_queue_pop.
>>>>
>>>> This allows breaking the hidden dependency that queue_node has to
>>>> be the
>>>> first element in struct drm_sched_job.
>>>>
>>>> A comment is also added with a reference to the mailing list
>>>> discussion
>>>> explaining the copied helper will be removed when the whole broken
>>>> amdgpu_job_stop_all_jobs_on_sched is 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>
>>>> Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> I think this v3 has been supplanted by a v4 by now.
> 
> I've seen the larger v4 series as well, but at least that patch here 
> looks identical on first glance. So my rb still counts.

Effectively identical - I only removed the double underscore in v4 on a 
(good) suggestion from Danilo.

>> @Tvrtko: btw, do you create patches with
>> git format-patch -v4 ?
>>
>> That way the v4 label will be included in all patch titles, too, not
>> just the cover letter. That makes searching etc. easier in large
>> inboxes

I don't typically use -vN for the whole series since not all patches in 
the series change equally and I assume people use threaded views so vN 
in the root is sufficient.

Regards,

Tvrtko

>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++-
>>>> -- 
>>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> index 100f04475943..22cb48bab24d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> @@ -411,8 +411,24 @@ 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)
>>>> +/*
>>>> + * This is a duplicate function from DRM scheduler
>>>> sched_internal.h.
>>>> + * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is
>>>> removed, due
>>>> + * latter being incorrect and racy.
>>>> + *
>>>> + * See
>>>> https://lore.kernel.org/amd-gfx/44edde63-7181-44fb- 
>>>> a4f7-94e50514f539@amd.com/
>>>> + */
>>>> +static struct drm_sched_job *
>>>> +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>>>> +{
>>>> +    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);
>>>> +}
>>>>    void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler
>>>> *sched)
>>>>    {
>>>> @@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct
>>>> drm_gpu_scheduler *sched)
>>>>            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)))) {
>>>> +            while ((s_job =
>>>> __drm_sched_entity_queue_pop(s_entity))) {
>>>>                    struct drm_sched_fence *s_fence =
>>>> s_job->s_fence;
>>>>                    dma_fence_signal(&s_fence-
>>>>> scheduled);
> 

Re: [PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Tvrtko Ursulin]

Hi Christian,

On 11/02/2025 10:21, Christian König wrote:
> Am 11.02.25 um 11:08 schrieb Philipp Stanner:
>> On Tue, 2025-02-11 at 09:22 +0100, Christian König wrote:
>>> Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
>>>> Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a
>>>> newly
>>>> added __drm_sched_entity_queue_pop.
>>>>
>>>> This allows breaking the hidden dependency that queue_node has to
>>>> be the
>>>> first element in struct drm_sched_job.
>>>>
>>>> A comment is also added with a reference to the mailing list
>>>> discussion
>>>> explaining the copied helper will be removed when the whole broken
>>>> amdgpu_job_stop_all_jobs_on_sched is 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>
>>>> Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>> I think this v3 has been supplanted by a v4 by now.
> 
> I've seen the larger v4 series as well, but at least that patch here 
> looks identical on first glance. So my rb still counts.

Is it okay for you to merge the whole series (including this single 
amdgpu patch) via drm-misc?

Regards,

Tvrtko

>> @Tvrtko: btw, do you create patches with
>> git format-patch -v4 ?
>>
>> That way the v4 label will be included in all patch titles, too, not
>> just the cover letter. That makes searching etc. easier in large
>> inboxes
>>
>> P.
>>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++-
>>>> -- 
>>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> index 100f04475943..22cb48bab24d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>> @@ -411,8 +411,24 @@ 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)
>>>> +/*
>>>> + * This is a duplicate function from DRM scheduler
>>>> sched_internal.h.
>>>> + * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is
>>>> removed, due
>>>> + * latter being incorrect and racy.
>>>> + *
>>>> + * See
>>>> https://lore.kernel.org/amd-gfx/44edde63-7181-44fb- 
>>>> a4f7-94e50514f539@amd.com/
>>>> + */
>>>> +static struct drm_sched_job *
>>>> +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>>>> +{
>>>> +    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);
>>>> +}
>>>>    void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler
>>>> *sched)
>>>>    {
>>>> @@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct
>>>> drm_gpu_scheduler *sched)
>>>>            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)))) {
>>>> +            while ((s_job =
>>>> __drm_sched_entity_queue_pop(s_entity))) {
>>>>                    struct drm_sched_fence *s_fence =
>>>> s_job->s_fence;
>>>>                    dma_fence_signal(&s_fence-
>>>>> scheduled);
> 

Re: [PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Christian König]

Am 14.02.25 um 11:21 schrieb Tvrtko Ursulin:
>
> Hi Christian,
>
> On 11/02/2025 10:21, Christian König wrote:
>> Am 11.02.25 um 11:08 schrieb Philipp Stanner:
>>> On Tue, 2025-02-11 at 09:22 +0100, Christian König wrote:
>>>> Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
>>>>> Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a
>>>>> newly
>>>>> added __drm_sched_entity_queue_pop.
>>>>>
>>>>> This allows breaking the hidden dependency that queue_node has to
>>>>> be the
>>>>> first element in struct drm_sched_job.
>>>>>
>>>>> A comment is also added with a reference to the mailing list
>>>>> discussion
>>>>> explaining the copied helper will be removed when the whole broken
>>>>> amdgpu_job_stop_all_jobs_on_sched is 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>
>>>>> Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>
>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>> I think this v3 has been supplanted by a v4 by now.
>>
>> I've seen the larger v4 series as well, but at least that patch here looks identical on first glance. So my rb still counts.
>
> Is it okay for you to merge the whole series (including this single amdgpu patch) via drm-misc?

I can do that, but don't want the scheduler maintainer want to pick them up?

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>> @Tvrtko: btw, do you create patches with
>>> git format-patch -v4 ?
>>>
>>> That way the v4 label will be included in all patch titles, too, not
>>> just the cover letter. That makes searching etc. easier in large
>>> inboxes
>>>
>>> P.
>>>
>>>>> ---
>>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++-
>>>>> -- 
>>>>>    1 file changed, 19 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> index 100f04475943..22cb48bab24d 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>> @@ -411,8 +411,24 @@ 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)
>>>>> +/*
>>>>> + * This is a duplicate function from DRM scheduler
>>>>> sched_internal.h.
>>>>> + * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is
>>>>> removed, due
>>>>> + * latter being incorrect and racy.
>>>>> + *
>>>>> + * See
>>>>> https://lore.kernel.org/amd-gfx/44edde63-7181-44fb- a4f7-94e50514f539@amd.com/
>>>>> + */
>>>>> +static struct drm_sched_job *
>>>>> +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>>>>> +{
>>>>> +    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);
>>>>> +}
>>>>>    void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler
>>>>> *sched)
>>>>>    {
>>>>> @@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct
>>>>> drm_gpu_scheduler *sched)
>>>>>            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)))) {
>>>>> +            while ((s_job =
>>>>> __drm_sched_entity_queue_pop(s_entity))) {
>>>>>                    struct drm_sched_fence *s_fence =
>>>>> s_job->s_fence;
>>>>>                    dma_fence_signal(&s_fence-
>>>>>> scheduled);
>>
>

Re: [PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Tvrtko Ursulin]

On 14/02/2025 10:31, Christian König wrote:
> Am 14.02.25 um 11:21 schrieb Tvrtko Ursulin:
>>
>> Hi Christian,
>>
>> On 11/02/2025 10:21, Christian König wrote:
>>> Am 11.02.25 um 11:08 schrieb Philipp Stanner:
>>>> On Tue, 2025-02-11 at 09:22 +0100, Christian König wrote:
>>>>> Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
>>>>>> Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a
>>>>>> newly
>>>>>> added __drm_sched_entity_queue_pop.
>>>>>>
>>>>>> This allows breaking the hidden dependency that queue_node has to
>>>>>> be the
>>>>>> first element in struct drm_sched_job.
>>>>>>
>>>>>> A comment is also added with a reference to the mailing list
>>>>>> discussion
>>>>>> explaining the copied helper will be removed when the whole broken
>>>>>> amdgpu_job_stop_all_jobs_on_sched is 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>
>>>>>> Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>
>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>> I think this v3 has been supplanted by a v4 by now.
>>>
>>> I've seen the larger v4 series as well, but at least that patch here looks identical on first glance. So my rb still counts.
>>
>> Is it okay for you to merge the whole series (including this single amdgpu patch) via drm-misc?
> 
> I can do that, but don't want the scheduler maintainer want to pick them up?

Sorry that was some bad and unclear English. :(

It is as you suggest - what I meant was, is it okay from your point of 
view that the whole series is merged via drm-misc? I assume Philipp 
would indeed be the one to merge it, once all patches get r-b-ed.

Regards,

Tvrtko

>>>> @Tvrtko: btw, do you create patches with
>>>> git format-patch -v4 ?
>>>>
>>>> That way the v4 label will be included in all patch titles, too, not
>>>> just the cover letter. That makes searching etc. easier in large
>>>> inboxes
>>>>
>>>> P.
>>>>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++-
>>>>>> -- 
>>>>>>     1 file changed, 19 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> index 100f04475943..22cb48bab24d 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>> @@ -411,8 +411,24 @@ 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)
>>>>>> +/*
>>>>>> + * This is a duplicate function from DRM scheduler
>>>>>> sched_internal.h.
>>>>>> + * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is
>>>>>> removed, due
>>>>>> + * latter being incorrect and racy.
>>>>>> + *
>>>>>> + * See
>>>>>> https://lore.kernel.org/amd-gfx/44edde63-7181-44fb- a4f7-94e50514f539@amd.com/
>>>>>> + */
>>>>>> +static struct drm_sched_job *
>>>>>> +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>>>>>> +{
>>>>>> +    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);
>>>>>> +}
>>>>>>     void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler
>>>>>> *sched)
>>>>>>     {
>>>>>> @@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct
>>>>>> drm_gpu_scheduler *sched)
>>>>>>             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)))) {
>>>>>> +            while ((s_job =
>>>>>> __drm_sched_entity_queue_pop(s_entity))) {
>>>>>>                     struct drm_sched_fence *s_fence =
>>>>>> s_job->s_fence;
>>>>>>                     dma_fence_signal(&s_fence-
>>>>>>> scheduled);
>>>
>>
> 

Re: [PATCH 2/3] drm/amdgpu: Pop jobs from the queue more robustly

[Christian König]

Am 14.02.25 um 11:34 schrieb Tvrtko Ursulin:
>
> On 14/02/2025 10:31, Christian König wrote:
>> Am 14.02.25 um 11:21 schrieb Tvrtko Ursulin:
>>>
>>> Hi Christian,
>>>
>>> On 11/02/2025 10:21, Christian König wrote:
>>>> Am 11.02.25 um 11:08 schrieb Philipp Stanner:
>>>>> On Tue, 2025-02-11 at 09:22 +0100, Christian König wrote:
>>>>>> Am 06.02.25 um 17:40 schrieb Tvrtko Ursulin:
>>>>>>> Replace a copy of DRM scheduler's to_drm_sched_job with a copy of a
>>>>>>> newly
>>>>>>> added __drm_sched_entity_queue_pop.
>>>>>>>
>>>>>>> This allows breaking the hidden dependency that queue_node has to
>>>>>>> be the
>>>>>>> first element in struct drm_sched_job.
>>>>>>>
>>>>>>> A comment is also added with a reference to the mailing list
>>>>>>> discussion
>>>>>>> explaining the copied helper will be removed when the whole broken
>>>>>>> amdgpu_job_stop_all_jobs_on_sched is 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>
>>>>>>> Cc: "Zhang, Hawking" <Hawking.Zhang@amd.com>
>>>>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>>> I think this v3 has been supplanted by a v4 by now.
>>>>
>>>> I've seen the larger v4 series as well, but at least that patch here looks identical on first glance. So my rb still counts.
>>>
>>> Is it okay for you to merge the whole series (including this single amdgpu patch) via drm-misc?
>>
>> I can do that, but don't want the scheduler maintainer want to pick them up?
>
> Sorry that was some bad and unclear English. :(

Don't worry, I'm not a native speaker either and had only very minimal formal education on it :)

>
> It is as you suggest - what I meant was, is it okay from your point of view that the whole series is merged via drm-misc? I assume Philipp would indeed be the one to merge it, once all patches get r-b-ed.

Ah! Yes of course it. Feel free to go ahead.

Could only be that Alex runs into merge issues, but that is extremely unlikely I think.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>>>> @Tvrtko: btw, do you create patches with
>>>>> git format-patch -v4 ?
>>>>>
>>>>> That way the v4 label will be included in all patch titles, too, not
>>>>> just the cover letter. That makes searching etc. easier in large
>>>>> inboxes
>>>>>
>>>>> P.
>>>>>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 22 +++++++++++++++++++-
>>>>>>> -- 
>>>>>>>     1 file changed, 19 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>> index 100f04475943..22cb48bab24d 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
>>>>>>> @@ -411,8 +411,24 @@ 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)
>>>>>>> +/*
>>>>>>> + * This is a duplicate function from DRM scheduler
>>>>>>> sched_internal.h.
>>>>>>> + * Plan is to remove it when amdgpu_job_stop_all_jobs_on_sched is
>>>>>>> removed, due
>>>>>>> + * latter being incorrect and racy.
>>>>>>> + *
>>>>>>> + * See
>>>>>>> https://lore.kernel.org/amd-gfx/44edde63-7181-44fb- a4f7-94e50514f539@amd.com/
>>>>>>> + */
>>>>>>> +static struct drm_sched_job *
>>>>>>> +__drm_sched_entity_queue_pop(struct drm_sched_entity *entity)
>>>>>>> +{
>>>>>>> +    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);
>>>>>>> +}
>>>>>>>     void amdgpu_job_stop_all_jobs_on_sched(struct drm_gpu_scheduler
>>>>>>> *sched)
>>>>>>>     {
>>>>>>> @@ -425,7 +441,7 @@ void amdgpu_job_stop_all_jobs_on_sched(struct
>>>>>>> drm_gpu_scheduler *sched)
>>>>>>>             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)))) {
>>>>>>> +            while ((s_job =
>>>>>>> __drm_sched_entity_queue_pop(s_entity))) {
>>>>>>>                     struct drm_sched_fence *s_fence =
>>>>>>> s_job->s_fence;
>>>>>>>                     dma_fence_signal(&s_fence-
>>>>>>>> scheduled);
>>>>
>>>
>>
>