[PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Christian König]

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

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/scheduler/sched_main.c | 44 ++++++++++++++++++++++++--
 include/drm/gpu_scheduler.h            |  2 ++
 2 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
index 4d4219fbe49d..ee3701f346b2 100644
--- a/drivers/gpu/drm/scheduler/sched_main.c
+++ b/drivers/gpu/drm/scheduler/sched_main.c
@@ -852,6 +852,39 @@ 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
+  *
+ * 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.
+ *
+ * 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)
+{
+	u32 id = 0;
+	int ret;
+
+	while (num_deps--) {
+		ret = xa_alloc(&job->dependencies, &id, XA_ZERO_ENTRY,
+			       xa_limit_32b, GFP_KERNEL);
+		if (ret != 0)
+			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 +911,15 @@ 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)
+		if (xa_is_zero(entry)) {
+			/*
+			 * Reserved entries must not alloc memory, but let's
+			 * use GFP_ATOMIC just to be on the defensive side.
+			*/
+			xa_store(&job->dependencies, index, fence, GFP_ATOMIC);
+		} else if (entry->context != fence->context) {
 			continue;
-
-		if (dma_fence_is_later(fence, entry)) {
+		} else if (dma_fence_is_later(fence, entry)) {
 			dma_fence_put(entry);
 			xa_store(&job->dependencies, index, fence, GFP_KERNEL);
 		} else {
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,
-- 
2.34.1

[PATCH 2/2] drm/amdgpu: fix gang submission error handling

[Christian König]

For the unlikely case that we ran into an ENOMEM while fixing up the gang
submission dependencies we can't clean up any more since the gang
members are already armed.

Fix this by using pre-allocated dependency slots and re-ordering the
code, also fix a double unref since the fence reference is also dropped
on error.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 57 +++++++++++++++-----------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 5cc5f59e3018..25e7f7d356d7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1285,36 +1285,21 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 	uint64_t seq;
 	int r;
 
-	for (i = 0; i < p->gang_size; ++i)
-		drm_sched_job_arm(&p->jobs[i]->base);
-
-	for (i = 0; i < p->gang_size; ++i) {
-		struct dma_fence *fence;
-
-		if (p->jobs[i] == leader)
-			continue;
-
-		fence = &p->jobs[i]->base.s_fence->scheduled;
-		dma_fence_get(fence);
-		r = drm_sched_job_add_dependency(&leader->base, fence);
-		if (r) {
-			dma_fence_put(fence);
-			return r;
-		}
-	}
-
-	if (p->gang_size > 1) {
-		for (i = 0; i < p->gang_size; ++i)
-			amdgpu_job_set_gang_leader(p->jobs[i], leader);
-	}
+	/* Preallocate the memory for the gang dependencies */
+	r = drm_sched_job_prealloc_dependency_slots(&leader->base,
+						    p->gang_size - 1);
+	if (r)
+		return r;
 
-	/* No memory allocation is allowed while holding the notifier lock.
+	/*
+	 * No memory allocation is allowed while holding the notifier lock.
 	 * The lock is held until amdgpu_cs_submit is finished and fence is
 	 * added to BOs.
 	 */
 	mutex_lock(&p->adev->notifier_lock);
 
-	/* If userptr are invalidated after amdgpu_cs_parser_bos(), return
+	/*
+	 * If userptr are invalidated after amdgpu_cs_parser_bos(), return
 	 * -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl.
 	 */
 	r = 0;
@@ -1329,6 +1314,30 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 		return r;
 	}
 
+	for (i = 0; i < p->gang_size; ++i)
+		drm_sched_job_arm(&p->jobs[i]->base);
+
+	for (i = 0; i < p->gang_size; ++i) {
+		struct dma_fence *fence;
+
+		if (p->jobs[i] == leader)
+			continue;
+
+		fence = dma_fence_get(&p->jobs[i]->base.s_fence->scheduled);
+		r = drm_sched_job_add_dependency(&leader->base, fence);
+		/*
+		 * We can't abort here with an error any more, but we should
+		 * also never run into an error since the slots for the
+		 * dependency fences are preallocated.
+		 */
+		WARN_ON(r);
+	}
+
+	if (p->gang_size > 1) {
+		for (i = 0; i < p->gang_size; ++i)
+			amdgpu_job_set_gang_leader(p->jobs[i], leader);
+	}
+
 	p->fence = dma_fence_get(&leader->base.s_fence->finished);
 	drm_exec_for_each_locked_object(&p->exec, index, gobj) {
 
-- 
2.34.1

Re: [PATCH 2/2] drm/amdgpu: fix gang submission error handling

[Tvrtko Ursulin]

On 21/03/2025 15:58, Christian König wrote:
> For the unlikely case that we ran into an ENOMEM while fixing up the gang
> submission dependencies we can't clean up any more since the gang
> members are already armed.
> 
> Fix this by using pre-allocated dependency slots and re-ordering the
> code, also fix a double unref since the fence reference is also dropped
> on error.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 57 +++++++++++++++-----------
>   1 file changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 5cc5f59e3018..25e7f7d356d7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1285,36 +1285,21 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>   	uint64_t seq;
>   	int r;
>   
> -	for (i = 0; i < p->gang_size; ++i)
> -		drm_sched_job_arm(&p->jobs[i]->base);
> -
> -	for (i = 0; i < p->gang_size; ++i) {
> -		struct dma_fence *fence;
> -
> -		if (p->jobs[i] == leader)
> -			continue;
> -
> -		fence = &p->jobs[i]->base.s_fence->scheduled;
> -		dma_fence_get(fence);
> -		r = drm_sched_job_add_dependency(&leader->base, fence);
> -		if (r) {
> -			dma_fence_put(fence);
> -			return r;
> -		}
> -	}
> -
> -	if (p->gang_size > 1) {
> -		for (i = 0; i < p->gang_size; ++i)
> -			amdgpu_job_set_gang_leader(p->jobs[i], leader);
> -	}
> +	/* Preallocate the memory for the gang dependencies */
> +	r = drm_sched_job_prealloc_dependency_slots(&leader->base,
> +						    p->gang_size - 1);
> +	if (r)
> +		return r;
>   
> -	/* No memory allocation is allowed while holding the notifier lock.
> +	/*
> +	 * No memory allocation is allowed while holding the notifier lock.
>   	 * The lock is held until amdgpu_cs_submit is finished and fence is
>   	 * added to BOs.
>   	 */
>   	mutex_lock(&p->adev->notifier_lock);
>   
> -	/* If userptr are invalidated after amdgpu_cs_parser_bos(), return
> +	/*
> +	 * If userptr are invalidated after amdgpu_cs_parser_bos(), return
>   	 * -EAGAIN, drmIoctl in libdrm will restart the amdgpu_cs_ioctl.
>   	 */
>   	r = 0;
> @@ -1329,6 +1314,30 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>   		return r;
>   	}
>   
> +	for (i = 0; i < p->gang_size; ++i)
> +		drm_sched_job_arm(&p->jobs[i]->base);
> +
> +	for (i = 0; i < p->gang_size; ++i) {
> +		struct dma_fence *fence;
> +
> +		if (p->jobs[i] == leader)
> +			continue;
> +
> +		fence = dma_fence_get(&p->jobs[i]->base.s_fence->scheduled);
> +		r = drm_sched_job_add_dependency(&leader->base, fence);
> +		/*
> +		 * We can't abort here with an error any more, but we should
> +		 * also never run into an error since the slots for the
> +		 * dependency fences are preallocated.
> +		 */
> +		WARN_ON(r);
> +	}
> +
> +	if (p->gang_size > 1) {
> +		for (i = 0; i < p->gang_size; ++i)
> +			amdgpu_job_set_gang_leader(p->jobs[i], leader);
> +	}
> +
>   	p->fence = dma_fence_get(&leader->base.s_fence->finished);
>   	drm_exec_for_each_locked_object(&p->exec, index, gobj) {
>   

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

Re: [PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Philipp Stanner]

On Fri, 2025-03-21 at 16:58 +0100, 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

For drm/sched Danilo & I think that changelogs shouldn't be in the
commit message anymore. The Link: applied by the DRM tools will be
sufficient to find the history in the archives if necessary.

> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 44
> ++++++++++++++++++++++++--
>  include/drm/gpu_scheduler.h            |  2 ++
>  2 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 4d4219fbe49d..ee3701f346b2 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -852,6 +852,39 @@ 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
> +  *
> + * 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.
> + *
> + * 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)
> +{
> +	u32 id = 0;
> +	int ret;
> +
> +	while (num_deps--) {
> +		ret = xa_alloc(&job->dependencies, &id,
> XA_ZERO_ENTRY,
> +			       xa_limit_32b, GFP_KERNEL);

Fine by me, but out of curiousity about the xarray: you mentioned
xa_reserve() might work, too?


P.

> +		if (ret != 0)
> +			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 +911,15 @@ 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)
> +		if (xa_is_zero(entry)) {
> +			/*
> +			 * Reserved entries must not alloc memory,
> but let's
> +			 * use GFP_ATOMIC just to be on the
> defensive side.
> +			*/
> +			xa_store(&job->dependencies, index, fence,
> GFP_ATOMIC);
> +		} else if (entry->context != fence->context) {
>  			continue;
> -
> -		if (dma_fence_is_later(fence, entry)) {
> +		} else if (dma_fence_is_later(fence, entry)) {
>  			dma_fence_put(entry);
>  			xa_store(&job->dependencies, index, fence,
> GFP_KERNEL);
>  		} else {
> 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,

Re: [PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Christian König]

Am 21.03.25 um 19:05 schrieb Philipp Stanner:
> On Fri, 2025-03-21 at 16:58 +0100, 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
> For drm/sched Danilo & I think that changelogs shouldn't be in the
> commit message anymore. The Link: applied by the DRM tools will be
> sufficient to find the history in the archives if necessary.

Sure, going to drop that.

>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>  drivers/gpu/drm/scheduler/sched_main.c | 44
>> ++++++++++++++++++++++++--
>>  include/drm/gpu_scheduler.h            |  2 ++
>>  2 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index 4d4219fbe49d..ee3701f346b2 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -852,6 +852,39 @@ 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
>> +  *
>> + * 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.
>> + *
>> + * 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)
>> +{
>> +	u32 id = 0;
>> +	int ret;
>> +
>> +	while (num_deps--) {
>> +		ret = xa_alloc(&job->dependencies, &id,
>> XA_ZERO_ENTRY,
>> +			       xa_limit_32b, GFP_KERNEL);
> Fine by me, but out of curiousity about the xarray: you mentioned
> xa_reserve() might work, too?

Different use case, xa_reserve() can only reserve a specific index and not allocate one.

Allocating and reserving at the same time seems to be done by the pattern xa_alloc(...XA_ZERO_ENTRY...) like we use here.

Regards,
Christian.

>
>
> P.
>
>> +		if (ret != 0)
>> +			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 +911,15 @@ 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)
>> +		if (xa_is_zero(entry)) {
>> +			/*
>> +			 * Reserved entries must not alloc memory,
>> but let's
>> +			 * use GFP_ATOMIC just to be on the
>> defensive side.
>> +			*/
>> +			xa_store(&job->dependencies, index, fence,
>> GFP_ATOMIC);
>> +		} else if (entry->context != fence->context) {
>>  			continue;
>> -
>> -		if (dma_fence_is_later(fence, entry)) {
>> +		} else if (dma_fence_is_later(fence, entry)) {
>>  			dma_fence_put(entry);
>>  			xa_store(&job->dependencies, index, fence,
>> GFP_KERNEL);
>>  		} else {
>> 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,

Re: [PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Tvrtko Ursulin]

On 21/03/2025 15:58, 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
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 44 ++++++++++++++++++++++++--
>   include/drm/gpu_scheduler.h            |  2 ++
>   2 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 4d4219fbe49d..ee3701f346b2 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -852,6 +852,39 @@ 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
> +  *
> + * 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.
> + *
> + * 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)
> +{
> +	u32 id = 0;
> +	int ret;
> +
> +	while (num_deps--) {
> +		ret = xa_alloc(&job->dependencies, &id, XA_ZERO_ENTRY,
> +			       xa_limit_32b, GFP_KERNEL);
> +		if (ret != 0)
> +			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 +911,15 @@ 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)
> +		if (xa_is_zero(entry)) {

If xa_for_each shows them I think we also need to skip them in 
drm_sched_job_dependency() and drm_sched_job_cleanup(). Or remove the 
unused ones at arm time. Hm, we could also put a noisy fail if 
add_dependency allocates after a job is armed. (I can imagine a few unit 
tests for this once we merge them.)

Regards,

Tvrtko

> +			/*
> +			 * Reserved entries must not alloc memory, but let's
> +			 * use GFP_ATOMIC just to be on the defensive side.
> +			*/
> +			xa_store(&job->dependencies, index, fence, GFP_ATOMIC);
> +		} else if (entry->context != fence->context) {
>   			continue;
> -
> -		if (dma_fence_is_later(fence, entry)) {
> +		} else if (dma_fence_is_later(fence, entry)) {
>   			dma_fence_put(entry);
>   			xa_store(&job->dependencies, index, fence, GFP_KERNEL);
>   		} else {
> 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,

Re: [PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Christian König]

Am 24.03.25 um 09:35 schrieb Tvrtko Ursulin:
>
> On 21/03/2025 15:58, 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
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 44 ++++++++++++++++++++++++--
>>   include/drm/gpu_scheduler.h            |  2 ++
>>   2 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index 4d4219fbe49d..ee3701f346b2 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -852,6 +852,39 @@ 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
>> +  *
>> + * 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.
>> + *
>> + * 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)
>> +{
>> +    u32 id = 0;
>> +    int ret;
>> +
>> +    while (num_deps--) {
>> +        ret = xa_alloc(&job->dependencies, &id, XA_ZERO_ENTRY,
>> +                   xa_limit_32b, GFP_KERNEL);
>> +        if (ret != 0)
>> +            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 +911,15 @@ 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)
>> +        if (xa_is_zero(entry)) {
>
> If xa_for_each shows them I think we also need to skip them in drm_sched_job_dependency() and drm_sched_job_cleanup().

Oh, really good point. I've completely forgotten about them.

> Or remove the unused ones at arm time. Hm, we could also put a noisy fail if add_dependency allocates after a job is armed. (I can imagine a few unit tests for this once we merge them.)

Good point as well. I was also waiting for your unit testing stuff to land in drm-misc-next so I can write a test for that.

Thanks,
Christian.

>
> Regards,
>
> Tvrtko
>
>> +            /*
>> +             * Reserved entries must not alloc memory, but let's
>> +             * use GFP_ATOMIC just to be on the defensive side.
>> +            */
>> +            xa_store(&job->dependencies, index, fence, GFP_ATOMIC);
>> +        } else if (entry->context != fence->context) {
>>               continue;
>> -
>> -        if (dma_fence_is_later(fence, entry)) {
>> +        } else if (dma_fence_is_later(fence, entry)) {
>>               dma_fence_put(entry);
>>               xa_store(&job->dependencies, index, fence, GFP_KERNEL);
>>           } else {
>> 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,
>

Re: [PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Philipp Stanner]

On Fri, 2025-03-21 at 16:58 +0100, 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
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/scheduler/sched_main.c | 44
> ++++++++++++++++++++++++--
>  include/drm/gpu_scheduler.h            |  2 ++
>  2 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> b/drivers/gpu/drm/scheduler/sched_main.c
> index 4d4219fbe49d..ee3701f346b2 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -852,6 +852,39 @@ 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
> +  *
> + * 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.
> + *
> + * 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)
> +{
> +	u32 id = 0;
> +	int ret;
> +
> +	while (num_deps--) {
> +		ret = xa_alloc(&job->dependencies, &id,
> XA_ZERO_ENTRY,
> +			       xa_limit_32b, GFP_KERNEL);

I've had some time to re-read the xarray documentation and I think that
this is what xa_reserve() was born for. The Book of Documentation/core-
api/xarray.rst sayeth:

"Sometimes you need to ensure that a subsequent call to xa_store()
will not need to allocate memory.  The xa_reserve() function
will store a reserved entry at the indicated index.  Users of the
normal API will see this entry as containing ``NULL``."

That's far better, this way we don't have to use that more or less
xarray-internal flag.


> +		if (ret != 0)
> +			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 +911,15 @@ 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)
> +		if (xa_is_zero(entry)) {
> +			/*
> +			 * Reserved entries must not alloc memory,
> but let's
> +			 * use GFP_ATOMIC just to be on the
> defensive side.
> +			*/
> +			xa_store(&job->dependencies, index, fence,
> GFP_ATOMIC);

And regarding this – it can actually never happen, but you provide
ATOMIC just to be sure?

I think it would be better if we'd just run into an obvious bug here
instead, so like a deadlock with GFP_KERNEL.

That's how we do it with pointers that cannot be NULL, too. If the
impossible were to happen and it were NULL, we'd crash.

P.

> +		} else if (entry->context != fence->context) {
>  			continue;
> -
> -		if (dma_fence_is_later(fence, entry)) {
> +		} else if (dma_fence_is_later(fence, entry)) {
>  			dma_fence_put(entry);
>  			xa_store(&job->dependencies, index, fence,
> GFP_KERNEL);
>  		} else {
> 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,

Re: [PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Christian König]

Am 09.04.25 um 12:28 schrieb Philipp Stanner:
> On Fri, 2025-03-21 at 16:58 +0100, 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
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>  drivers/gpu/drm/scheduler/sched_main.c | 44
>> ++++++++++++++++++++++++--
>>  include/drm/gpu_scheduler.h            |  2 ++
>>  2 files changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c
>> b/drivers/gpu/drm/scheduler/sched_main.c
>> index 4d4219fbe49d..ee3701f346b2 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -852,6 +852,39 @@ 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
>> +  *
>> + * 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.
>> + *
>> + * 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)
>> +{
>> +	u32 id = 0;
>> +	int ret;
>> +
>> +	while (num_deps--) {
>> +		ret = xa_alloc(&job->dependencies, &id,
>> XA_ZERO_ENTRY,
>> +			       xa_limit_32b, GFP_KERNEL);
> I've had some time to re-read the xarray documentation and I think that
> this is what xa_reserve() was born for. The Book of Documentation/core-
> api/xarray.rst sayeth:
>
> "Sometimes you need to ensure that a subsequent call to xa_store()
> will not need to allocate memory.  The xa_reserve() function
> will store a reserved entry at the indicated index.  Users of the
> normal API will see this entry as containing ``NULL``."
>
> That's far better, this way we don't have to use that more or less
> xarray-internal flag.

Yeah I have seen that as well. The reason why I didn't followed this route was that I wasn't sure if I then need to check for NULL entries while iterating over the XA.

Additional to that I couldn't figure out of hand how to determine a the next free index slot.

Have you found any example how to use that? I mean the documentation could certainly be improved a bit.

Regards,
Christian.

>
>
>> +		if (ret != 0)
>> +			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 +911,15 @@ 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)
>> +		if (xa_is_zero(entry)) {
>> +			/*
>> +			 * Reserved entries must not alloc memory,
>> but let's
>> +			 * use GFP_ATOMIC just to be on the
>> defensive side.
>> +			*/
>> +			xa_store(&job->dependencies, index, fence,
>> GFP_ATOMIC);
> And regarding this – it can actually never happen, but you provide
> ATOMIC just to be sure?
>
> I think it would be better if we'd just run into an obvious bug here
> instead, so like a deadlock with GFP_KERNEL.
>
> That's how we do it with pointers that cannot be NULL, too. If the
> impossible were to happen and it were NULL, we'd crash.
>
> P.
>
>> +		} else if (entry->context != fence->context) {
>>  			continue;
>> -
>> -		if (dma_fence_is_later(fence, entry)) {
>> +		} else if (dma_fence_is_later(fence, entry)) {
>>  			dma_fence_put(entry);
>>  			xa_store(&job->dependencies, index, fence,
>> GFP_KERNEL);
>>  		} else {
>> 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,

Re: [PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Philipp Stanner]

+Cc Matthew

On Wed, 2025-04-09 at 15:55 +0200, Christian König wrote:
> Am 09.04.25 um 12:28 schrieb Philipp Stanner:
> > On Fri, 2025-03-21 at 16:58 +0100, 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
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > ---
> > >  drivers/gpu/drm/scheduler/sched_main.c | 44
> > > ++++++++++++++++++++++++--
> > >  include/drm/gpu_scheduler.h            |  2 ++
> > >  2 files changed, 43 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > index 4d4219fbe49d..ee3701f346b2 100644
> > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > @@ -852,6 +852,39 @@ 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
> > > +  *
> > > + * 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.
> > > + *
> > > + * 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)
> > > +{
> > > +	u32 id = 0;
> > > +	int ret;
> > > +
> > > +	while (num_deps--) {
> > > +		ret = xa_alloc(&job->dependencies, &id,
> > > XA_ZERO_ENTRY,
> > > +			       xa_limit_32b, GFP_KERNEL);
> > I've had some time to re-read the xarray documentation and I think
> > that
> > this is what xa_reserve() was born for. The Book of
> > Documentation/core-
> > api/xarray.rst sayeth:
> > 
> > "Sometimes you need to ensure that a subsequent call to xa_store()
> > will not need to allocate memory.  The xa_reserve() function
> > will store a reserved entry at the indicated index.  Users of the
> > normal API will see this entry as containing ``NULL``."
> > 
> > That's far better, this way we don't have to use that more or less
> > xarray-internal flag.
> 
> Yeah I have seen that as well. The reason why I didn't followed this
> route was that I wasn't sure if I then need to check for NULL entries
> while iterating over the XA.
> 
> Additional to that I couldn't figure out of hand how to determine a
> the next free index slot.
> 
> Have you found any example how to use that? I mean the documentation
> could certainly be improved a bit.

Maybe Matthew can help us out here?

Matthew, what would be the idiomatic way to do this, and can we help
out with improving the Xarray's documentation?

Thx,
P.

> 
> Regards,
> Christian.
> 
> > 
> > 
> > > +		if (ret != 0)
> > > +			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 +911,15 @@ 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)
> > > +		if (xa_is_zero(entry)) {
> > > +			/*
> > > +			 * Reserved entries must not alloc
> > > memory,
> > > but let's
> > > +			 * use GFP_ATOMIC just to be on the
> > > defensive side.
> > > +			*/
> > > +			xa_store(&job->dependencies, index,
> > > fence,
> > > GFP_ATOMIC);
> > And regarding this – it can actually never happen, but you provide
> > ATOMIC just to be sure?
> > 
> > I think it would be better if we'd just run into an obvious bug
> > here
> > instead, so like a deadlock with GFP_KERNEL.
> > 
> > That's how we do it with pointers that cannot be NULL, too. If the
> > impossible were to happen and it were NULL, we'd crash.
> > 
> > P.
> > 
> > > +		} else if (entry->context != fence->context) {
> > >  			continue;
> > > -
> > > -		if (dma_fence_is_later(fence, entry)) {
> > > +		} else if (dma_fence_is_later(fence, entry)) {
> > >  			dma_fence_put(entry);
> > >  			xa_store(&job->dependencies, index,
> > > fence,
> > > GFP_KERNEL);
> > >  		} else {
> > > 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,
> 

Re: [PATCH 1/2] drm/sched: add drm_sched_prealloc_dependency_slots v2

[Philipp Stanner]

On Wed, 2025-04-09 at 16:04 +0200, Philipp Stanner wrote:
> +Cc Matthew
> 
> On Wed, 2025-04-09 at 15:55 +0200, Christian König wrote:
> > Am 09.04.25 um 12:28 schrieb Philipp Stanner:
> > > On Fri, 2025-03-21 at 16:58 +0100, 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
> > > > 
> > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > ---
> > > >  drivers/gpu/drm/scheduler/sched_main.c | 44
> > > > ++++++++++++++++++++++++--
> > > >  include/drm/gpu_scheduler.h            |  2 ++
> > > >  2 files changed, 43 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/scheduler/sched_main.c
> > > > b/drivers/gpu/drm/scheduler/sched_main.c
> > > > index 4d4219fbe49d..ee3701f346b2 100644
> > > > --- a/drivers/gpu/drm/scheduler/sched_main.c
> > > > +++ b/drivers/gpu/drm/scheduler/sched_main.c
> > > > @@ -852,6 +852,39 @@ 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
> > > > +  *
> > > > + * 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.
> > > > + *
> > > > + * 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)
> > > > +{
> > > > +	u32 id = 0;
> > > > +	int ret;
> > > > +
> > > > +	while (num_deps--) {
> > > > +		ret = xa_alloc(&job->dependencies, &id,
> > > > XA_ZERO_ENTRY,
> > > > +			       xa_limit_32b, GFP_KERNEL);
> > > I've had some time to re-read the xarray documentation and I
> > > think
> > > that
> > > this is what xa_reserve() was born for. The Book of
> > > Documentation/core-
> > > api/xarray.rst sayeth:
> > > 
> > > "Sometimes you need to ensure that a subsequent call to
> > > xa_store()
> > > will not need to allocate memory.  The xa_reserve() function
> > > will store a reserved entry at the indicated index.  Users of the
> > > normal API will see this entry as containing ``NULL``."
> > > 
> > > That's far better, this way we don't have to use that more or
> > > less
> > > xarray-internal flag.

I've tried to look through the code and think it through…

> > 
> > Yeah I have seen that as well. The reason why I didn't followed
> > this
> > route was that I wasn't sure if I then need to check for NULL
> > entries
> > while iterating over the XA.

AFAICS, when you use xa_reserve(), xa_load() and xa_for_each() will
return NULL for the reserved entries – therefore potentially blowing up
the scheduler without NULL checks, when someone uses the new prealloc
function without actually filling in the dependencies later.
At least the documentation says so:

"The xa_reserve() function will store a reserved entry at the indicated
index.  Users of the normal API will see this entry as containing
``NULL``."

So that's definitely not a good idea.


BUT the same seems to be the case for xa_alloc(…, XA_ZERO_ENTRY, …)?

xa_load() will *definitely* return NULL, since it utilizes
xa_zero_to_null(). We have one use, in sched_entity.c. That use should
only ever evaluate a valid dependency, so can't realistically speaking
be NULL.

So the more interesting question is how, xa_for_each(), our main work
horse, behaves. It uses xa_find(), which uses xas_find(), which… seems
to be OK?

xa_find()'s docu says:
"* Return: The entry, if found, otherwise %NULL."

???

I agree we should aim for documenting that better. It could also make
sense to _consider_ changing xa_for_each() so that it doesn't return
reserved entries, but only 'used' entries.


P.


> > 
> > Additional to that I couldn't figure out of hand how to determine a
> > the next free index slot.
> > 
> > Have you found any example how to use that? I mean the
> > documentation
> > could certainly be improved a bit.
> 
> Maybe Matthew can help us out here?
> 
> Matthew, what would be the idiomatic way to do this, and can we help
> out with improving the Xarray's documentation?
> 
> Thx,
> P.
> 
> > 
> > Regards,
> > Christian.
> > 
> > > 
> > > 
> > > > +		if (ret != 0)
> > > > +			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 +911,15 @@ 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)
> > > > +		if (xa_is_zero(entry)) {
> > > > +			/*
> > > > +			 * Reserved entries must not alloc
> > > > memory,
> > > > but let's
> > > > +			 * use GFP_ATOMIC just to be on the
> > > > defensive side.
> > > > +			*/
> > > > +			xa_store(&job->dependencies, index,
> > > > fence,
> > > > GFP_ATOMIC);
> > > And regarding this – it can actually never happen, but you
> > > provide
> > > ATOMIC just to be sure?
> > > 
> > > I think it would be better if we'd just run into an obvious bug
> > > here
> > > instead, so like a deadlock with GFP_KERNEL.
> > > 
> > > That's how we do it with pointers that cannot be NULL, too. If
> > > the
> > > impossible were to happen and it were NULL, we'd crash.
> > > 
> > > P.
> > > 
> > > > +		} else if (entry->context != fence->context) {
> > > >  			continue;
> > > > -
> > > > -		if (dma_fence_is_later(fence, entry)) {
> > > > +		} else if (dma_fence_is_later(fence, entry)) {
> > > >  			dma_fence_put(entry);
> > > >  			xa_store(&job->dependencies, index,
> > > > fence,
> > > > GFP_KERNEL);
> > > >  		} else {
> > > > 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,
> > 
>