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.koe...@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,
>
