[PATCH 1/8] drm/amdgpu: grab an additional reference on the gang fence v2

[PATCH 1/8] drm/amdgpu: grab an additional reference on the gang fence v2

[Christian König]

We keep the gang submission fence around in adev, make sure that it
stays alive.

v2: fix memory leak on retry

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 198d29faa754..337543ec615c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -6889,18 +6889,26 @@ struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
 {
 	struct dma_fence *old = NULL;
 
+	dma_fence_get(gang);
 	do {
 		dma_fence_put(old);
 		old = amdgpu_device_get_gang(adev);
 		if (old == gang)
 			break;
 
-		if (!dma_fence_is_signaled(old))
+		if (!dma_fence_is_signaled(old)) {
+			dma_fence_put(gang);
 			return old;
+		}
 
 	} while (cmpxchg((struct dma_fence __force **)&adev->gang_submit,
 			 old, gang) != old);
 
+	/*
+	 * Drop it once for the exchanged reference in adev and once for the
+	 * thread local reference acquired in amdgpu_device_get_gang().
+	 */
+	dma_fence_put(old);
 	dma_fence_put(old);
 	return NULL;
 }
-- 
2.34.1

[PATCH 2/8] drm/amdgpu: use GFP_NOWAIT for memory allocations

[Christian König]

In the critical submission path memory allocations can't wait for
reclaim since that can potentially wait for submissions to finish.

Finally clean that up and mark most memory allocations in the critical
path with GFP_NOWAIT. The only exception left is the dma_fence_array()
used when no VMID is available, but that will be cleaned up later on.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c   |  8 ++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c         | 18 +++++++++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c        | 11 +++++++----
 drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c        |  4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c       | 11 ++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h       |  3 ++-
 6 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index 2ac6d4fa0601..d2ec4130a316 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -491,7 +491,7 @@ static int vm_update_pds(struct amdgpu_vm *vm, struct amdgpu_sync *sync)
 	if (ret)
 		return ret;
 
-	return amdgpu_sync_fence(sync, vm->last_update);
+	return amdgpu_sync_fence(sync, vm->last_update, GFP_KERNEL);
 }
 
 static uint64_t get_pte_flags(struct amdgpu_device *adev, struct kgd_mem *mem)
@@ -1249,7 +1249,7 @@ static int unmap_bo_from_gpuvm(struct kgd_mem *mem,
 
 	(void)amdgpu_vm_clear_freed(adev, vm, &bo_va->last_pt_update);
 
-	(void)amdgpu_sync_fence(sync, bo_va->last_pt_update);
+	(void)amdgpu_sync_fence(sync, bo_va->last_pt_update, GFP_KERNEL);
 
 	return 0;
 }
@@ -1273,7 +1273,7 @@ static int update_gpuvm_pte(struct kgd_mem *mem,
 		return ret;
 	}
 
-	return amdgpu_sync_fence(sync, bo_va->last_pt_update);
+	return amdgpu_sync_fence(sync, bo_va->last_pt_update, GFP_KERNEL);
 }
 
 static int map_bo_to_gpuvm(struct kgd_mem *mem,
@@ -2913,7 +2913,7 @@ int amdgpu_amdkfd_gpuvm_restore_process_bos(void *info, struct dma_fence __rcu *
 		}
 		dma_resv_for_each_fence(&cursor, bo->tbo.base.resv,
 					DMA_RESV_USAGE_KERNEL, fence) {
-			ret = amdgpu_sync_fence(&sync_obj, fence);
+			ret = amdgpu_sync_fence(&sync_obj, fence, GFP_KERNEL);
 			if (ret) {
 				pr_debug("Memory eviction: Sync BO fence failed. Try again\n");
 				goto validate_map_fail;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 5df21529b3b1..2ce0c6a152a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -428,7 +428,7 @@ static int amdgpu_cs_p2_dependencies(struct amdgpu_cs_parser *p,
 			dma_fence_put(old);
 		}
 
-		r = amdgpu_sync_fence(&p->sync, fence);
+		r = amdgpu_sync_fence(&p->sync, fence, GFP_KERNEL);
 		dma_fence_put(fence);
 		if (r)
 			return r;
@@ -450,7 +450,7 @@ static int amdgpu_syncobj_lookup_and_add(struct amdgpu_cs_parser *p,
 		return r;
 	}
 
-	r = amdgpu_sync_fence(&p->sync, fence);
+	r = amdgpu_sync_fence(&p->sync, fence, GFP_KERNEL);
 	dma_fence_put(fence);
 	return r;
 }
@@ -1124,7 +1124,8 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	if (r)
 		return r;
 
-	r = amdgpu_sync_fence(&p->sync, fpriv->prt_va->last_pt_update);
+	r = amdgpu_sync_fence(&p->sync, fpriv->prt_va->last_pt_update,
+			      GFP_KERNEL);
 	if (r)
 		return r;
 
@@ -1135,7 +1136,8 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 		if (r)
 			return r;
 
-		r = amdgpu_sync_fence(&p->sync, bo_va->last_pt_update);
+		r = amdgpu_sync_fence(&p->sync, bo_va->last_pt_update,
+				      GFP_KERNEL);
 		if (r)
 			return r;
 	}
@@ -1154,7 +1156,8 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 		if (r)
 			return r;
 
-		r = amdgpu_sync_fence(&p->sync, bo_va->last_pt_update);
+		r = amdgpu_sync_fence(&p->sync, bo_va->last_pt_update,
+				      GFP_KERNEL);
 		if (r)
 			return r;
 	}
@@ -1167,7 +1170,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	if (r)
 		return r;
 
-	r = amdgpu_sync_fence(&p->sync, vm->last_update);
+	r = amdgpu_sync_fence(&p->sync, vm->last_update, GFP_KERNEL);
 	if (r)
 		return r;
 
@@ -1248,7 +1251,8 @@ static int amdgpu_cs_sync_rings(struct amdgpu_cs_parser *p)
 			continue;
 		}
 
-		r = amdgpu_sync_fence(&p->gang_leader->explicit_sync, fence);
+		r = amdgpu_sync_fence(&p->gang_leader->explicit_sync, fence,
+				      GFP_KERNEL);
 		dma_fence_put(fence);
 		if (r)
 			return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index 8e712a11aba5..56d27cea052e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -209,7 +209,7 @@ static int amdgpu_vmid_grab_idle(struct amdgpu_ring *ring,
 		return 0;
 	}
 
-	fences = kmalloc_array(id_mgr->num_ids, sizeof(void *), GFP_KERNEL);
+	fences = kmalloc_array(id_mgr->num_ids, sizeof(void *), GFP_NOWAIT);
 	if (!fences)
 		return -ENOMEM;
 
@@ -326,7 +326,8 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
 	/* Good we can use this VMID. Remember this submission as
 	* user of the VMID.
 	*/
-	r = amdgpu_sync_fence(&(*id)->active, &job->base.s_fence->finished);
+	r = amdgpu_sync_fence(&(*id)->active, &job->base.s_fence->finished,
+			      GFP_NOWAIT);
 	if (r)
 		return r;
 
@@ -385,7 +386,8 @@ static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm,
 		 * user of the VMID.
 		 */
 		r = amdgpu_sync_fence(&(*id)->active,
-				      &job->base.s_fence->finished);
+				      &job->base.s_fence->finished,
+				      GFP_NOWAIT);
 		if (r)
 			return r;
 
@@ -437,7 +439,8 @@ 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);
+					      &job->base.s_fence->finished,
+					      GFP_NOWAIT);
 			if (r)
 				goto error;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
index ca076306adba..aa7e1104a9c7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mes.c
@@ -1332,14 +1332,14 @@ int amdgpu_mes_ctx_map_meta_data(struct amdgpu_device *adev,
 		DRM_ERROR("failed to do vm_bo_update on meta data\n");
 		goto error_del_bo_va;
 	}
-	amdgpu_sync_fence(&sync, bo_va->last_pt_update);
+	amdgpu_sync_fence(&sync, bo_va->last_pt_update, GFP_KERNEL);
 
 	r = amdgpu_vm_update_pdes(adev, vm, false);
 	if (r) {
 		DRM_ERROR("failed to update pdes on meta data\n");
 		goto error_del_bo_va;
 	}
-	amdgpu_sync_fence(&sync, vm->last_update);
+	amdgpu_sync_fence(&sync, vm->last_update, GFP_KERNEL);
 
 	amdgpu_sync_wait(&sync, false);
 	drm_exec_fini(&exec);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index c586ab4c911b..86c17a8946f5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -152,7 +152,8 @@ static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f)
  *
  * Add the fence to the sync object.
  */
-int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f)
+int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f,
+		      gfp_t flags)
 {
 	struct amdgpu_sync_entry *e;
 
@@ -162,7 +163,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f)
 	if (amdgpu_sync_add_later(sync, f))
 		return 0;
 
-	e = kmem_cache_alloc(amdgpu_sync_slab, GFP_KERNEL);
+	e = kmem_cache_alloc(amdgpu_sync_slab, flags);
 	if (!e)
 		return -ENOMEM;
 
@@ -249,7 +250,7 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
 			struct dma_fence *tmp = dma_fence_chain_contained(f);
 
 			if (amdgpu_sync_test_fence(adev, mode, owner, tmp)) {
-				r = amdgpu_sync_fence(sync, f);
+				r = amdgpu_sync_fence(sync, f, GFP_KERNEL);
 				dma_fence_put(f);
 				if (r)
 					return r;
@@ -281,7 +282,7 @@ int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv)
 		if (fence_owner != AMDGPU_FENCE_OWNER_KFD)
 			continue;
 
-		r = amdgpu_sync_fence(sync, f);
+		r = amdgpu_sync_fence(sync, f, GFP_KERNEL);
 		if (r)
 			break;
 	}
@@ -388,7 +389,7 @@ int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone)
 	hash_for_each_safe(source->fences, i, tmp, e, node) {
 		f = e->fence;
 		if (!dma_fence_is_signaled(f)) {
-			r = amdgpu_sync_fence(clone, f);
+			r = amdgpu_sync_fence(clone, f, GFP_KERNEL);
 			if (r)
 				return r;
 		} else {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
index e3272dce798d..1504f5e7fc46 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
@@ -47,7 +47,8 @@ struct amdgpu_sync {
 };
 
 void amdgpu_sync_create(struct amdgpu_sync *sync);
-int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f);
+int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f,
+		      gfp_t flags);
 int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
 		     struct dma_resv *resv, enum amdgpu_sync_mode mode,
 		     void *owner);
-- 
2.34.1

[PATCH 3/8] drm/amdgpu: overwrite signaled fence in amdgpu_sync

[Christian König]

This allows using amdgpu_sync even without peeking into the fences for a
long time.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index 86c17a8946f5..bfe12164d27d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -135,11 +135,16 @@ static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f)
 	struct amdgpu_sync_entry *e;
 
 	hash_for_each_possible(sync->fences, e, node, f->context) {
-		if (unlikely(e->fence->context != f->context))
-			continue;
+		if (dma_fence_is_signaled(e->fence)) {
+			dma_fence_put(e->fence);
+			e->fence = dma_fence_get(f);
+			return true;
+		}
 
-		amdgpu_sync_keep_later(&e->fence, f);
-		return true;
+		if (likely(e->fence->context == f->context)) {
+			amdgpu_sync_keep_later(&e->fence, f);
+			return true;
+		}
 	}
 	return false;
 }
-- 
2.34.1

Re: [PATCH 3/8] drm/amdgpu: overwrite signaled fence in amdgpu_sync

[SRINIVASAN SHANMUGAM]

On 3/7/2025 7:18 PM, Christian König wrote:
> This allows using amdgpu_sync even without peeking into the fences for a
> long time.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 13 +++++++++----
>   1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index 86c17a8946f5..bfe12164d27d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -135,11 +135,16 @@ static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f)
>   	struct amdgpu_sync_entry *e;
>   
>   	hash_for_each_possible(sync->fences, e, node, f->context) {
> -		if (unlikely(e->fence->context != f->context))
> -			continue;
> +		if (dma_fence_is_signaled(e->fence)) {
> +			dma_fence_put(e->fence);
> +			e->fence = dma_fence_get(f);
> +			return true;
> +		}
>   
> -		amdgpu_sync_keep_later(&e->fence, f);
> -		return true;
> +		if (likely(e->fence->context == f->context)) {
> +			amdgpu_sync_keep_later(&e->fence, f);
--> The call to amdgpu_sync_keep_later(&e->fence, f); ensures that the 
new fence is tracked for future synchronization., ie., so If the driver 
only replaced the old fence without keeping a reference to the new one 
of the next job or second job for example, it could lead to situations 
where the synchronization state is lost. This could cause race 
conditions where one job might proceed before another job has completed, 
leading to errors. , so this is  " amdgpu_sync_keep_later(&e->fence, 
f);" done, for tracking purpose of next job/second job, if we have 
multiple jobs in gang submissions of same context/client, is my 
understanding is correct here pls?
> +			return true;
> +		}
>   	}
>   	return false;
>   }

Re: [PATCH 3/8] drm/amdgpu: overwrite signaled fence in amdgpu_sync

[Christian König]

Am 12.03.25 um 16:06 schrieb SRINIVASAN SHANMUGAM:
>
> On 3/7/2025 7:18 PM, Christian König wrote:
>> This allows using amdgpu_sync even without peeking into the fences for a
>> long time.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 13 +++++++++----
>>   1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> index 86c17a8946f5..bfe12164d27d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> @@ -135,11 +135,16 @@ static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f)
>>       struct amdgpu_sync_entry *e;
>>         hash_for_each_possible(sync->fences, e, node, f->context) {
>> -        if (unlikely(e->fence->context != f->context))
>> -            continue;
>> +        if (dma_fence_is_signaled(e->fence)) {
>> +            dma_fence_put(e->fence);
>> +            e->fence = dma_fence_get(f);
>> +            return true;
>> +        }
>>   -        amdgpu_sync_keep_later(&e->fence, f);
>> -        return true;
>> +        if (likely(e->fence->context == f->context)) {
>> +            amdgpu_sync_keep_later(&e->fence, f);
> --> The call to amdgpu_sync_keep_later(&e->fence, f); ensures that the new fence is tracked for future synchronization., ie., so If the driver only replaced the old fence without keeping a reference to the new one of the next job or second job for example, it could lead to situations where the synchronization state is lost. This could cause race conditions where one job might proceed before another job has completed, leading to errors. , so this is  " amdgpu_sync_keep_later(&e->fence, f);" done, for tracking purpose of next job/second job, if we have multiple jobs in gang submissions of same context/client, is my understanding is correct here pls?

Your questions is not easy to understand, but I think your understanding is correct.

Basically if you have submissions A,B,C to the same ring buffer and the HW guarantees that they execute in order you only need to keep a reference to C to wait for A and B as well.

Regards,
Christian.

>> +            return true;
>> +        }
>>       }
>>       return false;
>>   }

[PATCH 4/8] drm/amdgpu: rework how isolation is enforced v2

[Christian König]

Limiting the number of available VMIDs to enforce isolation causes some
issues with gang submit and applying certain HW workarounds which
require multiple VMIDs to work correctly.

So instead start to track all submissions to the relevant engines in a
per partition data structure and use the dma_fences of the submissions
to enforce isolation similar to what a VMID limit does.

v2: use ~0l for jobs without isolation to distinct it from kernel
    submissions which uses NULL for the owner. Add some warning when we
    are OOM.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 13 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 98 +++++++++++++++++++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c    | 43 ++++------
 drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 16 +++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c   | 19 +++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h   |  1 +
 6 files changed, 155 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 87062c1adcdf..f68a348dcec9 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1211,9 +1211,15 @@ struct amdgpu_device {
 	bool                            debug_exp_resets;
 	bool                            debug_disable_gpu_ring_reset;
 
-	bool				enforce_isolation[MAX_XCP];
-	/* Added this mutex for cleaner shader isolation between GFX and compute processes */
+	/* Protection for the following isolation structure */
 	struct mutex                    enforce_isolation_mutex;
+	bool				enforce_isolation[MAX_XCP];
+	struct amdgpu_isolation {
+		void			*owner;
+		struct dma_fence	*spearhead;
+		struct amdgpu_sync	active;
+		struct amdgpu_sync	prev;
+	} isolation[MAX_XCP];
 
 	struct amdgpu_init_level *init_lvl;
 
@@ -1499,6 +1505,9 @@ void amdgpu_device_pcie_port_wreg(struct amdgpu_device *adev,
 struct dma_fence *amdgpu_device_get_gang(struct amdgpu_device *adev);
 struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
 					    struct dma_fence *gang);
+struct dma_fence *amdgpu_device_enforce_isolation(struct amdgpu_device *adev,
+						  struct amdgpu_ring *ring,
+						  struct amdgpu_job *job);
 bool amdgpu_device_has_display_hardware(struct amdgpu_device *adev);
 ssize_t amdgpu_get_soft_full_reset_mask(struct amdgpu_ring *ring);
 ssize_t amdgpu_show_reset_mask(char *buf, uint32_t supported_reset);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 337543ec615c..3fa7788b4e12 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4272,6 +4272,11 @@ int amdgpu_device_init(struct amdgpu_device *adev,
 	mutex_init(&adev->gfx.reset_sem_mutex);
 	/* Initialize the mutex for cleaner shader isolation between GFX and compute processes */
 	mutex_init(&adev->enforce_isolation_mutex);
+	for (i = 0; i < MAX_XCP; ++i) {
+		adev->isolation[i].spearhead = dma_fence_get_stub();
+		amdgpu_sync_create(&adev->isolation[i].active);
+		amdgpu_sync_create(&adev->isolation[i].prev);
+	}
 	mutex_init(&adev->gfx.kfd_sch_mutex);
 
 	amdgpu_device_init_apu_flags(adev);
@@ -4770,7 +4775,7 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 
 void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 {
-	int idx;
+	int i, idx;
 	bool px;
 
 	amdgpu_device_ip_fini(adev);
@@ -4778,6 +4783,11 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 	amdgpu_ucode_release(&adev->firmware.gpu_info_fw);
 	adev->accel_working = false;
 	dma_fence_put(rcu_dereference_protected(adev->gang_submit, true));
+	for (i = 0; i < MAX_XCP; ++i) {
+		dma_fence_put(adev->isolation[i].spearhead);
+		amdgpu_sync_free(&adev->isolation[i].active);
+		amdgpu_sync_free(&adev->isolation[i].prev);
+	}
 
 	amdgpu_reset_fini(adev);
 
@@ -6913,6 +6923,92 @@ struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
 	return NULL;
 }
 
+/**
+ * amdgpu_device_enforce_isolation - enforce HW isolation
+ * @adev: the amdgpu device pointer
+ * @ring: the HW ring the job is supposed to run on
+ * @job: the job which is about to be pushed to the HW ring
+ *
+ * Makes sure that only one client at a time can use the GFX block.
+ * Returns: The dependency to wait on before the job can be pushed to the HW.
+ * The function is called multiple times until NULL is returned.
+ */
+struct dma_fence *amdgpu_device_enforce_isolation(struct amdgpu_device *adev,
+						  struct amdgpu_ring *ring,
+						  struct amdgpu_job *job)
+{
+	struct amdgpu_isolation *isolation = &adev->isolation[ring->xcp_id];
+	struct drm_sched_fence *f = job->base.s_fence;
+	struct dma_fence *dep;
+	void *owner;
+	int r;
+
+	/*
+	 * For now enforce isolation only for the GFX block since we only need
+	 * the cleaner shader on those rings.
+	 */
+	if (ring->funcs->type != AMDGPU_RING_TYPE_GFX &&
+	    ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE)
+		return NULL;
+
+	/*
+	 * All submissions where enforce isolation is false are handled as if
+	 * they come from a single client. Use ~0l as the owner to distinct it
+	 * from kernel submissions where the owner is NULL.
+	 */
+	owner = job->enforce_isolation ? f->owner : (void*)~0l;
+
+	mutex_lock(&adev->enforce_isolation_mutex);
+
+	/*
+	 * The "spearhead" submission is the first one which changes the
+	 * ownership to its client. We always need to wait for it to be
+	 * pushed to the HW before proceeding with anything.
+	 */
+	if (&f->scheduled != isolation->spearhead &&
+	    !dma_fence_is_signaled(isolation->spearhead)) {
+		dep = isolation->spearhead;
+		goto out_grab_ref;
+	}
+
+	if (isolation->owner != owner) {
+
+		/*
+		 * Wait for any gang to be assembled before switching to a
+		 * different owner or otherwise we could deadlock the
+		 * submissions.
+		 */
+		if (!job->gang_submit) {
+			dep = amdgpu_device_get_gang(adev);
+			if (!dma_fence_is_signaled(dep))
+				goto out_return_dep;
+			dma_fence_put(dep);
+		}
+
+		dma_fence_put(isolation->spearhead);
+		isolation->spearhead = dma_fence_get(&f->scheduled);
+		amdgpu_sync_move(&isolation->active, &isolation->prev);
+		isolation->owner = owner;
+	}
+
+	/*
+	 * Specifying the ring here helps to pipeline submissions even when
+	 * isolation is enabled. If that is not desired for testing NULL can be
+	 * used instead of the ring to enforce a CPU round trip while switching
+	 * between clients.
+	 */
+	dep = amdgpu_sync_peek_fence(&isolation->prev, ring);
+	r = amdgpu_sync_fence(&isolation->active, &f->finished, GFP_NOWAIT);
+	if (r)
+		DRM_WARN("OOM tracking isolation\n");
+
+out_grab_ref:
+	dma_fence_get(dep);
+out_return_dep:
+	mutex_unlock(&adev->enforce_isolation_mutex);
+	return dep;
+}
+
 bool amdgpu_device_has_display_hardware(struct amdgpu_device *adev)
 {
 	switch (adev->asic_type) {
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index 56d27cea052e..92ab821afc06 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -287,40 +287,27 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
 	    (*id)->flushed_updates < updates ||
 	    !(*id)->last_flush ||
 	    ((*id)->last_flush->context != fence_context &&
-	     !dma_fence_is_signaled((*id)->last_flush))) {
+	     !dma_fence_is_signaled((*id)->last_flush)))
+		needs_flush = true;
+
+	if ((*id)->owner != vm->immediate.fence_context ||
+	    (!adev->vm_manager.concurrent_flush && needs_flush)) {
 		struct dma_fence *tmp;
 
-		/* Wait for the gang to be assembled before using a
-		 * reserved VMID or otherwise the gang could deadlock.
+		/* Don't use per engine and per process VMID at the
+		 * same time
 		 */
-		tmp = amdgpu_device_get_gang(adev);
-		if (!dma_fence_is_signaled(tmp) && tmp != job->gang_submit) {
+		if (adev->vm_manager.concurrent_flush)
+			ring = NULL;
+
+		/* to prevent one context starved by another context */
+		(*id)->pd_gpu_addr = 0;
+		tmp = amdgpu_sync_peek_fence(&(*id)->active, ring);
+		if (tmp) {
 			*id = NULL;
-			*fence = tmp;
+			*fence = dma_fence_get(tmp);
 			return 0;
 		}
-		dma_fence_put(tmp);
-
-		/* Make sure the id is owned by the gang before proceeding */
-		if (!job->gang_submit ||
-		    (*id)->owner != vm->immediate.fence_context) {
-
-			/* Don't use per engine and per process VMID at the
-			 * same time
-			 */
-			if (adev->vm_manager.concurrent_flush)
-				ring = NULL;
-
-			/* to prevent one context starved by another context */
-			(*id)->pd_gpu_addr = 0;
-			tmp = amdgpu_sync_peek_fence(&(*id)->active, ring);
-			if (tmp) {
-				*id = NULL;
-				*fence = dma_fence_get(tmp);
-				return 0;
-			}
-		}
-		needs_flush = true;
 	}
 
 	/* Good we can use this VMID. Remember this submission as
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
index 5537c8bfd227..1a6cfef4c071 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
@@ -360,17 +360,24 @@ amdgpu_job_prepare_job(struct drm_sched_job *sched_job,
 {
 	struct amdgpu_ring *ring = to_amdgpu_ring(s_entity->rq->sched);
 	struct amdgpu_job *job = to_amdgpu_job(sched_job);
-	struct dma_fence *fence = NULL;
+	struct dma_fence *fence;
 	int r;
 
 	r = drm_sched_entity_error(s_entity);
 	if (r)
 		goto error;
 
-	if (job->gang_submit)
+	if (job->gang_submit) {
 		fence = amdgpu_device_switch_gang(ring->adev, job->gang_submit);
+		if (fence)
+			return fence;
+	}
+
+	fence = amdgpu_device_enforce_isolation(ring->adev, ring, job);
+	if (fence)
+		return fence;
 
-	if (!fence && job->vm && !job->vmid) {
+	if (job->vm && !job->vmid) {
 		r = amdgpu_vmid_grab(job->vm, ring, job, &fence);
 		if (r) {
 			dev_err(ring->adev->dev, "Error getting VM ID (%d)\n", r);
@@ -383,9 +390,10 @@ amdgpu_job_prepare_job(struct drm_sched_job *sched_job,
 		 */
 		if (!fence)
 			job->vm = NULL;
+		return fence;
 	}
 
-	return fence;
+	return NULL;
 
 error:
 	dma_fence_set_error(&job->base.s_fence->finished, r);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index bfe12164d27d..c5f9db6b32a4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -405,6 +405,25 @@ int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone)
 	return 0;
 }
 
+/**
+ * amdgpu_sync_move - move all fences from src to dst
+ *
+ * @src: source of the fences, empty after function
+ * @dst: destination for the fences
+ *
+ * Moves all fences from source to destination. All fences in destination are
+ * freed and source is empty after the function call.
+ */
+void amdgpu_sync_move(struct amdgpu_sync *src, struct amdgpu_sync *dst)
+{
+	unsigned int i;
+
+	amdgpu_sync_free(dst);
+
+	for (i = 0; i < HASH_SIZE(src->fences); ++i)
+		hlist_move_list(&src->fences[i], &dst->fences[i]);
+}
+
 /**
  * amdgpu_sync_push_to_job - push fences into job
  * @sync: sync object to get the fences from
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
index 1504f5e7fc46..51eb4382c91e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
@@ -57,6 +57,7 @@ struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
 				     struct amdgpu_ring *ring);
 struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync);
 int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone);
+void amdgpu_sync_move(struct amdgpu_sync *src, struct amdgpu_sync *dst);
 int amdgpu_sync_push_to_job(struct amdgpu_sync *sync, struct amdgpu_job *job);
 int amdgpu_sync_wait(struct amdgpu_sync *sync, bool intr);
 void amdgpu_sync_free(struct amdgpu_sync *sync);
-- 
2.34.1

Re: [PATCH 4/8] drm/amdgpu: rework how isolation is enforced v2

[Christian König]

Hi Siqueira,

as discussed on the call if you can wrap your head around how the amdgpu_device_enforce_isolation() function works it should be trivial to write a new function or extend the function to insert a CPU bubble whenever the ownership of one of the compute rings change.

IIRC we already do load balancing between the 8 available compute rings anyway, so the only part missing is the CPU bubble to ensure that a queue reset only affects a single application.

Regards,
Christian.

Am 07.03.25 um 14:48 schrieb Christian König:
> Limiting the number of available VMIDs to enforce isolation causes some
> issues with gang submit and applying certain HW workarounds which
> require multiple VMIDs to work correctly.
>
> So instead start to track all submissions to the relevant engines in a
> per partition data structure and use the dma_fences of the submissions
> to enforce isolation similar to what a VMID limit does.
>
> v2: use ~0l for jobs without isolation to distinct it from kernel
>     submissions which uses NULL for the owner. Add some warning when we
>     are OOM.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        | 13 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 98 +++++++++++++++++++++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c    | 43 ++++------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_job.c    | 16 +++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c   | 19 +++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h   |  1 +
>  6 files changed, 155 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 87062c1adcdf..f68a348dcec9 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1211,9 +1211,15 @@ struct amdgpu_device {
>  	bool                            debug_exp_resets;
>  	bool                            debug_disable_gpu_ring_reset;
>  
> -	bool				enforce_isolation[MAX_XCP];
> -	/* Added this mutex for cleaner shader isolation between GFX and compute processes */
> +	/* Protection for the following isolation structure */
>  	struct mutex                    enforce_isolation_mutex;
> +	bool				enforce_isolation[MAX_XCP];
> +	struct amdgpu_isolation {
> +		void			*owner;
> +		struct dma_fence	*spearhead;
> +		struct amdgpu_sync	active;
> +		struct amdgpu_sync	prev;
> +	} isolation[MAX_XCP];
>  
>  	struct amdgpu_init_level *init_lvl;
>  
> @@ -1499,6 +1505,9 @@ void amdgpu_device_pcie_port_wreg(struct amdgpu_device *adev,
>  struct dma_fence *amdgpu_device_get_gang(struct amdgpu_device *adev);
>  struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
>  					    struct dma_fence *gang);
> +struct dma_fence *amdgpu_device_enforce_isolation(struct amdgpu_device *adev,
> +						  struct amdgpu_ring *ring,
> +						  struct amdgpu_job *job);
>  bool amdgpu_device_has_display_hardware(struct amdgpu_device *adev);
>  ssize_t amdgpu_get_soft_full_reset_mask(struct amdgpu_ring *ring);
>  ssize_t amdgpu_show_reset_mask(char *buf, uint32_t supported_reset);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 337543ec615c..3fa7788b4e12 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4272,6 +4272,11 @@ int amdgpu_device_init(struct amdgpu_device *adev,
>  	mutex_init(&adev->gfx.reset_sem_mutex);
>  	/* Initialize the mutex for cleaner shader isolation between GFX and compute processes */
>  	mutex_init(&adev->enforce_isolation_mutex);
> +	for (i = 0; i < MAX_XCP; ++i) {
> +		adev->isolation[i].spearhead = dma_fence_get_stub();
> +		amdgpu_sync_create(&adev->isolation[i].active);
> +		amdgpu_sync_create(&adev->isolation[i].prev);
> +	}
>  	mutex_init(&adev->gfx.kfd_sch_mutex);
>  
>  	amdgpu_device_init_apu_flags(adev);
> @@ -4770,7 +4775,7 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
>  
>  void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>  {
> -	int idx;
> +	int i, idx;
>  	bool px;
>  
>  	amdgpu_device_ip_fini(adev);
> @@ -4778,6 +4783,11 @@ void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>  	amdgpu_ucode_release(&adev->firmware.gpu_info_fw);
>  	adev->accel_working = false;
>  	dma_fence_put(rcu_dereference_protected(adev->gang_submit, true));
> +	for (i = 0; i < MAX_XCP; ++i) {
> +		dma_fence_put(adev->isolation[i].spearhead);
> +		amdgpu_sync_free(&adev->isolation[i].active);
> +		amdgpu_sync_free(&adev->isolation[i].prev);
> +	}
>  
>  	amdgpu_reset_fini(adev);
>  
> @@ -6913,6 +6923,92 @@ struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
>  	return NULL;
>  }
>  
> +/**
> + * amdgpu_device_enforce_isolation - enforce HW isolation
> + * @adev: the amdgpu device pointer
> + * @ring: the HW ring the job is supposed to run on
> + * @job: the job which is about to be pushed to the HW ring
> + *
> + * Makes sure that only one client at a time can use the GFX block.
> + * Returns: The dependency to wait on before the job can be pushed to the HW.
> + * The function is called multiple times until NULL is returned.
> + */
> +struct dma_fence *amdgpu_device_enforce_isolation(struct amdgpu_device *adev,
> +						  struct amdgpu_ring *ring,
> +						  struct amdgpu_job *job)
> +{
> +	struct amdgpu_isolation *isolation = &adev->isolation[ring->xcp_id];
> +	struct drm_sched_fence *f = job->base.s_fence;
> +	struct dma_fence *dep;
> +	void *owner;
> +	int r;
> +
> +	/*
> +	 * For now enforce isolation only for the GFX block since we only need
> +	 * the cleaner shader on those rings.
> +	 */
> +	if (ring->funcs->type != AMDGPU_RING_TYPE_GFX &&
> +	    ring->funcs->type != AMDGPU_RING_TYPE_COMPUTE)
> +		return NULL;
> +
> +	/*
> +	 * All submissions where enforce isolation is false are handled as if
> +	 * they come from a single client. Use ~0l as the owner to distinct it
> +	 * from kernel submissions where the owner is NULL.
> +	 */
> +	owner = job->enforce_isolation ? f->owner : (void*)~0l;
> +
> +	mutex_lock(&adev->enforce_isolation_mutex);
> +
> +	/*
> +	 * The "spearhead" submission is the first one which changes the
> +	 * ownership to its client. We always need to wait for it to be
> +	 * pushed to the HW before proceeding with anything.
> +	 */
> +	if (&f->scheduled != isolation->spearhead &&
> +	    !dma_fence_is_signaled(isolation->spearhead)) {
> +		dep = isolation->spearhead;
> +		goto out_grab_ref;
> +	}
> +
> +	if (isolation->owner != owner) {
> +
> +		/*
> +		 * Wait for any gang to be assembled before switching to a
> +		 * different owner or otherwise we could deadlock the
> +		 * submissions.
> +		 */
> +		if (!job->gang_submit) {
> +			dep = amdgpu_device_get_gang(adev);
> +			if (!dma_fence_is_signaled(dep))
> +				goto out_return_dep;
> +			dma_fence_put(dep);
> +		}
> +
> +		dma_fence_put(isolation->spearhead);
> +		isolation->spearhead = dma_fence_get(&f->scheduled);
> +		amdgpu_sync_move(&isolation->active, &isolation->prev);
> +		isolation->owner = owner;
> +	}
> +
> +	/*
> +	 * Specifying the ring here helps to pipeline submissions even when
> +	 * isolation is enabled. If that is not desired for testing NULL can be
> +	 * used instead of the ring to enforce a CPU round trip while switching
> +	 * between clients.
> +	 */
> +	dep = amdgpu_sync_peek_fence(&isolation->prev, ring);
> +	r = amdgpu_sync_fence(&isolation->active, &f->finished, GFP_NOWAIT);
> +	if (r)
> +		DRM_WARN("OOM tracking isolation\n");
> +
> +out_grab_ref:
> +	dma_fence_get(dep);
> +out_return_dep:
> +	mutex_unlock(&adev->enforce_isolation_mutex);
> +	return dep;
> +}
> +
>  bool amdgpu_device_has_display_hardware(struct amdgpu_device *adev)
>  {
>  	switch (adev->asic_type) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 56d27cea052e..92ab821afc06 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -287,40 +287,27 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm,
>  	    (*id)->flushed_updates < updates ||
>  	    !(*id)->last_flush ||
>  	    ((*id)->last_flush->context != fence_context &&
> -	     !dma_fence_is_signaled((*id)->last_flush))) {
> +	     !dma_fence_is_signaled((*id)->last_flush)))
> +		needs_flush = true;
> +
> +	if ((*id)->owner != vm->immediate.fence_context ||
> +	    (!adev->vm_manager.concurrent_flush && needs_flush)) {
>  		struct dma_fence *tmp;
>  
> -		/* Wait for the gang to be assembled before using a
> -		 * reserved VMID or otherwise the gang could deadlock.
> +		/* Don't use per engine and per process VMID at the
> +		 * same time
>  		 */
> -		tmp = amdgpu_device_get_gang(adev);
> -		if (!dma_fence_is_signaled(tmp) && tmp != job->gang_submit) {
> +		if (adev->vm_manager.concurrent_flush)
> +			ring = NULL;
> +
> +		/* to prevent one context starved by another context */
> +		(*id)->pd_gpu_addr = 0;
> +		tmp = amdgpu_sync_peek_fence(&(*id)->active, ring);
> +		if (tmp) {
>  			*id = NULL;
> -			*fence = tmp;
> +			*fence = dma_fence_get(tmp);
>  			return 0;
>  		}
> -		dma_fence_put(tmp);
> -
> -		/* Make sure the id is owned by the gang before proceeding */
> -		if (!job->gang_submit ||
> -		    (*id)->owner != vm->immediate.fence_context) {
> -
> -			/* Don't use per engine and per process VMID at the
> -			 * same time
> -			 */
> -			if (adev->vm_manager.concurrent_flush)
> -				ring = NULL;
> -
> -			/* to prevent one context starved by another context */
> -			(*id)->pd_gpu_addr = 0;
> -			tmp = amdgpu_sync_peek_fence(&(*id)->active, ring);
> -			if (tmp) {
> -				*id = NULL;
> -				*fence = dma_fence_get(tmp);
> -				return 0;
> -			}
> -		}
> -		needs_flush = true;
>  	}
>  
>  	/* Good we can use this VMID. Remember this submission as
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> index 5537c8bfd227..1a6cfef4c071 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c
> @@ -360,17 +360,24 @@ amdgpu_job_prepare_job(struct drm_sched_job *sched_job,
>  {
>  	struct amdgpu_ring *ring = to_amdgpu_ring(s_entity->rq->sched);
>  	struct amdgpu_job *job = to_amdgpu_job(sched_job);
> -	struct dma_fence *fence = NULL;
> +	struct dma_fence *fence;
>  	int r;
>  
>  	r = drm_sched_entity_error(s_entity);
>  	if (r)
>  		goto error;
>  
> -	if (job->gang_submit)
> +	if (job->gang_submit) {
>  		fence = amdgpu_device_switch_gang(ring->adev, job->gang_submit);
> +		if (fence)
> +			return fence;
> +	}
> +
> +	fence = amdgpu_device_enforce_isolation(ring->adev, ring, job);
> +	if (fence)
> +		return fence;
>  
> -	if (!fence && job->vm && !job->vmid) {
> +	if (job->vm && !job->vmid) {
>  		r = amdgpu_vmid_grab(job->vm, ring, job, &fence);
>  		if (r) {
>  			dev_err(ring->adev->dev, "Error getting VM ID (%d)\n", r);
> @@ -383,9 +390,10 @@ amdgpu_job_prepare_job(struct drm_sched_job *sched_job,
>  		 */
>  		if (!fence)
>  			job->vm = NULL;
> +		return fence;
>  	}
>  
> -	return fence;
> +	return NULL;
>  
>  error:
>  	dma_fence_set_error(&job->base.s_fence->finished, r);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index bfe12164d27d..c5f9db6b32a4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -405,6 +405,25 @@ int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone)
>  	return 0;
>  }
>  
> +/**
> + * amdgpu_sync_move - move all fences from src to dst
> + *
> + * @src: source of the fences, empty after function
> + * @dst: destination for the fences
> + *
> + * Moves all fences from source to destination. All fences in destination are
> + * freed and source is empty after the function call.
> + */
> +void amdgpu_sync_move(struct amdgpu_sync *src, struct amdgpu_sync *dst)
> +{
> +	unsigned int i;
> +
> +	amdgpu_sync_free(dst);
> +
> +	for (i = 0; i < HASH_SIZE(src->fences); ++i)
> +		hlist_move_list(&src->fences[i], &dst->fences[i]);
> +}
> +
>  /**
>   * amdgpu_sync_push_to_job - push fences into job
>   * @sync: sync object to get the fences from
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> index 1504f5e7fc46..51eb4382c91e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> @@ -57,6 +57,7 @@ struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
>  				     struct amdgpu_ring *ring);
>  struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync);
>  int amdgpu_sync_clone(struct amdgpu_sync *source, struct amdgpu_sync *clone);
> +void amdgpu_sync_move(struct amdgpu_sync *src, struct amdgpu_sync *dst);
>  int amdgpu_sync_push_to_job(struct amdgpu_sync *sync, struct amdgpu_job *job);
>  int amdgpu_sync_wait(struct amdgpu_sync *sync, bool intr);
>  void amdgpu_sync_free(struct amdgpu_sync *sync);

[PATCH 5/8] drm/amdgpu: rework how the cleaner shader is emitted v3

[Christian König]

Instead of emitting the cleaner shader for every job which has the
enforce_isolation flag set only emit it for the first submission from
every client.

v2: add missing NULL check
v3: fix another NULL pointer deref

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ef4fe2df8398..dc10bea836db 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -643,6 +643,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 		    bool need_pipe_sync)
 {
 	struct amdgpu_device *adev = ring->adev;
+	struct amdgpu_isolation *isolation = &adev->isolation[ring->xcp_id];
 	unsigned vmhub = ring->vm_hub;
 	struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
 	struct amdgpu_vmid *id = &id_mgr->ids[job->vmid];
@@ -650,8 +651,9 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	bool gds_switch_needed = ring->funcs->emit_gds_switch &&
 		job->gds_switch_needed;
 	bool vm_flush_needed = job->vm_needs_flush;
-	struct dma_fence *fence = NULL;
+	bool cleaner_shader_needed = false;
 	bool pasid_mapping_needed = false;
+	struct dma_fence *fence = NULL;
 	unsigned int patch;
 	int r;
 
@@ -674,8 +676,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	pasid_mapping_needed &= adev->gmc.gmc_funcs->emit_pasid_mapping &&
 		ring->funcs->emit_wreg;
 
+	cleaner_shader_needed = adev->gfx.enable_cleaner_shader &&
+		ring->funcs->emit_cleaner_shader && job->base.s_fence &&
+		&job->base.s_fence->scheduled == isolation->spearhead;
+
 	if (!vm_flush_needed && !gds_switch_needed && !need_pipe_sync &&
-	    !(job->enforce_isolation && !job->vmid))
+	    !cleaner_shader_needed)
 		return 0;
 
 	amdgpu_ring_ib_begin(ring);
@@ -686,9 +692,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	if (need_pipe_sync)
 		amdgpu_ring_emit_pipeline_sync(ring);
 
-	if (adev->gfx.enable_cleaner_shader &&
-	    ring->funcs->emit_cleaner_shader &&
-	    job->enforce_isolation)
+	if (cleaner_shader_needed)
 		ring->funcs->emit_cleaner_shader(ring);
 
 	if (vm_flush_needed) {
@@ -710,7 +714,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 					    job->oa_size);
 	}
 
-	if (vm_flush_needed || pasid_mapping_needed) {
+	if (vm_flush_needed || pasid_mapping_needed || cleaner_shader_needed) {
 		r = amdgpu_fence_emit(ring, &fence, NULL, 0);
 		if (r)
 			return r;
@@ -732,6 +736,17 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 		id->pasid_mapping = dma_fence_get(fence);
 		mutex_unlock(&id_mgr->lock);
 	}
+
+	/*
+	 * Make sure that all other submissions wait for the cleaner shader to
+	 * finish before we push them to the HW.
+	 */
+	if (cleaner_shader_needed) {
+		mutex_lock(&adev->enforce_isolation_mutex);
+		dma_fence_put(isolation->spearhead);
+		isolation->spearhead = dma_fence_get(fence);
+		mutex_unlock(&adev->enforce_isolation_mutex);
+	}
 	dma_fence_put(fence);
 
 	amdgpu_ring_patch_cond_exec(ring, patch);
-- 
2.34.1

Re: [PATCH 5/8] drm/amdgpu: rework how the cleaner shader is emitted v3

[SRINIVASAN SHANMUGAM]

[-- Attachment #1: Type: text/plain, Size: 3637 bytes --]


On 3/7/2025 7:18 PM, Christian König wrote:
> Instead of emitting the cleaner shader for every job which has the
> enforce_isolation flag set only emit it for the first submission from
> every client.
>
> v2: add missing NULL check
> v3: fix another NULL pointer deref
>
> Signed-off-by: Christian König<christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 27 ++++++++++++++++++++------
>   1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index ef4fe2df8398..dc10bea836db 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -643,6 +643,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   		    bool need_pipe_sync)
>   {
>   	struct amdgpu_device *adev = ring->adev;
> +	struct amdgpu_isolation *isolation = &adev->isolation[ring->xcp_id];
>   	unsigned vmhub = ring->vm_hub;
>   	struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
>   	struct amdgpu_vmid *id = &id_mgr->ids[job->vmid];
> @@ -650,8 +651,9 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   	bool gds_switch_needed = ring->funcs->emit_gds_switch &&
>   		job->gds_switch_needed;
>   	bool vm_flush_needed = job->vm_needs_flush;
> -	struct dma_fence *fence = NULL;
> +	bool cleaner_shader_needed = false;
>   	bool pasid_mapping_needed = false;
> +	struct dma_fence *fence = NULL;
>   	unsigned int patch;
>   	int r;
>   
> @@ -674,8 +676,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   	pasid_mapping_needed &= adev->gmc.gmc_funcs->emit_pasid_mapping &&
>   		ring->funcs->emit_wreg;
>   
> +	cleaner_shader_needed = adev->gfx.enable_cleaner_shader &&
> +		ring->funcs->emit_cleaner_shader && job->base.s_fence &&
> +		&job->base.s_fence->scheduled == isolation->spearhead;
> +
>   	if (!vm_flush_needed && !gds_switch_needed && !need_pipe_sync &&
> -	    !(job->enforce_isolation && !job->vmid))
> +	    !cleaner_shader_needed)
>   		return 0;
>   
>   	amdgpu_ring_ib_begin(ring);
> @@ -686,9 +692,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   	if (need_pipe_sync)
>   		amdgpu_ring_emit_pipeline_sync(ring);
>   
> -	if (adev->gfx.enable_cleaner_shader &&
> -	    ring->funcs->emit_cleaner_shader &&
> -	    job->enforce_isolation)
> +	if (cleaner_shader_needed)

Here should we also need to check, for ring->funcs->emit_cleaner_shader?

if (cleaner_shader_needed && ring->funcs->emit_cleaner_shader)

>   		ring->funcs->emit_cleaner_shader(ring);
>   
>   	if (vm_flush_needed) {
> @@ -710,7 +714,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   					    job->oa_size);
>   	}
>   
> -	if (vm_flush_needed || pasid_mapping_needed) {
> +	if (vm_flush_needed || pasid_mapping_needed || cleaner_shader_needed) {
>   		r = amdgpu_fence_emit(ring, &fence, NULL, 0);
>   		if (r)
>   			return r;
> @@ -732,6 +736,17 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>   		id->pasid_mapping = dma_fence_get(fence);
>   		mutex_unlock(&id_mgr->lock);
>   	}
> +
> +	/*
> +	 * Make sure that all other submissions wait for the cleaner shader to
> +	 * finish before we push them to the HW.
> +	 */
> +	if (cleaner_shader_needed) {
> +		mutex_lock(&adev->enforce_isolation_mutex);
> +		dma_fence_put(isolation->spearhead);
> +		isolation->spearhead = dma_fence_get(fence);
> +		mutex_unlock(&adev->enforce_isolation_mutex);
> +	}
>   	dma_fence_put(fence);
>   
>   	amdgpu_ring_patch_cond_exec(ring, patch);

[-- Attachment #2: Type: text/html, Size: 4426 bytes --]

Re: [PATCH 5/8] drm/amdgpu: rework how the cleaner shader is emitted v3

[Christian König]

[-- Attachment #1: Type: text/plain, Size: 3930 bytes --]

Am 14.03.25 um 05:24 schrieb SRINIVASAN SHANMUGAM:
> On 3/7/2025 7:18 PM, Christian König wrote:
>> Instead of emitting the cleaner shader for every job which has the
>> enforce_isolation flag set only emit it for the first submission from
>> every client.
>>
>> v2: add missing NULL check
>> v3: fix another NULL pointer deref
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 27 ++++++++++++++++++++------
>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index ef4fe2df8398..dc10bea836db 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -643,6 +643,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>>  		    bool need_pipe_sync)
>>  {
>>  	struct amdgpu_device *adev = ring->adev;
>> +	struct amdgpu_isolation *isolation = &adev->isolation[ring->xcp_id];
>>  	unsigned vmhub = ring->vm_hub;
>>  	struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
>>  	struct amdgpu_vmid *id = &id_mgr->ids[job->vmid];
>> @@ -650,8 +651,9 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>>  	bool gds_switch_needed = ring->funcs->emit_gds_switch &&
>>  		job->gds_switch_needed;
>>  	bool vm_flush_needed = job->vm_needs_flush;
>> -	struct dma_fence *fence = NULL;
>> +	bool cleaner_shader_needed = false;
>>  	bool pasid_mapping_needed = false;
>> +	struct dma_fence *fence = NULL;
>>  	unsigned int patch;
>>  	int r;
>>  
>> @@ -674,8 +676,12 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>>  	pasid_mapping_needed &= adev->gmc.gmc_funcs->emit_pasid_mapping &&
>>  		ring->funcs->emit_wreg;
>>  
>> +	cleaner_shader_needed = adev->gfx.enable_cleaner_shader &&
>> +		ring->funcs->emit_cleaner_shader && job->base.s_fence &&
>> +		&job->base.s_fence->scheduled == isolation->spearhead;

*here*

>> +
>>  	if (!vm_flush_needed && !gds_switch_needed && !need_pipe_sync &&
>> -	    !(job->enforce_isolation && !job->vmid))
>> +	    !cleaner_shader_needed)
>>  		return 0;
>>  
>>  	amdgpu_ring_ib_begin(ring);
>> @@ -686,9 +692,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>>  	if (need_pipe_sync)
>>  		amdgpu_ring_emit_pipeline_sync(ring);
>>  
>> -	if (adev->gfx.enable_cleaner_shader &&
>> -	    ring->funcs->emit_cleaner_shader &&
>> -	    job->enforce_isolation)
>> +	if (cleaner_shader_needed)
>
> Here should we also need to check, for ring->funcs->emit_cleaner_shader?
>

I moved that up to where cleaner_shader_needed is set. See the *here* above.

That makes it easier to decide if we need fence after the preamble or not.

Regards,
Christian.

> if (cleaner_shader_needed && ring->funcs->emit_cleaner_shader)
>
>>  		ring->funcs->emit_cleaner_shader(ring);
>>  
>>  	if (vm_flush_needed) {
>> @@ -710,7 +714,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>>  					    job->oa_size);
>>  	}
>>  
>> -	if (vm_flush_needed || pasid_mapping_needed) {
>> +	if (vm_flush_needed || pasid_mapping_needed || cleaner_shader_needed) {
>>  		r = amdgpu_fence_emit(ring, &fence, NULL, 0);
>>  		if (r)
>>  			return r;
>> @@ -732,6 +736,17 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
>>  		id->pasid_mapping = dma_fence_get(fence);
>>  		mutex_unlock(&id_mgr->lock);
>>  	}
>> +
>> +	/*
>> +	 * Make sure that all other submissions wait for the cleaner shader to
>> +	 * finish before we push them to the HW.
>> +	 */
>> +	if (cleaner_shader_needed) {
>> +		mutex_lock(&adev->enforce_isolation_mutex);
>> +		dma_fence_put(isolation->spearhead);
>> +		isolation->spearhead = dma_fence_get(fence);
>> +		mutex_unlock(&adev->enforce_isolation_mutex);
>> +	}
>>  	dma_fence_put(fence);
>>  
>>  	amdgpu_ring_patch_cond_exec(ring, patch);

[-- Attachment #2: Type: text/html, Size: 5360 bytes --]

[PATCH 6/8] drm/amdgpu: stop reserving VMIDs to enforce isolation

[Christian König]

That was quite troublesome for gang submit. Completely drop this
approach and enforce the isolation separately.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c |  9 +--------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 11 +++--------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |  3 +--
 4 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 2ce0c6a152a6..4375e5019418 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1111,7 +1111,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 			struct drm_gpu_scheduler *sched = entity->rq->sched;
 			struct amdgpu_ring *ring = to_amdgpu_ring(sched);
 
-			if (amdgpu_vmid_uses_reserved(adev, vm, ring->vm_hub))
+			if (amdgpu_vmid_uses_reserved(vm, ring->vm_hub))
 				return -EINVAL;
 		}
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index a194bf3347cb..9e5f6b11d923 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -1665,15 +1665,8 @@ static ssize_t amdgpu_gfx_set_enforce_isolation(struct device *dev,
 	}
 
 	mutex_lock(&adev->enforce_isolation_mutex);
-	for (i = 0; i < num_partitions; i++) {
-		if (adev->enforce_isolation[i] && !partition_values[i])
-			/* Going from enabled to disabled */
-			amdgpu_vmid_free_reserved(adev, AMDGPU_GFXHUB(i));
-		else if (!adev->enforce_isolation[i] && partition_values[i])
-			/* Going from disabled to enabled */
-			amdgpu_vmid_alloc_reserved(adev, AMDGPU_GFXHUB(i));
+	for (i = 0; i < num_partitions; i++)
 		adev->enforce_isolation[i] = partition_values[i];
-	}
 	mutex_unlock(&adev->enforce_isolation_mutex);
 
 	amdgpu_mes_update_enforce_isolation(adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
index 92ab821afc06..4c4e087230ac 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
@@ -411,7 +411,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 	if (r || !idle)
 		goto error;
 
-	if (amdgpu_vmid_uses_reserved(adev, vm, vmhub)) {
+	if (amdgpu_vmid_uses_reserved(vm, vmhub)) {
 		r = amdgpu_vmid_grab_reserved(vm, ring, job, &id, fence);
 		if (r || !id)
 			goto error;
@@ -464,19 +464,14 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
 
 /*
  * amdgpu_vmid_uses_reserved - check if a VM will use a reserved VMID
- * @adev: amdgpu_device pointer
  * @vm: the VM to check
  * @vmhub: the VMHUB which will be used
  *
  * Returns: True if the VM will use a reserved VMID.
  */
-bool amdgpu_vmid_uses_reserved(struct amdgpu_device *adev,
-			       struct amdgpu_vm *vm, unsigned int vmhub)
+bool amdgpu_vmid_uses_reserved(struct amdgpu_vm *vm, unsigned int vmhub)
 {
-	return vm->reserved_vmid[vmhub] ||
-		(adev->enforce_isolation[(vm->root.bo->xcp_id != AMDGPU_XCP_NO_PARTITION) ?
-					 vm->root.bo->xcp_id : 0] &&
-		 AMDGPU_IS_GFXHUB(vmhub));
+	return vm->reserved_vmid[vmhub];
 }
 
 int amdgpu_vmid_alloc_reserved(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
index 4012fb2dd08a..240fa6751260 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
@@ -78,8 +78,7 @@ void amdgpu_pasid_free_delayed(struct dma_resv *resv,
 
 bool amdgpu_vmid_had_gpu_reset(struct amdgpu_device *adev,
 			       struct amdgpu_vmid *id);
-bool amdgpu_vmid_uses_reserved(struct amdgpu_device *adev,
-			       struct amdgpu_vm *vm, unsigned int vmhub);
+bool amdgpu_vmid_uses_reserved(struct amdgpu_vm *vm, unsigned int vmhub);
 int amdgpu_vmid_alloc_reserved(struct amdgpu_device *adev,
 				unsigned vmhub);
 void amdgpu_vmid_free_reserved(struct amdgpu_device *adev,
-- 
2.34.1

Re: [PATCH 6/8] drm/amdgpu: stop reserving VMIDs to enforce isolation

[SRINIVASAN SHANMUGAM]

[-- Attachment #1: Type: text/plain, Size: 5355 bytes --]


On 3/7/2025 7:18 PM, Christian König wrote:
> That was quite troublesome for gang submit. Completely drop this
> approach and enforce the isolation separately.
>
> Signed-off-by: Christian König<christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c |  9 +--------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 11 +++--------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |  3 +--
>   4 files changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 2ce0c6a152a6..4375e5019418 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1111,7 +1111,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   			struct drm_gpu_scheduler *sched = entity->rq->sched;
>   			struct amdgpu_ring *ring = to_amdgpu_ring(sched);
>   
> -			if (amdgpu_vmid_uses_reserved(adev, vm, ring->vm_hub))
> +			if (amdgpu_vmid_uses_reserved(vm, ring->vm_hub))
>   				return -EINVAL;
>   		}
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index a194bf3347cb..9e5f6b11d923 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -1665,15 +1665,8 @@ static ssize_t amdgpu_gfx_set_enforce_isolation(struct device *dev,
>   	}
>   
>   	mutex_lock(&adev->enforce_isolation_mutex);
> -	for (i = 0; i < num_partitions; i++) {
> -		if (adev->enforce_isolation[i] && !partition_values[i])
> -			/* Going from enabled to disabled */
> -			amdgpu_vmid_free_reserved(adev, AMDGPU_GFXHUB(i));
> -		else if (!adev->enforce_isolation[i] && partition_values[i])
> -			/* Going from disabled to enabled */
> -			amdgpu_vmid_alloc_reserved(adev, AMDGPU_GFXHUB(i));
> +	for (i = 0; i < num_partitions; i++)
>   		adev->enforce_isolation[i] = partition_values[i];
> -	}
>   	mutex_unlock(&adev->enforce_isolation_mutex);
>   
>   	amdgpu_mes_update_enforce_isolation(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 92ab821afc06..4c4e087230ac 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -411,7 +411,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   	if (r || !idle)
>   		goto error;
>   
> -	if (amdgpu_vmid_uses_reserved(adev, vm, vmhub)) {
> +	if (amdgpu_vmid_uses_reserved(vm, vmhub)) {
>   		r = amdgpu_vmid_grab_reserved(vm, ring, job, &id, fence);
>   		if (r || !id)
>   			goto error;
> @@ -464,19 +464,14 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   
>   /*
>    * amdgpu_vmid_uses_reserved - check if a VM will use a reserved VMID
> - * @adev: amdgpu_device pointer
>    * @vm: the VM to check
>    * @vmhub: the VMHUB which will be used
>    *
>    * Returns: True if the VM will use a reserved VMID.
>    */
> -bool amdgpu_vmid_uses_reserved(struct amdgpu_device *adev,
> -			       struct amdgpu_vm *vm, unsigned int vmhub)
> +bool amdgpu_vmid_uses_reserved(struct amdgpu_vm *vm, unsigned int vmhub)
>   {
> -	return vm->reserved_vmid[vmhub] ||
> -		(adev->enforce_isolation[(vm->root.bo->xcp_id != AMDGPU_XCP_NO_PARTITION) ?
> -					 vm->root.bo->xcp_id : 0] &&
> -		 AMDGPU_IS_GFXHUB(vmhub));
> +	return vm->reserved_vmid[vmhub];
>   }
>   
>   int amdgpu_vmid_alloc_reserved(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> index 4012fb2dd08a..240fa6751260 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> @@ -78,8 +78,7 @@ void amdgpu_pasid_free_delayed(struct dma_resv *resv,
>   
>   bool amdgpu_vmid_had_gpu_reset(struct amdgpu_device *adev,
>   			       struct amdgpu_vmid *id);
> -bool amdgpu_vmid_uses_reserved(struct amdgpu_device *adev,
> -			       struct amdgpu_vm *vm, unsigned int vmhub);
> +bool amdgpu_vmid_uses_reserved(struct amdgpu_vm *vm, unsigned int vmhub);
>   int amdgpu_vmid_alloc_reserved(struct amdgpu_device *adev,
>   				unsigned vmhub);
>   void amdgpu_vmid_free_reserved(struct amdgpu_device *adev,
here we are trying to remove process isolation based on VMID's  this is 
because the constraints that we have limited number of vimids, that 
could be assigned to limited number of clients, now we have switched one 
level above, that is rather than enforcing isolation on client VMID's , 
we are directly applying enforce_isolation onto clients, but not on 
client's vmids,? so based on my this understanding we have removed the 
enforce_isolation based on vmids in this patch6 & now we are doing 
process isolation directly on client based on 
https://patchwork.freedesktop.org/patch/638081/?series=145031&rev=1 
<https://patchwork.freedesktop.org/patch/638081/?series=145031&rev=1> 
ie., based on patch4
[4/8] drm/amdgpu: rework how isolation is enforced v2, we are doing 
process isolation directly based on first client ie., based on who is 
the first client or first owner & then after completing all jobs wrt 
first client or owner , then it is switched to second client or next 
owner? is my understanding is right please? ie., based on 
"amdgpu_device_enforce_isolation()" function

[-- Attachment #2: Type: text/html, Size: 6490 bytes --]

Re: [PATCH 6/8] drm/amdgpu: stop reserving VMIDs to enforce isolation

[Christian König]

[-- Attachment #1: Type: text/plain, Size: 5555 bytes --]

Am 12.03.25 um 16:10 schrieb SRINIVASAN SHANMUGAM:
> On 3/7/2025 7:18 PM, Christian König wrote:
>> That was quite troublesome for gang submit. Completely drop this
>> approach and enforce the isolation separately.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c |  9 +--------
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 11 +++--------
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |  3 +--
>>  4 files changed, 6 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 2ce0c6a152a6..4375e5019418 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1111,7 +1111,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>>  			struct drm_gpu_scheduler *sched = entity->rq->sched;
>>  			struct amdgpu_ring *ring = to_amdgpu_ring(sched);
>>  
>> -			if (amdgpu_vmid_uses_reserved(adev, vm, ring->vm_hub))
>> +			if (amdgpu_vmid_uses_reserved(vm, ring->vm_hub))
>>  				return -EINVAL;
>>  		}
>>  	}
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> index a194bf3347cb..9e5f6b11d923 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>> @@ -1665,15 +1665,8 @@ static ssize_t amdgpu_gfx_set_enforce_isolation(struct device *dev,
>>  	}
>>  
>>  	mutex_lock(&adev->enforce_isolation_mutex);
>> -	for (i = 0; i < num_partitions; i++) {
>> -		if (adev->enforce_isolation[i] && !partition_values[i])
>> -			/* Going from enabled to disabled */
>> -			amdgpu_vmid_free_reserved(adev, AMDGPU_GFXHUB(i));
>> -		else if (!adev->enforce_isolation[i] && partition_values[i])
>> -			/* Going from disabled to enabled */
>> -			amdgpu_vmid_alloc_reserved(adev, AMDGPU_GFXHUB(i));
>> +	for (i = 0; i < num_partitions; i++)
>>  		adev->enforce_isolation[i] = partition_values[i];
>> -	}
>>  	mutex_unlock(&adev->enforce_isolation_mutex);
>>  
>>  	amdgpu_mes_update_enforce_isolation(adev);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>> index 92ab821afc06..4c4e087230ac 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
>> @@ -411,7 +411,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>>  	if (r || !idle)
>>  		goto error;
>>  
>> -	if (amdgpu_vmid_uses_reserved(adev, vm, vmhub)) {
>> +	if (amdgpu_vmid_uses_reserved(vm, vmhub)) {
>>  		r = amdgpu_vmid_grab_reserved(vm, ring, job, &id, fence);
>>  		if (r || !id)
>>  			goto error;
>> @@ -464,19 +464,14 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>>  
>>  /*
>>   * amdgpu_vmid_uses_reserved - check if a VM will use a reserved VMID
>> - * @adev: amdgpu_device pointer
>>   * @vm: the VM to check
>>   * @vmhub: the VMHUB which will be used
>>   *
>>   * Returns: True if the VM will use a reserved VMID.
>>   */
>> -bool amdgpu_vmid_uses_reserved(struct amdgpu_device *adev,
>> -			       struct amdgpu_vm *vm, unsigned int vmhub)
>> +bool amdgpu_vmid_uses_reserved(struct amdgpu_vm *vm, unsigned int vmhub)
>>  {
>> -	return vm->reserved_vmid[vmhub] ||
>> -		(adev->enforce_isolation[(vm->root.bo->xcp_id != AMDGPU_XCP_NO_PARTITION) ?
>> -					 vm->root.bo->xcp_id : 0] &&
>> -		 AMDGPU_IS_GFXHUB(vmhub));
>> +	return vm->reserved_vmid[vmhub];
>>  }
>>  
>>  int amdgpu_vmid_alloc_reserved(struct amdgpu_device *adev,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
>> index 4012fb2dd08a..240fa6751260 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
>> @@ -78,8 +78,7 @@ void amdgpu_pasid_free_delayed(struct dma_resv *resv,
>>  
>>  bool amdgpu_vmid_had_gpu_reset(struct amdgpu_device *adev,
>>  			       struct amdgpu_vmid *id);
>> -bool amdgpu_vmid_uses_reserved(struct amdgpu_device *adev,
>> -			       struct amdgpu_vm *vm, unsigned int vmhub);
>> +bool amdgpu_vmid_uses_reserved(struct amdgpu_vm *vm, unsigned int vmhub);
>>  int amdgpu_vmid_alloc_reserved(struct amdgpu_device *adev,
>>  				unsigned vmhub);
>>  void amdgpu_vmid_free_reserved(struct amdgpu_device *adev,
> here we are trying to remove process isolation based on VMID's  this is because the constraints that we have limited number of vimids, that could be assigned to limited number of clients, now we have switched one level above, that is rather than enforcing isolation on client VMID's , we are directly applying enforce_isolation onto clients, but not on client's vmids,?

Exactly that, yes. We basically separate the two features.

> so based on my this understanding we have removed the enforce_isolation based on vmids in this patch6 & now we are doing process isolation directly on client based on https://patchwork.freedesktop.org/patch/638081/?series=145031&rev=1 <https://patchwork.freedesktop.org/patch/638081/?series=145031&rev=1> ie., based on patch4
> [4/8] drm/amdgpu: rework how isolation is enforced v2, we are doing process isolation directly based on first client ie., based on who is the first client or first owner & then after completing all jobs wrt first client or owner , then it is switched to second client or next owner? is my understanding is right please? ie., based on "amdgpu_device_enforce_isolation()" function

Yes, exactly that.

Regards,
Christian.


[-- Attachment #2: Type: text/html, Size: 7081 bytes --]

Re: [PATCH 6/8] drm/amdgpu: stop reserving VMIDs to enforce isolation

[SRINIVASAN SHANMUGAM]

On 3/7/2025 7:18 PM, Christian König wrote:
> That was quite troublesome for gang submit. Completely drop this
> approach and enforce the isolation separately.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c  |  2 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c |  9 +--------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 11 +++--------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h |  3 +--
>   4 files changed, 6 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 2ce0c6a152a6..4375e5019418 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1111,7 +1111,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
>   			struct drm_gpu_scheduler *sched = entity->rq->sched;
>   			struct amdgpu_ring *ring = to_amdgpu_ring(sched);
>   
> -			if (amdgpu_vmid_uses_reserved(adev, vm, ring->vm_hub))
> +			if (amdgpu_vmid_uses_reserved(vm, ring->vm_hub))
>   				return -EINVAL;
>   		}
>   	}
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index a194bf3347cb..9e5f6b11d923 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -1665,15 +1665,8 @@ static ssize_t amdgpu_gfx_set_enforce_isolation(struct device *dev,
>   	}
>   
>   	mutex_lock(&adev->enforce_isolation_mutex);
> -	for (i = 0; i < num_partitions; i++) {
> -		if (adev->enforce_isolation[i] && !partition_values[i])
> -			/* Going from enabled to disabled */
> -			amdgpu_vmid_free_reserved(adev, AMDGPU_GFXHUB(i));
> -		else if (!adev->enforce_isolation[i] && partition_values[i])
> -			/* Going from disabled to enabled */
> -			amdgpu_vmid_alloc_reserved(adev, AMDGPU_GFXHUB(i));
> +	for (i = 0; i < num_partitions; i++)
>   		adev->enforce_isolation[i] = partition_values[i];
> -	}
>   	mutex_unlock(&adev->enforce_isolation_mutex);
>   
>   	amdgpu_mes_update_enforce_isolation(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> index 92ab821afc06..4c4e087230ac 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c
> @@ -411,7 +411,7 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   	if (r || !idle)
>   		goto error;
>   
> -	if (amdgpu_vmid_uses_reserved(adev, vm, vmhub)) {
> +	if (amdgpu_vmid_uses_reserved(vm, vmhub)) {
>   		r = amdgpu_vmid_grab_reserved(vm, ring, job, &id, fence);
>   		if (r || !id)

Any Null checks here, needs to be considered?

if fence was expected to be valid after a failed call
         if (fence) {
             dma_fence_put(fence);
         }

>   			goto error;
> @@ -464,19 +464,14 @@ int amdgpu_vmid_grab(struct amdgpu_vm *vm, struct amdgpu_ring *ring,
>   
>   /*
>    * amdgpu_vmid_uses_reserved - check if a VM will use a reserved VMID
> - * @adev: amdgpu_device pointer
>    * @vm: the VM to check
>    * @vmhub: the VMHUB which will be used
>    *
>    * Returns: True if the VM will use a reserved VMID.
>    */
> -bool amdgpu_vmid_uses_reserved(struct amdgpu_device *adev,
> -			       struct amdgpu_vm *vm, unsigned int vmhub)
> +bool amdgpu_vmid_uses_reserved(struct amdgpu_vm *vm, unsigned int vmhub)
>   {
> -	return vm->reserved_vmid[vmhub] ||
> -		(adev->enforce_isolation[(vm->root.bo->xcp_id != AMDGPU_XCP_NO_PARTITION) ?
> -					 vm->root.bo->xcp_id : 0] &&
> -		 AMDGPU_IS_GFXHUB(vmhub));
> +	return vm->reserved_vmid[vmhub];
>   }
>   
>   int amdgpu_vmid_alloc_reserved(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> index 4012fb2dd08a..240fa6751260 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.h
> @@ -78,8 +78,7 @@ void amdgpu_pasid_free_delayed(struct dma_resv *resv,
>   
>   bool amdgpu_vmid_had_gpu_reset(struct amdgpu_device *adev,
>   			       struct amdgpu_vmid *id);
> -bool amdgpu_vmid_uses_reserved(struct amdgpu_device *adev,
> -			       struct amdgpu_vm *vm, unsigned int vmhub);
> +bool amdgpu_vmid_uses_reserved(struct amdgpu_vm *vm, unsigned int vmhub);
>   int amdgpu_vmid_alloc_reserved(struct amdgpu_device *adev,
>   				unsigned vmhub);
>   void amdgpu_vmid_free_reserved(struct amdgpu_device *adev,

[PATCH 7/8] drm/amdgpu: add isolation trace point

[Christian König]

Note when we switch from one isolation owner to another.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h  | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 3fa7788b4e12..c1e2ba96509f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -6988,6 +6988,7 @@ struct dma_fence *amdgpu_device_enforce_isolation(struct amdgpu_device *adev,
 		dma_fence_put(isolation->spearhead);
 		isolation->spearhead = dma_fence_get(&f->scheduled);
 		amdgpu_sync_move(&isolation->active, &isolation->prev);
+		trace_amdgpu_isolation(isolation->owner, owner);
 		isolation->owner = owner;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index 383fce40d4dd..e8147d9a54fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -457,6 +457,23 @@ DEFINE_EVENT(amdgpu_pasid, amdgpu_pasid_freed,
 	    TP_ARGS(pasid)
 );
 
+TRACE_EVENT(amdgpu_isolation,
+	    TP_PROTO(void *prev, void *next),
+	    TP_ARGS(prev, next),
+	    TP_STRUCT__entry(
+			     __field(void *, prev)
+			     __field(void *, next)
+			     ),
+
+	    TP_fast_assign(
+			   __entry->prev = prev;
+			   __entry->next = next;
+			   ),
+	    TP_printk("prev=%p, next=%p",
+		      __entry->prev,
+		      __entry->next)
+);
+
 TRACE_EVENT(amdgpu_bo_list_set,
 	    TP_PROTO(struct amdgpu_bo_list *list, struct amdgpu_bo *bo),
 	    TP_ARGS(list, bo),
-- 
2.34.1

[PATCH 8/8] drm/amdgpu: add cleaner shader trace point

[Christian König]

Note when the cleaner shader is executed.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h | 15 +++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c    |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
index e8147d9a54fc..11dd2e0f7979 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_trace.h
@@ -474,6 +474,21 @@ TRACE_EVENT(amdgpu_isolation,
 		      __entry->next)
 );
 
+TRACE_EVENT(amdgpu_cleaner_shader,
+	    TP_PROTO(struct amdgpu_ring *ring, struct dma_fence *fence),
+	    TP_ARGS(ring, fence),
+	    TP_STRUCT__entry(
+			     __string(ring, ring->name)
+			     __field(u64, seqno)
+			     ),
+
+	    TP_fast_assign(
+			   __assign_str(ring);
+			   __entry->seqno = fence->seqno;
+			   ),
+	    TP_printk("ring=%s, seqno=%Lu", __get_str(ring), __entry->seqno)
+);
+
 TRACE_EVENT(amdgpu_bo_list_set,
 	    TP_PROTO(struct amdgpu_bo_list *list, struct amdgpu_bo *bo),
 	    TP_ARGS(list, bo),
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index dc10bea836db..73c1c97c9b28 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -742,6 +742,7 @@ int amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
 	 * finish before we push them to the HW.
 	 */
 	if (cleaner_shader_needed) {
+		trace_amdgpu_cleaner_shader(ring, fence);
 		mutex_lock(&adev->enforce_isolation_mutex);
 		dma_fence_put(isolation->spearhead);
 		isolation->spearhead = dma_fence_get(fence);
-- 
2.34.1

Re: [PATCH 1/8] drm/amdgpu: grab an additional reference on the gang fence v2

[SRINIVASAN SHANMUGAM]

On 3/7/2025 7:18 PM, Christian König wrote:
> We keep the gang submission fence around in adev, make sure that it
> stays alive.
>
> v2: fix memory leak on retry
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 198d29faa754..337543ec615c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -6889,18 +6889,26 @@ struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
>   {
>   	struct dma_fence *old = NULL;
>   
> +	dma_fence_get(gang);
>   	do {
>   		dma_fence_put(old);
>   		old = amdgpu_device_get_gang(adev);
>   		if (old == gang)
>   			break;
>   
> -		if (!dma_fence_is_signaled(old))
> +		if (!dma_fence_is_signaled(old)) {

Here, should we need to check ?

     // Check if old fence isn't signaled
     if (old && !dma_fence_is_signaled(old)) {

> +			dma_fence_put(gang);
>   			return old;
> +		}
>   
>   	} while (cmpxchg((struct dma_fence __force **)&adev->gang_submit,
>   			 old, gang) != old);
>   
> +	/*
> +	 * Drop it once for the exchanged reference in adev and once for the
> +	 * thread local reference acquired in amdgpu_device_get_gang().
> +	 */
> +	dma_fence_put(old);

if (old)
     dma_fence_put(old); // Ensure to release old reference  only if it 
is valid?


>   	dma_fence_put(old);
>   	return NULL;
>   }

Re: [PATCH 1/8] drm/amdgpu: grab an additional reference on the gang fence v2

[Christian König]

Am 14.03.25 um 05:09 schrieb SRINIVASAN SHANMUGAM:
>
> On 3/7/2025 7:18 PM, Christian König wrote:
>> We keep the gang submission fence around in adev, make sure that it
>> stays alive.
>>
>> v2: fix memory leak on retry
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 198d29faa754..337543ec615c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -6889,18 +6889,26 @@ struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
>>   {
>>       struct dma_fence *old = NULL;
>>   +    dma_fence_get(gang);
>>       do {
>>           dma_fence_put(old);
>>           old = amdgpu_device_get_gang(adev);
>>           if (old == gang)
>>               break;
>>   -        if (!dma_fence_is_signaled(old))
>> +        if (!dma_fence_is_signaled(old)) {
>
> Here, should we need to check ?

The gang is initialized to a dummy fence on bootup. So even when there is never any gang submission the old value is never NULL.

Regards,
Christian.

>
>     // Check if old fence isn't signaled
>     if (old && !dma_fence_is_signaled(old)) {
>
>> +            dma_fence_put(gang);
>>               return old;
>> +        }
>>         } while (cmpxchg((struct dma_fence __force **)&adev->gang_submit,
>>                old, gang) != old);
>>   +    /*
>> +     * Drop it once for the exchanged reference in adev and once for the
>> +     * thread local reference acquired in amdgpu_device_get_gang().
>> +     */
>> +    dma_fence_put(old);
>
> if (old)
>     dma_fence_put(old); // Ensure to release old reference  only if it is valid?
>
>
>>       dma_fence_put(old);
>>       return NULL;
>>   }

Re: [PATCH 1/8] drm/amdgpu: grab an additional reference on the gang fence v2

[SRINIVASAN SHANMUGAM]

Thanks for these patches and feedback's.

The series is:
Acked-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>

On 3/14/2025 7:50 PM, Christian König wrote:
> Am 14.03.25 um 05:09 schrieb SRINIVASAN SHANMUGAM:
>> On 3/7/2025 7:18 PM, Christian König wrote:
>>> We keep the gang submission fence around in adev, make sure that it
>>> stays alive.
>>>
>>> v2: fix memory leak on retry
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +++++++++-
>>>    1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 198d29faa754..337543ec615c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -6889,18 +6889,26 @@ struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
>>>    {
>>>        struct dma_fence *old = NULL;
>>>    +    dma_fence_get(gang);
>>>        do {
>>>            dma_fence_put(old);
>>>            old = amdgpu_device_get_gang(adev);
>>>            if (old == gang)
>>>                break;
>>>    -        if (!dma_fence_is_signaled(old))
>>> +        if (!dma_fence_is_signaled(old)) {
>> Here, should we need to check ?
> The gang is initialized to a dummy fence on bootup. So even when there is never any gang submission the old value is never NULL.
>
> Regards,
> Christian.
>
>>      // Check if old fence isn't signaled
>>      if (old && !dma_fence_is_signaled(old)) {
>>
>>> +            dma_fence_put(gang);
>>>                return old;
>>> +        }
>>>          } while (cmpxchg((struct dma_fence __force **)&adev->gang_submit,
>>>                 old, gang) != old);
>>>    +    /*
>>> +     * Drop it once for the exchanged reference in adev and once for the
>>> +     * thread local reference acquired in amdgpu_device_get_gang().
>>> +     */
>>> +    dma_fence_put(old);
>> if (old)
>>      dma_fence_put(old); // Ensure to release old reference  only if it is valid?
>>
>>
>>>        dma_fence_put(old);
>>>        return NULL;
>>>    }

[PATCH 1/8] drm/amdgpu: grab an additional reference on the gang fence

[Christian König]

We keep the gang submission fence around in adev, make sure that it
stays alive.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index d6b473217c8b..aaa8f9f7b6b2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -6753,6 +6753,7 @@ struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
 {
 	struct dma_fence *old = NULL;
 
+	dma_fence_get(gang);
 	do {
 		dma_fence_put(old);
 		old = amdgpu_device_get_gang(adev);
@@ -6765,6 +6766,7 @@ struct dma_fence *amdgpu_device_switch_gang(struct amdgpu_device *adev,
 	} while (cmpxchg((struct dma_fence __force **)&adev->gang_submit,
 			 old, gang) != old);
 
+	dma_fence_put(old);
 	dma_fence_put(old);
 	return NULL;
 }
-- 
2.34.1

[PATCH 3/8] drm/amdgpu: overwrite signaled fence in amdgpu_sync

[Christian König]

This allows using amdgpu_sync even without peeking into the fences for a
long time.

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

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index 86c17a8946f5..bfe12164d27d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -135,11 +135,16 @@ static bool amdgpu_sync_add_later(struct amdgpu_sync *sync, struct dma_fence *f)
 	struct amdgpu_sync_entry *e;
 
 	hash_for_each_possible(sync->fences, e, node, f->context) {
-		if (unlikely(e->fence->context != f->context))
-			continue;
+		if (dma_fence_is_signaled(e->fence)) {
+			dma_fence_put(e->fence);
+			e->fence = dma_fence_get(f);
+			return true;
+		}
 
-		amdgpu_sync_keep_later(&e->fence, f);
-		return true;
+		if (likely(e->fence->context == f->context)) {
+			amdgpu_sync_keep_later(&e->fence, f);
+			return true;
+		}
 	}
 	return false;
 }
-- 
2.34.1