Re: [PATCH v7 4/9] drm/xe: Stop abusing DRM scheduler internals

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Dec 03, 2025 at 11:56:01AM +0100, Philipp Stanner wrote:
> On Mon, 2025-12-01 at 10:39 -0800, Matthew Brost wrote:
> > Use new pending job list iterator and new helper functions in Xe to
> > avoid reaching into DRM scheduler internals.
> 
> Cool.
> 
> Obviously this is your driver, but some comments below which you might
> want to take into account.
> 
> > 
> > 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.
> 
> This reads a bit like a contradiction to the first sentence.
> 
> > 
> > v4:
> >  - Add comment around DRM_GPU_SCHED_STAT_NO_HANG (Niranjana)
> 
> Revision info for just one of 7 revisions?
> 

Only v4 changed.

> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > Reviewed-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_gpu_scheduler.c    |  4 +-
> >  drivers/gpu/drm/xe/xe_gpu_scheduler.h    | 33 ++--------
> >  drivers/gpu/drm/xe/xe_guc_submit.c       | 81 ++++++------------------
> >  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, 27 insertions(+), 120 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);
> 
> Sharing the submit_wq is legal. But next-level cleanness would be if
> struct drm_gpu_scheduler's internal components wouldn't be touched.
> That's kind of a luxury request, though.
> 

Yes, perhaps a helper to extract the submit_wq too.

> >  }
> >  
> > @@ -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 dceb2cd0ee5b..664c2db56af3 100644
> > --- a/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > +++ b/drivers/gpu/drm/xe/xe_gpu_scheduler.h
> > @@ -56,12 +56,9 @@ static inline void xe_sched_resubmit_jobs(struct xe_gpu_scheduler *sched)
> >  	struct drm_sched_job *s_job;
> >  	bool restore_replay = false;
> >  
> > -	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) {
> >  		restore_replay |= to_xe_sched_job(s_job)->restore_replay;
> > -		if (restore_replay || (hw_fence && !dma_fence_is_signaled(hw_fence)))
> > +		if (restore_replay || !drm_sched_job_is_signaled(s_job))
> 
> So that's where this function is needed. You check whether that job in
> the pending_list is signaled. 
> 

Yes, during GT reset flows (think a device level reset) it is possible
we stop the scheduler between the window of a job signaling but before
free_job is called. We want avoid resubmission of jobs which have
signaled.

> >  			sched->base.ops->run_job(s_job);
> 
> Aaaaaahm. So you invoke your own callback. But basically just to access
> the function pointer I suppose?
> 
> Since this is effectively your drm_sched_resubmit_jobs(), it is
> definitely desirable to provide a text book example of how to do resets
> so that others can follow your usage.
> 

Yes, but drm_sched_resubmit_jobs() does some nonsense with dma-fence
that I don’t need here. Honestly, I’m a little unsure what that is
actually doing. We also use this function during VF restore after
migration. This is a multi-step process that needs to operate on the
same set of jobs at each step of the restore. That’s what the
restore_replay variable represents—it marks a job at the very beginning
of the restore process, and each step along the way ensures execution
starts at that job. Techincally once we here in a VF restore jobs can
start signaling as the hardware is live. So some of this really is
vendor-specific.

> Can't you replace ops->run_job() with a call to your functions where
> you push the jobs to the ring, directly?
> 

Yes, we could, but that function isn’t currently exported. Also, in
future products, we may assign a different run_job vfunc based on
hardware generation or queue type. So using a vfunc here makes sense as
a bit of future-proofing. Of course, we could also have a DRM
scheduler-level helper that invokes run_job for us.

Matt

> 
> P.
>