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

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

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
>> >