On Tue, 2025-03-18 at 13:03 +0100, Christian König wrote:
> The problem is that drivers sometimes need to add dependencies
> without
> allocating any memory.
>
> Add a function which preallocates slots by inserting signaled stub
> fences
> into the dependency array.
I think I get what you're doing here, but it would certainly be helpful
to provide some more justification in the commit message a la: "Some
drivers have to [...]. With drm_sched_job_add_dependency() that's not
possible because [...]"
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
> drivers/gpu/drm/scheduler/sched_main.c | 41
> ++++++++++++++++++++++++--
> include/drm/gpu_scheduler.h | 2 ++
> 2 files changed, 40 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 4d4219fbe49d..2085eea89513 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -852,6 +852,38 @@ void drm_sched_job_arm(struct drm_sched_job
> *job)
> }
> EXPORT_SYMBOL(drm_sched_job_arm);
>
> +/**
> + * drm_sched_job_prealloc_dependency_slots - avoid ENOMEM on adding
> dependencies
> + * @job: scheduler job where dependencies will be added
> + * @num_deps: number of dependencies to preallocate slots for
> + *
> + * Preallocate memory so that no ENOMEM can come later while adding
> + * dependencies.
Similarly, should have a sentence that clarifies for when this is
needed.
> + *
> + * Return:
> + * 0 on success, or an error on failing to expand the array.
> + */
> +int drm_sched_job_prealloc_dependency_slots(struct drm_sched_job
> *job,
> + unsigned int num_deps)
> +{
> + struct dma_fence *fence;
> + u32 id = 0;
> + int ret;
> +
> + while (num_deps--) {
> + fence = dma_fence_get_stub();
> + ret = xa_alloc(&job->dependencies, &id, fence,
> xa_limit_32b,
> + GFP_KERNEL);
So this would fill the xarr with already signaled fences which then
later will be replaced with unsignaled fences?
Help me out here: would it also work to add NULL instead of that stub-
fence?
> + if (ret != 0) {
btw. style should be consistent with the while() above where you ommit
'> 0'.
I personally prefer having the comparison operators, but implicitly
checking for 0 is well established in the kernel, so I guess both is
OK.
> + dma_fence_put(fence);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_sched_job_prealloc_dependency_slots);
> +
> /**
> * drm_sched_job_add_dependency - adds the fence as a job dependency
> * @job: scheduler job to add the dependencies to
> @@ -878,10 +910,12 @@ int drm_sched_job_add_dependency(struct
> drm_sched_job *job,
> * engines involved, rather than the number of BOs.
> */
> xa_for_each(&job->dependencies, index, entry) {
> - if (entry->context != fence->context)
> + bool signaled = dma_fence_is_signaled(entry);
> +
> + if (!signaled && entry->context != fence->context)
> continue;
>
> - if (dma_fence_is_later(fence, entry)) {
> + if (signaled || dma_fence_is_later(fence, entry)) {
> dma_fence_put(entry);
> xa_store(&job->dependencies, index, fence,
> GFP_KERNEL);
> } else {
> @@ -890,7 +924,8 @@ int drm_sched_job_add_dependency(struct
> drm_sched_job *job,
> return 0;
> }
>
> - ret = xa_alloc(&job->dependencies, &id, fence, xa_limit_32b,
> GFP_KERNEL);
> + ret = xa_alloc(&job->dependencies, &id, fence, xa_limit_32b,
> + GFP_KERNEL);
Would leave that unchanged for git-blame.
Thx
P.
> if (ret != 0)
> dma_fence_put(fence);
>
> diff --git a/include/drm/gpu_scheduler.h
> b/include/drm/gpu_scheduler.h
> index 1a7e377d4cbb..916e820b27ff 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -632,6 +632,8 @@ 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_slots(struct drm_sched_job
> *job,
> + unsigned int num_deps);
> 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,
