[PATCH v3 0/7] Fix DRM scheduler layering violations in Xe

[PATCH v3 0/7] Fix DRM scheduler layering violations in Xe

[Matthew Brost]

At XDC, we discussed that drivers should avoid accessing DRM scheduler
internals, misusing DRM scheduler locks, and adopt a well-defined
pending job list iterator. This series proposes the necessary changes to
the DRM scheduler to bring Xe in line with that agreement and updates Xe
to use the new DRM scheduler API.

While here, cleanup LR queue handling in Xe too.

v2:
 - Fix checkpatch / naming issues
v3:
 - Only allow pending job list iterator to be called on stopped schedulers
 - Cleanup LR queue handling / fix a few misselanous Xe scheduler issues

Matt

Matthew Brost (7):
  drm/sched: Add pending job list iterator
  drm/sched: Add several job helpers to avoid drivers touching scheduler
    state
  drm/xe: Add dedicated message lock
  drm/xe: Stop abusing DRM scheduler internals
  drm/xe: Do not deregister queues in TDR
  drm/xe: Remove special casing for LR queues in submission
  drm/xe: Only toggle scheduling in TDR if GuC is running

 drivers/gpu/drm/scheduler/sched_main.c       |   4 +-
 drivers/gpu/drm/xe/xe_gpu_scheduler.c        |   9 +-
 drivers/gpu/drm/xe/xe_gpu_scheduler.h        |  38 +--
 drivers/gpu/drm/xe/xe_gpu_scheduler_types.h  |   2 +
 drivers/gpu/drm/xe/xe_guc_exec_queue_types.h |   2 -
 drivers/gpu/drm/xe/xe_guc_submit.c           | 252 ++-----------------
 drivers/gpu/drm/xe/xe_guc_submit_types.h     |  11 -
 drivers/gpu/drm/xe/xe_hw_fence.c             |  16 --
 drivers/gpu/drm/xe/xe_hw_fence.h             |   2 -
 include/drm/gpu_scheduler.h                  |  80 ++++++
 10 files changed, 117 insertions(+), 299 deletions(-)

-- 
2.34.1

[PATCH v3 1/7] drm/sched: Add pending job list iterator

[Matthew Brost]

Stop open coding pending job list in drivers. Add pending job list
iterator which safely walks DRM scheduler list asserting DRM scheduler
is stopped.

v2:
 - Fix checkpatch (CI)
v3:
 - Drop locked version (Christian)

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 include/drm/gpu_scheduler.h | 52 +++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index fb88301b3c45..7f31eba3bd61 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -698,4 +698,56 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
 				   struct drm_gpu_scheduler **sched_list,
 				   unsigned int num_sched_list);
 
+/* Inlines */
+
+/**
+ * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
+ * @sched: DRM scheduler associated with pending job iterator
+ */
+struct drm_sched_pending_job_iter {
+	struct drm_gpu_scheduler *sched;
+};
+
+/* Drivers should never call this directly */
+static inline struct drm_sched_pending_job_iter
+__drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
+{
+	struct drm_sched_pending_job_iter iter = {
+		.sched = sched,
+	};
+
+	WARN_ON(!READ_ONCE(sched->pause_submit));
+	return iter;
+}
+
+/* Drivers should never call this directly */
+static inline void
+__drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
+{
+	WARN_ON(!READ_ONCE(iter.sched->pause_submit));
+}
+
+DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
+	     __drm_sched_pending_job_iter_end(_T),
+	     __drm_sched_pending_job_iter_begin(__sched),
+	     struct drm_gpu_scheduler *__sched);
+static inline void *
+class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T)
+{ return _T; }
+#define class_drm_sched_pending_job_iter_is_conditional false
+
+/**
+ * drm_sched_for_each_pending_job() - Iterator for each pending job in scheduler
+ * @__job: Current pending job being iterated over
+ * @__sched: DRM scheduler to iterate over pending jobs
+ * @__entity: DRM scheduler entity to filter jobs, NULL indicates no filter
+ *
+ * Iterator for each pending job in scheduler, filtering on an entity, and
+ * enforcing scheduler is fully stopped
+ */
+#define drm_sched_for_each_pending_job(__job, __sched, __entity)		\
+	scoped_guard(drm_sched_pending_job_iter, (__sched))			\
+		list_for_each_entry((__job), &(__sched)->pending_list, list)	\
+			for_each_if(!(__entity) || (__job)->entity == (__entity))
+
 #endif
-- 
2.34.1

[PATCH v3 2/7] drm/sched: Add several job helpers to avoid drivers touching scheduler state

[Matthew Brost]

Add helpers to see if scheduler is stopped and a jobs signaled state.
Expected to be used driver side on recovery and debug flows.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/scheduler/sched_main.c |  4 ++--
 include/drm/gpu_scheduler.h            | 32 ++++++++++++++++++++++++--
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 46119aacb809..69bd6e482268 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -344,7 +344,7 @@ drm_sched_rq_select_entity_fifo(struct drm_gpu_scheduler *sched,
  */
 static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
 {
-	if (!READ_ONCE(sched->pause_submit))
+	if (!drm_sched_is_stopped(sched))
 		queue_work(sched->submit_wq, &sched->work_run_job);
 }
 
@@ -354,7 +354,7 @@ static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
  */
 static void drm_sched_run_free_queue(struct drm_gpu_scheduler *sched)
 {
-	if (!READ_ONCE(sched->pause_submit))
+	if (!drm_sched_is_stopped(sched))
 		queue_work(sched->submit_wq, &sched->work_free_job);
 }
 
diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
index 7f31eba3bd61..d1a2d7f61c1d 100644
--- a/include/drm/gpu_scheduler.h
+++ b/include/drm/gpu_scheduler.h
@@ -700,6 +700,17 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
 
 /* Inlines */
 
+/**
+ * drm_sched_is_stopped() - DRM is stopped
+ * @sched: DRM scheduler
+ *
+ * Return: True if sched is stopped, False otherwise
+ */
+static inline bool drm_sched_is_stopped(struct drm_gpu_scheduler *sched)
+{
+	return READ_ONCE(sched->pause_submit);
+}
+
 /**
  * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
  * @sched: DRM scheduler associated with pending job iterator
@@ -716,7 +727,7 @@ __drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
 		.sched = sched,
 	};
 
-	WARN_ON(!READ_ONCE(sched->pause_submit));
+	WARN_ON(!drm_sched_is_stopped(sched));
 	return iter;
 }
 
@@ -724,7 +735,7 @@ __drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
 static inline void
 __drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
 {
-	WARN_ON(!READ_ONCE(iter.sched->pause_submit));
+	WARN_ON(!drm_sched_is_stopped(iter.sched));
 }
 
 DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
@@ -750,4 +761,21 @@ class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T
 		list_for_each_entry((__job), &(__sched)->pending_list, list)	\
 			for_each_if(!(__entity) || (__job)->entity == (__entity))
 
+/**
+ * drm_sched_job_is_signaled() - DRM scheduler job is signaled
+ * @job: DRM scheduler job
+ *
+ * Determine if DRM scheduler job is signaled. DRM scheduler should be stopped
+ * to obtain a stable snapshot of state.
+ *
+ * Return: True if job is signaled, False otherwise
+ */
+static inline bool drm_sched_job_is_signaled(struct drm_sched_job *job)
+{
+	struct drm_sched_fence *s_fence = job->s_fence;
+
+	WARN_ON(!drm_sched_is_stopped(job->sched));
+	return dma_fence_is_signaled(&s_fence->finished);
+}
+
 #endif
-- 
2.34.1

[PATCH v3 3/7] drm/xe: Add dedicated message lock

[Matthew Brost]

Stop abusing DRM scheduler job list lock for messages, add dedicated
message lock.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_gpu_scheduler.c       | 5 +++--
 drivers/gpu/drm/xe/xe_gpu_scheduler.h       | 4 ++--
 drivers/gpu/drm/xe/xe_gpu_scheduler_types.h | 2 ++
 3 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
index f91e06d03511..f4f23317191f 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
@@ -77,6 +77,7 @@ int xe_sched_init(struct xe_gpu_scheduler *sched,
 	};
 
 	sched->ops = xe_ops;
+	spin_lock_init(&sched->msg_lock);
 	INIT_LIST_HEAD(&sched->msgs);
 	INIT_WORK(&sched->work_process_msg, xe_sched_process_msg_work);
 
@@ -117,7 +118,7 @@ void xe_sched_add_msg(struct xe_gpu_scheduler *sched,
 void xe_sched_add_msg_locked(struct xe_gpu_scheduler *sched,
 			     struct xe_sched_msg *msg)
 {
-	lockdep_assert_held(&sched->base.job_list_lock);
+	lockdep_assert_held(&sched->msg_lock);
 
 	list_add_tail(&msg->link, &sched->msgs);
 	xe_sched_process_msg_queue(sched);
@@ -131,7 +132,7 @@ void xe_sched_add_msg_locked(struct xe_gpu_scheduler *sched,
 void xe_sched_add_msg_head(struct xe_gpu_scheduler *sched,
 			   struct xe_sched_msg *msg)
 {
-	lockdep_assert_held(&sched->base.job_list_lock);
+	lockdep_assert_held(&sched->msg_lock);
 
 	list_add(&msg->link, &sched->msgs);
 	xe_sched_process_msg_queue(sched);
diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
index 9955397aaaa9..b971b6b69419 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
@@ -33,12 +33,12 @@ void xe_sched_add_msg_head(struct xe_gpu_scheduler *sched,
 
 static inline void xe_sched_msg_lock(struct xe_gpu_scheduler *sched)
 {
-	spin_lock(&sched->base.job_list_lock);
+	spin_lock(&sched->msg_lock);
 }
 
 static inline void xe_sched_msg_unlock(struct xe_gpu_scheduler *sched)
 {
-	spin_unlock(&sched->base.job_list_lock);
+	spin_unlock(&sched->msg_lock);
 }
 
 static inline void xe_sched_stop(struct xe_gpu_scheduler *sched)
diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
index 6731b13da8bb..63d9bf92583c 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
@@ -47,6 +47,8 @@ struct xe_gpu_scheduler {
 	const struct xe_sched_backend_ops	*ops;
 	/** @msgs: list of messages to be processed in @work_process_msg */
 	struct list_head			msgs;
+	/** @msg_lock: Message lock */
+	spinlock_t				msg_lock;
 	/** @work_process_msg: processes messages */
 	struct work_struct		work_process_msg;
 };
-- 
2.34.1

[PATCH v3 4/7] drm/xe: Stop abusing DRM scheduler internals

[Matthew Brost]

Use new pending job list iterator and new helper functions in Xe to
avoid reaching into DRM scheduler internals.

Part of this change involves removing pending jobs debug information
from debugfs and devcoredump. As agreed, the pending job list should
only be accessed when the scheduler is stopped. However, it's not
straightforward to determine whether the scheduler is stopped from the
shared debugfs/devcoredump code path. Additionally, the pending job list
provides little useful information, as pending jobs can be inferred from
seqnos and ring head/tail positions. Therefore, this debug information
is being removed.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_gpu_scheduler.c    |  4 +-
 drivers/gpu/drm/xe/xe_gpu_scheduler.h    | 34 +++--------
 drivers/gpu/drm/xe/xe_guc_submit.c       | 74 ++++--------------------
 drivers/gpu/drm/xe/xe_guc_submit_types.h | 11 ----
 drivers/gpu/drm/xe/xe_hw_fence.c         | 16 -----
 drivers/gpu/drm/xe/xe_hw_fence.h         |  2 -
 6 files changed, 20 insertions(+), 121 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
index f4f23317191f..9c8004d5dd91 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
@@ -7,7 +7,7 @@
 
 static void xe_sched_process_msg_queue(struct xe_gpu_scheduler *sched)
 {
-	if (!READ_ONCE(sched->base.pause_submit))
+	if (!drm_sched_is_stopped(&sched->base))
 		queue_work(sched->base.submit_wq, &sched->work_process_msg);
 }
 
@@ -43,7 +43,7 @@ static void xe_sched_process_msg_work(struct work_struct *w)
 		container_of(w, struct xe_gpu_scheduler, work_process_msg);
 	struct xe_sched_msg *msg;
 
-	if (READ_ONCE(sched->base.pause_submit))
+	if (drm_sched_is_stopped(&sched->base))
 		return;
 
 	msg = xe_sched_get_msg(sched);
diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
index b971b6b69419..583372a78140 100644
--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
@@ -55,14 +55,10 @@ static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler *sched)
 {
 	struct drm_sched_job *s_job;
 
-	list_for_each_entry(s_job, &sched->base.pending_list, list) {
-		struct drm_sched_fence *s_fence = s_job->s_fence;
-		struct dma_fence *hw_fence = s_fence->parent;
-
+	drm_sched_for_each_pending_job(s_job, &sched->base, NULL)
 		if (to_xe_sched_job(s_job)->skip_emit ||
-		    (hw_fence && !dma_fence_is_signaled(hw_fence)))
+		    !drm_sched_job_is_signaled(s_job))
 			sched->base.ops->run_job(s_job);
-	}
 }
 
 static inline bool
@@ -71,14 +67,6 @@ xe_sched_invalidate_job(struct xe_sched_job *job, int threshold)
 	return drm_sched_invalidate_job(&job->drm, threshold);
 }
 
-static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
-					    struct xe_sched_job *job)
-{
-	spin_lock(&sched->base.job_list_lock);
-	list_add(&job->drm.list, &sched->base.pending_list);
-	spin_unlock(&sched->base.job_list_lock);
-}
-
 /**
  * xe_sched_first_pending_job() - Find first pending job which is unsignaled
  * @sched: Xe GPU scheduler
@@ -88,21 +76,13 @@ static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
 static inline
 struct xe_sched_job *xe_sched_first_pending_job(struct xe_gpu_scheduler *sched)
 {
-	struct xe_sched_job *job, *r_job = NULL;
-
-	spin_lock(&sched->base.job_list_lock);
-	list_for_each_entry(job, &sched->base.pending_list, drm.list) {
-		struct drm_sched_fence *s_fence = job->drm.s_fence;
-		struct dma_fence *hw_fence = s_fence->parent;
+	struct drm_sched_job *job;
 
-		if (hw_fence && !dma_fence_is_signaled(hw_fence)) {
-			r_job = job;
-			break;
-		}
-	}
-	spin_unlock(&sched->base.job_list_lock);
+	drm_sched_for_each_pending_job(job, &sched->base, NULL)
+		if (!drm_sched_job_is_signaled(job))
+			return to_xe_sched_job(job);
 
-	return r_job;
+	return NULL;
 }
 
 static inline int
diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index 0ef67d3523a7..680696efc434 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1032,7 +1032,7 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
 	struct xe_exec_queue *q = ge->q;
 	struct xe_guc *guc = exec_queue_to_guc(q);
 	struct xe_gpu_scheduler *sched = &ge->sched;
-	struct xe_sched_job *job;
+	struct drm_sched_job *job;
 	bool wedged = false;
 
 	xe_gt_assert(guc_to_gt(guc), xe_exec_queue_is_lr(q));
@@ -1091,16 +1091,10 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
 	if (!exec_queue_killed(q) && !xe_lrc_ring_is_idle(q->lrc[0]))
 		xe_devcoredump(q, NULL, "LR job cleanup, guc_id=%d", q->guc->id);
 
-	xe_hw_fence_irq_stop(q->fence_irq);
+	drm_sched_for_each_pending_job(job, &sched->base, NULL)
+		xe_sched_job_set_error(to_xe_sched_job(job), -ECANCELED);
 
 	xe_sched_submission_start(sched);
-
-	spin_lock(&sched->base.job_list_lock);
-	list_for_each_entry(job, &sched->base.pending_list, drm.list)
-		xe_sched_job_set_error(job, -ECANCELED);
-	spin_unlock(&sched->base.job_list_lock);
-
-	xe_hw_fence_irq_start(q->fence_irq);
 }
 
 #define ADJUST_FIVE_PERCENT(__t)	mul_u64_u32_div(__t, 105, 100)
@@ -1219,7 +1213,7 @@ static enum drm_gpu_sched_stat
 guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
 {
 	struct xe_sched_job *job = to_xe_sched_job(drm_job);
-	struct xe_sched_job *tmp_job;
+	struct drm_sched_job *tmp_job;
 	struct xe_exec_queue *q = job->q;
 	struct xe_gpu_scheduler *sched = &q->guc->sched;
 	struct xe_guc *guc = exec_queue_to_guc(q);
@@ -1228,7 +1222,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
 	unsigned int fw_ref;
 	int err = -ETIME;
 	pid_t pid = -1;
-	int i = 0;
 	bool wedged = false, skip_timeout_check;
 
 	xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
@@ -1395,28 +1388,15 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
 		__deregister_exec_queue(guc, q);
 	}
 
-	/* Stop fence signaling */
-	xe_hw_fence_irq_stop(q->fence_irq);
+	/* Mark all outstanding jobs as bad, thus completing them */
+	xe_sched_job_set_error(job, err);
+	drm_sched_for_each_pending_job(tmp_job, &sched->base, NULL)
+		xe_sched_job_set_error(to_xe_sched_job(tmp_job), -ECANCELED);
 
-	/*
-	 * Fence state now stable, stop / start scheduler which cleans up any
-	 * fences that are complete
-	 */
-	xe_sched_add_pending_job(sched, job);
 	xe_sched_submission_start(sched);
-
 	xe_guc_exec_queue_trigger_cleanup(q);
 
-	/* Mark all outstanding jobs as bad, thus completing them */
-	spin_lock(&sched->base.job_list_lock);
-	list_for_each_entry(tmp_job, &sched->base.pending_list, drm.list)
-		xe_sched_job_set_error(tmp_job, !i++ ? err : -ECANCELED);
-	spin_unlock(&sched->base.job_list_lock);
-
-	/* Start fence signaling */
-	xe_hw_fence_irq_start(q->fence_irq);
-
-	return DRM_GPU_SCHED_STAT_RESET;
+	return DRM_GPU_SCHED_STAT_NO_HANG;
 
 sched_enable:
 	set_exec_queue_pending_tdr_exit(q);
@@ -2244,7 +2224,7 @@ static void guc_exec_queue_unpause_prepare(struct xe_guc *guc,
 	struct drm_sched_job *s_job;
 	struct xe_sched_job *job = NULL;
 
-	list_for_each_entry(s_job, &sched->base.pending_list, list) {
+	drm_sched_for_each_pending_job(s_job, &sched->base, NULL) {
 		job = to_xe_sched_job(s_job);
 
 		xe_gt_dbg(guc_to_gt(guc), "Replay JOB - guc_id=%d, seqno=%d",
@@ -2349,7 +2329,7 @@ void xe_guc_submit_unpause(struct xe_guc *guc)
 		 * created after resfix done.
 		 */
 		if (q->guc->id != index ||
-		    !READ_ONCE(q->guc->sched.base.pause_submit))
+		    !drm_sched_is_stopped(&q->guc->sched.base))
 			continue;
 
 		guc_exec_queue_unpause(guc, q);
@@ -2771,30 +2751,6 @@ xe_guc_exec_queue_snapshot_capture(struct xe_exec_queue *q)
 	if (snapshot->parallel_execution)
 		guc_exec_queue_wq_snapshot_capture(q, snapshot);
 
-	spin_lock(&sched->base.job_list_lock);
-	snapshot->pending_list_size = list_count_nodes(&sched->base.pending_list);
-	snapshot->pending_list = kmalloc_array(snapshot->pending_list_size,
-					       sizeof(struct pending_list_snapshot),
-					       GFP_ATOMIC);
-
-	if (snapshot->pending_list) {
-		struct xe_sched_job *job_iter;
-
-		i = 0;
-		list_for_each_entry(job_iter, &sched->base.pending_list, drm.list) {
-			snapshot->pending_list[i].seqno =
-				xe_sched_job_seqno(job_iter);
-			snapshot->pending_list[i].fence =
-				dma_fence_is_signaled(job_iter->fence) ? 1 : 0;
-			snapshot->pending_list[i].finished =
-				dma_fence_is_signaled(&job_iter->drm.s_fence->finished)
-				? 1 : 0;
-			i++;
-		}
-	}
-
-	spin_unlock(&sched->base.job_list_lock);
-
 	return snapshot;
 }
 
@@ -2852,13 +2808,6 @@ xe_guc_exec_queue_snapshot_print(struct xe_guc_submit_exec_queue_snapshot *snaps
 
 	if (snapshot->parallel_execution)
 		guc_exec_queue_wq_snapshot_print(snapshot, p);
-
-	for (i = 0; snapshot->pending_list && i < snapshot->pending_list_size;
-	     i++)
-		drm_printf(p, "\tJob: seqno=%d, fence=%d, finished=%d\n",
-			   snapshot->pending_list[i].seqno,
-			   snapshot->pending_list[i].fence,
-			   snapshot->pending_list[i].finished);
 }
 
 /**
@@ -2881,7 +2830,6 @@ void xe_guc_exec_queue_snapshot_free(struct xe_guc_submit_exec_queue_snapshot *s
 			xe_lrc_snapshot_free(snapshot->lrc[i]);
 		kfree(snapshot->lrc);
 	}
-	kfree(snapshot->pending_list);
 	kfree(snapshot);
 }
 
diff --git a/drivers/gpu/drm/xe/xe_guc_submit_types.h b/drivers/gpu/drm/xe/xe_guc_submit_types.h
index dc7456c34583..0b08c79cf3b9 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit_types.h
+++ b/drivers/gpu/drm/xe/xe_guc_submit_types.h
@@ -61,12 +61,6 @@ struct guc_submit_parallel_scratch {
 	u32 wq[WQ_SIZE / sizeof(u32)];
 };
 
-struct pending_list_snapshot {
-	u32 seqno;
-	bool fence;
-	bool finished;
-};
-
 /**
  * struct xe_guc_submit_exec_queue_snapshot - Snapshot for devcoredump
  */
@@ -134,11 +128,6 @@ struct xe_guc_submit_exec_queue_snapshot {
 		/** @wq: Workqueue Items */
 		u32 wq[WQ_SIZE / sizeof(u32)];
 	} parallel;
-
-	/** @pending_list_size: Size of the pending list snapshot array */
-	int pending_list_size;
-	/** @pending_list: snapshot of the pending list info */
-	struct pending_list_snapshot *pending_list;
 };
 
 #endif
diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
index b2a0c46dfcd4..e65dfcdfdbc5 100644
--- a/drivers/gpu/drm/xe/xe_hw_fence.c
+++ b/drivers/gpu/drm/xe/xe_hw_fence.c
@@ -110,22 +110,6 @@ void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq)
 	irq_work_queue(&irq->work);
 }
 
-void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq)
-{
-	spin_lock_irq(&irq->lock);
-	irq->enabled = false;
-	spin_unlock_irq(&irq->lock);
-}
-
-void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq)
-{
-	spin_lock_irq(&irq->lock);
-	irq->enabled = true;
-	spin_unlock_irq(&irq->lock);
-
-	irq_work_queue(&irq->work);
-}
-
 void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
 			  struct xe_hw_fence_irq *irq, const char *name)
 {
diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h
index f13a1c4982c7..599492c13f80 100644
--- a/drivers/gpu/drm/xe/xe_hw_fence.h
+++ b/drivers/gpu/drm/xe/xe_hw_fence.h
@@ -17,8 +17,6 @@ void xe_hw_fence_module_exit(void);
 void xe_hw_fence_irq_init(struct xe_hw_fence_irq *irq);
 void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq);
 void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq);
-void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq);
-void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq);
 
 void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
 			  struct xe_hw_fence_irq *irq, const char *name);
-- 
2.34.1

[PATCH v3 5/7] drm/xe: Do not deregister queues in TDR

[Matthew Brost]

Deregistering queues in the TDR introduces unnecessary complexity,
requiring reference counting tricks to function correctly. All that's
needed in the TDR is to kick the queue off the hardware, which is
achieved by disabling scheduling. Queue deregistration should be handled
in a single, well-defined point in the cleanup path, tied to the queue's
reference count.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_submit.c | 57 +++---------------------------
 1 file changed, 5 insertions(+), 52 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index 680696efc434..ab0f1a2d4871 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -69,9 +69,8 @@ exec_queue_to_guc(struct xe_exec_queue *q)
 #define EXEC_QUEUE_STATE_WEDGED			(1 << 8)
 #define EXEC_QUEUE_STATE_BANNED			(1 << 9)
 #define EXEC_QUEUE_STATE_CHECK_TIMEOUT		(1 << 10)
-#define EXEC_QUEUE_STATE_EXTRA_REF		(1 << 11)
-#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 12)
-#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 13)
+#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 11)
+#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 12)
 
 static bool exec_queue_registered(struct xe_exec_queue *q)
 {
@@ -218,21 +217,6 @@ static void clear_exec_queue_check_timeout(struct xe_exec_queue *q)
 	atomic_and(~EXEC_QUEUE_STATE_CHECK_TIMEOUT, &q->guc->state);
 }
 
-static bool exec_queue_extra_ref(struct xe_exec_queue *q)
-{
-	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_EXTRA_REF;
-}
-
-static void set_exec_queue_extra_ref(struct xe_exec_queue *q)
-{
-	atomic_or(EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
-}
-
-static void clear_exec_queue_extra_ref(struct xe_exec_queue *q)
-{
-	atomic_and(~EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
-}
-
 static bool exec_queue_pending_resume(struct xe_exec_queue *q)
 {
 	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_PENDING_RESUME;
@@ -1190,25 +1174,6 @@ static void disable_scheduling(struct xe_exec_queue *q, bool immediate)
 		       G2H_LEN_DW_SCHED_CONTEXT_MODE_SET, 1);
 }
 
-static void __deregister_exec_queue(struct xe_guc *guc, struct xe_exec_queue *q)
-{
-	u32 action[] = {
-		XE_GUC_ACTION_DEREGISTER_CONTEXT,
-		q->guc->id,
-	};
-
-	xe_gt_assert(guc_to_gt(guc), !exec_queue_destroyed(q));
-	xe_gt_assert(guc_to_gt(guc), exec_queue_registered(q));
-	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_enable(q));
-	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_disable(q));
-
-	set_exec_queue_destroyed(q);
-	trace_xe_exec_queue_deregister(q);
-
-	xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
-		       G2H_LEN_DW_DEREGISTER_CONTEXT, 1);
-}
-
 static enum drm_gpu_sched_stat
 guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
 {
@@ -1326,8 +1291,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
 			xe_devcoredump(q, job,
 				       "Schedule disable failed to respond, guc_id=%d, ret=%d, guc_read=%d",
 				       q->guc->id, ret, xe_guc_read_stopped(guc));
-			set_exec_queue_extra_ref(q);
-			xe_exec_queue_get(q);	/* GT reset owns this */
 			set_exec_queue_banned(q);
 			xe_gt_reset_async(q->gt);
 			xe_sched_tdr_queue_imm(sched);
@@ -1380,13 +1343,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
 		}
 	}
 
-	/* Finish cleaning up exec queue via deregister */
 	set_exec_queue_banned(q);
-	if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
-		set_exec_queue_extra_ref(q);
-		xe_exec_queue_get(q);
-		__deregister_exec_queue(guc, q);
-	}
 
 	/* Mark all outstanding jobs as bad, thus completing them */
 	xe_sched_job_set_error(job, err);
@@ -1928,7 +1885,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
 
 	/* Clean up lost G2H + reset engine state */
 	if (exec_queue_registered(q)) {
-		if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
+		if (xe_exec_queue_is_lr(q))
 			xe_exec_queue_put(q);
 		else if (exec_queue_destroyed(q))
 			__guc_exec_queue_destroy(guc, q);
@@ -2062,11 +2019,7 @@ static void guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
 
 	if (exec_queue_destroyed(q) && exec_queue_registered(q)) {
 		clear_exec_queue_destroyed(q);
-		if (exec_queue_extra_ref(q))
-			xe_exec_queue_put(q);
-		else
-			q->guc->needs_cleanup = true;
-		clear_exec_queue_extra_ref(q);
+		q->guc->needs_cleanup = true;
 		xe_gt_dbg(guc_to_gt(guc), "Replay CLEANUP - guc_id=%d",
 			  q->guc->id);
 	}
@@ -2483,7 +2436,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
 
 	clear_exec_queue_registered(q);
 
-	if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
+	if (xe_exec_queue_is_lr(q))
 		xe_exec_queue_put(q);
 	else
 		__guc_exec_queue_destroy(guc, q);
-- 
2.34.1

[PATCH v3 6/7] drm/xe: Remove special casing for LR queues in submission

[Matthew Brost]

Now that LR jobs are tracked by the DRM scheduler, there's no longer a
need to special-case LR queues. This change removes all LR
queue-specific handling, including dedicated TDR logic, reference
counting schemes, and other related mechanisms.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_exec_queue_types.h |   2 -
 drivers/gpu/drm/xe/xe_guc_submit.c           | 129 +------------------
 2 files changed, 7 insertions(+), 124 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
index a3b034e4b205..fd0915ed8eb1 100644
--- a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
+++ b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
@@ -33,8 +33,6 @@ struct xe_guc_exec_queue {
 	 */
 #define MAX_STATIC_MSG_TYPE	3
 	struct xe_sched_msg static_msgs[MAX_STATIC_MSG_TYPE];
-	/** @lr_tdr: long running TDR worker */
-	struct work_struct lr_tdr;
 	/** @destroy_async: do final destroy async from this worker */
 	struct work_struct destroy_async;
 	/** @resume_time: time of last resume */
diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index ab0f1a2d4871..bb1f2929441c 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -674,14 +674,6 @@ static void register_exec_queue(struct xe_exec_queue *q, int ctx_type)
 		parallel_write(xe, map, wq_desc.wq_status, WQ_STATUS_ACTIVE);
 	}
 
-	/*
-	 * We must keep a reference for LR engines if engine is registered with
-	 * the GuC as jobs signal immediately and can't destroy an engine if the
-	 * GuC has a reference to it.
-	 */
-	if (xe_exec_queue_is_lr(q))
-		xe_exec_queue_get(q);
-
 	set_exec_queue_registered(q);
 	trace_xe_exec_queue_register(q);
 	if (xe_exec_queue_is_parallel(q))
@@ -854,7 +846,7 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job)
 	struct xe_sched_job *job = to_xe_sched_job(drm_job);
 	struct xe_exec_queue *q = job->q;
 	struct xe_guc *guc = exec_queue_to_guc(q);
-	bool lr = xe_exec_queue_is_lr(q), killed_or_banned_or_wedged =
+	bool killed_or_banned_or_wedged =
 		exec_queue_killed_or_banned_or_wedged(q);
 
 	xe_gt_assert(guc_to_gt(guc), !(exec_queue_destroyed(q) || exec_queue_pending_disable(q)) ||
@@ -871,15 +863,6 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job)
 		job->skip_emit = false;
 	}
 
-	/*
-	 * We don't care about job-fence ordering in LR VMs because these fences
-	 * are never exported; they are used solely to keep jobs on the pending
-	 * list. Once a queue enters an error state, there's no need to track
-	 * them.
-	 */
-	if (killed_or_banned_or_wedged && lr)
-		xe_sched_job_set_error(job, -ECANCELED);
-
 	return job->fence;
 }
 
@@ -923,8 +906,7 @@ static void disable_scheduling_deregister(struct xe_guc *guc,
 		xe_gt_warn(q->gt, "Pending enable/disable failed to respond\n");
 		xe_sched_submission_start(sched);
 		xe_gt_reset_async(q->gt);
-		if (!xe_exec_queue_is_lr(q))
-			xe_sched_tdr_queue_imm(sched);
+		xe_sched_tdr_queue_imm(sched);
 		return;
 	}
 
@@ -950,10 +932,7 @@ static void xe_guc_exec_queue_trigger_cleanup(struct xe_exec_queue *q)
 	/** to wakeup xe_wait_user_fence ioctl if exec queue is reset */
 	wake_up_all(&xe->ufence_wq);
 
-	if (xe_exec_queue_is_lr(q))
-		queue_work(guc_to_gt(guc)->ordered_wq, &q->guc->lr_tdr);
-	else
-		xe_sched_tdr_queue_imm(&q->guc->sched);
+	xe_sched_tdr_queue_imm(&q->guc->sched);
 }
 
 /**
@@ -1009,78 +988,6 @@ static bool guc_submit_hint_wedged(struct xe_guc *guc)
 	return true;
 }
 
-static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
-{
-	struct xe_guc_exec_queue *ge =
-		container_of(w, struct xe_guc_exec_queue, lr_tdr);
-	struct xe_exec_queue *q = ge->q;
-	struct xe_guc *guc = exec_queue_to_guc(q);
-	struct xe_gpu_scheduler *sched = &ge->sched;
-	struct drm_sched_job *job;
-	bool wedged = false;
-
-	xe_gt_assert(guc_to_gt(guc), xe_exec_queue_is_lr(q));
-
-	if (vf_recovery(guc))
-		return;
-
-	trace_xe_exec_queue_lr_cleanup(q);
-
-	if (!exec_queue_killed(q))
-		wedged = guc_submit_hint_wedged(exec_queue_to_guc(q));
-
-	/* Kill the run_job / process_msg entry points */
-	xe_sched_submission_stop(sched);
-
-	/*
-	 * Engine state now mostly stable, disable scheduling / deregister if
-	 * needed. This cleanup routine might be called multiple times, where
-	 * the actual async engine deregister drops the final engine ref.
-	 * Calling disable_scheduling_deregister will mark the engine as
-	 * destroyed and fire off the CT requests to disable scheduling /
-	 * deregister, which we only want to do once. We also don't want to mark
-	 * the engine as pending_disable again as this may race with the
-	 * xe_guc_deregister_done_handler() which treats it as an unexpected
-	 * state.
-	 */
-	if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
-		struct xe_guc *guc = exec_queue_to_guc(q);
-		int ret;
-
-		set_exec_queue_banned(q);
-		disable_scheduling_deregister(guc, q);
-
-		/*
-		 * Must wait for scheduling to be disabled before signalling
-		 * any fences, if GT broken the GT reset code should signal us.
-		 */
-		ret = wait_event_timeout(guc->ct.wq,
-					 !exec_queue_pending_disable(q) ||
-					 xe_guc_read_stopped(guc) ||
-					 vf_recovery(guc), HZ * 5);
-		if (vf_recovery(guc))
-			return;
-
-		if (!ret) {
-			xe_gt_warn(q->gt, "Schedule disable failed to respond, guc_id=%d\n",
-				   q->guc->id);
-			xe_devcoredump(q, NULL, "Schedule disable failed to respond, guc_id=%d\n",
-				       q->guc->id);
-			xe_sched_submission_start(sched);
-			xe_gt_reset_async(q->gt);
-			return;
-		}
-	}
-
-	if (!exec_queue_killed(q) && !xe_lrc_ring_is_idle(q->lrc[0]))
-		xe_devcoredump(q, NULL, "LR job cleanup, guc_id=%d", q->guc->id);
-
-	drm_sched_for_each_pending_job(job, &sched->base, NULL)
-		xe_sched_job_set_error(to_xe_sched_job(job), -ECANCELED);
-
-	xe_sched_submission_start(sched);
-}
-
 #define ADJUST_FIVE_PERCENT(__t)	mul_u64_u32_div(__t, 105, 100)
 
 static bool check_timeout(struct xe_exec_queue *q, struct xe_sched_job *job)
@@ -1150,8 +1057,7 @@ static void enable_scheduling(struct xe_exec_queue *q)
 		xe_gt_warn(guc_to_gt(guc), "Schedule enable failed to respond");
 		set_exec_queue_banned(q);
 		xe_gt_reset_async(q->gt);
-		if (!xe_exec_queue_is_lr(q))
-			xe_sched_tdr_queue_imm(&q->guc->sched);
+		xe_sched_tdr_queue_imm(&q->guc->sched);
 	}
 }
 
@@ -1189,8 +1095,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
 	pid_t pid = -1;
 	bool wedged = false, skip_timeout_check;
 
-	xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
-
 	/*
 	 * TDR has fired before free job worker. Common if exec queue
 	 * immediately closed after last fence signaled. Add back to pending
@@ -1395,8 +1299,6 @@ static void __guc_exec_queue_destroy_async(struct work_struct *w)
 	xe_pm_runtime_get(guc_to_xe(guc));
 	trace_xe_exec_queue_destroy(q);
 
-	if (xe_exec_queue_is_lr(q))
-		cancel_work_sync(&ge->lr_tdr);
 	/* Confirm no work left behind accessing device structures */
 	cancel_delayed_work_sync(&ge->sched.base.work_tdr);
 
@@ -1629,9 +1531,6 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
 	if (err)
 		goto err_sched;
 
-	if (xe_exec_queue_is_lr(q))
-		INIT_WORK(&q->guc->lr_tdr, xe_guc_exec_queue_lr_cleanup);
-
 	mutex_lock(&guc->submission_state.lock);
 
 	err = alloc_guc_id(guc, q);
@@ -1885,9 +1784,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
 
 	/* Clean up lost G2H + reset engine state */
 	if (exec_queue_registered(q)) {
-		if (xe_exec_queue_is_lr(q))
-			xe_exec_queue_put(q);
-		else if (exec_queue_destroyed(q))
+		if (exec_queue_destroyed(q))
 			__guc_exec_queue_destroy(guc, q);
 	}
 	if (q->guc->suspend_pending) {
@@ -1917,9 +1814,6 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
 				trace_xe_sched_job_ban(job);
 				ban = true;
 			}
-		} else if (xe_exec_queue_is_lr(q) &&
-			   !xe_lrc_ring_is_idle(q->lrc[0])) {
-			ban = true;
 		}
 
 		if (ban) {
@@ -2002,8 +1896,6 @@ static void guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
 	if (pending_enable && !pending_resume &&
 	    !exec_queue_pending_tdr_exit(q)) {
 		clear_exec_queue_registered(q);
-		if (xe_exec_queue_is_lr(q))
-			xe_exec_queue_put(q);
 		xe_gt_dbg(guc_to_gt(guc), "Replay REGISTER - guc_id=%d",
 			  q->guc->id);
 	}
@@ -2060,10 +1952,7 @@ static void guc_exec_queue_pause(struct xe_guc *guc, struct xe_exec_queue *q)
 
 	/* Stop scheduling + flush any DRM scheduler operations */
 	xe_sched_submission_stop(sched);
-	if (xe_exec_queue_is_lr(q))
-		cancel_work_sync(&q->guc->lr_tdr);
-	else
-		cancel_delayed_work_sync(&sched->base.work_tdr);
+	cancel_delayed_work_sync(&sched->base.work_tdr);
 
 	guc_exec_queue_revert_pending_state_change(guc, q);
 
@@ -2435,11 +2324,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
 	trace_xe_exec_queue_deregister_done(q);
 
 	clear_exec_queue_registered(q);
-
-	if (xe_exec_queue_is_lr(q))
-		xe_exec_queue_put(q);
-	else
-		__guc_exec_queue_destroy(guc, q);
+	__guc_exec_queue_destroy(guc, q);
 }
 
 int xe_guc_deregister_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
-- 
2.34.1

[PATCH v3 7/7] drm/xe: Only toggle scheduling in TDR if GuC is running

[Matthew Brost]

If the firmware is not running during TDR (e.g., when the driver is
unloading), there's no need to toggle scheduling in the GuC. In such
cases, skip this step.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_guc_submit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
index bb1f2929441c..ea0cfd866981 100644
--- a/drivers/gpu/drm/xe/xe_guc_submit.c
+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
@@ -1146,7 +1146,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
 		if (exec_queue_reset(q))
 			err = -EIO;
 
-		if (!exec_queue_destroyed(q)) {
+		if (!exec_queue_destroyed(q) && xe_uc_fw_is_running(&guc->fw)) {
 			/*
 			 * Wait for any pending G2H to flush out before
 			 * modifying state
-- 
2.34.1

Re: [PATCH v3 7/7] drm/xe: Only toggle scheduling in TDR if GuC is running

[Niranjana Vishwanathapura]

On Thu, Oct 16, 2025 at 01:48:26PM -0700, Matthew Brost wrote:
>If the firmware is not running during TDR (e.g., when the driver is
>unloading), there's no need to toggle scheduling in the GuC. In such
>cases, skip this step.
>
>Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>---
> drivers/gpu/drm/xe/xe_guc_submit.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>index bb1f2929441c..ea0cfd866981 100644
>--- a/drivers/gpu/drm/xe/xe_guc_submit.c
>+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>@@ -1146,7 +1146,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> 		if (exec_queue_reset(q))
> 			err = -EIO;
>
>-		if (!exec_queue_destroyed(q)) {
>+		if (!exec_queue_destroyed(q) && xe_uc_fw_is_running(&guc->fw)) {
> 			/*
> 			 * Wait for any pending G2H to flush out before
> 			 * modifying state

Looking at the code, it seems like if we skip this 'if' statement (when fw is
not running), then it will go wait for ct->wq. Not sure how that gets woken up
and logic might try to reset gt after that? Not sure if we should check
xe_uc_fw_is_running() here will one of the conditions to wait_event_timeout()
call cover this case and we can handle it appropriately after wait_event_timeout()
returns?

Niranjana

>-- 
>2.34.1
>

Re: [PATCH v3 1/7] drm/sched: Add pending job list iterator

[Niranjana Vishwanathapura]

On Thu, Oct 16, 2025 at 01:48:20PM -0700, Matthew Brost wrote:
>Stop open coding pending job list in drivers. Add pending job list
>iterator which safely walks DRM scheduler list asserting DRM scheduler
>is stopped.
>
>v2:
> - Fix checkpatch (CI)
>v3:
> - Drop locked version (Christian)
>
>Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>---
> include/drm/gpu_scheduler.h | 52 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 52 insertions(+)
>
>diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>index fb88301b3c45..7f31eba3bd61 100644
>--- a/include/drm/gpu_scheduler.h
>+++ b/include/drm/gpu_scheduler.h
>@@ -698,4 +698,56 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> 				   struct drm_gpu_scheduler **sched_list,
> 				   unsigned int num_sched_list);
>
>+/* Inlines */
>+
>+/**
>+ * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
>+ * @sched: DRM scheduler associated with pending job iterator
>+ */
>+struct drm_sched_pending_job_iter {
>+	struct drm_gpu_scheduler *sched;
>+};
>+
>+/* Drivers should never call this directly */
>+static inline struct drm_sched_pending_job_iter
>+__drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
>+{
>+	struct drm_sched_pending_job_iter iter = {
>+		.sched = sched,
>+	};
>+
>+	WARN_ON(!READ_ONCE(sched->pause_submit));
>+	return iter;
>+}
>+
>+/* Drivers should never call this directly */
>+static inline void
>+__drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
>+{
>+	WARN_ON(!READ_ONCE(iter.sched->pause_submit));
>+}

May be instead of these inline functions, we can add the code in a '({' block
in the below DEFINE_CLASS itself to avoid drivers from calling these inline
funcions? Though I agree these inline functions makes it cleaner to read.

>+
>+DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
>+	     __drm_sched_pending_job_iter_end(_T),
>+	     __drm_sched_pending_job_iter_begin(__sched),
>+	     struct drm_gpu_scheduler *__sched);
>+static inline void *
>+class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T)
>+{ return _T; }
>+#define class_drm_sched_pending_job_iter_is_conditional false
>+
>+/**
>+ * drm_sched_for_each_pending_job() - Iterator for each pending job in scheduler
>+ * @__job: Current pending job being iterated over
>+ * @__sched: DRM scheduler to iterate over pending jobs
>+ * @__entity: DRM scheduler entity to filter jobs, NULL indicates no filter
>+ *
>+ * Iterator for each pending job in scheduler, filtering on an entity, and
>+ * enforcing scheduler is fully stopped
>+ */
>+#define drm_sched_for_each_pending_job(__job, __sched, __entity)		\
>+	scoped_guard(drm_sched_pending_job_iter, (__sched))			\
>+		list_for_each_entry((__job), &(__sched)->pending_list, list)	\
>+			for_each_if(!(__entity) || (__job)->entity == (__entity))
>+

I am comparing it with DEFINE_CLASS usage in ttm driver here.
It looks like the body of this macro (where we call list_for_each_entry()),
doesn't use the drm_sched_pending_job_iter at all. So, looks like the only
reason we are using a DEFINE_CLASS with scoped_guard here is for those
WARN_ON() messages at the beginning and end of loop iteration, which is not
fully fool proof. Right?
I wonder if we really need DEFINE_CLASS here for that, though I am not
against using it.

Niranjana

> #endif
>-- 
>2.34.1
>

Re: [PATCH v3 2/7] drm/sched: Add several job helpers to avoid drivers touching scheduler state

[Niranjana Vishwanathapura]

On Thu, Oct 16, 2025 at 01:48:21PM -0700, Matthew Brost wrote:
>Add helpers to see if scheduler is stopped and a jobs signaled state.
>Expected to be used driver side on recovery and debug flows.
>
>Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>---
> drivers/gpu/drm/scheduler/sched_main.c |  4 ++--
> include/drm/gpu_scheduler.h            | 32 ++++++++++++++++++++++++--
> 2 files changed, 32 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>index 46119aacb809..69bd6e482268 100644
>--- a/drivers/gpu/drm/scheduler/sched_main.c
>+++ b/drivers/gpu/drm/scheduler/sched_main.c
>@@ -344,7 +344,7 @@ drm_sched_rq_select_entity_fifo(struct drm_gpu_scheduler *sched,
>  */
> static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> {
>-	if (!READ_ONCE(sched->pause_submit))
>+	if (!drm_sched_is_stopped(sched))
> 		queue_work(sched->submit_wq, &sched->work_run_job);
> }
>
>@@ -354,7 +354,7 @@ static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
>  */
> static void drm_sched_run_free_queue(struct drm_gpu_scheduler *sched)
> {
>-	if (!READ_ONCE(sched->pause_submit))
>+	if (!drm_sched_is_stopped(sched))
> 		queue_work(sched->submit_wq, &sched->work_free_job);
> }
>
>diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>index 7f31eba3bd61..d1a2d7f61c1d 100644
>--- a/include/drm/gpu_scheduler.h
>+++ b/include/drm/gpu_scheduler.h
>@@ -700,6 +700,17 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>
> /* Inlines */
>
>+/**
>+ * drm_sched_is_stopped() - DRM is stopped
>+ * @sched: DRM scheduler
>+ *
>+ * Return: True if sched is stopped, False otherwise
>+ */
>+static inline bool drm_sched_is_stopped(struct drm_gpu_scheduler *sched)
>+{
>+	return READ_ONCE(sched->pause_submit);
>+}
>+
> /**
>  * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
>  * @sched: DRM scheduler associated with pending job iterator
>@@ -716,7 +727,7 @@ __drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
> 		.sched = sched,
> 	};
>
>-	WARN_ON(!READ_ONCE(sched->pause_submit));
>+	WARN_ON(!drm_sched_is_stopped(sched));
> 	return iter;
> }

NIT...instead of modifying the functions added in previous patch, may be this
patch should go in first and the previous patch can be added after that with
drm_sched_is_stopped() usage?

>
>@@ -724,7 +735,7 @@ __drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
> static inline void
> __drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
> {
>-	WARN_ON(!READ_ONCE(iter.sched->pause_submit));
>+	WARN_ON(!drm_sched_is_stopped(iter.sched));
> }
>
> DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
>@@ -750,4 +761,21 @@ class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T
> 		list_for_each_entry((__job), &(__sched)->pending_list, list)	\
> 			for_each_if(!(__entity) || (__job)->entity == (__entity))
>
>+/**
>+ * drm_sched_job_is_signaled() - DRM scheduler job is signaled
>+ * @job: DRM scheduler job
>+ *
>+ * Determine if DRM scheduler job is signaled. DRM scheduler should be stopped
>+ * to obtain a stable snapshot of state.
>+ *
>+ * Return: True if job is signaled, False otherwise
>+ */
>+static inline bool drm_sched_job_is_signaled(struct drm_sched_job *job)
>+{
>+	struct drm_sched_fence *s_fence = job->s_fence;
>+
>+	WARN_ON(!drm_sched_is_stopped(job->sched));
>+	return dma_fence_is_signaled(&s_fence->finished);
>+}

NIT..In patch#4 where xe driver uses this function in couple places,
I am seeing originally it checks if the s_fence->parent is signaled
instead of &s_fence->finished as done here.
I do see below message in the 's_fence->parent' kernel-doc,
"We signal the &drm_sched_fence.finished fence once parent is signalled."
So, probably it is fine, but just want to ensure.

Niranjana

>+
> #endif
>-- 
>2.34.1
>

Re: [PATCH v3 3/7] drm/xe: Add dedicated message lock

[Niranjana Vishwanathapura]

On Thu, Oct 16, 2025 at 01:48:22PM -0700, Matthew Brost wrote:
>Stop abusing DRM scheduler job list lock for messages, add dedicated
>message lock.
>
>Signed-off-by: Matthew Brost <matthew.brost@intel.com>

LGTM.
Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>

>---
> drivers/gpu/drm/xe/xe_gpu_scheduler.c       | 5 +++--
> drivers/gpu/drm/xe/xe_gpu_scheduler.h       | 4 ++--
> drivers/gpu/drm/xe/xe_gpu_scheduler_types.h | 2 ++
> 3 files changed, 7 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>index f91e06d03511..f4f23317191f 100644
>--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>@@ -77,6 +77,7 @@ int xe_sched_init(struct xe_gpu_scheduler *sched,
> 	};
>
> 	sched->ops = xe_ops;
>+	spin_lock_init(&sched->msg_lock);
> 	INIT_LIST_HEAD(&sched->msgs);
> 	INIT_WORK(&sched->work_process_msg, xe_sched_process_msg_work);
>
>@@ -117,7 +118,7 @@ void xe_sched_add_msg(struct xe_gpu_scheduler *sched,
> void xe_sched_add_msg_locked(struct xe_gpu_scheduler *sched,
> 			     struct xe_sched_msg *msg)
> {
>-	lockdep_assert_held(&sched->base.job_list_lock);
>+	lockdep_assert_held(&sched->msg_lock);
>
> 	list_add_tail(&msg->link, &sched->msgs);
> 	xe_sched_process_msg_queue(sched);
>@@ -131,7 +132,7 @@ void xe_sched_add_msg_locked(struct xe_gpu_scheduler *sched,
> void xe_sched_add_msg_head(struct xe_gpu_scheduler *sched,
> 			   struct xe_sched_msg *msg)
> {
>-	lockdep_assert_held(&sched->base.job_list_lock);
>+	lockdep_assert_held(&sched->msg_lock);
>
> 	list_add(&msg->link, &sched->msgs);
> 	xe_sched_process_msg_queue(sched);
>diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>index 9955397aaaa9..b971b6b69419 100644
>--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>@@ -33,12 +33,12 @@ void xe_sched_add_msg_head(struct xe_gpu_scheduler *sched,
>
> static inline void xe_sched_msg_lock(struct xe_gpu_scheduler *sched)
> {
>-	spin_lock(&sched->base.job_list_lock);
>+	spin_lock(&sched->msg_lock);
> }
>
> static inline void xe_sched_msg_unlock(struct xe_gpu_scheduler *sched)
> {
>-	spin_unlock(&sched->base.job_list_lock);
>+	spin_unlock(&sched->msg_lock);
> }
>
> static inline void xe_sched_stop(struct xe_gpu_scheduler *sched)
>diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
>index 6731b13da8bb..63d9bf92583c 100644
>--- a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
>+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
>@@ -47,6 +47,8 @@ struct xe_gpu_scheduler {
> 	const struct xe_sched_backend_ops	*ops;
> 	/** @msgs: list of messages to be processed in @work_process_msg */
> 	struct list_head			msgs;
>+	/** @msg_lock: Message lock */
>+	spinlock_t				msg_lock;
> 	/** @work_process_msg: processes messages */
> 	struct work_struct		work_process_msg;
> };
>-- 
>2.34.1
>

Re: [PATCH v3 4/7] drm/xe: Stop abusing DRM scheduler internals

[Niranjana Vishwanathapura]

On Thu, Oct 16, 2025 at 01:48:23PM -0700, Matthew Brost wrote:
>Use new pending job list iterator and new helper functions in Xe to
>avoid reaching into DRM scheduler internals.
>
>Part of this change involves removing pending jobs debug information
>from debugfs and devcoredump. As agreed, the pending job list should
>only be accessed when the scheduler is stopped. However, it's not
>straightforward to determine whether the scheduler is stopped from the
>shared debugfs/devcoredump code path. Additionally, the pending job list
>provides little useful information, as pending jobs can be inferred from
>seqnos and ring head/tail positions. Therefore, this debug information
>is being removed.
>
>Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>---
> drivers/gpu/drm/xe/xe_gpu_scheduler.c    |  4 +-
> drivers/gpu/drm/xe/xe_gpu_scheduler.h    | 34 +++--------
> drivers/gpu/drm/xe/xe_guc_submit.c       | 74 ++++--------------------
> drivers/gpu/drm/xe/xe_guc_submit_types.h | 11 ----
> drivers/gpu/drm/xe/xe_hw_fence.c         | 16 -----
> drivers/gpu/drm/xe/xe_hw_fence.h         |  2 -
> 6 files changed, 20 insertions(+), 121 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>index f4f23317191f..9c8004d5dd91 100644
>--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>@@ -7,7 +7,7 @@
>
> static void xe_sched_process_msg_queue(struct xe_gpu_scheduler *sched)
> {
>-	if (!READ_ONCE(sched->base.pause_submit))
>+	if (!drm_sched_is_stopped(&sched->base))
> 		queue_work(sched->base.submit_wq, &sched->work_process_msg);
> }
>
>@@ -43,7 +43,7 @@ static void xe_sched_process_msg_work(struct work_struct *w)
> 		container_of(w, struct xe_gpu_scheduler, work_process_msg);
> 	struct xe_sched_msg *msg;
>
>-	if (READ_ONCE(sched->base.pause_submit))
>+	if (drm_sched_is_stopped(&sched->base))
> 		return;
>
> 	msg = xe_sched_get_msg(sched);
>diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>index b971b6b69419..583372a78140 100644
>--- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>+++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>@@ -55,14 +55,10 @@ static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler *sched)
> {
> 	struct drm_sched_job *s_job;
>
>-	list_for_each_entry(s_job, &sched->base.pending_list, list) {
>-		struct drm_sched_fence *s_fence = s_job->s_fence;
>-		struct dma_fence *hw_fence = s_fence->parent;
>-
>+	drm_sched_for_each_pending_job(s_job, &sched->base, NULL)
> 		if (to_xe_sched_job(s_job)->skip_emit ||
>-		    (hw_fence && !dma_fence_is_signaled(hw_fence)))
>+		    !drm_sched_job_is_signaled(s_job))
> 			sched->base.ops->run_job(s_job);
>-	}
> }
>
> static inline bool
>@@ -71,14 +67,6 @@ xe_sched_invalidate_job(struct xe_sched_job *job, int threshold)
> 	return drm_sched_invalidate_job(&job->drm, threshold);
> }
>
>-static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
>-					    struct xe_sched_job *job)
>-{
>-	spin_lock(&sched->base.job_list_lock);
>-	list_add(&job->drm.list, &sched->base.pending_list);
>-	spin_unlock(&sched->base.job_list_lock);
>-}
>-
> /**
>  * xe_sched_first_pending_job() - Find first pending job which is unsignaled
>  * @sched: Xe GPU scheduler
>@@ -88,21 +76,13 @@ static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
> static inline
> struct xe_sched_job *xe_sched_first_pending_job(struct xe_gpu_scheduler *sched)
> {
>-	struct xe_sched_job *job, *r_job = NULL;
>-
>-	spin_lock(&sched->base.job_list_lock);
>-	list_for_each_entry(job, &sched->base.pending_list, drm.list) {
>-		struct drm_sched_fence *s_fence = job->drm.s_fence;
>-		struct dma_fence *hw_fence = s_fence->parent;
>+	struct drm_sched_job *job;
>
>-		if (hw_fence && !dma_fence_is_signaled(hw_fence)) {
>-			r_job = job;
>-			break;
>-		}
>-	}
>-	spin_unlock(&sched->base.job_list_lock);
>+	drm_sched_for_each_pending_job(job, &sched->base, NULL)
>+		if (!drm_sched_job_is_signaled(job))
>+			return to_xe_sched_job(job);
>
>-	return r_job;
>+	return NULL;
> }
>
> static inline int
>diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>index 0ef67d3523a7..680696efc434 100644
>--- a/drivers/gpu/drm/xe/xe_guc_submit.c
>+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>@@ -1032,7 +1032,7 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> 	struct xe_exec_queue *q = ge->q;
> 	struct xe_guc *guc = exec_queue_to_guc(q);
> 	struct xe_gpu_scheduler *sched = &ge->sched;
>-	struct xe_sched_job *job;
>+	struct drm_sched_job *job;
> 	bool wedged = false;
>
> 	xe_gt_assert(guc_to_gt(guc), xe_exec_queue_is_lr(q));
>@@ -1091,16 +1091,10 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> 	if (!exec_queue_killed(q) && !xe_lrc_ring_is_idle(q->lrc[0]))
> 		xe_devcoredump(q, NULL, "LR job cleanup, guc_id=%d", q->guc->id);
>
>-	xe_hw_fence_irq_stop(q->fence_irq);
>+	drm_sched_for_each_pending_job(job, &sched->base, NULL)
>+		xe_sched_job_set_error(to_xe_sched_job(job), -ECANCELED);
>
> 	xe_sched_submission_start(sched);
>-
>-	spin_lock(&sched->base.job_list_lock);
>-	list_for_each_entry(job, &sched->base.pending_list, drm.list)
>-		xe_sched_job_set_error(job, -ECANCELED);
>-	spin_unlock(&sched->base.job_list_lock);
>-
>-	xe_hw_fence_irq_start(q->fence_irq);
> }
>
> #define ADJUST_FIVE_PERCENT(__t)	mul_u64_u32_div(__t, 105, 100)
>@@ -1219,7 +1213,7 @@ static enum drm_gpu_sched_stat
> guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> {
> 	struct xe_sched_job *job = to_xe_sched_job(drm_job);
>-	struct xe_sched_job *tmp_job;
>+	struct drm_sched_job *tmp_job;
> 	struct xe_exec_queue *q = job->q;
> 	struct xe_gpu_scheduler *sched = &q->guc->sched;
> 	struct xe_guc *guc = exec_queue_to_guc(q);
>@@ -1228,7 +1222,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> 	unsigned int fw_ref;
> 	int err = -ETIME;
> 	pid_t pid = -1;
>-	int i = 0;
> 	bool wedged = false, skip_timeout_check;
>
> 	xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
>@@ -1395,28 +1388,15 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> 		__deregister_exec_queue(guc, q);
> 	}
>
>-	/* Stop fence signaling */
>-	xe_hw_fence_irq_stop(q->fence_irq);
>+	/* Mark all outstanding jobs as bad, thus completing them */
>+	xe_sched_job_set_error(job, err);

This setting error for this timed out job is newly added.
Why was it not there before and being added now?

>+	drm_sched_for_each_pending_job(tmp_job, &sched->base, NULL)
>+		xe_sched_job_set_error(to_xe_sched_job(tmp_job), -ECANCELED);
>
>-	/*
>-	 * Fence state now stable, stop / start scheduler which cleans up any
>-	 * fences that are complete
>-	 */
>-	xe_sched_add_pending_job(sched, job);

Why xe_sched_add_pending_job() was there before?

> 	xe_sched_submission_start(sched);
>-
> 	xe_guc_exec_queue_trigger_cleanup(q);

Why do we need to trigger cleanup again here?

>
>-	/* Mark all outstanding jobs as bad, thus completing them */
>-	spin_lock(&sched->base.job_list_lock);
>-	list_for_each_entry(tmp_job, &sched->base.pending_list, drm.list)
>-		xe_sched_job_set_error(tmp_job, !i++ ? err : -ECANCELED);
>-	spin_unlock(&sched->base.job_list_lock);
>-
>-	/* Start fence signaling */
>-	xe_hw_fence_irq_start(q->fence_irq);
>-
>-	return DRM_GPU_SCHED_STAT_RESET;
>+	return DRM_GPU_SCHED_STAT_NO_HANG;

This is error case. So, why return is changed to NO_HANG?

Niranjana

>
> sched_enable:
> 	set_exec_queue_pending_tdr_exit(q);
>@@ -2244,7 +2224,7 @@ static void guc_exec_queue_unpause_prepare(struct xe_guc *guc,
> 	struct drm_sched_job *s_job;
> 	struct xe_sched_job *job = NULL;
>
>-	list_for_each_entry(s_job, &sched->base.pending_list, list) {
>+	drm_sched_for_each_pending_job(s_job, &sched->base, NULL) {
> 		job = to_xe_sched_job(s_job);
>
> 		xe_gt_dbg(guc_to_gt(guc), "Replay JOB - guc_id=%d, seqno=%d",
>@@ -2349,7 +2329,7 @@ void xe_guc_submit_unpause(struct xe_guc *guc)
> 		 * created after resfix done.
> 		 */
> 		if (q->guc->id != index ||
>-		    !READ_ONCE(q->guc->sched.base.pause_submit))
>+		    !drm_sched_is_stopped(&q->guc->sched.base))
> 			continue;
>
> 		guc_exec_queue_unpause(guc, q);
>@@ -2771,30 +2751,6 @@ xe_guc_exec_queue_snapshot_capture(struct xe_exec_queue *q)
> 	if (snapshot->parallel_execution)
> 		guc_exec_queue_wq_snapshot_capture(q, snapshot);
>
>-	spin_lock(&sched->base.job_list_lock);
>-	snapshot->pending_list_size = list_count_nodes(&sched->base.pending_list);
>-	snapshot->pending_list = kmalloc_array(snapshot->pending_list_size,
>-					       sizeof(struct pending_list_snapshot),
>-					       GFP_ATOMIC);
>-
>-	if (snapshot->pending_list) {
>-		struct xe_sched_job *job_iter;
>-
>-		i = 0;
>-		list_for_each_entry(job_iter, &sched->base.pending_list, drm.list) {
>-			snapshot->pending_list[i].seqno =
>-				xe_sched_job_seqno(job_iter);
>-			snapshot->pending_list[i].fence =
>-				dma_fence_is_signaled(job_iter->fence) ? 1 : 0;
>-			snapshot->pending_list[i].finished =
>-				dma_fence_is_signaled(&job_iter->drm.s_fence->finished)
>-				? 1 : 0;
>-			i++;
>-		}
>-	}
>-
>-	spin_unlock(&sched->base.job_list_lock);
>-
> 	return snapshot;
> }
>
>@@ -2852,13 +2808,6 @@ xe_guc_exec_queue_snapshot_print(struct xe_guc_submit_exec_queue_snapshot *snaps
>
> 	if (snapshot->parallel_execution)
> 		guc_exec_queue_wq_snapshot_print(snapshot, p);
>-
>-	for (i = 0; snapshot->pending_list && i < snapshot->pending_list_size;
>-	     i++)
>-		drm_printf(p, "\tJob: seqno=%d, fence=%d, finished=%d\n",
>-			   snapshot->pending_list[i].seqno,
>-			   snapshot->pending_list[i].fence,
>-			   snapshot->pending_list[i].finished);
> }
>
> /**
>@@ -2881,7 +2830,6 @@ void xe_guc_exec_queue_snapshot_free(struct xe_guc_submit_exec_queue_snapshot *s
> 			xe_lrc_snapshot_free(snapshot->lrc[i]);
> 		kfree(snapshot->lrc);
> 	}
>-	kfree(snapshot->pending_list);
> 	kfree(snapshot);
> }
>
>diff --git a/drivers/gpu/drm/xe/xe_guc_submit_types.h b/drivers/gpu/drm/xe/xe_guc_submit_types.h
>index dc7456c34583..0b08c79cf3b9 100644
>--- a/drivers/gpu/drm/xe/xe_guc_submit_types.h
>+++ b/drivers/gpu/drm/xe/xe_guc_submit_types.h
>@@ -61,12 +61,6 @@ struct guc_submit_parallel_scratch {
> 	u32 wq[WQ_SIZE / sizeof(u32)];
> };
>
>-struct pending_list_snapshot {
>-	u32 seqno;
>-	bool fence;
>-	bool finished;
>-};
>-
> /**
>  * struct xe_guc_submit_exec_queue_snapshot - Snapshot for devcoredump
>  */
>@@ -134,11 +128,6 @@ struct xe_guc_submit_exec_queue_snapshot {
> 		/** @wq: Workqueue Items */
> 		u32 wq[WQ_SIZE / sizeof(u32)];
> 	} parallel;
>-
>-	/** @pending_list_size: Size of the pending list snapshot array */
>-	int pending_list_size;
>-	/** @pending_list: snapshot of the pending list info */
>-	struct pending_list_snapshot *pending_list;
> };
>
> #endif
>diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
>index b2a0c46dfcd4..e65dfcdfdbc5 100644
>--- a/drivers/gpu/drm/xe/xe_hw_fence.c
>+++ b/drivers/gpu/drm/xe/xe_hw_fence.c
>@@ -110,22 +110,6 @@ void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq)
> 	irq_work_queue(&irq->work);
> }
>
>-void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq)
>-{
>-	spin_lock_irq(&irq->lock);
>-	irq->enabled = false;
>-	spin_unlock_irq(&irq->lock);
>-}
>-
>-void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq)
>-{
>-	spin_lock_irq(&irq->lock);
>-	irq->enabled = true;
>-	spin_unlock_irq(&irq->lock);
>-
>-	irq_work_queue(&irq->work);
>-}
>-
> void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
> 			  struct xe_hw_fence_irq *irq, const char *name)
> {
>diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h
>index f13a1c4982c7..599492c13f80 100644
>--- a/drivers/gpu/drm/xe/xe_hw_fence.h
>+++ b/drivers/gpu/drm/xe/xe_hw_fence.h
>@@ -17,8 +17,6 @@ void xe_hw_fence_module_exit(void);
> void xe_hw_fence_irq_init(struct xe_hw_fence_irq *irq);
> void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq);
> void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq);
>-void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq);
>-void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq);
>
> void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
> 			  struct xe_hw_fence_irq *irq, const char *name);
>-- 
>2.34.1
>

Re: [PATCH v3 5/7] drm/xe: Do not deregister queues in TDR

[Niranjana Vishwanathapura]

On Thu, Oct 16, 2025 at 01:48:24PM -0700, Matthew Brost wrote:
>Deregistering queues in the TDR introduces unnecessary complexity,
>requiring reference counting tricks to function correctly. All that's
>needed in the TDR is to kick the queue off the hardware, which is
>achieved by disabling scheduling. Queue deregistration should be handled
>in a single, well-defined point in the cleanup path, tied to the queue's
>reference count.
>

Overall looks good to me.
But it would help if the commit text describes why this extra reference
taking was there before for lr jobs and why it is not needed now.

Niranjana

>Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>---
> drivers/gpu/drm/xe/xe_guc_submit.c | 57 +++---------------------------
> 1 file changed, 5 insertions(+), 52 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>index 680696efc434..ab0f1a2d4871 100644
>--- a/drivers/gpu/drm/xe/xe_guc_submit.c
>+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>@@ -69,9 +69,8 @@ exec_queue_to_guc(struct xe_exec_queue *q)
> #define EXEC_QUEUE_STATE_WEDGED			(1 << 8)
> #define EXEC_QUEUE_STATE_BANNED			(1 << 9)
> #define EXEC_QUEUE_STATE_CHECK_TIMEOUT		(1 << 10)
>-#define EXEC_QUEUE_STATE_EXTRA_REF		(1 << 11)
>-#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 12)
>-#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 13)
>+#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 11)
>+#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 12)
>
> static bool exec_queue_registered(struct xe_exec_queue *q)
> {
>@@ -218,21 +217,6 @@ static void clear_exec_queue_check_timeout(struct xe_exec_queue *q)
> 	atomic_and(~EXEC_QUEUE_STATE_CHECK_TIMEOUT, &q->guc->state);
> }
>
>-static bool exec_queue_extra_ref(struct xe_exec_queue *q)
>-{
>-	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_EXTRA_REF;
>-}
>-
>-static void set_exec_queue_extra_ref(struct xe_exec_queue *q)
>-{
>-	atomic_or(EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
>-}
>-
>-static void clear_exec_queue_extra_ref(struct xe_exec_queue *q)
>-{
>-	atomic_and(~EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
>-}
>-
> static bool exec_queue_pending_resume(struct xe_exec_queue *q)
> {
> 	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_PENDING_RESUME;
>@@ -1190,25 +1174,6 @@ static void disable_scheduling(struct xe_exec_queue *q, bool immediate)
> 		       G2H_LEN_DW_SCHED_CONTEXT_MODE_SET, 1);
> }
>
>-static void __deregister_exec_queue(struct xe_guc *guc, struct xe_exec_queue *q)
>-{
>-	u32 action[] = {
>-		XE_GUC_ACTION_DEREGISTER_CONTEXT,
>-		q->guc->id,
>-	};
>-
>-	xe_gt_assert(guc_to_gt(guc), !exec_queue_destroyed(q));
>-	xe_gt_assert(guc_to_gt(guc), exec_queue_registered(q));
>-	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_enable(q));
>-	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_disable(q));
>-
>-	set_exec_queue_destroyed(q);
>-	trace_xe_exec_queue_deregister(q);
>-
>-	xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
>-		       G2H_LEN_DW_DEREGISTER_CONTEXT, 1);
>-}
>-
> static enum drm_gpu_sched_stat
> guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> {
>@@ -1326,8 +1291,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> 			xe_devcoredump(q, job,
> 				       "Schedule disable failed to respond, guc_id=%d, ret=%d, guc_read=%d",
> 				       q->guc->id, ret, xe_guc_read_stopped(guc));
>-			set_exec_queue_extra_ref(q);
>-			xe_exec_queue_get(q);	/* GT reset owns this */
> 			set_exec_queue_banned(q);
> 			xe_gt_reset_async(q->gt);
> 			xe_sched_tdr_queue_imm(sched);
>@@ -1380,13 +1343,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> 		}
> 	}
>
>-	/* Finish cleaning up exec queue via deregister */
> 	set_exec_queue_banned(q);
>-	if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
>-		set_exec_queue_extra_ref(q);
>-		xe_exec_queue_get(q);
>-		__deregister_exec_queue(guc, q);
>-	}
>
> 	/* Mark all outstanding jobs as bad, thus completing them */
> 	xe_sched_job_set_error(job, err);
>@@ -1928,7 +1885,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
>
> 	/* Clean up lost G2H + reset engine state */
> 	if (exec_queue_registered(q)) {
>-		if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
>+		if (xe_exec_queue_is_lr(q))
> 			xe_exec_queue_put(q);
> 		else if (exec_queue_destroyed(q))
> 			__guc_exec_queue_destroy(guc, q);
>@@ -2062,11 +2019,7 @@ static void guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
>
> 	if (exec_queue_destroyed(q) && exec_queue_registered(q)) {
> 		clear_exec_queue_destroyed(q);
>-		if (exec_queue_extra_ref(q))
>-			xe_exec_queue_put(q);
>-		else
>-			q->guc->needs_cleanup = true;
>-		clear_exec_queue_extra_ref(q);
>+		q->guc->needs_cleanup = true;
> 		xe_gt_dbg(guc_to_gt(guc), "Replay CLEANUP - guc_id=%d",
> 			  q->guc->id);
> 	}
>@@ -2483,7 +2436,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
>
> 	clear_exec_queue_registered(q);
>
>-	if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
>+	if (xe_exec_queue_is_lr(q))
> 		xe_exec_queue_put(q);
> 	else
> 		__guc_exec_queue_destroy(guc, q);
>-- 
>2.34.1
>

Re: [PATCH v3 6/7] drm/xe: Remove special casing for LR queues in submission

[Niranjana Vishwanathapura]

On Thu, Oct 16, 2025 at 01:48:25PM -0700, Matthew Brost wrote:
>Now that LR jobs are tracked by the DRM scheduler, there's no longer a
>need to special-case LR queues. This change removes all LR
>queue-specific handling, including dedicated TDR logic, reference
>counting schemes, and other related mechanisms.
>
>Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>---
> drivers/gpu/drm/xe/xe_guc_exec_queue_types.h |   2 -
> drivers/gpu/drm/xe/xe_guc_submit.c           | 129 +------------------
> 2 files changed, 7 insertions(+), 124 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
>index a3b034e4b205..fd0915ed8eb1 100644
>--- a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
>+++ b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
>@@ -33,8 +33,6 @@ struct xe_guc_exec_queue {
> 	 */
> #define MAX_STATIC_MSG_TYPE	3
> 	struct xe_sched_msg static_msgs[MAX_STATIC_MSG_TYPE];
>-	/** @lr_tdr: long running TDR worker */
>-	struct work_struct lr_tdr;
> 	/** @destroy_async: do final destroy async from this worker */
> 	struct work_struct destroy_async;
> 	/** @resume_time: time of last resume */
>diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>index ab0f1a2d4871..bb1f2929441c 100644
>--- a/drivers/gpu/drm/xe/xe_guc_submit.c
>+++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>@@ -674,14 +674,6 @@ static void register_exec_queue(struct xe_exec_queue *q, int ctx_type)
> 		parallel_write(xe, map, wq_desc.wq_status, WQ_STATUS_ACTIVE);
> 	}
>
>-	/*
>-	 * We must keep a reference for LR engines if engine is registered with
>-	 * the GuC as jobs signal immediately and can't destroy an engine if the
>-	 * GuC has a reference to it.
>-	 */
>-	if (xe_exec_queue_is_lr(q))
>-		xe_exec_queue_get(q);
>-
> 	set_exec_queue_registered(q);
> 	trace_xe_exec_queue_register(q);
> 	if (xe_exec_queue_is_parallel(q))
>@@ -854,7 +846,7 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job)
> 	struct xe_sched_job *job = to_xe_sched_job(drm_job);
> 	struct xe_exec_queue *q = job->q;
> 	struct xe_guc *guc = exec_queue_to_guc(q);
>-	bool lr = xe_exec_queue_is_lr(q), killed_or_banned_or_wedged =
>+	bool killed_or_banned_or_wedged =
> 		exec_queue_killed_or_banned_or_wedged(q);
>
> 	xe_gt_assert(guc_to_gt(guc), !(exec_queue_destroyed(q) || exec_queue_pending_disable(q)) ||
>@@ -871,15 +863,6 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job)
> 		job->skip_emit = false;
> 	}
>
>-	/*
>-	 * We don't care about job-fence ordering in LR VMs because these fences
>-	 * are never exported; they are used solely to keep jobs on the pending
>-	 * list. Once a queue enters an error state, there's no need to track
>-	 * them.
>-	 */
>-	if (killed_or_banned_or_wedged && lr)
>-		xe_sched_job_set_error(job, -ECANCELED);
>-

Why this piece of code here is being removed?

> 	return job->fence;
> }
>
>@@ -923,8 +906,7 @@ static void disable_scheduling_deregister(struct xe_guc *guc,
> 		xe_gt_warn(q->gt, "Pending enable/disable failed to respond\n");
> 		xe_sched_submission_start(sched);
> 		xe_gt_reset_async(q->gt);
>-		if (!xe_exec_queue_is_lr(q))
>-			xe_sched_tdr_queue_imm(sched);
>+		xe_sched_tdr_queue_imm(sched);
> 		return;
> 	}
>
>@@ -950,10 +932,7 @@ static void xe_guc_exec_queue_trigger_cleanup(struct xe_exec_queue *q)
> 	/** to wakeup xe_wait_user_fence ioctl if exec queue is reset */
> 	wake_up_all(&xe->ufence_wq);
>
>-	if (xe_exec_queue_is_lr(q))
>-		queue_work(guc_to_gt(guc)->ordered_wq, &q->guc->lr_tdr);
>-	else
>-		xe_sched_tdr_queue_imm(&q->guc->sched);
>+	xe_sched_tdr_queue_imm(&q->guc->sched);
> }
>
> /**
>@@ -1009,78 +988,6 @@ static bool guc_submit_hint_wedged(struct xe_guc *guc)
> 	return true;
> }
>
>-static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
>-{
>-	struct xe_guc_exec_queue *ge =
>-		container_of(w, struct xe_guc_exec_queue, lr_tdr);
>-	struct xe_exec_queue *q = ge->q;
>-	struct xe_guc *guc = exec_queue_to_guc(q);
>-	struct xe_gpu_scheduler *sched = &ge->sched;
>-	struct drm_sched_job *job;
>-	bool wedged = false;
>-
>-	xe_gt_assert(guc_to_gt(guc), xe_exec_queue_is_lr(q));
>-
>-	if (vf_recovery(guc))
>-		return;
>-
>-	trace_xe_exec_queue_lr_cleanup(q);

Remove the trace event as well in xe_trace.h?

Niranjana

>-
>-	if (!exec_queue_killed(q))
>-		wedged = guc_submit_hint_wedged(exec_queue_to_guc(q));
>-
>-	/* Kill the run_job / process_msg entry points */
>-	xe_sched_submission_stop(sched);
>-
>-	/*
>-	 * Engine state now mostly stable, disable scheduling / deregister if
>-	 * needed. This cleanup routine might be called multiple times, where
>-	 * the actual async engine deregister drops the final engine ref.
>-	 * Calling disable_scheduling_deregister will mark the engine as
>-	 * destroyed and fire off the CT requests to disable scheduling /
>-	 * deregister, which we only want to do once. We also don't want to mark
>-	 * the engine as pending_disable again as this may race with the
>-	 * xe_guc_deregister_done_handler() which treats it as an unexpected
>-	 * state.
>-	 */
>-	if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
>-		struct xe_guc *guc = exec_queue_to_guc(q);
>-		int ret;
>-
>-		set_exec_queue_banned(q);
>-		disable_scheduling_deregister(guc, q);
>-
>-		/*
>-		 * Must wait for scheduling to be disabled before signalling
>-		 * any fences, if GT broken the GT reset code should signal us.
>-		 */
>-		ret = wait_event_timeout(guc->ct.wq,
>-					 !exec_queue_pending_disable(q) ||
>-					 xe_guc_read_stopped(guc) ||
>-					 vf_recovery(guc), HZ * 5);
>-		if (vf_recovery(guc))
>-			return;
>-
>-		if (!ret) {
>-			xe_gt_warn(q->gt, "Schedule disable failed to respond, guc_id=%d\n",
>-				   q->guc->id);
>-			xe_devcoredump(q, NULL, "Schedule disable failed to respond, guc_id=%d\n",
>-				       q->guc->id);
>-			xe_sched_submission_start(sched);
>-			xe_gt_reset_async(q->gt);
>-			return;
>-		}
>-	}
>-
>-	if (!exec_queue_killed(q) && !xe_lrc_ring_is_idle(q->lrc[0]))
>-		xe_devcoredump(q, NULL, "LR job cleanup, guc_id=%d", q->guc->id);
>-
>-	drm_sched_for_each_pending_job(job, &sched->base, NULL)
>-		xe_sched_job_set_error(to_xe_sched_job(job), -ECANCELED);
>-
>-	xe_sched_submission_start(sched);
>-}
>-
> #define ADJUST_FIVE_PERCENT(__t)	mul_u64_u32_div(__t, 105, 100)
>
> static bool check_timeout(struct xe_exec_queue *q, struct xe_sched_job *job)
>@@ -1150,8 +1057,7 @@ static void enable_scheduling(struct xe_exec_queue *q)
> 		xe_gt_warn(guc_to_gt(guc), "Schedule enable failed to respond");
> 		set_exec_queue_banned(q);
> 		xe_gt_reset_async(q->gt);
>-		if (!xe_exec_queue_is_lr(q))
>-			xe_sched_tdr_queue_imm(&q->guc->sched);
>+		xe_sched_tdr_queue_imm(&q->guc->sched);
> 	}
> }
>
>@@ -1189,8 +1095,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> 	pid_t pid = -1;
> 	bool wedged = false, skip_timeout_check;
>
>-	xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
>-
> 	/*
> 	 * TDR has fired before free job worker. Common if exec queue
> 	 * immediately closed after last fence signaled. Add back to pending
>@@ -1395,8 +1299,6 @@ static void __guc_exec_queue_destroy_async(struct work_struct *w)
> 	xe_pm_runtime_get(guc_to_xe(guc));
> 	trace_xe_exec_queue_destroy(q);
>
>-	if (xe_exec_queue_is_lr(q))
>-		cancel_work_sync(&ge->lr_tdr);
> 	/* Confirm no work left behind accessing device structures */
> 	cancel_delayed_work_sync(&ge->sched.base.work_tdr);
>
>@@ -1629,9 +1531,6 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
> 	if (err)
> 		goto err_sched;
>
>-	if (xe_exec_queue_is_lr(q))
>-		INIT_WORK(&q->guc->lr_tdr, xe_guc_exec_queue_lr_cleanup);
>-
> 	mutex_lock(&guc->submission_state.lock);
>
> 	err = alloc_guc_id(guc, q);
>@@ -1885,9 +1784,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
>
> 	/* Clean up lost G2H + reset engine state */
> 	if (exec_queue_registered(q)) {
>-		if (xe_exec_queue_is_lr(q))
>-			xe_exec_queue_put(q);
>-		else if (exec_queue_destroyed(q))
>+		if (exec_queue_destroyed(q))
> 			__guc_exec_queue_destroy(guc, q);
> 	}
> 	if (q->guc->suspend_pending) {
>@@ -1917,9 +1814,6 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
> 				trace_xe_sched_job_ban(job);
> 				ban = true;
> 			}
>-		} else if (xe_exec_queue_is_lr(q) &&
>-			   !xe_lrc_ring_is_idle(q->lrc[0])) {
>-			ban = true;
> 		}
>
> 		if (ban) {
>@@ -2002,8 +1896,6 @@ static void guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
> 	if (pending_enable && !pending_resume &&
> 	    !exec_queue_pending_tdr_exit(q)) {
> 		clear_exec_queue_registered(q);
>-		if (xe_exec_queue_is_lr(q))
>-			xe_exec_queue_put(q);
> 		xe_gt_dbg(guc_to_gt(guc), "Replay REGISTER - guc_id=%d",
> 			  q->guc->id);
> 	}
>@@ -2060,10 +1952,7 @@ static void guc_exec_queue_pause(struct xe_guc *guc, struct xe_exec_queue *q)
>
> 	/* Stop scheduling + flush any DRM scheduler operations */
> 	xe_sched_submission_stop(sched);
>-	if (xe_exec_queue_is_lr(q))
>-		cancel_work_sync(&q->guc->lr_tdr);
>-	else
>-		cancel_delayed_work_sync(&sched->base.work_tdr);
>+	cancel_delayed_work_sync(&sched->base.work_tdr);
>
> 	guc_exec_queue_revert_pending_state_change(guc, q);
>
>@@ -2435,11 +2324,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
> 	trace_xe_exec_queue_deregister_done(q);
>
> 	clear_exec_queue_registered(q);
>-
>-	if (xe_exec_queue_is_lr(q))
>-		xe_exec_queue_put(q);
>-	else
>-		__guc_exec_queue_destroy(guc, q);
>+	__guc_exec_queue_destroy(guc, q);
> }
>
> int xe_guc_deregister_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
>-- 
>2.34.1
>

Re: [PATCH v3 2/7] drm/sched: Add several job helpers to avoid drivers touching scheduler state

[Matthew Brost]

On Mon, Nov 17, 2025 at 11:57:44AM -0800, Niranjana Vishwanathapura wrote:
> On Thu, Oct 16, 2025 at 01:48:21PM -0700, Matthew Brost wrote:
> > Add helpers to see if scheduler is stopped and a jobs signaled state.
> > Expected to be used driver side on recovery and debug flows.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/scheduler/sched_main.c |  4 ++--
> > include/drm/gpu_scheduler.h            | 32 ++++++++++++++++++++++++--
> > 2 files changed, 32 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> > index 46119aacb809..69bd6e482268 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -344,7 +344,7 @@ drm_sched_rq_select_entity_fifo(struct drm_gpu_scheduler *sched,
> >  */
> > static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> > {
> > -	if (!READ_ONCE(sched->pause_submit))
> > +	if (!drm_sched_is_stopped(sched))
> > 		queue_work(sched->submit_wq, &sched->work_run_job);
> > }
> > 
> > @@ -354,7 +354,7 @@ static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
> >  */
> > static void drm_sched_run_free_queue(struct drm_gpu_scheduler *sched)
> > {
> > -	if (!READ_ONCE(sched->pause_submit))
> > +	if (!drm_sched_is_stopped(sched))
> > 		queue_work(sched->submit_wq, &sched->work_free_job);
> > }
> > 
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index 7f31eba3bd61..d1a2d7f61c1d 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -700,6 +700,17 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> > 
> > /* Inlines */
> > 
> > +/**
> > + * drm_sched_is_stopped() - DRM is stopped
> > + * @sched: DRM scheduler
> > + *
> > + * Return: True if sched is stopped, False otherwise
> > + */
> > +static inline bool drm_sched_is_stopped(struct drm_gpu_scheduler *sched)
> > +{
> > +	return READ_ONCE(sched->pause_submit);
> > +}
> > +
> > /**
> >  * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
> >  * @sched: DRM scheduler associated with pending job iterator
> > @@ -716,7 +727,7 @@ __drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
> > 		.sched = sched,
> > 	};
> > 
> > -	WARN_ON(!READ_ONCE(sched->pause_submit));
> > +	WARN_ON(!drm_sched_is_stopped(sched));
> > 	return iter;
> > }
> 
> NIT...instead of modifying the functions added in previous patch, may be this
> patch should go in first and the previous patch can be added after that with
> drm_sched_is_stopped() usage?
> 

Yes, I think that would be better ordering. Will fix.

> > 
> > @@ -724,7 +735,7 @@ __drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
> > static inline void
> > __drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
> > {
> > -	WARN_ON(!READ_ONCE(iter.sched->pause_submit));
> > +	WARN_ON(!drm_sched_is_stopped(iter.sched));
> > }
> > 
> > DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
> > @@ -750,4 +761,21 @@ class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T
> > 		list_for_each_entry((__job), &(__sched)->pending_list, list)	\
> > 			for_each_if(!(__entity) || (__job)->entity == (__entity))
> > 
> > +/**
> > + * drm_sched_job_is_signaled() - DRM scheduler job is signaled
> > + * @job: DRM scheduler job
> > + *
> > + * Determine if DRM scheduler job is signaled. DRM scheduler should be stopped
> > + * to obtain a stable snapshot of state.
> > + *
> > + * Return: True if job is signaled, False otherwise
> > + */
> > +static inline bool drm_sched_job_is_signaled(struct drm_sched_job *job)
> > +{
> > +	struct drm_sched_fence *s_fence = job->s_fence;
> > +
> > +	WARN_ON(!drm_sched_is_stopped(job->sched));
> > +	return dma_fence_is_signaled(&s_fence->finished);
> > +}
> 
> NIT..In patch#4 where xe driver uses this function in couple places,
> I am seeing originally it checks if the s_fence->parent is signaled
> instead of &s_fence->finished as done here.
> I do see below message in the 's_fence->parent' kernel-doc,
> "We signal the &drm_sched_fence.finished fence once parent is signalled."
> So, probably it is fine, but just want to ensure.
> 

It more or less is the same check. Techincally the hardware fence
(parent) can signal before software fence (finished) but it is pretty
small race window which practice should never be hit but I could make
this function more robust can check on the parent fence too, that is
probably better I guess. Let me change this.

Matt

> Niranjana
> 
> > +
> > #endif
> > -- 
> > 2.34.1
> > 

Re: [PATCH v3 1/7] drm/sched: Add pending job list iterator

[Matthew Brost]

On Fri, Nov 14, 2025 at 05:25:47PM -0800, Niranjana Vishwanathapura wrote:
> On Thu, Oct 16, 2025 at 01:48:20PM -0700, Matthew Brost wrote:
> > Stop open coding pending job list in drivers. Add pending job list
> > iterator which safely walks DRM scheduler list asserting DRM scheduler
> > is stopped.
> > 
> > v2:
> > - Fix checkpatch (CI)
> > v3:
> > - Drop locked version (Christian)
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > include/drm/gpu_scheduler.h | 52 +++++++++++++++++++++++++++++++++++++
> > 1 file changed, 52 insertions(+)
> > 
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index fb88301b3c45..7f31eba3bd61 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -698,4 +698,56 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> > 				   struct drm_gpu_scheduler **sched_list,
> > 				   unsigned int num_sched_list);
> > 
> > +/* Inlines */
> > +
> > +/**
> > + * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
> > + * @sched: DRM scheduler associated with pending job iterator
> > + */
> > +struct drm_sched_pending_job_iter {
> > +	struct drm_gpu_scheduler *sched;
> > +};
> > +
> > +/* Drivers should never call this directly */
> > +static inline struct drm_sched_pending_job_iter
> > +__drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
> > +{
> > +	struct drm_sched_pending_job_iter iter = {
> > +		.sched = sched,
> > +	};
> > +
> > +	WARN_ON(!READ_ONCE(sched->pause_submit));
> > +	return iter;
> > +}
> > +
> > +/* Drivers should never call this directly */
> > +static inline void
> > +__drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
> > +{
> > +	WARN_ON(!READ_ONCE(iter.sched->pause_submit));
> > +}
> 
> May be instead of these inline functions, we can add the code in a '({' block
> in the below DEFINE_CLASS itself to avoid drivers from calling these inline
> funcions? Though I agree these inline functions makes it cleaner to read.
> 

I'm not sure we can just call code inline from DEFINE_CLASS, rather only
functions.

> > +
> > +DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
> > +	     __drm_sched_pending_job_iter_end(_T),
> > +	     __drm_sched_pending_job_iter_begin(__sched),
> > +	     struct drm_gpu_scheduler *__sched);
> > +static inline void *
> > +class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T)
> > +{ return _T; }
> > +#define class_drm_sched_pending_job_iter_is_conditional false
> > +
> > +/**
> > + * drm_sched_for_each_pending_job() - Iterator for each pending job in scheduler
> > + * @__job: Current pending job being iterated over
> > + * @__sched: DRM scheduler to iterate over pending jobs
> > + * @__entity: DRM scheduler entity to filter jobs, NULL indicates no filter
> > + *
> > + * Iterator for each pending job in scheduler, filtering on an entity, and
> > + * enforcing scheduler is fully stopped
> > + */
> > +#define drm_sched_for_each_pending_job(__job, __sched, __entity)		\
> > +	scoped_guard(drm_sched_pending_job_iter, (__sched))			\
> > +		list_for_each_entry((__job), &(__sched)->pending_list, list)	\
> > +			for_each_if(!(__entity) || (__job)->entity == (__entity))
> > +
> 
> I am comparing it with DEFINE_CLASS usage in ttm driver here.
> It looks like the body of this macro (where we call list_for_each_entry()),
> doesn't use the drm_sched_pending_job_iter at all. So, looks like the only
> reason we are using a DEFINE_CLASS with scoped_guard here is for those
> WARN_ON() messages at the beginning and end of loop iteration, which is not
> fully fool proof. Right?

The drm_sched_pending_job_iter is for futuring proofing (e.g., if we
need more information than drm_gpu_scheduler, we have iterator
structure).

The define class is purpose is to ensure at the start of iterator and
end of the iterator the scheduler is paused which is only time we (DRM
scheduler maintainers) have agreed it is safe for driver to look at the
pending list. FWIW, this caught some bugs in Xe VF restore
implementation.

> I wonder if we really need DEFINE_CLASS here for that, though I am not
> against using it.
> 

So yes, I think a DEFINE_CLASS is apporiate here to implement the
iterator.

Matt

> Niranjana
> 
> > #endif
> > -- 
> > 2.34.1
> > 

Re: [PATCH v3 3/7] drm/xe: Add dedicated message lock

[Matthew Brost]

On Mon, Nov 17, 2025 at 11:58:46AM -0800, Niranjana Vishwanathapura wrote:
> On Thu, Oct 16, 2025 at 01:48:22PM -0700, Matthew Brost wrote:
> > Stop abusing DRM scheduler job list lock for messages, add dedicated
> > message lock.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> 
> LGTM.
> Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> 

Going to send this out on its own for CI and merge. Thanks for the review.

Matt

> > ---
> > drivers/gpu/drm/xe/xe_gpu_scheduler.c       | 5 +++--
> > drivers/gpu/drm/xe/xe_gpu_scheduler.h       | 4 ++--
> > drivers/gpu/drm/xe/xe_gpu_scheduler_types.h | 2 ++
> > 3 files changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > index f91e06d03511..f4f23317191f 100644
> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > @@ -77,6 +77,7 @@ int xe_sched_init(struct xe_gpu_scheduler *sched,
> > 	};
> > 
> > 	sched->ops = xe_ops;
> > +	spin_lock_init(&sched->msg_lock);
> > 	INIT_LIST_HEAD(&sched->msgs);
> > 	INIT_WORK(&sched->work_process_msg, xe_sched_process_msg_work);
> > 
> > @@ -117,7 +118,7 @@ void xe_sched_add_msg(struct xe_gpu_scheduler *sched,
> > void xe_sched_add_msg_locked(struct xe_gpu_scheduler *sched,
> > 			     struct xe_sched_msg *msg)
> > {
> > -	lockdep_assert_held(&sched->base.job_list_lock);
> > +	lockdep_assert_held(&sched->msg_lock);
> > 
> > 	list_add_tail(&msg->link, &sched->msgs);
> > 	xe_sched_process_msg_queue(sched);
> > @@ -131,7 +132,7 @@ void xe_sched_add_msg_locked(struct xe_gpu_scheduler *sched,
> > void xe_sched_add_msg_head(struct xe_gpu_scheduler *sched,
> > 			   struct xe_sched_msg *msg)
> > {
> > -	lockdep_assert_held(&sched->base.job_list_lock);
> > +	lockdep_assert_held(&sched->msg_lock);
> > 
> > 	list_add(&msg->link, &sched->msgs);
> > 	xe_sched_process_msg_queue(sched);
> > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > index 9955397aaaa9..b971b6b69419 100644
> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > @@ -33,12 +33,12 @@ void xe_sched_add_msg_head(struct xe_gpu_scheduler *sched,
> > 
> > static inline void xe_sched_msg_lock(struct xe_gpu_scheduler *sched)
> > {
> > -	spin_lock(&sched->base.job_list_lock);
> > +	spin_lock(&sched->msg_lock);
> > }
> > 
> > static inline void xe_sched_msg_unlock(struct xe_gpu_scheduler *sched)
> > {
> > -	spin_unlock(&sched->base.job_list_lock);
> > +	spin_unlock(&sched->msg_lock);
> > }
> > 
> > static inline void xe_sched_stop(struct xe_gpu_scheduler *sched)
> > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
> > index 6731b13da8bb..63d9bf92583c 100644
> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler_types.h
> > @@ -47,6 +47,8 @@ struct xe_gpu_scheduler {
> > 	const struct xe_sched_backend_ops	*ops;
> > 	/** @msgs: list of messages to be processed in @work_process_msg */
> > 	struct list_head			msgs;
> > +	/** @msg_lock: Message lock */
> > +	spinlock_t				msg_lock;
> > 	/** @work_process_msg: processes messages */
> > 	struct work_struct		work_process_msg;
> > };
> > -- 
> > 2.34.1
> > 

Re: [PATCH v3 4/7] drm/xe: Stop abusing DRM scheduler internals

[Matthew Brost]

On Mon, Nov 17, 2025 at 10:39:42PM -0800, Niranjana Vishwanathapura wrote:
> On Thu, Oct 16, 2025 at 01:48:23PM -0700, Matthew Brost wrote:
> > Use new pending job list iterator and new helper functions in Xe to
> > avoid reaching into DRM scheduler internals.
> > 
> > Part of this change involves removing pending jobs debug information
> > from debugfs and devcoredump. As agreed, the pending job list should
> > only be accessed when the scheduler is stopped. However, it's not
> > straightforward to determine whether the scheduler is stopped from the
> > shared debugfs/devcoredump code path. Additionally, the pending job list
> > provides little useful information, as pending jobs can be inferred from
> > seqnos and ring head/tail positions. Therefore, this debug information
> > is being removed.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gpu_scheduler.c    |  4 +-
> > drivers/gpu/drm/xe/xe_gpu_scheduler.h    | 34 +++--------
> > drivers/gpu/drm/xe/xe_guc_submit.c       | 74 ++++--------------------
> > drivers/gpu/drm/xe/xe_guc_submit_types.h | 11 ----
> > drivers/gpu/drm/xe/xe_hw_fence.c         | 16 -----
> > drivers/gpu/drm/xe/xe_hw_fence.h         |  2 -
> > 6 files changed, 20 insertions(+), 121 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > index f4f23317191f..9c8004d5dd91 100644
> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > @@ -7,7 +7,7 @@
> > 
> > static void xe_sched_process_msg_queue(struct xe_gpu_scheduler *sched)
> > {
> > -	if (!READ_ONCE(sched->base.pause_submit))
> > +	if (!drm_sched_is_stopped(&sched->base))
> > 		queue_work(sched->base.submit_wq, &sched->work_process_msg);
> > }
> > 
> > @@ -43,7 +43,7 @@ static void xe_sched_process_msg_work(struct work_struct *w)
> > 		container_of(w, struct xe_gpu_scheduler, work_process_msg);
> > 	struct xe_sched_msg *msg;
> > 
> > -	if (READ_ONCE(sched->base.pause_submit))
> > +	if (drm_sched_is_stopped(&sched->base))
> > 		return;
> > 
> > 	msg = xe_sched_get_msg(sched);
> > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > index b971b6b69419..583372a78140 100644
> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > @@ -55,14 +55,10 @@ static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler *sched)
> > {
> > 	struct drm_sched_job *s_job;
> > 
> > -	list_for_each_entry(s_job, &sched->base.pending_list, list) {
> > -		struct drm_sched_fence *s_fence = s_job->s_fence;
> > -		struct dma_fence *hw_fence = s_fence->parent;
> > -
> > +	drm_sched_for_each_pending_job(s_job, &sched->base, NULL)
> > 		if (to_xe_sched_job(s_job)->skip_emit ||
> > -		    (hw_fence && !dma_fence_is_signaled(hw_fence)))
> > +		    !drm_sched_job_is_signaled(s_job))
> > 			sched->base.ops->run_job(s_job);
> > -	}
> > }
> > 
> > static inline bool
> > @@ -71,14 +67,6 @@ xe_sched_invalidate_job(struct xe_sched_job *job, int threshold)
> > 	return drm_sched_invalidate_job(&job->drm, threshold);
> > }
> > 
> > -static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
> > -					    struct xe_sched_job *job)
> > -{
> > -	spin_lock(&sched->base.job_list_lock);
> > -	list_add(&job->drm.list, &sched->base.pending_list);
> > -	spin_unlock(&sched->base.job_list_lock);
> > -}
> > -
> > /**
> >  * xe_sched_first_pending_job() - Find first pending job which is unsignaled
> >  * @sched: Xe GPU scheduler
> > @@ -88,21 +76,13 @@ static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
> > static inline
> > struct xe_sched_job *xe_sched_first_pending_job(struct xe_gpu_scheduler *sched)
> > {
> > -	struct xe_sched_job *job, *r_job = NULL;
> > -
> > -	spin_lock(&sched->base.job_list_lock);
> > -	list_for_each_entry(job, &sched->base.pending_list, drm.list) {
> > -		struct drm_sched_fence *s_fence = job->drm.s_fence;
> > -		struct dma_fence *hw_fence = s_fence->parent;
> > +	struct drm_sched_job *job;
> > 
> > -		if (hw_fence && !dma_fence_is_signaled(hw_fence)) {
> > -			r_job = job;
> > -			break;
> > -		}
> > -	}
> > -	spin_unlock(&sched->base.job_list_lock);
> > +	drm_sched_for_each_pending_job(job, &sched->base, NULL)
> > +		if (!drm_sched_job_is_signaled(job))
> > +			return to_xe_sched_job(job);
> > 
> > -	return r_job;
> > +	return NULL;
> > }
> > 
> > static inline int
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 0ef67d3523a7..680696efc434 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -1032,7 +1032,7 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> > 	struct xe_exec_queue *q = ge->q;
> > 	struct xe_guc *guc = exec_queue_to_guc(q);
> > 	struct xe_gpu_scheduler *sched = &ge->sched;
> > -	struct xe_sched_job *job;
> > +	struct drm_sched_job *job;
> > 	bool wedged = false;
> > 
> > 	xe_gt_assert(guc_to_gt(guc), xe_exec_queue_is_lr(q));
> > @@ -1091,16 +1091,10 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> > 	if (!exec_queue_killed(q) && !xe_lrc_ring_is_idle(q->lrc[0]))
> > 		xe_devcoredump(q, NULL, "LR job cleanup, guc_id=%d", q->guc->id);
> > 
> > -	xe_hw_fence_irq_stop(q->fence_irq);
> > +	drm_sched_for_each_pending_job(job, &sched->base, NULL)
> > +		xe_sched_job_set_error(to_xe_sched_job(job), -ECANCELED);
> > 
> > 	xe_sched_submission_start(sched);
> > -
> > -	spin_lock(&sched->base.job_list_lock);
> > -	list_for_each_entry(job, &sched->base.pending_list, drm.list)
> > -		xe_sched_job_set_error(job, -ECANCELED);
> > -	spin_unlock(&sched->base.job_list_lock);
> > -
> > -	xe_hw_fence_irq_start(q->fence_irq);
> > }
> > 
> > #define ADJUST_FIVE_PERCENT(__t)	mul_u64_u32_div(__t, 105, 100)
> > @@ -1219,7 +1213,7 @@ static enum drm_gpu_sched_stat
> > guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > {
> > 	struct xe_sched_job *job = to_xe_sched_job(drm_job);
> > -	struct xe_sched_job *tmp_job;
> > +	struct drm_sched_job *tmp_job;
> > 	struct xe_exec_queue *q = job->q;
> > 	struct xe_gpu_scheduler *sched = &q->guc->sched;
> > 	struct xe_guc *guc = exec_queue_to_guc(q);
> > @@ -1228,7 +1222,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > 	unsigned int fw_ref;
> > 	int err = -ETIME;
> > 	pid_t pid = -1;
> > -	int i = 0;
> > 	bool wedged = false, skip_timeout_check;
> > 
> > 	xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
> > @@ -1395,28 +1388,15 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > 		__deregister_exec_queue(guc, q);
> > 	}
> > 
> > -	/* Stop fence signaling */
> > -	xe_hw_fence_irq_stop(q->fence_irq);
> > +	/* Mark all outstanding jobs as bad, thus completing them */
> > +	xe_sched_job_set_error(job, err);
> 
> This setting error for this timed out job is newly added.
> Why was it not there before and being added now?
> 

Because the TDR job was added back into the pending list first, so in
fact we did set the error on the job. 

> > +	drm_sched_for_each_pending_job(tmp_job, &sched->base, NULL)
> > +		xe_sched_job_set_error(to_xe_sched_job(tmp_job), -ECANCELED);
> > 
> > -	/*
> > -	 * Fence state now stable, stop / start scheduler which cleans up any
> > -	 * fences that are complete
> > -	 */
> > -	xe_sched_add_pending_job(sched, job);
> 
> Why xe_sched_add_pending_job() was there before?
> 

We (DRM scheduler maintainers agreed drivers shouldn't touch the pending
list), below returning DRM_GPU_SCHED_STAT_NO_HANG defers this step to
the DRM scheduler core.

> > 	xe_sched_submission_start(sched);
> > -
> > 	xe_guc_exec_queue_trigger_cleanup(q);
> 
> Why do we need to trigger cleanup again here?
> 

This is existing code and it should only be called once in this
function. At this point in time, we don't know if the TDR fired
naturally with a normal timeout value or if we are already in process of
cleaning up. If it is the former, then we switch to cleanup immediately
mode which is why this call is needed.

> > 
> > -	/* Mark all outstanding jobs as bad, thus completing them */
> > -	spin_lock(&sched->base.job_list_lock);
> > -	list_for_each_entry(tmp_job, &sched->base.pending_list, drm.list)
> > -		xe_sched_job_set_error(tmp_job, !i++ ? err : -ECANCELED);
> > -	spin_unlock(&sched->base.job_list_lock);
> > -
> > -	/* Start fence signaling */
> > -	xe_hw_fence_irq_start(q->fence_irq);
> > -
> > -	return DRM_GPU_SCHED_STAT_RESET;
> > +	return DRM_GPU_SCHED_STAT_NO_HANG;
> 
> This is error case. So, why return is changed to NO_HANG?
> 

See above, this how we can delete xe_sched_add_pending_job.

> Niranjana
> 
> > 
> > sched_enable:
> > 	set_exec_queue_pending_tdr_exit(q);
> > @@ -2244,7 +2224,7 @@ static void guc_exec_queue_unpause_prepare(struct xe_guc *guc,
> > 	struct drm_sched_job *s_job;
> > 	struct xe_sched_job *job = NULL;
> > 
> > -	list_for_each_entry(s_job, &sched->base.pending_list, list) {
> > +	drm_sched_for_each_pending_job(s_job, &sched->base, NULL) {
> > 		job = to_xe_sched_job(s_job);
> > 
> > 		xe_gt_dbg(guc_to_gt(guc), "Replay JOB - guc_id=%d, seqno=%d",
> > @@ -2349,7 +2329,7 @@ void xe_guc_submit_unpause(struct xe_guc *guc)
> > 		 * created after resfix done.
> > 		 */
> > 		if (q->guc->id != index ||
> > -		    !READ_ONCE(q->guc->sched.base.pause_submit))
> > +		    !drm_sched_is_stopped(&q->guc->sched.base))
> > 			continue;
> > 
> > 		guc_exec_queue_unpause(guc, q);
> > @@ -2771,30 +2751,6 @@ xe_guc_exec_queue_snapshot_capture(struct xe_exec_queue *q)
> > 	if (snapshot->parallel_execution)
> > 		guc_exec_queue_wq_snapshot_capture(q, snapshot);
> > 
> > -	spin_lock(&sched->base.job_list_lock);
> > -	snapshot->pending_list_size = list_count_nodes(&sched->base.pending_list);
> > -	snapshot->pending_list = kmalloc_array(snapshot->pending_list_size,
> > -					       sizeof(struct pending_list_snapshot),
> > -					       GFP_ATOMIC);
> > -
> > -	if (snapshot->pending_list) {
> > -		struct xe_sched_job *job_iter;
> > -
> > -		i = 0;
> > -		list_for_each_entry(job_iter, &sched->base.pending_list, drm.list) {
> > -			snapshot->pending_list[i].seqno =
> > -				xe_sched_job_seqno(job_iter);
> > -			snapshot->pending_list[i].fence =
> > -				dma_fence_is_signaled(job_iter->fence) ? 1 : 0;
> > -			snapshot->pending_list[i].finished =
> > -				dma_fence_is_signaled(&job_iter->drm.s_fence->finished)
> > -				? 1 : 0;
> > -			i++;
> > -		}
> > -	}
> > -
> > -	spin_unlock(&sched->base.job_list_lock);
> > -
> > 	return snapshot;
> > }
> > 
> > @@ -2852,13 +2808,6 @@ xe_guc_exec_queue_snapshot_print(struct xe_guc_submit_exec_queue_snapshot *snaps
> > 
> > 	if (snapshot->parallel_execution)
> > 		guc_exec_queue_wq_snapshot_print(snapshot, p);
> > -
> > -	for (i = 0; snapshot->pending_list && i < snapshot->pending_list_size;
> > -	     i++)
> > -		drm_printf(p, "\tJob: seqno=%d, fence=%d, finished=%d\n",
> > -			   snapshot->pending_list[i].seqno,
> > -			   snapshot->pending_list[i].fence,
> > -			   snapshot->pending_list[i].finished);
> > }
> > 
> > /**
> > @@ -2881,7 +2830,6 @@ void xe_guc_exec_queue_snapshot_free(struct xe_guc_submit_exec_queue_snapshot *s
> > 			xe_lrc_snapshot_free(snapshot->lrc[i]);
> > 		kfree(snapshot->lrc);
> > 	}
> > -	kfree(snapshot->pending_list);
> > 	kfree(snapshot);
> > }
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit_types.h b/drivers/gpu/drm/xe/xe_guc_submit_types.h
> > index dc7456c34583..0b08c79cf3b9 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit_types.h
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit_types.h
> > @@ -61,12 +61,6 @@ struct guc_submit_parallel_scratch {
> > 	u32 wq[WQ_SIZE / sizeof(u32)];
> > };
> > 
> > -struct pending_list_snapshot {
> > -	u32 seqno;
> > -	bool fence;
> > -	bool finished;
> > -};
> > -
> > /**
> >  * struct xe_guc_submit_exec_queue_snapshot - Snapshot for devcoredump
> >  */
> > @@ -134,11 +128,6 @@ struct xe_guc_submit_exec_queue_snapshot {
> > 		/** @wq: Workqueue Items */
> > 		u32 wq[WQ_SIZE / sizeof(u32)];
> > 	} parallel;
> > -
> > -	/** @pending_list_size: Size of the pending list snapshot array */
> > -	int pending_list_size;
> > -	/** @pending_list: snapshot of the pending list info */
> > -	struct pending_list_snapshot *pending_list;
> > };
> > 
> > #endif
> > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> > index b2a0c46dfcd4..e65dfcdfdbc5 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> > +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> > @@ -110,22 +110,6 @@ void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq)
> > 	irq_work_queue(&irq->work);
> > }
> > 
> > -void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq)
> > -{
> > -	spin_lock_irq(&irq->lock);
> > -	irq->enabled = false;
> > -	spin_unlock_irq(&irq->lock);
> > -}
> > -
> > -void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq)
> > -{
> > -	spin_lock_irq(&irq->lock);
> > -	irq->enabled = true;
> > -	spin_unlock_irq(&irq->lock);
> > -
> > -	irq_work_queue(&irq->work);
> > -}
> > -
> > void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
> > 			  struct xe_hw_fence_irq *irq, const char *name)
> > {
> > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h
> > index f13a1c4982c7..599492c13f80 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_fence.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_fence.h
> > @@ -17,8 +17,6 @@ void xe_hw_fence_module_exit(void);
> > void xe_hw_fence_irq_init(struct xe_hw_fence_irq *irq);
> > void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq);
> > void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq);
> > -void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq);
> > -void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq);
> > 
> > void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
> > 			  struct xe_hw_fence_irq *irq, const char *name);
> > -- 
> > 2.34.1
> > 

Re: [PATCH v3 5/7] drm/xe: Do not deregister queues in TDR

[Matthew Brost]

On Mon, Nov 17, 2025 at 10:41:52PM -0800, Niranjana Vishwanathapura wrote:
> On Thu, Oct 16, 2025 at 01:48:24PM -0700, Matthew Brost wrote:
> > Deregistering queues in the TDR introduces unnecessary complexity,
> > requiring reference counting tricks to function correctly. All that's
> > needed in the TDR is to kick the queue off the hardware, which is
> > achieved by disabling scheduling. Queue deregistration should be handled
> > in a single, well-defined point in the cleanup path, tied to the queue's
> > reference count.
> > 
> 
> Overall looks good to me.
> But it would help if the commit text describes why this extra reference
> taking was there before for lr jobs and why it is not needed now.
> 

This patch isn't related to LR jobs, the following patch is.

The deregistering queues in TDR was never required, and this patches
removes that flow.

Matt

> Niranjana
> 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_guc_submit.c | 57 +++---------------------------
> > 1 file changed, 5 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index 680696efc434..ab0f1a2d4871 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -69,9 +69,8 @@ exec_queue_to_guc(struct xe_exec_queue *q)
> > #define EXEC_QUEUE_STATE_WEDGED			(1 << 8)
> > #define EXEC_QUEUE_STATE_BANNED			(1 << 9)
> > #define EXEC_QUEUE_STATE_CHECK_TIMEOUT		(1 << 10)
> > -#define EXEC_QUEUE_STATE_EXTRA_REF		(1 << 11)
> > -#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 12)
> > -#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 13)
> > +#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 11)
> > +#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 12)
> > 
> > static bool exec_queue_registered(struct xe_exec_queue *q)
> > {
> > @@ -218,21 +217,6 @@ static void clear_exec_queue_check_timeout(struct xe_exec_queue *q)
> > 	atomic_and(~EXEC_QUEUE_STATE_CHECK_TIMEOUT, &q->guc->state);
> > }
> > 
> > -static bool exec_queue_extra_ref(struct xe_exec_queue *q)
> > -{
> > -	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_EXTRA_REF;
> > -}
> > -
> > -static void set_exec_queue_extra_ref(struct xe_exec_queue *q)
> > -{
> > -	atomic_or(EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
> > -}
> > -
> > -static void clear_exec_queue_extra_ref(struct xe_exec_queue *q)
> > -{
> > -	atomic_and(~EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
> > -}
> > -
> > static bool exec_queue_pending_resume(struct xe_exec_queue *q)
> > {
> > 	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_PENDING_RESUME;
> > @@ -1190,25 +1174,6 @@ static void disable_scheduling(struct xe_exec_queue *q, bool immediate)
> > 		       G2H_LEN_DW_SCHED_CONTEXT_MODE_SET, 1);
> > }
> > 
> > -static void __deregister_exec_queue(struct xe_guc *guc, struct xe_exec_queue *q)
> > -{
> > -	u32 action[] = {
> > -		XE_GUC_ACTION_DEREGISTER_CONTEXT,
> > -		q->guc->id,
> > -	};
> > -
> > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_destroyed(q));
> > -	xe_gt_assert(guc_to_gt(guc), exec_queue_registered(q));
> > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_enable(q));
> > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_disable(q));
> > -
> > -	set_exec_queue_destroyed(q);
> > -	trace_xe_exec_queue_deregister(q);
> > -
> > -	xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
> > -		       G2H_LEN_DW_DEREGISTER_CONTEXT, 1);
> > -}
> > -
> > static enum drm_gpu_sched_stat
> > guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > {
> > @@ -1326,8 +1291,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > 			xe_devcoredump(q, job,
> > 				       "Schedule disable failed to respond, guc_id=%d, ret=%d, guc_read=%d",
> > 				       q->guc->id, ret, xe_guc_read_stopped(guc));
> > -			set_exec_queue_extra_ref(q);
> > -			xe_exec_queue_get(q);	/* GT reset owns this */
> > 			set_exec_queue_banned(q);
> > 			xe_gt_reset_async(q->gt);
> > 			xe_sched_tdr_queue_imm(sched);
> > @@ -1380,13 +1343,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > 		}
> > 	}
> > 
> > -	/* Finish cleaning up exec queue via deregister */
> > 	set_exec_queue_banned(q);
> > -	if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
> > -		set_exec_queue_extra_ref(q);
> > -		xe_exec_queue_get(q);
> > -		__deregister_exec_queue(guc, q);
> > -	}
> > 
> > 	/* Mark all outstanding jobs as bad, thus completing them */
> > 	xe_sched_job_set_error(job, err);
> > @@ -1928,7 +1885,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
> > 
> > 	/* Clean up lost G2H + reset engine state */
> > 	if (exec_queue_registered(q)) {
> > -		if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
> > +		if (xe_exec_queue_is_lr(q))
> > 			xe_exec_queue_put(q);
> > 		else if (exec_queue_destroyed(q))
> > 			__guc_exec_queue_destroy(guc, q);
> > @@ -2062,11 +2019,7 @@ static void guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
> > 
> > 	if (exec_queue_destroyed(q) && exec_queue_registered(q)) {
> > 		clear_exec_queue_destroyed(q);
> > -		if (exec_queue_extra_ref(q))
> > -			xe_exec_queue_put(q);
> > -		else
> > -			q->guc->needs_cleanup = true;
> > -		clear_exec_queue_extra_ref(q);
> > +		q->guc->needs_cleanup = true;
> > 		xe_gt_dbg(guc_to_gt(guc), "Replay CLEANUP - guc_id=%d",
> > 			  q->guc->id);
> > 	}
> > @@ -2483,7 +2436,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
> > 
> > 	clear_exec_queue_registered(q);
> > 
> > -	if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
> > +	if (xe_exec_queue_is_lr(q))
> > 		xe_exec_queue_put(q);
> > 	else
> > 		__guc_exec_queue_destroy(guc, q);
> > -- 
> > 2.34.1
> > 

Re: [PATCH v3 6/7] drm/xe: Remove special casing for LR queues in submission

[Matthew Brost]

On Mon, Nov 17, 2025 at 10:45:31PM -0800, Niranjana Vishwanathapura wrote:
> On Thu, Oct 16, 2025 at 01:48:25PM -0700, Matthew Brost wrote:
> > Now that LR jobs are tracked by the DRM scheduler, there's no longer a
> > need to special-case LR queues. This change removes all LR
> > queue-specific handling, including dedicated TDR logic, reference
> > counting schemes, and other related mechanisms.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_guc_exec_queue_types.h |   2 -
> > drivers/gpu/drm/xe/xe_guc_submit.c           | 129 +------------------
> > 2 files changed, 7 insertions(+), 124 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> > index a3b034e4b205..fd0915ed8eb1 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> > +++ b/drivers/gpu/drm/xe/xe_guc_exec_queue_types.h
> > @@ -33,8 +33,6 @@ struct xe_guc_exec_queue {
> > 	 */
> > #define MAX_STATIC_MSG_TYPE	3
> > 	struct xe_sched_msg static_msgs[MAX_STATIC_MSG_TYPE];
> > -	/** @lr_tdr: long running TDR worker */
> > -	struct work_struct lr_tdr;
> > 	/** @destroy_async: do final destroy async from this worker */
> > 	struct work_struct destroy_async;
> > 	/** @resume_time: time of last resume */
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index ab0f1a2d4871..bb1f2929441c 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -674,14 +674,6 @@ static void register_exec_queue(struct xe_exec_queue *q, int ctx_type)
> > 		parallel_write(xe, map, wq_desc.wq_status, WQ_STATUS_ACTIVE);
> > 	}
> > 
> > -	/*
> > -	 * We must keep a reference for LR engines if engine is registered with
> > -	 * the GuC as jobs signal immediately and can't destroy an engine if the
> > -	 * GuC has a reference to it.
> > -	 */
> > -	if (xe_exec_queue_is_lr(q))
> > -		xe_exec_queue_get(q);
> > -
> > 	set_exec_queue_registered(q);
> > 	trace_xe_exec_queue_register(q);
> > 	if (xe_exec_queue_is_parallel(q))
> > @@ -854,7 +846,7 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job)
> > 	struct xe_sched_job *job = to_xe_sched_job(drm_job);
> > 	struct xe_exec_queue *q = job->q;
> > 	struct xe_guc *guc = exec_queue_to_guc(q);
> > -	bool lr = xe_exec_queue_is_lr(q), killed_or_banned_or_wedged =
> > +	bool killed_or_banned_or_wedged =
> > 		exec_queue_killed_or_banned_or_wedged(q);
> > 
> > 	xe_gt_assert(guc_to_gt(guc), !(exec_queue_destroyed(q) || exec_queue_pending_disable(q)) ||
> > @@ -871,15 +863,6 @@ guc_exec_queue_run_job(struct drm_sched_job *drm_job)
> > 		job->skip_emit = false;
> > 	}
> > 
> > -	/*
> > -	 * We don't care about job-fence ordering in LR VMs because these fences
> > -	 * are never exported; they are used solely to keep jobs on the pending
> > -	 * list. Once a queue enters an error state, there's no need to track
> > -	 * them.
> > -	 */
> > -	if (killed_or_banned_or_wedged && lr)
> > -		xe_sched_job_set_error(job, -ECANCELED);
> > -
> 
> Why this piece of code here is being removed?
> 

The TDR will always run for LR jobs now, that path will error out the
job. Prior to this, the LR cleanup function only ran once.

> > 	return job->fence;
> > }
> > 
> > @@ -923,8 +906,7 @@ static void disable_scheduling_deregister(struct xe_guc *guc,
> > 		xe_gt_warn(q->gt, "Pending enable/disable failed to respond\n");
> > 		xe_sched_submission_start(sched);
> > 		xe_gt_reset_async(q->gt);
> > -		if (!xe_exec_queue_is_lr(q))
> > -			xe_sched_tdr_queue_imm(sched);
> > +		xe_sched_tdr_queue_imm(sched);
> > 		return;
> > 	}
> > 
> > @@ -950,10 +932,7 @@ static void xe_guc_exec_queue_trigger_cleanup(struct xe_exec_queue *q)
> > 	/** to wakeup xe_wait_user_fence ioctl if exec queue is reset */
> > 	wake_up_all(&xe->ufence_wq);
> > 
> > -	if (xe_exec_queue_is_lr(q))
> > -		queue_work(guc_to_gt(guc)->ordered_wq, &q->guc->lr_tdr);
> > -	else
> > -		xe_sched_tdr_queue_imm(&q->guc->sched);
> > +	xe_sched_tdr_queue_imm(&q->guc->sched);
> > }
> > 
> > /**
> > @@ -1009,78 +988,6 @@ static bool guc_submit_hint_wedged(struct xe_guc *guc)
> > 	return true;
> > }
> > 
> > -static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> > -{
> > -	struct xe_guc_exec_queue *ge =
> > -		container_of(w, struct xe_guc_exec_queue, lr_tdr);
> > -	struct xe_exec_queue *q = ge->q;
> > -	struct xe_guc *guc = exec_queue_to_guc(q);
> > -	struct xe_gpu_scheduler *sched = &ge->sched;
> > -	struct drm_sched_job *job;
> > -	bool wedged = false;
> > -
> > -	xe_gt_assert(guc_to_gt(guc), xe_exec_queue_is_lr(q));
> > -
> > -	if (vf_recovery(guc))
> > -		return;
> > -
> > -	trace_xe_exec_queue_lr_cleanup(q);
> 
> Remove the trace event as well in xe_trace.h?
> 

Yes, will do.

Matt

> Niranjana
> 
> > -
> > -	if (!exec_queue_killed(q))
> > -		wedged = guc_submit_hint_wedged(exec_queue_to_guc(q));
> > -
> > -	/* Kill the run_job / process_msg entry points */
> > -	xe_sched_submission_stop(sched);
> > -
> > -	/*
> > -	 * Engine state now mostly stable, disable scheduling / deregister if
> > -	 * needed. This cleanup routine might be called multiple times, where
> > -	 * the actual async engine deregister drops the final engine ref.
> > -	 * Calling disable_scheduling_deregister will mark the engine as
> > -	 * destroyed and fire off the CT requests to disable scheduling /
> > -	 * deregister, which we only want to do once. We also don't want to mark
> > -	 * the engine as pending_disable again as this may race with the
> > -	 * xe_guc_deregister_done_handler() which treats it as an unexpected
> > -	 * state.
> > -	 */
> > -	if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
> > -		struct xe_guc *guc = exec_queue_to_guc(q);
> > -		int ret;
> > -
> > -		set_exec_queue_banned(q);
> > -		disable_scheduling_deregister(guc, q);
> > -
> > -		/*
> > -		 * Must wait for scheduling to be disabled before signalling
> > -		 * any fences, if GT broken the GT reset code should signal us.
> > -		 */
> > -		ret = wait_event_timeout(guc->ct.wq,
> > -					 !exec_queue_pending_disable(q) ||
> > -					 xe_guc_read_stopped(guc) ||
> > -					 vf_recovery(guc), HZ * 5);
> > -		if (vf_recovery(guc))
> > -			return;
> > -
> > -		if (!ret) {
> > -			xe_gt_warn(q->gt, "Schedule disable failed to respond, guc_id=%d\n",
> > -				   q->guc->id);
> > -			xe_devcoredump(q, NULL, "Schedule disable failed to respond, guc_id=%d\n",
> > -				       q->guc->id);
> > -			xe_sched_submission_start(sched);
> > -			xe_gt_reset_async(q->gt);
> > -			return;
> > -		}
> > -	}
> > -
> > -	if (!exec_queue_killed(q) && !xe_lrc_ring_is_idle(q->lrc[0]))
> > -		xe_devcoredump(q, NULL, "LR job cleanup, guc_id=%d", q->guc->id);
> > -
> > -	drm_sched_for_each_pending_job(job, &sched->base, NULL)
> > -		xe_sched_job_set_error(to_xe_sched_job(job), -ECANCELED);
> > -
> > -	xe_sched_submission_start(sched);
> > -}
> > -
> > #define ADJUST_FIVE_PERCENT(__t)	mul_u64_u32_div(__t, 105, 100)
> > 
> > static bool check_timeout(struct xe_exec_queue *q, struct xe_sched_job *job)
> > @@ -1150,8 +1057,7 @@ static void enable_scheduling(struct xe_exec_queue *q)
> > 		xe_gt_warn(guc_to_gt(guc), "Schedule enable failed to respond");
> > 		set_exec_queue_banned(q);
> > 		xe_gt_reset_async(q->gt);
> > -		if (!xe_exec_queue_is_lr(q))
> > -			xe_sched_tdr_queue_imm(&q->guc->sched);
> > +		xe_sched_tdr_queue_imm(&q->guc->sched);
> > 	}
> > }
> > 
> > @@ -1189,8 +1095,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > 	pid_t pid = -1;
> > 	bool wedged = false, skip_timeout_check;
> > 
> > -	xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
> > -
> > 	/*
> > 	 * TDR has fired before free job worker. Common if exec queue
> > 	 * immediately closed after last fence signaled. Add back to pending
> > @@ -1395,8 +1299,6 @@ static void __guc_exec_queue_destroy_async(struct work_struct *w)
> > 	xe_pm_runtime_get(guc_to_xe(guc));
> > 	trace_xe_exec_queue_destroy(q);
> > 
> > -	if (xe_exec_queue_is_lr(q))
> > -		cancel_work_sync(&ge->lr_tdr);
> > 	/* Confirm no work left behind accessing device structures */
> > 	cancel_delayed_work_sync(&ge->sched.base.work_tdr);
> > 
> > @@ -1629,9 +1531,6 @@ static int guc_exec_queue_init(struct xe_exec_queue *q)
> > 	if (err)
> > 		goto err_sched;
> > 
> > -	if (xe_exec_queue_is_lr(q))
> > -		INIT_WORK(&q->guc->lr_tdr, xe_guc_exec_queue_lr_cleanup);
> > -
> > 	mutex_lock(&guc->submission_state.lock);
> > 
> > 	err = alloc_guc_id(guc, q);
> > @@ -1885,9 +1784,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
> > 
> > 	/* Clean up lost G2H + reset engine state */
> > 	if (exec_queue_registered(q)) {
> > -		if (xe_exec_queue_is_lr(q))
> > -			xe_exec_queue_put(q);
> > -		else if (exec_queue_destroyed(q))
> > +		if (exec_queue_destroyed(q))
> > 			__guc_exec_queue_destroy(guc, q);
> > 	}
> > 	if (q->guc->suspend_pending) {
> > @@ -1917,9 +1814,6 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
> > 				trace_xe_sched_job_ban(job);
> > 				ban = true;
> > 			}
> > -		} else if (xe_exec_queue_is_lr(q) &&
> > -			   !xe_lrc_ring_is_idle(q->lrc[0])) {
> > -			ban = true;
> > 		}
> > 
> > 		if (ban) {
> > @@ -2002,8 +1896,6 @@ static void guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
> > 	if (pending_enable && !pending_resume &&
> > 	    !exec_queue_pending_tdr_exit(q)) {
> > 		clear_exec_queue_registered(q);
> > -		if (xe_exec_queue_is_lr(q))
> > -			xe_exec_queue_put(q);
> > 		xe_gt_dbg(guc_to_gt(guc), "Replay REGISTER - guc_id=%d",
> > 			  q->guc->id);
> > 	}
> > @@ -2060,10 +1952,7 @@ static void guc_exec_queue_pause(struct xe_guc *guc, struct xe_exec_queue *q)
> > 
> > 	/* Stop scheduling + flush any DRM scheduler operations */
> > 	xe_sched_submission_stop(sched);
> > -	if (xe_exec_queue_is_lr(q))
> > -		cancel_work_sync(&q->guc->lr_tdr);
> > -	else
> > -		cancel_delayed_work_sync(&sched->base.work_tdr);
> > +	cancel_delayed_work_sync(&sched->base.work_tdr);
> > 
> > 	guc_exec_queue_revert_pending_state_change(guc, q);
> > 
> > @@ -2435,11 +2324,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
> > 	trace_xe_exec_queue_deregister_done(q);
> > 
> > 	clear_exec_queue_registered(q);
> > -
> > -	if (xe_exec_queue_is_lr(q))
> > -		xe_exec_queue_put(q);
> > -	else
> > -		__guc_exec_queue_destroy(guc, q);
> > +	__guc_exec_queue_destroy(guc, q);
> > }
> > 
> > int xe_guc_deregister_done_handler(struct xe_guc *guc, u32 *msg, u32 len)
> > -- 
> > 2.34.1
> > 

Re: [PATCH v3 7/7] drm/xe: Only toggle scheduling in TDR if GuC is running

[Matthew Brost]

On Fri, Nov 14, 2025 at 05:01:45PM -0800, Niranjana Vishwanathapura wrote:
> On Thu, Oct 16, 2025 at 01:48:26PM -0700, Matthew Brost wrote:
> > If the firmware is not running during TDR (e.g., when the driver is
> > unloading), there's no need to toggle scheduling in the GuC. In such
> > cases, skip this step.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_guc_submit.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > index bb1f2929441c..ea0cfd866981 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > @@ -1146,7 +1146,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > 		if (exec_queue_reset(q))
> > 			err = -EIO;
> > 
> > -		if (!exec_queue_destroyed(q)) {
> > +		if (!exec_queue_destroyed(q) && xe_uc_fw_is_running(&guc->fw)) {
> > 			/*
> > 			 * Wait for any pending G2H to flush out before
> > 			 * modifying state
> 
> Looking at the code, it seems like if we skip this 'if' statement (when fw is
> not running), then it will go wait for ct->wq. Not sure how that gets woken up
> and logic might try to reset gt after that? Not sure if we should check
> xe_uc_fw_is_running() here will one of the conditions to wait_event_timeout()
> call cover this case and we can handle it appropriately after wait_event_timeout()
> returns?

I believe exec_queue_pending_disable will never be true, but maybe there
is race there. Let me and this condition for safety.

Matt

> 
> Niranjana
> 
> > -- 
> > 2.34.1
> > 

Re: [PATCH v3 1/7] drm/sched: Add pending job list iterator

[Niranjana Vishwanathapura]

On Tue, Nov 18, 2025 at 09:52:40AM -0800, Matthew Brost wrote:
>On Fri, Nov 14, 2025 at 05:25:47PM -0800, Niranjana Vishwanathapura wrote:
>> On Thu, Oct 16, 2025 at 01:48:20PM -0700, Matthew Brost wrote:
>> > Stop open coding pending job list in drivers. Add pending job list
>> > iterator which safely walks DRM scheduler list asserting DRM scheduler
>> > is stopped.
>> >
>> > v2:
>> > - Fix checkpatch (CI)
>> > v3:
>> > - Drop locked version (Christian)
>> >
>> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> > ---
>> > include/drm/gpu_scheduler.h | 52 +++++++++++++++++++++++++++++++++++++
>> > 1 file changed, 52 insertions(+)
>> >
>> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> > index fb88301b3c45..7f31eba3bd61 100644
>> > --- a/include/drm/gpu_scheduler.h
>> > +++ b/include/drm/gpu_scheduler.h
>> > @@ -698,4 +698,56 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
>> > 				   struct drm_gpu_scheduler **sched_list,
>> > 				   unsigned int num_sched_list);
>> >
>> > +/* Inlines */
>> > +
>> > +/**
>> > + * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
>> > + * @sched: DRM scheduler associated with pending job iterator
>> > + */
>> > +struct drm_sched_pending_job_iter {
>> > +	struct drm_gpu_scheduler *sched;
>> > +};
>> > +
>> > +/* Drivers should never call this directly */
>> > +static inline struct drm_sched_pending_job_iter
>> > +__drm_sched_pending_job_iter_begin(struct drm_gpu_scheduler *sched)
>> > +{
>> > +	struct drm_sched_pending_job_iter iter = {
>> > +		.sched = sched,
>> > +	};
>> > +
>> > +	WARN_ON(!READ_ONCE(sched->pause_submit));
>> > +	return iter;
>> > +}
>> > +
>> > +/* Drivers should never call this directly */
>> > +static inline void
>> > +__drm_sched_pending_job_iter_end(const struct drm_sched_pending_job_iter iter)
>> > +{
>> > +	WARN_ON(!READ_ONCE(iter.sched->pause_submit));
>> > +}
>>
>> May be instead of these inline functions, we can add the code in a '({' block
>> in the below DEFINE_CLASS itself to avoid drivers from calling these inline
>> funcions? Though I agree these inline functions makes it cleaner to read.
>>
>
>I'm not sure we can just call code inline from DEFINE_CLASS, rather only
>functions.

I do see some examples of it.
https://elixir.bootlin.com/linux/v6.18-rc6/source/drivers/gpu/drm/xe/xe_validation.h#L167
https://elixir.bootlin.com/linux/v6.18-rc6/source/drivers/gpio/gpiolib.h#L229

But DEFINE_CLASS also inserts static inline functions here. So, not super critical.

>
>> > +
>> > +DEFINE_CLASS(drm_sched_pending_job_iter, struct drm_sched_pending_job_iter,
>> > +	     __drm_sched_pending_job_iter_end(_T),
>> > +	     __drm_sched_pending_job_iter_begin(__sched),
>> > +	     struct drm_gpu_scheduler *__sched);
>> > +static inline void *
>> > +class_drm_sched_pending_job_iter_lock_ptr(class_drm_sched_pending_job_iter_t *_T)
>> > +{ return _T; }
>> > +#define class_drm_sched_pending_job_iter_is_conditional false
>> > +
>> > +/**
>> > + * drm_sched_for_each_pending_job() - Iterator for each pending job in scheduler
>> > + * @__job: Current pending job being iterated over
>> > + * @__sched: DRM scheduler to iterate over pending jobs
>> > + * @__entity: DRM scheduler entity to filter jobs, NULL indicates no filter
>> > + *
>> > + * Iterator for each pending job in scheduler, filtering on an entity, and
>> > + * enforcing scheduler is fully stopped
>> > + */
>> > +#define drm_sched_for_each_pending_job(__job, __sched, __entity)		\
>> > +	scoped_guard(drm_sched_pending_job_iter, (__sched))			\
>> > +		list_for_each_entry((__job), &(__sched)->pending_list, list)	\
>> > +			for_each_if(!(__entity) || (__job)->entity == (__entity))
>> > +
>>
>> I am comparing it with DEFINE_CLASS usage in ttm driver here.
>> It looks like the body of this macro (where we call list_for_each_entry()),
>> doesn't use the drm_sched_pending_job_iter at all. So, looks like the only
>> reason we are using a DEFINE_CLASS with scoped_guard here is for those
>> WARN_ON() messages at the beginning and end of loop iteration, which is not
>> fully fool proof. Right?
>
>The drm_sched_pending_job_iter is for futuring proofing (e.g., if we
>need more information than drm_gpu_scheduler, we have iterator
>structure).
>
>The define class is purpose is to ensure at the start of iterator and
>end of the iterator the scheduler is paused which is only time we (DRM
>scheduler maintainers) have agreed it is safe for driver to look at the
>pending list. FWIW, this caught some bugs in Xe VF restore
>implementation.
>
>> I wonder if we really need DEFINE_CLASS here for that, though I am not
>> against using it.
>>
>
>So yes, I think a DEFINE_CLASS is apporiate here to implement the
>iterator.
>

Ok, thanks.

Nianjana

>Matt
>
>> Niranjana
>>
>> > #endif
>> > --
>> > 2.34.1
>> >

Re: [PATCH v3 4/7] drm/xe: Stop abusing DRM scheduler internals

[Niranjana Vishwanathapura]

On Tue, Nov 18, 2025 at 09:59:32AM -0800, Matthew Brost wrote:
>On Mon, Nov 17, 2025 at 10:39:42PM -0800, Niranjana Vishwanathapura wrote:
>> On Thu, Oct 16, 2025 at 01:48:23PM -0700, Matthew Brost wrote:
>> > Use new pending job list iterator and new helper functions in Xe to
>> > avoid reaching into DRM scheduler internals.
>> >
>> > Part of this change involves removing pending jobs debug information
>> > from debugfs and devcoredump. As agreed, the pending job list should
>> > only be accessed when the scheduler is stopped. However, it's not
>> > straightforward to determine whether the scheduler is stopped from the
>> > shared debugfs/devcoredump code path. Additionally, the pending job list
>> > provides little useful information, as pending jobs can be inferred from
>> > seqnos and ring head/tail positions. Therefore, this debug information
>> > is being removed.
>> >
>> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_gpu_scheduler.c    |  4 +-
>> > drivers/gpu/drm/xe/xe_gpu_scheduler.h    | 34 +++--------
>> > drivers/gpu/drm/xe/xe_guc_submit.c       | 74 ++++--------------------
>> > drivers/gpu/drm/xe/xe_guc_submit_types.h | 11 ----
>> > drivers/gpu/drm/xe/xe_hw_fence.c         | 16 -----
>> > drivers/gpu/drm/xe/xe_hw_fence.h         |  2 -
>> > 6 files changed, 20 insertions(+), 121 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>> > index f4f23317191f..9c8004d5dd91 100644
>> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
>> > @@ -7,7 +7,7 @@
>> >
>> > static void xe_sched_process_msg_queue(struct xe_gpu_scheduler *sched)
>> > {
>> > -	if (!READ_ONCE(sched->base.pause_submit))
>> > +	if (!drm_sched_is_stopped(&sched->base))
>> > 		queue_work(sched->base.submit_wq, &sched->work_process_msg);
>> > }
>> >
>> > @@ -43,7 +43,7 @@ static void xe_sched_process_msg_work(struct work_struct *w)
>> > 		container_of(w, struct xe_gpu_scheduler, work_process_msg);
>> > 	struct xe_sched_msg *msg;
>> >
>> > -	if (READ_ONCE(sched->base.pause_submit))
>> > +	if (drm_sched_is_stopped(&sched->base))
>> > 		return;
>> >
>> > 	msg = xe_sched_get_msg(sched);
>> > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> > index b971b6b69419..583372a78140 100644
>> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
>> > @@ -55,14 +55,10 @@ static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler *sched)
>> > {
>> > 	struct drm_sched_job *s_job;
>> >
>> > -	list_for_each_entry(s_job, &sched->base.pending_list, list) {
>> > -		struct drm_sched_fence *s_fence = s_job->s_fence;
>> > -		struct dma_fence *hw_fence = s_fence->parent;
>> > -
>> > +	drm_sched_for_each_pending_job(s_job, &sched->base, NULL)
>> > 		if (to_xe_sched_job(s_job)->skip_emit ||
>> > -		    (hw_fence && !dma_fence_is_signaled(hw_fence)))
>> > +		    !drm_sched_job_is_signaled(s_job))
>> > 			sched->base.ops->run_job(s_job);
>> > -	}
>> > }
>> >
>> > static inline bool
>> > @@ -71,14 +67,6 @@ xe_sched_invalidate_job(struct xe_sched_job *job, int threshold)
>> > 	return drm_sched_invalidate_job(&job->drm, threshold);
>> > }
>> >
>> > -static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
>> > -					    struct xe_sched_job *job)
>> > -{
>> > -	spin_lock(&sched->base.job_list_lock);
>> > -	list_add(&job->drm.list, &sched->base.pending_list);
>> > -	spin_unlock(&sched->base.job_list_lock);
>> > -}
>> > -
>> > /**
>> >  * xe_sched_first_pending_job() - Find first pending job which is unsignaled
>> >  * @sched: Xe GPU scheduler
>> > @@ -88,21 +76,13 @@ static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
>> > static inline
>> > struct xe_sched_job *xe_sched_first_pending_job(struct xe_gpu_scheduler *sched)
>> > {
>> > -	struct xe_sched_job *job, *r_job = NULL;
>> > -
>> > -	spin_lock(&sched->base.job_list_lock);
>> > -	list_for_each_entry(job, &sched->base.pending_list, drm.list) {
>> > -		struct drm_sched_fence *s_fence = job->drm.s_fence;
>> > -		struct dma_fence *hw_fence = s_fence->parent;
>> > +	struct drm_sched_job *job;
>> >
>> > -		if (hw_fence && !dma_fence_is_signaled(hw_fence)) {
>> > -			r_job = job;
>> > -			break;
>> > -		}
>> > -	}
>> > -	spin_unlock(&sched->base.job_list_lock);
>> > +	drm_sched_for_each_pending_job(job, &sched->base, NULL)
>> > +		if (!drm_sched_job_is_signaled(job))
>> > +			return to_xe_sched_job(job);
>> >
>> > -	return r_job;
>> > +	return NULL;
>> > }
>> >
>> > static inline int
>> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> > index 0ef67d3523a7..680696efc434 100644
>> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> > @@ -1032,7 +1032,7 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
>> > 	struct xe_exec_queue *q = ge->q;
>> > 	struct xe_guc *guc = exec_queue_to_guc(q);
>> > 	struct xe_gpu_scheduler *sched = &ge->sched;
>> > -	struct xe_sched_job *job;
>> > +	struct drm_sched_job *job;
>> > 	bool wedged = false;
>> >
>> > 	xe_gt_assert(guc_to_gt(guc), xe_exec_queue_is_lr(q));
>> > @@ -1091,16 +1091,10 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
>> > 	if (!exec_queue_killed(q) && !xe_lrc_ring_is_idle(q->lrc[0]))
>> > 		xe_devcoredump(q, NULL, "LR job cleanup, guc_id=%d", q->guc->id);
>> >
>> > -	xe_hw_fence_irq_stop(q->fence_irq);
>> > +	drm_sched_for_each_pending_job(job, &sched->base, NULL)
>> > +		xe_sched_job_set_error(to_xe_sched_job(job), -ECANCELED);
>> >
>> > 	xe_sched_submission_start(sched);
>> > -
>> > -	spin_lock(&sched->base.job_list_lock);
>> > -	list_for_each_entry(job, &sched->base.pending_list, drm.list)
>> > -		xe_sched_job_set_error(job, -ECANCELED);
>> > -	spin_unlock(&sched->base.job_list_lock);
>> > -
>> > -	xe_hw_fence_irq_start(q->fence_irq);
>> > }
>> >
>> > #define ADJUST_FIVE_PERCENT(__t)	mul_u64_u32_div(__t, 105, 100)
>> > @@ -1219,7 +1213,7 @@ static enum drm_gpu_sched_stat
>> > guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>> > {
>> > 	struct xe_sched_job *job = to_xe_sched_job(drm_job);
>> > -	struct xe_sched_job *tmp_job;
>> > +	struct drm_sched_job *tmp_job;
>> > 	struct xe_exec_queue *q = job->q;
>> > 	struct xe_gpu_scheduler *sched = &q->guc->sched;
>> > 	struct xe_guc *guc = exec_queue_to_guc(q);
>> > @@ -1228,7 +1222,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>> > 	unsigned int fw_ref;
>> > 	int err = -ETIME;
>> > 	pid_t pid = -1;
>> > -	int i = 0;
>> > 	bool wedged = false, skip_timeout_check;
>> >
>> > 	xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
>> > @@ -1395,28 +1388,15 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>> > 		__deregister_exec_queue(guc, q);
>> > 	}
>> >
>> > -	/* Stop fence signaling */
>> > -	xe_hw_fence_irq_stop(q->fence_irq);
>> > +	/* Mark all outstanding jobs as bad, thus completing them */
>> > +	xe_sched_job_set_error(job, err);
>>
>> This setting error for this timed out job is newly added.
>> Why was it not there before and being added now?
>>
>
>Because the TDR job was added back into the pending list first, so in
>fact we did set the error on the job.
>

Ok, got it. Thanks.

>> > +	drm_sched_for_each_pending_job(tmp_job, &sched->base, NULL)
>> > +		xe_sched_job_set_error(to_xe_sched_job(tmp_job), -ECANCELED);
>> >
>> > -	/*
>> > -	 * Fence state now stable, stop / start scheduler which cleans up any
>> > -	 * fences that are complete
>> > -	 */
>> > -	xe_sched_add_pending_job(sched, job);
>>
>> Why xe_sched_add_pending_job() was there before?
>>
>
>We (DRM scheduler maintainers agreed drivers shouldn't touch the pending
>list), below returning DRM_GPU_SCHED_STAT_NO_HANG defers this step to
>the DRM scheduler core.
>
>> > 	xe_sched_submission_start(sched);
>> > -
>> > 	xe_guc_exec_queue_trigger_cleanup(q);
>>
>> Why do we need to trigger cleanup again here?
>>
>
>This is existing code and it should only be called once in this
>function. At this point in time, we don't know if the TDR fired
>naturally with a normal timeout value or if we are already in process of
>cleaning up. If it is the former, then we switch to cleanup immediately
>mode which is why this call is needed.
>
>> >
>> > -	/* Mark all outstanding jobs as bad, thus completing them */
>> > -	spin_lock(&sched->base.job_list_lock);
>> > -	list_for_each_entry(tmp_job, &sched->base.pending_list, drm.list)
>> > -		xe_sched_job_set_error(tmp_job, !i++ ? err : -ECANCELED);
>> > -	spin_unlock(&sched->base.job_list_lock);
>> > -
>> > -	/* Start fence signaling */
>> > -	xe_hw_fence_irq_start(q->fence_irq);
>> > -
>> > -	return DRM_GPU_SCHED_STAT_RESET;
>> > +	return DRM_GPU_SCHED_STAT_NO_HANG;
>>
>> This is error case. So, why return is changed to NO_HANG?
>>
>
>See above, this how we can delete xe_sched_add_pending_job.
>

Ok, returning NO_HANG here so that drm scheduler adds the job
back into the pending list. It is bit confusing to reader as
to why we return NO_HANG even the case of a hang (error)
condition here. May be a comment will help.

Niranjana

>> Niranjana
>>
>> >
>> > sched_enable:
>> > 	set_exec_queue_pending_tdr_exit(q);
>> > @@ -2244,7 +2224,7 @@ static void guc_exec_queue_unpause_prepare(struct xe_guc *guc,
>> > 	struct drm_sched_job *s_job;
>> > 	struct xe_sched_job *job = NULL;
>> >
>> > -	list_for_each_entry(s_job, &sched->base.pending_list, list) {
>> > +	drm_sched_for_each_pending_job(s_job, &sched->base, NULL) {
>> > 		job = to_xe_sched_job(s_job);
>> >
>> > 		xe_gt_dbg(guc_to_gt(guc), "Replay JOB - guc_id=%d, seqno=%d",
>> > @@ -2349,7 +2329,7 @@ void xe_guc_submit_unpause(struct xe_guc *guc)
>> > 		 * created after resfix done.
>> > 		 */
>> > 		if (q->guc->id != index ||
>> > -		    !READ_ONCE(q->guc->sched.base.pause_submit))
>> > +		    !drm_sched_is_stopped(&q->guc->sched.base))
>> > 			continue;
>> >
>> > 		guc_exec_queue_unpause(guc, q);
>> > @@ -2771,30 +2751,6 @@ xe_guc_exec_queue_snapshot_capture(struct xe_exec_queue *q)
>> > 	if (snapshot->parallel_execution)
>> > 		guc_exec_queue_wq_snapshot_capture(q, snapshot);
>> >
>> > -	spin_lock(&sched->base.job_list_lock);
>> > -	snapshot->pending_list_size = list_count_nodes(&sched->base.pending_list);
>> > -	snapshot->pending_list = kmalloc_array(snapshot->pending_list_size,
>> > -					       sizeof(struct pending_list_snapshot),
>> > -					       GFP_ATOMIC);
>> > -
>> > -	if (snapshot->pending_list) {
>> > -		struct xe_sched_job *job_iter;
>> > -
>> > -		i = 0;
>> > -		list_for_each_entry(job_iter, &sched->base.pending_list, drm.list) {
>> > -			snapshot->pending_list[i].seqno =
>> > -				xe_sched_job_seqno(job_iter);
>> > -			snapshot->pending_list[i].fence =
>> > -				dma_fence_is_signaled(job_iter->fence) ? 1 : 0;
>> > -			snapshot->pending_list[i].finished =
>> > -				dma_fence_is_signaled(&job_iter->drm.s_fence->finished)
>> > -				? 1 : 0;
>> > -			i++;
>> > -		}
>> > -	}
>> > -
>> > -	spin_unlock(&sched->base.job_list_lock);
>> > -
>> > 	return snapshot;
>> > }
>> >
>> > @@ -2852,13 +2808,6 @@ xe_guc_exec_queue_snapshot_print(struct xe_guc_submit_exec_queue_snapshot *snaps
>> >
>> > 	if (snapshot->parallel_execution)
>> > 		guc_exec_queue_wq_snapshot_print(snapshot, p);
>> > -
>> > -	for (i = 0; snapshot->pending_list && i < snapshot->pending_list_size;
>> > -	     i++)
>> > -		drm_printf(p, "\tJob: seqno=%d, fence=%d, finished=%d\n",
>> > -			   snapshot->pending_list[i].seqno,
>> > -			   snapshot->pending_list[i].fence,
>> > -			   snapshot->pending_list[i].finished);
>> > }
>> >
>> > /**
>> > @@ -2881,7 +2830,6 @@ void xe_guc_exec_queue_snapshot_free(struct xe_guc_submit_exec_queue_snapshot *s
>> > 			xe_lrc_snapshot_free(snapshot->lrc[i]);
>> > 		kfree(snapshot->lrc);
>> > 	}
>> > -	kfree(snapshot->pending_list);
>> > 	kfree(snapshot);
>> > }
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit_types.h b/drivers/gpu/drm/xe/xe_guc_submit_types.h
>> > index dc7456c34583..0b08c79cf3b9 100644
>> > --- a/drivers/gpu/drm/xe/xe_guc_submit_types.h
>> > +++ b/drivers/gpu/drm/xe/xe_guc_submit_types.h
>> > @@ -61,12 +61,6 @@ struct guc_submit_parallel_scratch {
>> > 	u32 wq[WQ_SIZE / sizeof(u32)];
>> > };
>> >
>> > -struct pending_list_snapshot {
>> > -	u32 seqno;
>> > -	bool fence;
>> > -	bool finished;
>> > -};
>> > -
>> > /**
>> >  * struct xe_guc_submit_exec_queue_snapshot - Snapshot for devcoredump
>> >  */
>> > @@ -134,11 +128,6 @@ struct xe_guc_submit_exec_queue_snapshot {
>> > 		/** @wq: Workqueue Items */
>> > 		u32 wq[WQ_SIZE / sizeof(u32)];
>> > 	} parallel;
>> > -
>> > -	/** @pending_list_size: Size of the pending list snapshot array */
>> > -	int pending_list_size;
>> > -	/** @pending_list: snapshot of the pending list info */
>> > -	struct pending_list_snapshot *pending_list;
>> > };
>> >
>> > #endif
>> > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
>> > index b2a0c46dfcd4..e65dfcdfdbc5 100644
>> > --- a/drivers/gpu/drm/xe/xe_hw_fence.c
>> > +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
>> > @@ -110,22 +110,6 @@ void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq)
>> > 	irq_work_queue(&irq->work);
>> > }
>> >
>> > -void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq)
>> > -{
>> > -	spin_lock_irq(&irq->lock);
>> > -	irq->enabled = false;
>> > -	spin_unlock_irq(&irq->lock);
>> > -}
>> > -
>> > -void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq)
>> > -{
>> > -	spin_lock_irq(&irq->lock);
>> > -	irq->enabled = true;
>> > -	spin_unlock_irq(&irq->lock);
>> > -
>> > -	irq_work_queue(&irq->work);
>> > -}
>> > -
>> > void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
>> > 			  struct xe_hw_fence_irq *irq, const char *name)
>> > {
>> > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h
>> > index f13a1c4982c7..599492c13f80 100644
>> > --- a/drivers/gpu/drm/xe/xe_hw_fence.h
>> > +++ b/drivers/gpu/drm/xe/xe_hw_fence.h
>> > @@ -17,8 +17,6 @@ void xe_hw_fence_module_exit(void);
>> > void xe_hw_fence_irq_init(struct xe_hw_fence_irq *irq);
>> > void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq);
>> > void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq);
>> > -void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq);
>> > -void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq);
>> >
>> > void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
>> > 			  struct xe_hw_fence_irq *irq, const char *name);
>> > --
>> > 2.34.1
>> >

Re: [PATCH v3 5/7] drm/xe: Do not deregister queues in TDR

[Niranjana Vishwanathapura]

On Tue, Nov 18, 2025 at 10:02:00AM -0800, Matthew Brost wrote:
>On Mon, Nov 17, 2025 at 10:41:52PM -0800, Niranjana Vishwanathapura wrote:
>> On Thu, Oct 16, 2025 at 01:48:24PM -0700, Matthew Brost wrote:
>> > Deregistering queues in the TDR introduces unnecessary complexity,
>> > requiring reference counting tricks to function correctly. All that's
>> > needed in the TDR is to kick the queue off the hardware, which is
>> > achieved by disabling scheduling. Queue deregistration should be handled
>> > in a single, well-defined point in the cleanup path, tied to the queue's
>> > reference count.
>> >
>>
>> Overall looks good to me.
>> But it would help if the commit text describes why this extra reference
>> taking was there before for lr jobs and why it is not needed now.
>>
>
>This patch isn't related to LR jobs, the following patch is.
>

I was talking about the set/clear_exec_queue_extra_ref() and its usage
being removed in this patchset.

>The deregistering queues in TDR was never required, and this patches
>removes that flow.
>

Ok, thanks.

Niranjana

>Matt
>
>> Niranjana
>>
>> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_guc_submit.c | 57 +++---------------------------
>> > 1 file changed, 5 insertions(+), 52 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
>> > index 680696efc434..ab0f1a2d4871 100644
>> > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
>> > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
>> > @@ -69,9 +69,8 @@ exec_queue_to_guc(struct xe_exec_queue *q)
>> > #define EXEC_QUEUE_STATE_WEDGED			(1 << 8)
>> > #define EXEC_QUEUE_STATE_BANNED			(1 << 9)
>> > #define EXEC_QUEUE_STATE_CHECK_TIMEOUT		(1 << 10)
>> > -#define EXEC_QUEUE_STATE_EXTRA_REF		(1 << 11)
>> > -#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 12)
>> > -#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 13)
>> > +#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 11)
>> > +#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 12)
>> >
>> > static bool exec_queue_registered(struct xe_exec_queue *q)
>> > {
>> > @@ -218,21 +217,6 @@ static void clear_exec_queue_check_timeout(struct xe_exec_queue *q)
>> > 	atomic_and(~EXEC_QUEUE_STATE_CHECK_TIMEOUT, &q->guc->state);
>> > }
>> >
>> > -static bool exec_queue_extra_ref(struct xe_exec_queue *q)
>> > -{
>> > -	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_EXTRA_REF;
>> > -}
>> > -
>> > -static void set_exec_queue_extra_ref(struct xe_exec_queue *q)
>> > -{
>> > -	atomic_or(EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
>> > -}
>> > -
>> > -static void clear_exec_queue_extra_ref(struct xe_exec_queue *q)
>> > -{
>> > -	atomic_and(~EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
>> > -}
>> > -
>> > static bool exec_queue_pending_resume(struct xe_exec_queue *q)
>> > {
>> > 	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_PENDING_RESUME;
>> > @@ -1190,25 +1174,6 @@ static void disable_scheduling(struct xe_exec_queue *q, bool immediate)
>> > 		       G2H_LEN_DW_SCHED_CONTEXT_MODE_SET, 1);
>> > }
>> >
>> > -static void __deregister_exec_queue(struct xe_guc *guc, struct xe_exec_queue *q)
>> > -{
>> > -	u32 action[] = {
>> > -		XE_GUC_ACTION_DEREGISTER_CONTEXT,
>> > -		q->guc->id,
>> > -	};
>> > -
>> > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_destroyed(q));
>> > -	xe_gt_assert(guc_to_gt(guc), exec_queue_registered(q));
>> > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_enable(q));
>> > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_disable(q));
>> > -
>> > -	set_exec_queue_destroyed(q);
>> > -	trace_xe_exec_queue_deregister(q);
>> > -
>> > -	xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
>> > -		       G2H_LEN_DW_DEREGISTER_CONTEXT, 1);
>> > -}
>> > -
>> > static enum drm_gpu_sched_stat
>> > guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>> > {
>> > @@ -1326,8 +1291,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>> > 			xe_devcoredump(q, job,
>> > 				       "Schedule disable failed to respond, guc_id=%d, ret=%d, guc_read=%d",
>> > 				       q->guc->id, ret, xe_guc_read_stopped(guc));
>> > -			set_exec_queue_extra_ref(q);
>> > -			xe_exec_queue_get(q);	/* GT reset owns this */
>> > 			set_exec_queue_banned(q);
>> > 			xe_gt_reset_async(q->gt);
>> > 			xe_sched_tdr_queue_imm(sched);
>> > @@ -1380,13 +1343,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
>> > 		}
>> > 	}
>> >
>> > -	/* Finish cleaning up exec queue via deregister */
>> > 	set_exec_queue_banned(q);
>> > -	if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
>> > -		set_exec_queue_extra_ref(q);
>> > -		xe_exec_queue_get(q);
>> > -		__deregister_exec_queue(guc, q);
>> > -	}
>> >
>> > 	/* Mark all outstanding jobs as bad, thus completing them */
>> > 	xe_sched_job_set_error(job, err);
>> > @@ -1928,7 +1885,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
>> >
>> > 	/* Clean up lost G2H + reset engine state */
>> > 	if (exec_queue_registered(q)) {
>> > -		if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
>> > +		if (xe_exec_queue_is_lr(q))
>> > 			xe_exec_queue_put(q);
>> > 		else if (exec_queue_destroyed(q))
>> > 			__guc_exec_queue_destroy(guc, q);
>> > @@ -2062,11 +2019,7 @@ static void guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
>> >
>> > 	if (exec_queue_destroyed(q) && exec_queue_registered(q)) {
>> > 		clear_exec_queue_destroyed(q);
>> > -		if (exec_queue_extra_ref(q))
>> > -			xe_exec_queue_put(q);
>> > -		else
>> > -			q->guc->needs_cleanup = true;
>> > -		clear_exec_queue_extra_ref(q);
>> > +		q->guc->needs_cleanup = true;
>> > 		xe_gt_dbg(guc_to_gt(guc), "Replay CLEANUP - guc_id=%d",
>> > 			  q->guc->id);
>> > 	}
>> > @@ -2483,7 +2436,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
>> >
>> > 	clear_exec_queue_registered(q);
>> >
>> > -	if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
>> > +	if (xe_exec_queue_is_lr(q))
>> > 		xe_exec_queue_put(q);
>> > 	else
>> > 		__guc_exec_queue_destroy(guc, q);
>> > --
>> > 2.34.1
>> >

Re: [PATCH v3 4/7] drm/xe: Stop abusing DRM scheduler internals

[Matthew Brost]

On Tue, Nov 18, 2025 at 01:17:22PM -0800, Niranjana Vishwanathapura wrote:
> On Tue, Nov 18, 2025 at 09:59:32AM -0800, Matthew Brost wrote:
> > On Mon, Nov 17, 2025 at 10:39:42PM -0800, Niranjana Vishwanathapura wrote:
> > > On Thu, Oct 16, 2025 at 01:48:23PM -0700, Matthew Brost wrote:
> > > > Use new pending job list iterator and new helper functions in Xe to
> > > > avoid reaching into DRM scheduler internals.
> > > >
> > > > Part of this change involves removing pending jobs debug information
> > > > from debugfs and devcoredump. As agreed, the pending job list should
> > > > only be accessed when the scheduler is stopped. However, it's not
> > > > straightforward to determine whether the scheduler is stopped from the
> > > > shared debugfs/devcoredump code path. Additionally, the pending job list
> > > > provides little useful information, as pending jobs can be inferred from
> > > > seqnos and ring head/tail positions. Therefore, this debug information
> > > > is being removed.
> > > >
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_gpu_scheduler.c    |  4 +-
> > > > drivers/gpu/drm/xe/xe_gpu_scheduler.h    | 34 +++--------
> > > > drivers/gpu/drm/xe/xe_guc_submit.c       | 74 ++++--------------------
> > > > drivers/gpu/drm/xe/xe_guc_submit_types.h | 11 ----
> > > > drivers/gpu/drm/xe/xe_hw_fence.c         | 16 -----
> > > > drivers/gpu/drm/xe/xe_hw_fence.h         |  2 -
> > > > 6 files changed, 20 insertions(+), 121 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.c b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > > > index f4f23317191f..9c8004d5dd91 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.c
> > > > @@ -7,7 +7,7 @@
> > > >
> > > > static void xe_sched_process_msg_queue(struct xe_gpu_scheduler *sched)
> > > > {
> > > > -	if (!READ_ONCE(sched->base.pause_submit))
> > > > +	if (!drm_sched_is_stopped(&sched->base))
> > > > 		queue_work(sched->base.submit_wq, &sched->work_process_msg);
> > > > }
> > > >
> > > > @@ -43,7 +43,7 @@ static void xe_sched_process_msg_work(struct work_struct *w)
> > > > 		container_of(w, struct xe_gpu_scheduler, work_process_msg);
> > > > 	struct xe_sched_msg *msg;
> > > >
> > > > -	if (READ_ONCE(sched->base.pause_submit))
> > > > +	if (drm_sched_is_stopped(&sched->base))
> > > > 		return;
> > > >
> > > > 	msg = xe_sched_get_msg(sched);
> > > > diff --git a/drivers/gpu/drm/xe/xe_gpu_scheduler.h b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > > > index b971b6b69419..583372a78140 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > > > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > > > @@ -55,14 +55,10 @@ static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler *sched)
> > > > {
> > > > 	struct drm_sched_job *s_job;
> > > >
> > > > -	list_for_each_entry(s_job, &sched->base.pending_list, list) {
> > > > -		struct drm_sched_fence *s_fence = s_job->s_fence;
> > > > -		struct dma_fence *hw_fence = s_fence->parent;
> > > > -
> > > > +	drm_sched_for_each_pending_job(s_job, &sched->base, NULL)
> > > > 		if (to_xe_sched_job(s_job)->skip_emit ||
> > > > -		    (hw_fence && !dma_fence_is_signaled(hw_fence)))
> > > > +		    !drm_sched_job_is_signaled(s_job))
> > > > 			sched->base.ops->run_job(s_job);
> > > > -	}
> > > > }
> > > >
> > > > static inline bool
> > > > @@ -71,14 +67,6 @@ xe_sched_invalidate_job(struct xe_sched_job *job, int threshold)
> > > > 	return drm_sched_invalidate_job(&job->drm, threshold);
> > > > }
> > > >
> > > > -static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
> > > > -					    struct xe_sched_job *job)
> > > > -{
> > > > -	spin_lock(&sched->base.job_list_lock);
> > > > -	list_add(&job->drm.list, &sched->base.pending_list);
> > > > -	spin_unlock(&sched->base.job_list_lock);
> > > > -}
> > > > -
> > > > /**
> > > >  * xe_sched_first_pending_job() - Find first pending job which is unsignaled
> > > >  * @sched: Xe GPU scheduler
> > > > @@ -88,21 +76,13 @@ static inline void xe_sched_add_pending_job(struct xe_gpu_scheduler *sched,
> > > > static inline
> > > > struct xe_sched_job *xe_sched_first_pending_job(struct xe_gpu_scheduler *sched)
> > > > {
> > > > -	struct xe_sched_job *job, *r_job = NULL;
> > > > -
> > > > -	spin_lock(&sched->base.job_list_lock);
> > > > -	list_for_each_entry(job, &sched->base.pending_list, drm.list) {
> > > > -		struct drm_sched_fence *s_fence = job->drm.s_fence;
> > > > -		struct dma_fence *hw_fence = s_fence->parent;
> > > > +	struct drm_sched_job *job;
> > > >
> > > > -		if (hw_fence && !dma_fence_is_signaled(hw_fence)) {
> > > > -			r_job = job;
> > > > -			break;
> > > > -		}
> > > > -	}
> > > > -	spin_unlock(&sched->base.job_list_lock);
> > > > +	drm_sched_for_each_pending_job(job, &sched->base, NULL)
> > > > +		if (!drm_sched_job_is_signaled(job))
> > > > +			return to_xe_sched_job(job);
> > > >
> > > > -	return r_job;
> > > > +	return NULL;
> > > > }
> > > >
> > > > static inline int
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > index 0ef67d3523a7..680696efc434 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > @@ -1032,7 +1032,7 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> > > > 	struct xe_exec_queue *q = ge->q;
> > > > 	struct xe_guc *guc = exec_queue_to_guc(q);
> > > > 	struct xe_gpu_scheduler *sched = &ge->sched;
> > > > -	struct xe_sched_job *job;
> > > > +	struct drm_sched_job *job;
> > > > 	bool wedged = false;
> > > >
> > > > 	xe_gt_assert(guc_to_gt(guc), xe_exec_queue_is_lr(q));
> > > > @@ -1091,16 +1091,10 @@ static void xe_guc_exec_queue_lr_cleanup(struct work_struct *w)
> > > > 	if (!exec_queue_killed(q) && !xe_lrc_ring_is_idle(q->lrc[0]))
> > > > 		xe_devcoredump(q, NULL, "LR job cleanup, guc_id=%d", q->guc->id);
> > > >
> > > > -	xe_hw_fence_irq_stop(q->fence_irq);
> > > > +	drm_sched_for_each_pending_job(job, &sched->base, NULL)
> > > > +		xe_sched_job_set_error(to_xe_sched_job(job), -ECANCELED);
> > > >
> > > > 	xe_sched_submission_start(sched);
> > > > -
> > > > -	spin_lock(&sched->base.job_list_lock);
> > > > -	list_for_each_entry(job, &sched->base.pending_list, drm.list)
> > > > -		xe_sched_job_set_error(job, -ECANCELED);
> > > > -	spin_unlock(&sched->base.job_list_lock);
> > > > -
> > > > -	xe_hw_fence_irq_start(q->fence_irq);
> > > > }
> > > >
> > > > #define ADJUST_FIVE_PERCENT(__t)	mul_u64_u32_div(__t, 105, 100)
> > > > @@ -1219,7 +1213,7 @@ static enum drm_gpu_sched_stat
> > > > guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > > > {
> > > > 	struct xe_sched_job *job = to_xe_sched_job(drm_job);
> > > > -	struct xe_sched_job *tmp_job;
> > > > +	struct drm_sched_job *tmp_job;
> > > > 	struct xe_exec_queue *q = job->q;
> > > > 	struct xe_gpu_scheduler *sched = &q->guc->sched;
> > > > 	struct xe_guc *guc = exec_queue_to_guc(q);
> > > > @@ -1228,7 +1222,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > > > 	unsigned int fw_ref;
> > > > 	int err = -ETIME;
> > > > 	pid_t pid = -1;
> > > > -	int i = 0;
> > > > 	bool wedged = false, skip_timeout_check;
> > > >
> > > > 	xe_gt_assert(guc_to_gt(guc), !xe_exec_queue_is_lr(q));
> > > > @@ -1395,28 +1388,15 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > > > 		__deregister_exec_queue(guc, q);
> > > > 	}
> > > >
> > > > -	/* Stop fence signaling */
> > > > -	xe_hw_fence_irq_stop(q->fence_irq);
> > > > +	/* Mark all outstanding jobs as bad, thus completing them */
> > > > +	xe_sched_job_set_error(job, err);
> > > 
> > > This setting error for this timed out job is newly added.
> > > Why was it not there before and being added now?
> > > 
> > 
> > Because the TDR job was added back into the pending list first, so in
> > fact we did set the error on the job.
> > 
> 
> Ok, got it. Thanks.
> 
> > > > +	drm_sched_for_each_pending_job(tmp_job, &sched->base, NULL)
> > > > +		xe_sched_job_set_error(to_xe_sched_job(tmp_job), -ECANCELED);
> > > >
> > > > -	/*
> > > > -	 * Fence state now stable, stop / start scheduler which cleans up any
> > > > -	 * fences that are complete
> > > > -	 */
> > > > -	xe_sched_add_pending_job(sched, job);
> > > 
> > > Why xe_sched_add_pending_job() was there before?
> > > 
> > 
> > We (DRM scheduler maintainers agreed drivers shouldn't touch the pending
> > list), below returning DRM_GPU_SCHED_STAT_NO_HANG defers this step to
> > the DRM scheduler core.
> > 
> > > > 	xe_sched_submission_start(sched);
> > > > -
> > > > 	xe_guc_exec_queue_trigger_cleanup(q);
> > > 
> > > Why do we need to trigger cleanup again here?
> > > 
> > 
> > This is existing code and it should only be called once in this
> > function. At this point in time, we don't know if the TDR fired
> > naturally with a normal timeout value or if we are already in process of
> > cleaning up. If it is the former, then we switch to cleanup immediately
> > mode which is why this call is needed.
> > 
> > > >
> > > > -	/* Mark all outstanding jobs as bad, thus completing them */
> > > > -	spin_lock(&sched->base.job_list_lock);
> > > > -	list_for_each_entry(tmp_job, &sched->base.pending_list, drm.list)
> > > > -		xe_sched_job_set_error(tmp_job, !i++ ? err : -ECANCELED);
> > > > -	spin_unlock(&sched->base.job_list_lock);
> > > > -
> > > > -	/* Start fence signaling */
> > > > -	xe_hw_fence_irq_start(q->fence_irq);
> > > > -
> > > > -	return DRM_GPU_SCHED_STAT_RESET;
> > > > +	return DRM_GPU_SCHED_STAT_NO_HANG;
> > > 
> > > This is error case. So, why return is changed to NO_HANG?
> > > 
> > 
> > See above, this how we can delete xe_sched_add_pending_job.
> > 
> 
> Ok, returning NO_HANG here so that drm scheduler adds the job
> back into the pending list. It is bit confusing to reader as
> to why we return NO_HANG even the case of a hang (error)
> condition here. May be a comment will help.
> 

This is in the DRM scheduler doc, but will add comment here too.

Matt

> Niranjana
> 
> > > Niranjana
> > > 
> > > >
> > > > sched_enable:
> > > > 	set_exec_queue_pending_tdr_exit(q);
> > > > @@ -2244,7 +2224,7 @@ static void guc_exec_queue_unpause_prepare(struct xe_guc *guc,
> > > > 	struct drm_sched_job *s_job;
> > > > 	struct xe_sched_job *job = NULL;
> > > >
> > > > -	list_for_each_entry(s_job, &sched->base.pending_list, list) {
> > > > +	drm_sched_for_each_pending_job(s_job, &sched->base, NULL) {
> > > > 		job = to_xe_sched_job(s_job);
> > > >
> > > > 		xe_gt_dbg(guc_to_gt(guc), "Replay JOB - guc_id=%d, seqno=%d",
> > > > @@ -2349,7 +2329,7 @@ void xe_guc_submit_unpause(struct xe_guc *guc)
> > > > 		 * created after resfix done.
> > > > 		 */
> > > > 		if (q->guc->id != index ||
> > > > -		    !READ_ONCE(q->guc->sched.base.pause_submit))
> > > > +		    !drm_sched_is_stopped(&q->guc->sched.base))
> > > > 			continue;
> > > >
> > > > 		guc_exec_queue_unpause(guc, q);
> > > > @@ -2771,30 +2751,6 @@ xe_guc_exec_queue_snapshot_capture(struct xe_exec_queue *q)
> > > > 	if (snapshot->parallel_execution)
> > > > 		guc_exec_queue_wq_snapshot_capture(q, snapshot);
> > > >
> > > > -	spin_lock(&sched->base.job_list_lock);
> > > > -	snapshot->pending_list_size = list_count_nodes(&sched->base.pending_list);
> > > > -	snapshot->pending_list = kmalloc_array(snapshot->pending_list_size,
> > > > -					       sizeof(struct pending_list_snapshot),
> > > > -					       GFP_ATOMIC);
> > > > -
> > > > -	if (snapshot->pending_list) {
> > > > -		struct xe_sched_job *job_iter;
> > > > -
> > > > -		i = 0;
> > > > -		list_for_each_entry(job_iter, &sched->base.pending_list, drm.list) {
> > > > -			snapshot->pending_list[i].seqno =
> > > > -				xe_sched_job_seqno(job_iter);
> > > > -			snapshot->pending_list[i].fence =
> > > > -				dma_fence_is_signaled(job_iter->fence) ? 1 : 0;
> > > > -			snapshot->pending_list[i].finished =
> > > > -				dma_fence_is_signaled(&job_iter->drm.s_fence->finished)
> > > > -				? 1 : 0;
> > > > -			i++;
> > > > -		}
> > > > -	}
> > > > -
> > > > -	spin_unlock(&sched->base.job_list_lock);
> > > > -
> > > > 	return snapshot;
> > > > }
> > > >
> > > > @@ -2852,13 +2808,6 @@ xe_guc_exec_queue_snapshot_print(struct xe_guc_submit_exec_queue_snapshot *snaps
> > > >
> > > > 	if (snapshot->parallel_execution)
> > > > 		guc_exec_queue_wq_snapshot_print(snapshot, p);
> > > > -
> > > > -	for (i = 0; snapshot->pending_list && i < snapshot->pending_list_size;
> > > > -	     i++)
> > > > -		drm_printf(p, "\tJob: seqno=%d, fence=%d, finished=%d\n",
> > > > -			   snapshot->pending_list[i].seqno,
> > > > -			   snapshot->pending_list[i].fence,
> > > > -			   snapshot->pending_list[i].finished);
> > > > }
> > > >
> > > > /**
> > > > @@ -2881,7 +2830,6 @@ void xe_guc_exec_queue_snapshot_free(struct xe_guc_submit_exec_queue_snapshot *s
> > > > 			xe_lrc_snapshot_free(snapshot->lrc[i]);
> > > > 		kfree(snapshot->lrc);
> > > > 	}
> > > > -	kfree(snapshot->pending_list);
> > > > 	kfree(snapshot);
> > > > }
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit_types.h b/drivers/gpu/drm/xe/xe_guc_submit_types.h
> > > > index dc7456c34583..0b08c79cf3b9 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_submit_types.h
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_submit_types.h
> > > > @@ -61,12 +61,6 @@ struct guc_submit_parallel_scratch {
> > > > 	u32 wq[WQ_SIZE / sizeof(u32)];
> > > > };
> > > >
> > > > -struct pending_list_snapshot {
> > > > -	u32 seqno;
> > > > -	bool fence;
> > > > -	bool finished;
> > > > -};
> > > > -
> > > > /**
> > > >  * struct xe_guc_submit_exec_queue_snapshot - Snapshot for devcoredump
> > > >  */
> > > > @@ -134,11 +128,6 @@ struct xe_guc_submit_exec_queue_snapshot {
> > > > 		/** @wq: Workqueue Items */
> > > > 		u32 wq[WQ_SIZE / sizeof(u32)];
> > > > 	} parallel;
> > > > -
> > > > -	/** @pending_list_size: Size of the pending list snapshot array */
> > > > -	int pending_list_size;
> > > > -	/** @pending_list: snapshot of the pending list info */
> > > > -	struct pending_list_snapshot *pending_list;
> > > > };
> > > >
> > > > #endif
> > > > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.c b/drivers/gpu/drm/xe/xe_hw_fence.c
> > > > index b2a0c46dfcd4..e65dfcdfdbc5 100644
> > > > --- a/drivers/gpu/drm/xe/xe_hw_fence.c
> > > > +++ b/drivers/gpu/drm/xe/xe_hw_fence.c
> > > > @@ -110,22 +110,6 @@ void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq)
> > > > 	irq_work_queue(&irq->work);
> > > > }
> > > >
> > > > -void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq)
> > > > -{
> > > > -	spin_lock_irq(&irq->lock);
> > > > -	irq->enabled = false;
> > > > -	spin_unlock_irq(&irq->lock);
> > > > -}
> > > > -
> > > > -void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq)
> > > > -{
> > > > -	spin_lock_irq(&irq->lock);
> > > > -	irq->enabled = true;
> > > > -	spin_unlock_irq(&irq->lock);
> > > > -
> > > > -	irq_work_queue(&irq->work);
> > > > -}
> > > > -
> > > > void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
> > > > 			  struct xe_hw_fence_irq *irq, const char *name)
> > > > {
> > > > diff --git a/drivers/gpu/drm/xe/xe_hw_fence.h b/drivers/gpu/drm/xe/xe_hw_fence.h
> > > > index f13a1c4982c7..599492c13f80 100644
> > > > --- a/drivers/gpu/drm/xe/xe_hw_fence.h
> > > > +++ b/drivers/gpu/drm/xe/xe_hw_fence.h
> > > > @@ -17,8 +17,6 @@ void xe_hw_fence_module_exit(void);
> > > > void xe_hw_fence_irq_init(struct xe_hw_fence_irq *irq);
> > > > void xe_hw_fence_irq_finish(struct xe_hw_fence_irq *irq);
> > > > void xe_hw_fence_irq_run(struct xe_hw_fence_irq *irq);
> > > > -void xe_hw_fence_irq_stop(struct xe_hw_fence_irq *irq);
> > > > -void xe_hw_fence_irq_start(struct xe_hw_fence_irq *irq);
> > > >
> > > > void xe_hw_fence_ctx_init(struct xe_hw_fence_ctx *ctx, struct xe_gt *gt,
> > > > 			  struct xe_hw_fence_irq *irq, const char *name);
> > > > --
> > > > 2.34.1
> > > >

Re: [PATCH v3 5/7] drm/xe: Do not deregister queues in TDR

[Matthew Brost]

On Tue, Nov 18, 2025 at 01:19:16PM -0800, Niranjana Vishwanathapura wrote:
> On Tue, Nov 18, 2025 at 10:02:00AM -0800, Matthew Brost wrote:
> > On Mon, Nov 17, 2025 at 10:41:52PM -0800, Niranjana Vishwanathapura wrote:
> > > On Thu, Oct 16, 2025 at 01:48:24PM -0700, Matthew Brost wrote:
> > > > Deregistering queues in the TDR introduces unnecessary complexity,
> > > > requiring reference counting tricks to function correctly. All that's
> > > > needed in the TDR is to kick the queue off the hardware, which is
> > > > achieved by disabling scheduling. Queue deregistration should be handled
> > > > in a single, well-defined point in the cleanup path, tied to the queue's
> > > > reference count.
> > > >
> > > 
> > > Overall looks good to me.
> > > But it would help if the commit text describes why this extra reference
> > > taking was there before for lr jobs and why it is not needed now.
> > > 
> > 
> > This patch isn't related to LR jobs, the following patch is.
> > 
> 
> I was talking about the set/clear_exec_queue_extra_ref() and its usage
> being removed in this patchset.
> 

Oh, the extra was needed before to prevent the queue from disappearing
on the final put or by a GT reset while a deregister from the TDR was in
flight. It was a pretty hacky W/A to this odd UAF case. I can adjust the
commit message with to include this information.

Matt

> > The deregistering queues in TDR was never required, and this patches
> > removes that flow.
> > 
> 
> Ok, thanks.
> 
> Niranjana
> 
> > Matt
> > 
> > > Niranjana
> > > 
> > > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > > ---
> > > > drivers/gpu/drm/xe/xe_guc_submit.c | 57 +++---------------------------
> > > > 1 file changed, 5 insertions(+), 52 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_guc_submit.c b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > index 680696efc434..ab0f1a2d4871 100644
> > > > --- a/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > +++ b/drivers/gpu/drm/xe/xe_guc_submit.c
> > > > @@ -69,9 +69,8 @@ exec_queue_to_guc(struct xe_exec_queue *q)
> > > > #define EXEC_QUEUE_STATE_WEDGED			(1 << 8)
> > > > #define EXEC_QUEUE_STATE_BANNED			(1 << 9)
> > > > #define EXEC_QUEUE_STATE_CHECK_TIMEOUT		(1 << 10)
> > > > -#define EXEC_QUEUE_STATE_EXTRA_REF		(1 << 11)
> > > > -#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 12)
> > > > -#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 13)
> > > > +#define EXEC_QUEUE_STATE_PENDING_RESUME		(1 << 11)
> > > > +#define EXEC_QUEUE_STATE_PENDING_TDR_EXIT	(1 << 12)
> > > >
> > > > static bool exec_queue_registered(struct xe_exec_queue *q)
> > > > {
> > > > @@ -218,21 +217,6 @@ static void clear_exec_queue_check_timeout(struct xe_exec_queue *q)
> > > > 	atomic_and(~EXEC_QUEUE_STATE_CHECK_TIMEOUT, &q->guc->state);
> > > > }
> > > >
> > > > -static bool exec_queue_extra_ref(struct xe_exec_queue *q)
> > > > -{
> > > > -	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_EXTRA_REF;
> > > > -}
> > > > -
> > > > -static void set_exec_queue_extra_ref(struct xe_exec_queue *q)
> > > > -{
> > > > -	atomic_or(EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
> > > > -}
> > > > -
> > > > -static void clear_exec_queue_extra_ref(struct xe_exec_queue *q)
> > > > -{
> > > > -	atomic_and(~EXEC_QUEUE_STATE_EXTRA_REF, &q->guc->state);
> > > > -}
> > > > -
> > > > static bool exec_queue_pending_resume(struct xe_exec_queue *q)
> > > > {
> > > > 	return atomic_read(&q->guc->state) & EXEC_QUEUE_STATE_PENDING_RESUME;
> > > > @@ -1190,25 +1174,6 @@ static void disable_scheduling(struct xe_exec_queue *q, bool immediate)
> > > > 		       G2H_LEN_DW_SCHED_CONTEXT_MODE_SET, 1);
> > > > }
> > > >
> > > > -static void __deregister_exec_queue(struct xe_guc *guc, struct xe_exec_queue *q)
> > > > -{
> > > > -	u32 action[] = {
> > > > -		XE_GUC_ACTION_DEREGISTER_CONTEXT,
> > > > -		q->guc->id,
> > > > -	};
> > > > -
> > > > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_destroyed(q));
> > > > -	xe_gt_assert(guc_to_gt(guc), exec_queue_registered(q));
> > > > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_enable(q));
> > > > -	xe_gt_assert(guc_to_gt(guc), !exec_queue_pending_disable(q));
> > > > -
> > > > -	set_exec_queue_destroyed(q);
> > > > -	trace_xe_exec_queue_deregister(q);
> > > > -
> > > > -	xe_guc_ct_send(&guc->ct, action, ARRAY_SIZE(action),
> > > > -		       G2H_LEN_DW_DEREGISTER_CONTEXT, 1);
> > > > -}
> > > > -
> > > > static enum drm_gpu_sched_stat
> > > > guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > > > {
> > > > @@ -1326,8 +1291,6 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > > > 			xe_devcoredump(q, job,
> > > > 				       "Schedule disable failed to respond, guc_id=%d, ret=%d, guc_read=%d",
> > > > 				       q->guc->id, ret, xe_guc_read_stopped(guc));
> > > > -			set_exec_queue_extra_ref(q);
> > > > -			xe_exec_queue_get(q);	/* GT reset owns this */
> > > > 			set_exec_queue_banned(q);
> > > > 			xe_gt_reset_async(q->gt);
> > > > 			xe_sched_tdr_queue_imm(sched);
> > > > @@ -1380,13 +1343,7 @@ guc_exec_queue_timedout_job(struct drm_sched_job *drm_job)
> > > > 		}
> > > > 	}
> > > >
> > > > -	/* Finish cleaning up exec queue via deregister */
> > > > 	set_exec_queue_banned(q);
> > > > -	if (!wedged && exec_queue_registered(q) && !exec_queue_destroyed(q)) {
> > > > -		set_exec_queue_extra_ref(q);
> > > > -		xe_exec_queue_get(q);
> > > > -		__deregister_exec_queue(guc, q);
> > > > -	}
> > > >
> > > > 	/* Mark all outstanding jobs as bad, thus completing them */
> > > > 	xe_sched_job_set_error(job, err);
> > > > @@ -1928,7 +1885,7 @@ static void guc_exec_queue_stop(struct xe_guc *guc, struct xe_exec_queue *q)
> > > >
> > > > 	/* Clean up lost G2H + reset engine state */
> > > > 	if (exec_queue_registered(q)) {
> > > > -		if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
> > > > +		if (xe_exec_queue_is_lr(q))
> > > > 			xe_exec_queue_put(q);
> > > > 		else if (exec_queue_destroyed(q))
> > > > 			__guc_exec_queue_destroy(guc, q);
> > > > @@ -2062,11 +2019,7 @@ static void guc_exec_queue_revert_pending_state_change(struct xe_guc *guc,
> > > >
> > > > 	if (exec_queue_destroyed(q) && exec_queue_registered(q)) {
> > > > 		clear_exec_queue_destroyed(q);
> > > > -		if (exec_queue_extra_ref(q))
> > > > -			xe_exec_queue_put(q);
> > > > -		else
> > > > -			q->guc->needs_cleanup = true;
> > > > -		clear_exec_queue_extra_ref(q);
> > > > +		q->guc->needs_cleanup = true;
> > > > 		xe_gt_dbg(guc_to_gt(guc), "Replay CLEANUP - guc_id=%d",
> > > > 			  q->guc->id);
> > > > 	}
> > > > @@ -2483,7 +2436,7 @@ static void handle_deregister_done(struct xe_guc *guc, struct xe_exec_queue *q)
> > > >
> > > > 	clear_exec_queue_registered(q);
> > > >
> > > > -	if (exec_queue_extra_ref(q) || xe_exec_queue_is_lr(q))
> > > > +	if (xe_exec_queue_is_lr(q))
> > > > 		xe_exec_queue_put(q);
> > > > 	else
> > > > 		__guc_exec_queue_destroy(guc, q);
> > > > --
> > > > 2.34.1
> > > >