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

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

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