[PATCH 1/3] drm/sched: add drm_sched_prealloc_dependency_slots v3

[Philipp Stanner]

On Thu, 2025-05-15 at 17:17 +0100, Tvrtko Ursulin wrote:
> 
> On 15/05/2025 16:00, Christian König wrote:
> > Sometimes drivers need to be able to submit multiple jobs which
> > depend on
> > each other to different schedulers at the same time, but using
> > drm_sched_job_add_dependency() can't fail any more after the first
> > job is
> > initialized.
> > 
> > This function preallocate memory for dependency slots so that no
> > ENOMEM
> > can come later while adding dependencies.
> > 
> > v2: rework implementation an documentation
> > v3: rework from scratch, use separate function to add preallocated
> > deps

I think we agreed to not put change logs into commit messages anymore
:)

They aren't useful for any reader. Who needs the changelog afterwards
can retreive it through the mail thread link that we add.

> > 
> > Signed-off-by: Christian König <christian.koenig at amd.com>
> > ---
> >   drivers/gpu/drm/scheduler/sched_main.c | 45
> > ++++++++++++++++++++++++++
> >   include/drm/gpu_scheduler.h            |  4 +++
> >   2 files changed, 49 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > b/drivers/gpu/drm/scheduler/sched_main.c
> > index f7118497e47a..b95e7089aa70 100644
> > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > @@ -858,6 +858,51 @@ void drm_sched_job_arm(struct drm_sched_job
> > *job)
> >   }
> >   EXPORT_SYMBOL(drm_sched_job_arm);
> >   
> > +/**
> > + * drm_sched_job_prealloc_dependency_slot - avoid ENOMEM on adding
> > dependencies
> > + * @job: scheduler job where dependencies will be added
> > + * @id: id for the allocated slot
> > +  *
> > + * Sometimes drivers need to be able to submit multiple jobs which
> > depend on
> > + * each other to different schedulers at the same time, but using
> > + * drm_sched_job_add_dependency() can't fail any more after the
> > first job is
> > + * initialized.
> > + *
> > + * This function preallocate memory for a dependency slot so that
> > no ENOMEM can
> > + * come later while adding dependencies. The index of the
> > preallocated slot is
> > + * returned in @id.
> > + *
> > + * Return:
> > + * 0 on success, or an error on failing to expand the array.
> > + */
> > +int drm_sched_job_prealloc_dependency_slot(struct drm_sched_job
> > *job,
> > +					   u32 *id)
> > +{
> > +	return xa_alloc(&job->dependencies, id, NULL,
> > xa_limit_32b, GFP_KERNEL);
> > +}
> > +EXPORT_SYMBOL(drm_sched_job_prealloc_dependency_slot);
> > +
> > +/**
> > + * drm_sched_job_add_prealloc_dep - add dependency to preallocated
> > slot
> > + * @job: scheduler job where dependencies will be added
> > + * @id: the preallocated slot index
> > + * @fence: the dependency to add
> > + *
> > + * Consumes @fence and adds it to the preallocated slot
> > dependency.
> > + */
> > +void drm_sched_job_add_prealloc_dep(struct drm_sched_job *job, u32
> > id,
> > +				    struct dma_fence *fence)
> > +{
> > +	fence = xa_store(&job->dependencies, id, fence,
> > GFP_ATOMIC);
> 
> Add assert that the passed id exists (was preallocated) and is NULL?

You 

> 
> Also, if someone preallocates and does not consume the slot will that
> confuse the iteration in drm_sched_job_dependency()?

drm_sched_job_add_dependency() you mean.

Yes, it would. All operations simply give you NULL for those slots. So
seems to me you have to check for NULL wherever a preallocated slot
might drop out. That would then be a bug.

It's kind of tricky, all that. It's a pity that Wilcox didn't answer
our questions about the idiomatic way to do it.

Maybe reserving slots with already signaled fences wasn't such a bad
idea after all?

If we go for the NULL approach, it's probably the only sane way to then
check for NULL wherever dependencies are accessed :(

Opinions?

P.

> 
> Regards,
> 
> Tvrtko
> 
> > +	/*
> > +	 * Be defensive just in case driver messed it up and used
> > preallocated
> > +	 * slot twice.
> > +	 */
> > +	if (WARN_ON(fence))
> > +		dma_fence_put(fence);
> > +}
> > +EXPORT_SYMBOL(drm_sched_job_add_prealloc_dep);
> > +
> >   /**
> >    * drm_sched_job_add_dependency - adds the fence as a job
> > dependency
> >    * @job: scheduler job to add the dependencies to
> > diff --git a/include/drm/gpu_scheduler.h
> > b/include/drm/gpu_scheduler.h
> > index d860db087ea5..0286e0934317 100644
> > --- a/include/drm/gpu_scheduler.h
> > +++ b/include/drm/gpu_scheduler.h
> > @@ -632,6 +632,10 @@ int drm_sched_job_init(struct drm_sched_job
> > *job,
> >   		       u32 credits, void *owner);
> >   void drm_sched_job_arm(struct drm_sched_job *job);
> >   void drm_sched_entity_push_job(struct drm_sched_job *sched_job);
> > +int drm_sched_job_prealloc_dependency_slot(struct drm_sched_job
> > *job,
> > +					   u32 *id);
> > +void drm_sched_job_add_prealloc_dep(struct drm_sched_job *job, u32
> > id,
> > +				    struct dma_fence *fence);
> >   int drm_sched_job_add_dependency(struct drm_sched_job *job,
> >   				 struct dma_fence *fence);
> >   int drm_sched_job_add_syncobj_dependency(struct drm_sched_job
> > *job,
>