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

[Philipp Stanner <pstanner@redhat.com>]

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?

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

>  }
>  
> @@ -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. 

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

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


P.