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

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

On Tue, Nov 18, 2025 at 09:52:40AM -0800, Matthew Brost wrote:
>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.

I do see some examples of it.
https://elixir.bootlin.com/linux/v6.18-rc6/source/drivers/gpu/drm/xe/xe_validation.h#L167
https://elixir.bootlin.com/linux/v6.18-rc6/source/drivers/gpio/gpiolib.h#L229

But DEFINE_CLASS also inserts static inline functions here. So, not super critical.

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

Ok, thanks.

Nianjana

>Matt
>
>> Niranjana
>>
>> > #endif
>> > --
>> > 2.34.1
>> >