Re: [RFC PATCH v2 1/4] drm/sched: Add pending job list iterator

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

On Mon, Oct 06, 2025 at 12:19:29PM +0300, Jani Nikula wrote:
> On Fri, 03 Oct 2025, Matthew Brost <matthew.brost@intel.com> wrote:
> > Stop open coding pending job list in drivers. Add pending job list
> > iterator which safely walks DRM scheduler list either locklessly
> > asserting DRM scheduler is stopped or takes pending job list lock.
> >
> > v2:
> >  - Fix checkpatch (CI)
> >
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  include/drm/gpu_scheduler.h | 64 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> > index fb88301b3c45..bb49d8b715eb 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -698,4 +698,68 @@ void drm_sched_entity_modify_sched(struct drm_sched_entity *entity,
> >  				   struct drm_gpu_scheduler **sched_list,
> >  				   unsigned int num_sched_list);
> >  
> > +/* Inlines */
> 
> Do they need to be inlines for perf reasons? Otherwise, inlines just
> make proper encapsulation harder, proliferate header interdependencies,
> and make the incremental builds slower.
> 

The iterator needs to b inline as it is a macro. Everything else, no.
All the inlines are in this series are a couple of lines so stuck them
in header, easy enough to move if needed.

> Have you tried running the header through the compiler to see if it's
> self-contained?
> 

I would think they are self-contained but I'm not exactly sure what this
means.

Matt

> Unfortunately, DRM_HEADER_TEST still depends on BROKEN so we don't get
> that check as part of the build. :(
> 
> BR,
> Jani.
> 
> 
> > +
> > +/**
> > + * struct drm_sched_pending_job_iter - DRM scheduler pending job iterator state
> > + * @sched: DRM scheduler associated with pending job iterator
> > + * @stopped: DRM scheduler stopped state associated with pending job iterator
> > + */
> > +struct drm_sched_pending_job_iter {
> > +	struct drm_gpu_scheduler *sched;
> > +	bool stopped;
> > +};
> > +
> > +/* 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, bool stopped)
> > +{
> > +	struct drm_sched_pending_job_iter iter = {
> > +		.sched = sched,
> > +		.stopped = stopped,
> > +	};
> > +
> > +	if (stopped)
> > +		WARN_ON(!READ_ONCE(sched->pause_submit));
> > +	else
> > +		spin_lock(&sched->job_list_lock);
> > +
> > +	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)
> > +{
> > +	if (iter.stopped)
> > +		WARN_ON(!READ_ONCE(iter.sched->pause_submit));
> > +	else
> > +		spin_unlock(&iter.sched->job_list_lock);
> > +}
> > +
> > +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, __stopped),
> > +	     struct drm_gpu_scheduler *__sched, bool __stopped);
> > +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
> > + * @__stopped: DRM scheduler stopped state
> > + *
> > + * Iterator for each pending job in scheduler, filtering on an entity, and
> > + * enforcing locking rules (either scheduler fully stopped or correctly takes
> > + * job_list_lock).
> > + */
> > +#define drm_sched_for_each_pending_job(__job, __sched, __entity, __stopped)	\
> > +	scoped_guard(drm_sched_pending_job_iter, (__sched), (__stopped))	\
> > +		list_for_each_entry((__job), &(__sched)->pending_list, list)	\
> > +			for_each_if(!(__entity) || (__job)->entity == (__entity))
> > +
> >  #endif
> 
> -- 
> Jani Nikula, Intel