[PATCH 1/2] drm/amdgpu: avoid memory allocation in the critical code path

[PATCH 1/2] drm/amdgpu: avoid memory allocation in the critical code path

[Christian König]

When we run out of VMIDs we need to wait for some to become available.
Previously we were using a dma_fence_array for that, but this means that
we have to allocate memory.

Instead just wait for the first not signaled fence from the least recently
used VMID to signal. That is not as efficient since we end up in this
function multiple times again, but allocating memory can easily fail or
deadlock if we have to wait for memory to become available.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index 3ef5bc95642c..5f94a66511af 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -201,58 +201,31 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_ring *ring,
 	struct amdgpu_device *adev = ring->adev;
 	unsigned vmhub = ring->vm_hub;
 	struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
-	struct dma_fence **fences;
-	unsigned i;
 
+	/* If anybody is waiting for a VMID let everybody wait for fairness */
 	if (!dma_fence_is_signaled(ring->vmid_wait)) {
 		*fence = dma_fence_get(ring->vmid_wait);
 		return 0;
 	}
 
-	fences = kmalloc_array(id_mgr->num_ids, sizeof(void *), GFP_NOWAIT);
-	if (!fences)
-		return -ENOMEM;
-
 	/* Check if we have an idle VMID */
-	i = 0;
-	list_for_each_entry((*idle), &id_mgr->ids_lru, list) {
+	list_for_each_entry_reverse((*idle), &id_mgr->ids_lru, list) {
 		/* Don't use per engine and per process VMID at the same time */
 		struct amdgpu_ring *r = adev->vm_manager.concurrent_flush ?
 			NULL : ring;
 
-		fences[i] = amdgpu_sync_peek_fence(&(*idle)->active, r);
-		if (!fences[i])
-			break;
-		++i;
-	}
-
-	/* If we can't find a idle VMID to use, wait till one becomes available */
-	if (&(*idle)->list == &id_mgr->ids_lru) {
-		u64 fence_context = adev->vm_manager.fence_context + ring->idx;
-		unsigned seqno = ++adev->vm_manager.seqno[ring->idx];
-		struct dma_fence_array *array;
-		unsigned j;
-
-		*idle = NULL;
-		for (j = 0; j < i; ++j)
-			dma_fence_get(fences[j]);
-
-		array = dma_fence_array_create(i, fences, fence_context,
-					       seqno, true);
-		if (!array) {
-			for (j = 0; j < i; ++j)
-				dma_fence_put(fences[j]);
-			kfree(fences);
-			return -ENOMEM;
-		}
-
-		*fence = dma_fence_get(&array->base);
-		dma_fence_put(ring->vmid_wait);
-		ring->vmid_wait = &array->base;
-		return 0;
+		*fence = amdgpu_sync_peek_fence(&(*idle)->active, r);
+		if (!(*fence))
+			return 0;
 	}
-	kfree(fences);
 
+	/*
+	 * If we can't find a idle VMID to use, wait on a fence from the least
+	 * recently used in the hope that it will be available soon.
+	 */
+	*idle = NULL;
+	dma_fence_put(ring->vmid_wait);
+	ring->vmid_wait = dma_fence_get(*fence);
 	return 0;
 }
 
-- 
2.43.0

[PATCH 2/2] drm/amdgpu: use GFP_ATOMIC instead of NOWAIT in the critical path

[Christian König]

Otherwise job submissions can fail with ENOMEM.

We probably need to re-design the per VMID tracking at some point.

Signed-off-by: Christian König <christian.koenig@amd.com>
Fixes: https://gitlab.freedesktop.org/drm/amd/-/issues/4258#note_3179934
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index 5f94a66511af..ecf2b1f13ca7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -286,7 +286,7 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
 	* user of the VMID.
 	*/
 	r = amdgpu_sync_fence(&(*id)->active, &job->base.s_fence->finished,
-			      GFP_NOWAIT);
+			      GFP_ATOMIC);
 	if (r)
 		return r;
 
@@ -346,7 +346,7 @@ static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm,
 		 */
 		r = amdgpu_sync_fence(&(*id)->active,
 				      &job->base.s_fence->finished,
-				      GFP_NOWAIT);
+				      GFP_ATOMIC);
 		if (r)
 			return r;
 
@@ -399,7 +399,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 			/* Remember this submission as user of the VMID */
 			r = amdgpu_sync_fence(&id->active,
 					      &job->base.s_fence->finished,
-					      GFP_NOWAIT);
+					      GFP_ATOMIC);
 			if (r)
 				goto error;
 
-- 
2.43.0

Re: [PATCH 2/2] drm/amdgpu: use GFP_ATOMIC instead of NOWAIT in the critical path

[Alex Deucher]

On Thu, Nov 6, 2025 at 8:06 AM Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
>
> Otherwise job submissions can fail with ENOMEM.
>
> We probably need to re-design the per VMID tracking at some point.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> Fixes: https://gitlab.freedesktop.org/drm/amd/-/issues/4258#note_3179934

Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4258

With that fixed, series is:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 5f94a66511af..ecf2b1f13ca7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -286,7 +286,7 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
>         * user of the VMID.
>         */
>         r = amdgpu_sync_fence(&(*id)->active, &job->base.s_fence->finished,
> -                             GFP_NOWAIT);
> +                             GFP_ATOMIC);
>         if (r)
>                 return r;
>
> @@ -346,7 +346,7 @@ static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm,
>                  */
>                 r = amdgpu_sync_fence(&(*id)->active,
>                                       &job->base.s_fence->finished,
> -                                     GFP_NOWAIT);
> +                                     GFP_ATOMIC);
>                 if (r)
>                         return r;
>
> @@ -399,7 +399,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>                         /* Remember this submission as user of the VMID */
>                         r = amdgpu_sync_fence(&id->active,
>                                               &job->base.s_fence->finished,
> -                                             GFP_NOWAIT);
> +                                             GFP_ATOMIC);
>                         if (r)
>                                 goto error;
>
> --
> 2.43.0
>

Re: [PATCH 1/2] drm/amdgpu: avoid memory allocation in the critical code path

[Tvrtko Ursulin]

On 06/11/2025 13:06, Christian König wrote:
> When we run out of VMIDs we need to wait for some to become available.
> Previously we were using a dma_fence_array for that, but this means that
> we have to allocate memory.
> 
> Instead just wait for the first not signaled fence from the least recently
> used VMID to signal. That is not as efficient since we end up in this
> function multiple times again, but allocating memory can easily fail or
> deadlock if we have to wait for memory to become available.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 51 ++++++-------------------
>   1 file changed, 12 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 3ef5bc95642c..5f94a66511af 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -201,58 +201,31 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_ring *ring,
>   	struct amdgpu_device *adev = ring->adev;
>   	unsigned vmhub = ring->vm_hub;
>   	struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> -	struct dma_fence **fences;
> -	unsigned i;
>   
> +	/* If anybody is waiting for a VMID let everybody wait for fairness */
>   	if (!dma_fence_is_signaled(ring->vmid_wait)) {
>   		*fence = dma_fence_get(ring->vmid_wait);
>   		return 0;
>   	}
>   
> -	fences = kmalloc_array(id_mgr->num_ids, sizeof(void *), GFP_NOWAIT);
> -	if (!fences)
> -		return -ENOMEM;
> -
>   	/* Check if we have an idle VMID */
> -	i = 0;
> -	list_for_each_entry((*idle), &id_mgr->ids_lru, list) {
> +	list_for_each_entry_reverse((*idle), &id_mgr->ids_lru, list) {
>   		/* Don't use per engine and per process VMID at the same time */
>   		struct amdgpu_ring *r = adev->vm_manager.concurrent_flush ?
>   			NULL : ring;
>   
> -		fences[i] = amdgpu_sync_peek_fence(&(*idle)->active, r);
> -		if (!fences[i])
> -			break;
> -		++i;
> -	}
> -
> -	/* If we can't find a idle VMID to use, wait till one becomes available */
> -	if (&(*idle)->list == &id_mgr->ids_lru) {
> -		u64 fence_context = adev->vm_manager.fence_context + ring->idx;
> -		unsigned seqno = ++adev->vm_manager.seqno[ring->idx];

Maybe vm_manager.fence_context && seqno are now unused and can be removed?

Regards,

Tvrtko

> -		struct dma_fence_array *array;
> -		unsigned j;
> -
> -		*idle = NULL;
> -		for (j = 0; j < i; ++j)
> -			dma_fence_get(fences[j]);
> -
> -		array = dma_fence_array_create(i, fences, fence_context,
> -					       seqno, true);
> -		if (!array) {
> -			for (j = 0; j < i; ++j)
> -				dma_fence_put(fences[j]);
> -			kfree(fences);
> -			return -ENOMEM;
> -		}
> -
> -		*fence = dma_fence_get(&array->base);
> -		dma_fence_put(ring->vmid_wait);
> -		ring->vmid_wait = &array->base;
> -		return 0;
> +		*fence = amdgpu_sync_peek_fence(&(*idle)->active, r);
> +		if (!(*fence))
> +			return 0;
>   	}
> -	kfree(fences);
>   
> +	/*
> +	 * If we can't find a idle VMID to use, wait on a fence from the least
> +	 * recently used in the hope that it will be available soon.
> +	 */
> +	*idle = NULL;
> +	dma_fence_put(ring->vmid_wait);
> +	ring->vmid_wait = dma_fence_get(*fence);
>   	return 0;
>   }
>   

Re: [PATCH 1/2] drm/amdgpu: avoid memory allocation in the critical code path

[Christian König]

On 11/6/25 16:55, Tvrtko Ursulin wrote:
> 
> On 06/11/2025 13:06, Christian König wrote:
>> When we run out of VMIDs we need to wait for some to become available.
>> Previously we were using a dma_fence_array for that, but this means that
>> we have to allocate memory.
>>
>> Instead just wait for the first not signaled fence from the least recently
>> used VMID to signal. That is not as efficient since we end up in this
>> function multiple times again, but allocating memory can easily fail or
>> deadlock if we have to wait for memory to become available.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 51 ++++++-------------------
>>   1 file changed, 12 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>> index 3ef5bc95642c..5f94a66511af 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>> @@ -201,58 +201,31 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_ring *ring,
>>       struct amdgpu_device *adev = ring->adev;
>>       unsigned vmhub = ring->vm_hub;
>>       struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
>> -    struct dma_fence **fences;
>> -    unsigned i;
>>   +    /* If anybody is waiting for a VMID let everybody wait for fairness */
>>       if (!dma_fence_is_signaled(ring->vmid_wait)) {
>>           *fence = dma_fence_get(ring->vmid_wait);
>>           return 0;
>>       }
>>   -    fences = kmalloc_array(id_mgr->num_ids, sizeof(void *), GFP_NOWAIT);
>> -    if (!fences)
>> -        return -ENOMEM;
>> -
>>       /* Check if we have an idle VMID */
>> -    i = 0;
>> -    list_for_each_entry((*idle), &id_mgr->ids_lru, list) {
>> +    list_for_each_entry_reverse((*idle), &id_mgr->ids_lru, list) {
>>           /* Don't use per engine and per process VMID at the same time */
>>           struct amdgpu_ring *r = adev->vm_manager.concurrent_flush ?
>>               NULL : ring;
>>   -        fences[i] = amdgpu_sync_peek_fence(&(*idle)->active, r);
>> -        if (!fences[i])
>> -            break;
>> -        ++i;
>> -    }
>> -
>> -    /* If we can't find a idle VMID to use, wait till one becomes available */
>> -    if (&(*idle)->list == &id_mgr->ids_lru) {
>> -        u64 fence_context = adev->vm_manager.fence_context + ring->idx;
>> -        unsigned seqno = ++adev->vm_manager.seqno[ring->idx];
> 
> Maybe vm_manager.fence_context && seqno are now unused and can be removed?

Good point!

Thanks,
Christian.

> 
> Regards,
> 
> Tvrtko
> 
>> -        struct dma_fence_array *array;
>> -        unsigned j;
>> -
>> -        *idle = NULL;
>> -        for (j = 0; j < i; ++j)
>> -            dma_fence_get(fences[j]);
>> -
>> -        array = dma_fence_array_create(i, fences, fence_context,
>> -                           seqno, true);
>> -        if (!array) {
>> -            for (j = 0; j < i; ++j)
>> -                dma_fence_put(fences[j]);
>> -            kfree(fences);
>> -            return -ENOMEM;
>> -        }
>> -
>> -        *fence = dma_fence_get(&array->base);
>> -        dma_fence_put(ring->vmid_wait);
>> -        ring->vmid_wait = &array->base;
>> -        return 0;
>> +        *fence = amdgpu_sync_peek_fence(&(*idle)->active, r);
>> +        if (!(*fence))
>> +            return 0;
>>       }
>> -    kfree(fences);
>>   +    /*
>> +     * If we can't find a idle VMID to use, wait on a fence from the least
>> +     * recently used in the hope that it will be available soon.
>> +     */
>> +    *idle = NULL;
>> +    dma_fence_put(ring->vmid_wait);
>> +    ring->vmid_wait = dma_fence_get(*fence);
>>       return 0;
>>   }
>>   
>