[PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump

[PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump

[Prike Liang]

This debug dump will help on debugging the evf attached gem obj fence
related issue.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 13 +++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c         |  6 +++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
index 0075469550b0..7030d721196b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
@@ -86,6 +86,19 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 		if (ret) {
 			DRM_ERROR("Failed to attch new eviction fence\n");
 			goto free_err;
+		} else {
+			struct dma_resv_iter cursor;
+			struct dma_fence *fence;
+			static const char *usage[] = { "kernel", "write", "read", "bookkeep" };
+
+			dma_resv_for_each_fence(&cursor, obj->resv, DMA_RESV_USAGE_READ, fence) {
+				DRM_DEBUG("after attach evf the resv dump usage:%s\n"
+					   "after attach evf name:%s timeline name:%s seq:%lld %ssingned\n",
+					usage[dma_resv_iter_usage(&cursor)],
+					fence->ops->get_driver_name(fence),
+					fence->ops->get_timeline_name(fence), fence->seqno,
+					dma_fence_is_signaled(fence) ? "" : "un");
+			}
 		}
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index d09db052e282..1e73ce30d4d7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1675,7 +1675,11 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
 	amdgpu_bo_print_flag(m, bo, VRAM_CONTIGUOUS);
 	amdgpu_bo_print_flag(m, bo, VM_ALWAYS_VALID);
 	amdgpu_bo_print_flag(m, bo, EXPLICIT_SYNC);
-
+	/* Add the gem obj resv fence dump*/
+	if (dma_resv_trylock(bo->tbo.base.resv)) {
+		dma_resv_describe(bo->tbo.base.resv, m);
+		dma_resv_unlock(bo->tbo.base.resv);
+	}
 	seq_puts(m, "\n");
 
 	return size;
-- 
2.34.1

[PATCH 2/4] drm/amdgpu: set the evf name to identify the userq case

[Prike Liang]

The evf fence name can clearly identify the userq usage.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
index 7030d721196b..b34225bbd85d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
@@ -31,7 +31,7 @@
 static const char *
 amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence)
 {
-	return "amdgpu";
+	return "amdgpu_userq_evf";
 }
 
 static const char *
-- 
2.34.1

Re: [PATCH 2/4] drm/amdgpu: set the evf name to identify the userq case

[Christian König]

Am 16.04.25 um 10:50 schrieb Prike Liang:
> The evf fence name can clearly identify the userq usage.
>
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> index 7030d721196b..b34225bbd85d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> @@ -31,7 +31,7 @@
>  static const char *
>  amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence)
>  {
> -	return "amdgpu";
> +	return "amdgpu_userq_evf";

Please use amdgpu_eviction as name here.

Apart from that looks good to me.

Regards,
Christian.

>  }
>  
>  static const char *

RE: [PATCH 2/4] drm/amdgpu: set the evf name to identify the userq case

[Liang, Prike]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, April 16, 2025 7:02 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 2/4] drm/amdgpu: set the evf name to identify the userq
> case
>
> Am 16.04.25 um 10:50 schrieb Prike Liang:
> > The evf fence name can clearly identify the userq usage.
> >
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > index 7030d721196b..b34225bbd85d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > @@ -31,7 +31,7 @@
> >  static const char *
> >  amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence)  {
> > -   return "amdgpu";
> > +   return "amdgpu_userq_evf";
>
> Please use amdgpu_eviction as name here.
> Apart from that looks good to me.

Thanks for the suggestion. I will update in the following later version.

>
> Regards,
> Christian.
>
> >  }
> >
> >  static const char *

[PATCH 3/4] drm/amdgpu: trace the scheduler dependent job fence name

[Prike Liang]

This trace will help in tracking the scheduler dependent
job fence.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 drivers/gpu/drm/scheduler/gpu_scheduler_trace.h | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
index c75302ca3427..473a2cbbd7bd 100644
--- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
+++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
@@ -91,6 +91,7 @@ TRACE_EVENT(drm_sched_job_wait_dep,
 			     __field(struct dma_fence *, fence)
 			     __field(uint64_t, ctx)
 			     __field(unsigned, seqno)
+			     __string(dep_fence_name, fence->ops->get_driver_name(fence))
 			     ),
 
 	    TP_fast_assign(
@@ -99,9 +100,11 @@ TRACE_EVENT(drm_sched_job_wait_dep,
 			   __entry->fence = fence;
 			   __entry->ctx = fence->context;
 			   __entry->seqno = fence->seqno;
+			   __assign_str(dep_fence_name);
 			   ),
-	    TP_printk("job ring=%s, id=%llu, depends fence=%p, context=%llu, seq=%u",
+	    TP_printk("job ring=%s, id=%llu, depends fence(name: %s) addr =%p, context=%llu, seq=%u",
 		      __get_str(name), __entry->id,
+		      __get_str(dep_fence_name),
 		      __entry->fence, __entry->ctx,
 		      __entry->seqno)
 );
-- 
2.34.1

Re: [PATCH 3/4] drm/amdgpu: trace the scheduler dependent job fence name

[Christian König]

Am 16.04.25 um 10:50 schrieb Prike Liang:
> This trace will help in tracking the scheduler dependent
> job fence.

Changes for general DRM code need to got o the appropriate mailing list.

Apart from that IIRC we intentionally didn't do that. Why should the driver name be relevant here?

Regards,
Christian.

>
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/gpu/drm/scheduler/gpu_scheduler_trace.h | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> index c75302ca3427..473a2cbbd7bd 100644
> --- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> @@ -91,6 +91,7 @@ TRACE_EVENT(drm_sched_job_wait_dep,
>  			     __field(struct dma_fence *, fence)
>  			     __field(uint64_t, ctx)
>  			     __field(unsigned, seqno)
> +			     __string(dep_fence_name, fence->ops->get_driver_name(fence))
>  			     ),
>  
>  	    TP_fast_assign(
> @@ -99,9 +100,11 @@ TRACE_EVENT(drm_sched_job_wait_dep,
>  			   __entry->fence = fence;
>  			   __entry->ctx = fence->context;
>  			   __entry->seqno = fence->seqno;
> +			   __assign_str(dep_fence_name);
>  			   ),
> -	    TP_printk("job ring=%s, id=%llu, depends fence=%p, context=%llu, seq=%u",
> +	    TP_printk("job ring=%s, id=%llu, depends fence(name: %s) addr =%p, context=%llu, seq=%u",
>  		      __get_str(name), __entry->id,
> +		      __get_str(dep_fence_name),
>  		      __entry->fence, __entry->ctx,
>  		      __entry->seqno)
>  );

RE: [PATCH 3/4] drm/amdgpu: trace the scheduler dependent job fence name

[Liang, Prike]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, April 16, 2025 7:04 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 3/4] drm/amdgpu: trace the scheduler dependent job fence
> name
>
> Am 16.04.25 um 10:50 schrieb Prike Liang:
> > This trace will help in tracking the scheduler dependent job fence.
>
> Changes for general DRM code need to got o the appropriate mailing list.

Yes, it should be sent to dri-dev group.

> Apart from that IIRC we intentionally didn't do that. Why should the driver name be
> relevant here?

By adding the scheduler-dependent fence name dump, this can help identify which exact fence is depended on by the scheduler job. With this dump, successfully catch the case about the SDMA and GFX kernel queue scheduled jobs that are dependent on the eviction fence when enabling the kq and uq at the same time.

> Regards,
> Christian.
>
> >
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> >  drivers/gpu/drm/scheduler/gpu_scheduler_trace.h | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> > b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> > index c75302ca3427..473a2cbbd7bd 100644
> > --- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> > +++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
> > @@ -91,6 +91,7 @@ TRACE_EVENT(drm_sched_job_wait_dep,
> >                          __field(struct dma_fence *, fence)
> >                          __field(uint64_t, ctx)
> >                          __field(unsigned, seqno)
> > +                        __string(dep_fence_name, fence->ops-
> >get_driver_name(fence))
> >                          ),
> >
> >         TP_fast_assign(
> > @@ -99,9 +100,11 @@ TRACE_EVENT(drm_sched_job_wait_dep,
> >                        __entry->fence = fence;
> >                        __entry->ctx = fence->context;
> >                        __entry->seqno = fence->seqno;
> > +                      __assign_str(dep_fence_name);
> >                        ),
> > -       TP_printk("job ring=%s, id=%llu, depends fence=%p, context=%llu,
> seq=%u",
> > +       TP_printk("job ring=%s, id=%llu, depends fence(name: %s) addr
> > +=%p, context=%llu, seq=%u",
> >                   __get_str(name), __entry->id,
> > +                 __get_str(dep_fence_name),
> >                   __entry->fence, __entry->ctx,
> >                   __entry->seqno)
> >  );

Re: [PATCH 3/4] drm/amdgpu: trace the scheduler dependent job fence name

[Christian König]

Am 16.04.25 um 16:16 schrieb Liang, Prike:
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, April 16, 2025 7:04 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH 3/4] drm/amdgpu: trace the scheduler dependent job fence
>> name
>>
>> Am 16.04.25 um 10:50 schrieb Prike Liang:
>>> This trace will help in tracking the scheduler dependent job fence.
>> Changes for general DRM code need to got o the appropriate mailing list.
> Yes, it should be sent to dri-dev group.
>
>> Apart from that IIRC we intentionally didn't do that. Why should the driver name be
>> relevant here?
> By adding the scheduler-dependent fence name dump, this can help identify which exact fence is depended on by the scheduler job. With this dump, successfully catch the case about the SDMA and GFX kernel queue scheduled jobs that are dependent on the eviction fence when enabling the kq and uq at the same time.

Mhm, I see why you want to make that shortcut but we kind of agreed to identify fences in the scheduler always as context:seqno.

If you then want the driver/timeline names for that you should probably enable the fence trace points as well.

We are trying to reduce the accesses to those names to eventually allow dma_fence destruction independent of the driver module.

Regards,
Christian.

>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>> ---
>>>  drivers/gpu/drm/scheduler/gpu_scheduler_trace.h | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
>>> b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
>>> index c75302ca3427..473a2cbbd7bd 100644
>>> --- a/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
>>> +++ b/drivers/gpu/drm/scheduler/gpu_scheduler_trace.h
>>> @@ -91,6 +91,7 @@ TRACE_EVENT(drm_sched_job_wait_dep,
>>>                          __field(struct dma_fence *, fence)
>>>                          __field(uint64_t, ctx)
>>>                          __field(unsigned, seqno)
>>> +                        __string(dep_fence_name, fence->ops-
>>> get_driver_name(fence))
>>>                          ),
>>>
>>>         TP_fast_assign(
>>> @@ -99,9 +100,11 @@ TRACE_EVENT(drm_sched_job_wait_dep,
>>>                        __entry->fence = fence;
>>>                        __entry->ctx = fence->context;
>>>                        __entry->seqno = fence->seqno;
>>> +                      __assign_str(dep_fence_name);
>>>                        ),
>>> -       TP_printk("job ring=%s, id=%llu, depends fence=%p, context=%llu,
>> seq=%u",
>>> +       TP_printk("job ring=%s, id=%llu, depends fence(name: %s) addr
>>> +=%p, context=%llu, seq=%u",
>>>                   __get_str(name), __entry->id,
>>> +                 __get_str(dep_fence_name),
>>>                   __entry->fence, __entry->ctx,
>>>                   __entry->seqno)
>>>  );

[PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Prike Liang]

Free the evf when the attached bo released. The evf still
be dependent on and referred to by the attached bo that is
scheduled by the kernel queue SDMA or gfx after the evf signalled.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 31 ++++++++++++++++---
 .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
 3 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
index b34225bbd85d..60be1ac5047d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
@@ -27,6 +27,7 @@
 
 #define work_to_evf_mgr(w, name) container_of(w, struct amdgpu_eviction_fence_mgr, name)
 #define evf_mgr_to_fpriv(e) container_of(e, struct amdgpu_fpriv, evf_mgr)
+#define fence_to_evf(f)  container_of(f, struct amdgpu_eviction_fence, base)
 
 static const char *
 amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence)
@@ -47,7 +48,7 @@ int
 amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 				    struct drm_exec *exec)
 {
-	struct amdgpu_eviction_fence *old_ef, *new_ef;
+	struct amdgpu_eviction_fence *new_ef;
 	struct drm_gem_object *obj;
 	unsigned long index;
 	int ret;
@@ -72,7 +73,6 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 
 	/* Update the eviction fence now */
 	spin_lock(&evf_mgr->ev_fence_lock);
-	old_ef = evf_mgr->ev_fence;
 	evf_mgr->ev_fence = new_ef;
 	spin_unlock(&evf_mgr->ev_fence_lock);
 
@@ -102,9 +102,6 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 		}
 	}
 
-	/* Free old fence */
-	if (old_ef)
-		dma_fence_put(&old_ef->base);
 	return 0;
 
 free_err:
@@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct amdgpu_eviction_fence_mgr *evf_mgr,
 	dma_fence_put(stub);
 }
 
+void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo)
+{
+	struct dma_resv *resv = &bo->tbo.base._resv;
+	struct dma_fence *fence, *stub;
+	struct dma_resv_iter cursor;
+
+	dma_resv_assert_held(resv);
+
+	stub = dma_fence_get_stub();
+	dma_resv_for_each_fence(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP, fence) {
+		struct amdgpu_eviction_fence *ev_fence;
+
+		ev_fence = fence_to_evf(fence);
+		if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
+			continue;
+
+		dma_resv_replace_fences(resv, fence->context, stub,
+				DMA_RESV_USAGE_BOOKKEEP);
+
+	}
+
+	dma_fence_put(stub);
+}
+
 int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr *evf_mgr)
 {
 	/* This needs to be done one time per open */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
index fcd867b7147d..da99ac322a2e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
@@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct amdgpu_eviction_fence_mgr *evf_mgr,
 int
 amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 				    struct drm_exec *exec);
+void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo);
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 1e73ce30d4d7..f001018a01eb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo)
 	amdgpu_vram_mgr_set_cleared(bo->resource);
 	dma_resv_add_fence(&bo->base._resv, fence, DMA_RESV_USAGE_KERNEL);
 	dma_fence_put(fence);
+	amdgpu_userq_remove_all_eviction_fences(abo);
 
 out:
 	dma_resv_unlock(&bo->base._resv);
-- 
2.34.1

Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Christian König]

Am 16.04.25 um 10:50 schrieb Prike Liang:
> Free the evf when the attached bo released. The evf still
> be dependent on and referred to by the attached bo that is
> scheduled by the kernel queue SDMA or gfx after the evf signalled.
>
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 31 ++++++++++++++++---
>  .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
>  3 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> index b34225bbd85d..60be1ac5047d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> @@ -27,6 +27,7 @@
>  
>  #define work_to_evf_mgr(w, name) container_of(w, struct amdgpu_eviction_fence_mgr, name)
>  #define evf_mgr_to_fpriv(e) container_of(e, struct amdgpu_fpriv, evf_mgr)
> +#define fence_to_evf(f)  container_of(f, struct amdgpu_eviction_fence, base)
>  
>  static const char *
>  amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence)
> @@ -47,7 +48,7 @@ int
>  amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  				    struct drm_exec *exec)
>  {
> -	struct amdgpu_eviction_fence *old_ef, *new_ef;
> +	struct amdgpu_eviction_fence *new_ef;
>  	struct drm_gem_object *obj;
>  	unsigned long index;
>  	int ret;
> @@ -72,7 +73,6 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  
>  	/* Update the eviction fence now */
>  	spin_lock(&evf_mgr->ev_fence_lock);
> -	old_ef = evf_mgr->ev_fence;
>  	evf_mgr->ev_fence = new_ef;
>  	spin_unlock(&evf_mgr->ev_fence_lock);
>  
> @@ -102,9 +102,6 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  		}
>  	}
>  
> -	/* Free old fence */
> -	if (old_ef)
> -		dma_fence_put(&old_ef->base);

That change looks completely incorrect to me, you will now leak the old fence.

>  	return 0;
>  
>  free_err:
> @@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  	dma_fence_put(stub);
>  }
>  
> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo)

Please name that amdgpu_eviction_fence_remove_all().

Regards,
Christian.

> +{
> +	struct dma_resv *resv = &bo->tbo.base._resv;
> +	struct dma_fence *fence, *stub;
> +	struct dma_resv_iter cursor;
> +
> +	dma_resv_assert_held(resv);
> +
> +	stub = dma_fence_get_stub();
> +	dma_resv_for_each_fence(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP, fence) {
> +		struct amdgpu_eviction_fence *ev_fence;
> +
> +		ev_fence = fence_to_evf(fence);
> +		if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
> +			continue;
> +
> +		dma_resv_replace_fences(resv, fence->context, stub,
> +				DMA_RESV_USAGE_BOOKKEEP);
> +
> +	}
> +
> +	dma_fence_put(stub);
> +}
> +
>  int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr *evf_mgr)
>  {
>  	/* This needs to be done one time per open */
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> index fcd867b7147d..da99ac322a2e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> @@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  int
>  amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  				    struct drm_exec *exec);
> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo);
>  #endif
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 1e73ce30d4d7..f001018a01eb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo)
>  	amdgpu_vram_mgr_set_cleared(bo->resource);
>  	dma_resv_add_fence(&bo->base._resv, fence, DMA_RESV_USAGE_KERNEL);
>  	dma_fence_put(fence);
> +	amdgpu_userq_remove_all_eviction_fences(abo);
>  
>  out:
>  	dma_resv_unlock(&bo->base._resv);

RE: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Liang, Prike]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, April 16, 2025 7:07 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release
>
> Am 16.04.25 um 10:50 schrieb Prike Liang:
> > Free the evf when the attached bo released. The evf still be dependent
> > on and referred to by the attached bo that is scheduled by the kernel
> > queue SDMA or gfx after the evf signalled.
> >
> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> >  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 31 ++++++++++++++++---
> >  .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  1 +
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
> >  3 files changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > index b34225bbd85d..60be1ac5047d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > @@ -27,6 +27,7 @@
> >
> >  #define work_to_evf_mgr(w, name) container_of(w, struct
> > amdgpu_eviction_fence_mgr, name)  #define evf_mgr_to_fpriv(e)
> > container_of(e, struct amdgpu_fpriv, evf_mgr)
> > +#define fence_to_evf(f)  container_of(f, struct
> > +amdgpu_eviction_fence, base)
> >
> >  static const char *
> >  amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence) @@
> > -47,7 +48,7 @@ int  amdgpu_eviction_fence_replace_fence(struct
> > amdgpu_eviction_fence_mgr *evf_mgr,
> >                                 struct drm_exec *exec)
> >  {
> > -   struct amdgpu_eviction_fence *old_ef, *new_ef;
> > +   struct amdgpu_eviction_fence *new_ef;
> >     struct drm_gem_object *obj;
> >     unsigned long index;
> >     int ret;
> > @@ -72,7 +73,6 @@ amdgpu_eviction_fence_replace_fence(struct
> > amdgpu_eviction_fence_mgr *evf_mgr,
> >
> >     /* Update the eviction fence now */
> >     spin_lock(&evf_mgr->ev_fence_lock);
> > -   old_ef = evf_mgr->ev_fence;
> >     evf_mgr->ev_fence = new_ef;
> >     spin_unlock(&evf_mgr->ev_fence_lock);
> >
> > @@ -102,9 +102,6 @@ amdgpu_eviction_fence_replace_fence(struct
> amdgpu_eviction_fence_mgr *evf_mgr,
> >             }
> >     }
> >
> > -   /* Free old fence */
> > -   if (old_ef)
> > -           dma_fence_put(&old_ef->base);
>
> That change looks completely incorrect to me, you will now leak the old fence.

The eviction fence is attached and shared by all the restored validated VM BOs during UQ restore, and at this placement the eviction fence is only detached from one of the BOs. Using amdgpu_userq_remove_all_eviction_fences() will walk over the resv objects and detach the fence from the resv objs when freeing the BO.

But there's a problem: even though dropping all the evf attached to VM BOs with this patch, the evf still referred to by the SDMA and GFX kernel queue jobs at the case when enabling the kq and uq at the same time. Thoughts?

>
> >     return 0;
> >
> >  free_err:
> > @@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct
> amdgpu_eviction_fence_mgr *evf_mgr,
> >     dma_fence_put(stub);
> >  }
> >
> > +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo)
>
> Please name that amdgpu_eviction_fence_remove_all().

Noted.

> Regards,
> Christian.
>
> > +{
> > +   struct dma_resv *resv = &bo->tbo.base._resv;
> > +   struct dma_fence *fence, *stub;
> > +   struct dma_resv_iter cursor;
> > +
> > +   dma_resv_assert_held(resv);
> > +
> > +   stub = dma_fence_get_stub();
> > +   dma_resv_for_each_fence(&cursor, resv,
> DMA_RESV_USAGE_BOOKKEEP, fence) {
> > +           struct amdgpu_eviction_fence *ev_fence;
> > +
> > +           ev_fence = fence_to_evf(fence);
> > +           if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
> > +                   continue;
> > +
> > +           dma_resv_replace_fences(resv, fence->context, stub,
> > +                           DMA_RESV_USAGE_BOOKKEEP);
> > +
> > +   }
> > +
> > +   dma_fence_put(stub);
> > +}
> > +
> >  int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr
> > *evf_mgr)  {
> >     /* This needs to be done one time per open */ diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> > index fcd867b7147d..da99ac322a2e 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> > @@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct
> > amdgpu_eviction_fence_mgr *evf_mgr,  int
> > amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr
> *evf_mgr,
> >                                 struct drm_exec *exec);
> > +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo);
> >  #endif
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index 1e73ce30d4d7..f001018a01eb 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct
> ttm_buffer_object *bo)
> >     amdgpu_vram_mgr_set_cleared(bo->resource);
> >     dma_resv_add_fence(&bo->base._resv, fence,
> DMA_RESV_USAGE_KERNEL);
> >     dma_fence_put(fence);
> > +   amdgpu_userq_remove_all_eviction_fences(abo);
> >
> >  out:
> >     dma_resv_unlock(&bo->base._resv);

Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Christian König]

Am 16.04.25 um 16:47 schrieb Liang, Prike:
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, April 16, 2025 7:07 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release
>>
>> Am 16.04.25 um 10:50 schrieb Prike Liang:
>>> Free the evf when the attached bo released. The evf still be dependent
>>> on and referred to by the attached bo that is scheduled by the kernel
>>> queue SDMA or gfx after the evf signalled.
>>>
>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>> ---
>>>  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 31 ++++++++++++++++---
>>>  .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  1 +
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
>>>  3 files changed, 28 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> index b34225bbd85d..60be1ac5047d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> @@ -27,6 +27,7 @@
>>>
>>>  #define work_to_evf_mgr(w, name) container_of(w, struct
>>> amdgpu_eviction_fence_mgr, name)  #define evf_mgr_to_fpriv(e)
>>> container_of(e, struct amdgpu_fpriv, evf_mgr)
>>> +#define fence_to_evf(f)  container_of(f, struct
>>> +amdgpu_eviction_fence, base)
>>>
>>>  static const char *
>>>  amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence) @@
>>> -47,7 +48,7 @@ int  amdgpu_eviction_fence_replace_fence(struct
>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>                                 struct drm_exec *exec)
>>>  {
>>> -   struct amdgpu_eviction_fence *old_ef, *new_ef;
>>> +   struct amdgpu_eviction_fence *new_ef;
>>>     struct drm_gem_object *obj;
>>>     unsigned long index;
>>>     int ret;
>>> @@ -72,7 +73,6 @@ amdgpu_eviction_fence_replace_fence(struct
>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>
>>>     /* Update the eviction fence now */
>>>     spin_lock(&evf_mgr->ev_fence_lock);
>>> -   old_ef = evf_mgr->ev_fence;
>>>     evf_mgr->ev_fence = new_ef;
>>>     spin_unlock(&evf_mgr->ev_fence_lock);
>>>
>>> @@ -102,9 +102,6 @@ amdgpu_eviction_fence_replace_fence(struct
>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>             }
>>>     }
>>>
>>> -   /* Free old fence */
>>> -   if (old_ef)
>>> -           dma_fence_put(&old_ef->base);
>> That change looks completely incorrect to me, you will now leak the old fence.
> The eviction fence is attached and shared by all the restored validated VM BOs during UQ restore, and at this placement the eviction fence is only detached from one of the BOs. Using amdgpu_userq_remove_all_eviction_fences() will walk over the resv objects and detach the fence from the resv objs when freeing the BO.

Yeah, but that doesn't justify this change here. See you're completely messing up the fence reference count with that.

>
> But there's a problem: even though dropping all the evf attached to VM BOs with this patch, the evf still referred to by the SDMA and GFX kernel queue jobs at the case when enabling the kq and uq at the same time. Thoughts?

Mhm, the eviction fence is always added as bookmark isn't it? As long as the GFX and SDMA jobs are not for evicting something then they should only depend on fences with usage < bookmark.

Can you dig up when they are added to the dependencies of the job?

Thanks,
Christian.

PS: Please stop calling the eviction fence evf.

>
>>>     return 0;
>>>
>>>  free_err:
>>> @@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct
>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>     dma_fence_put(stub);
>>>  }
>>>
>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo)
>> Please name that amdgpu_eviction_fence_remove_all().
> Noted.
>
>> Regards,
>> Christian.
>>
>>> +{
>>> +   struct dma_resv *resv = &bo->tbo.base._resv;
>>> +   struct dma_fence *fence, *stub;
>>> +   struct dma_resv_iter cursor;
>>> +
>>> +   dma_resv_assert_held(resv);
>>> +
>>> +   stub = dma_fence_get_stub();
>>> +   dma_resv_for_each_fence(&cursor, resv,
>> DMA_RESV_USAGE_BOOKKEEP, fence) {
>>> +           struct amdgpu_eviction_fence *ev_fence;
>>> +
>>> +           ev_fence = fence_to_evf(fence);
>>> +           if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
>>> +                   continue;
>>> +
>>> +           dma_resv_replace_fences(resv, fence->context, stub,
>>> +                           DMA_RESV_USAGE_BOOKKEEP);
>>> +
>>> +   }
>>> +
>>> +   dma_fence_put(stub);
>>> +}
>>> +
>>>  int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr
>>> *evf_mgr)  {
>>>     /* This needs to be done one time per open */ diff --git
>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>> index fcd867b7147d..da99ac322a2e 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>> @@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct
>>> amdgpu_eviction_fence_mgr *evf_mgr,  int
>>> amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr
>> *evf_mgr,
>>>                                 struct drm_exec *exec);
>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo);
>>>  #endif
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index 1e73ce30d4d7..f001018a01eb 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct
>> ttm_buffer_object *bo)
>>>     amdgpu_vram_mgr_set_cleared(bo->resource);
>>>     dma_resv_add_fence(&bo->base._resv, fence,
>> DMA_RESV_USAGE_KERNEL);
>>>     dma_fence_put(fence);
>>> +   amdgpu_userq_remove_all_eviction_fences(abo);
>>>
>>>  out:
>>>     dma_resv_unlock(&bo->base._resv);

RE: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Liang, Prike]

[Public]

> -----Original Message-----
> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Thursday, April 17, 2025 3:40 PM
> To: Liang, Prike <Prike.Liang@amd.com>; Koenig, Christian
> <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release
>
> Am 16.04.25 um 16:47 schrieb Liang, Prike:
> > [Public]
> >
> >> From: Koenig, Christian <Christian.Koenig@amd.com>
> >> Sent: Wednesday, April 16, 2025 7:07 PM
> >> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> >> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> >> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached
> >> bo release
> >>
> >> Am 16.04.25 um 10:50 schrieb Prike Liang:
> >>> Free the evf when the attached bo released. The evf still be
> >>> dependent on and referred to by the attached bo that is scheduled by
> >>> the kernel queue SDMA or gfx after the evf signalled.
> >>>
> >>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> >>> ---
> >>>  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 31 ++++++++++++++++--
> -
> >>>  .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  1 +
> >>>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
> >>>  3 files changed, 28 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> >>> index b34225bbd85d..60be1ac5047d 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> >>> @@ -27,6 +27,7 @@
> >>>
> >>>  #define work_to_evf_mgr(w, name) container_of(w, struct
> >>> amdgpu_eviction_fence_mgr, name)  #define evf_mgr_to_fpriv(e)
> >>> container_of(e, struct amdgpu_fpriv, evf_mgr)
> >>> +#define fence_to_evf(f)  container_of(f, struct
> >>> +amdgpu_eviction_fence, base)
> >>>
> >>>  static const char *
> >>>  amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence) @@
> >>> -47,7 +48,7 @@ int  amdgpu_eviction_fence_replace_fence(struct
> >>> amdgpu_eviction_fence_mgr *evf_mgr,
> >>>                                 struct drm_exec *exec)  {
> >>> -   struct amdgpu_eviction_fence *old_ef, *new_ef;
> >>> +   struct amdgpu_eviction_fence *new_ef;
> >>>     struct drm_gem_object *obj;
> >>>     unsigned long index;
> >>>     int ret;
> >>> @@ -72,7 +73,6 @@ amdgpu_eviction_fence_replace_fence(struct
> >>> amdgpu_eviction_fence_mgr *evf_mgr,
> >>>
> >>>     /* Update the eviction fence now */
> >>>     spin_lock(&evf_mgr->ev_fence_lock);
> >>> -   old_ef = evf_mgr->ev_fence;
> >>>     evf_mgr->ev_fence = new_ef;
> >>>     spin_unlock(&evf_mgr->ev_fence_lock);
> >>>
> >>> @@ -102,9 +102,6 @@ amdgpu_eviction_fence_replace_fence(struct
> >> amdgpu_eviction_fence_mgr *evf_mgr,
> >>>             }
> >>>     }
> >>>
> >>> -   /* Free old fence */
> >>> -   if (old_ef)
> >>> -           dma_fence_put(&old_ef->base);
> >> That change looks completely incorrect to me, you will now leak the old fence.
> > The eviction fence is attached and shared by all the restored validated VM BOs
> during UQ restore, and at this placement the eviction fence is only detached from
> one of the BOs. Using amdgpu_userq_remove_all_eviction_fences() will walk over
> the resv objects and detach the fence from the resv objs when freeing the BO.
>
> Yeah, but that doesn't justify this change here. See you're completely messing up
> the fence reference count with that.
>
> >
> > But there's a problem: even though dropping all the evf attached to VM BOs with
> this patch, the evf still referred to by the SDMA and GFX kernel queue jobs at the
> case when enabling the kq and uq at the same time. Thoughts?
>
> Mhm, the eviction fence is always added as bookmark isn't it? As long as the GFX
> and SDMA jobs are not for evicting something then they should only depend on
> fences with usage < bookmark.
>
> Can you dig up when they are added to the dependencies of the job?

When the eviction fence was added to the user queue VM BOs reservation and then updated the BO page table, which will add the eviction fence to the VM sync at amdgpu_sync_resv(), and then the eviction fence will be added as a dependent fence by propagating with amdgpu_sync_push_to_job(). With removing the eviction fence from the VM sync at amdgpu_sync_resv(), then the eviction fence can be released properly.

Thanks,
Prike
>
> Thanks,
> Christian.
>
> PS: Please stop calling the eviction fence evf.
>
> >
> >>>     return 0;
> >>>
> >>>  free_err:
> >>> @@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct
> >> amdgpu_eviction_fence_mgr *evf_mgr,
> >>>     dma_fence_put(stub);
> >>>  }
> >>>
> >>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo)
> >> Please name that amdgpu_eviction_fence_remove_all().
> > Noted.
> >
> >> Regards,
> >> Christian.
> >>
> >>> +{
> >>> +   struct dma_resv *resv = &bo->tbo.base._resv;
> >>> +   struct dma_fence *fence, *stub;
> >>> +   struct dma_resv_iter cursor;
> >>> +
> >>> +   dma_resv_assert_held(resv);
> >>> +
> >>> +   stub = dma_fence_get_stub();
> >>> +   dma_resv_for_each_fence(&cursor, resv,
> >> DMA_RESV_USAGE_BOOKKEEP, fence) {
> >>> +           struct amdgpu_eviction_fence *ev_fence;
> >>> +
> >>> +           ev_fence = fence_to_evf(fence);
> >>> +           if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
> >>> +                   continue;
> >>> +
> >>> +           dma_resv_replace_fences(resv, fence->context, stub,
> >>> +                           DMA_RESV_USAGE_BOOKKEEP);
> >>> +
> >>> +   }
> >>> +
> >>> +   dma_fence_put(stub);
> >>> +}
> >>> +
> >>>  int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr
> >>> *evf_mgr)  {
> >>>     /* This needs to be done one time per open */ diff --git
> >>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> >>> index fcd867b7147d..da99ac322a2e 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> >>> @@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct
> >>> amdgpu_eviction_fence_mgr *evf_mgr,  int
> >>> amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr
> >> *evf_mgr,
> >>>                                 struct drm_exec *exec);
> >>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo);
> >>>  #endif
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>> index 1e73ce30d4d7..f001018a01eb 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>> @@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct
> >> ttm_buffer_object *bo)
> >>>     amdgpu_vram_mgr_set_cleared(bo->resource);
> >>>     dma_resv_add_fence(&bo->base._resv, fence,
> >> DMA_RESV_USAGE_KERNEL);
> >>>     dma_fence_put(fence);
> >>> +   amdgpu_userq_remove_all_eviction_fences(abo);
> >>>
> >>>  out:
> >>>     dma_resv_unlock(&bo->base._resv);

Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Christian König]

Am 22.04.25 um 11:14 schrieb Liang, Prike:
> [Public]
>
>> -----Original Message-----
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Thursday, April 17, 2025 3:40 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; Koenig, Christian
>> <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release
>>
>> Am 16.04.25 um 16:47 schrieb Liang, Prike:
>>> [Public]
>>>
>>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>>> Sent: Wednesday, April 16, 2025 7:07 PM
>>>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>>>> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached
>>>> bo release
>>>>
>>>> Am 16.04.25 um 10:50 schrieb Prike Liang:
>>>>> Free the evf when the attached bo released. The evf still be
>>>>> dependent on and referred to by the attached bo that is scheduled by
>>>>> the kernel queue SDMA or gfx after the evf signalled.
>>>>>
>>>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>>>> ---
>>>>>  .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 31 ++++++++++++++++--
>> -
>>>>>  .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  1 +
>>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
>>>>>  3 files changed, 28 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>>>> index b34225bbd85d..60be1ac5047d 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>>>> @@ -27,6 +27,7 @@
>>>>>
>>>>>  #define work_to_evf_mgr(w, name) container_of(w, struct
>>>>> amdgpu_eviction_fence_mgr, name)  #define evf_mgr_to_fpriv(e)
>>>>> container_of(e, struct amdgpu_fpriv, evf_mgr)
>>>>> +#define fence_to_evf(f)  container_of(f, struct
>>>>> +amdgpu_eviction_fence, base)
>>>>>
>>>>>  static const char *
>>>>>  amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence) @@
>>>>> -47,7 +48,7 @@ int  amdgpu_eviction_fence_replace_fence(struct
>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>                                 struct drm_exec *exec)  {
>>>>> -   struct amdgpu_eviction_fence *old_ef, *new_ef;
>>>>> +   struct amdgpu_eviction_fence *new_ef;
>>>>>     struct drm_gem_object *obj;
>>>>>     unsigned long index;
>>>>>     int ret;
>>>>> @@ -72,7 +73,6 @@ amdgpu_eviction_fence_replace_fence(struct
>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>
>>>>>     /* Update the eviction fence now */
>>>>>     spin_lock(&evf_mgr->ev_fence_lock);
>>>>> -   old_ef = evf_mgr->ev_fence;
>>>>>     evf_mgr->ev_fence = new_ef;
>>>>>     spin_unlock(&evf_mgr->ev_fence_lock);
>>>>>
>>>>> @@ -102,9 +102,6 @@ amdgpu_eviction_fence_replace_fence(struct
>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>             }
>>>>>     }
>>>>>
>>>>> -   /* Free old fence */
>>>>> -   if (old_ef)
>>>>> -           dma_fence_put(&old_ef->base);
>>>> That change looks completely incorrect to me, you will now leak the old fence.
>>> The eviction fence is attached and shared by all the restored validated VM BOs
>> during UQ restore, and at this placement the eviction fence is only detached from
>> one of the BOs. Using amdgpu_userq_remove_all_eviction_fences() will walk over
>> the resv objects and detach the fence from the resv objs when freeing the BO.
>>
>> Yeah, but that doesn't justify this change here. See you're completely messing up
>> the fence reference count with that.
>>
>>> But there's a problem: even though dropping all the evf attached to VM BOs with
>> this patch, the evf still referred to by the SDMA and GFX kernel queue jobs at the
>> case when enabling the kq and uq at the same time. Thoughts?
>>
>> Mhm, the eviction fence is always added as bookmark isn't it? As long as the GFX
>> and SDMA jobs are not for evicting something then they should only depend on
>> fences with usage < bookmark.
>>
>> Can you dig up when they are added to the dependencies of the job?
> When the eviction fence was added to the user queue VM BOs reservation and then updated the BO page table, which will add the eviction fence to the VM sync at amdgpu_sync_resv()

Stop, wait a second. That shouldn't happen at the first place. Why is the eviction fence considered a dependency for page table updates?

When it is added only as bookkeep then we should never consider that here. Looks like something in the sync obj is messed up.

Regards,
Christian.

> , and then the eviction fence will be added as a dependent fence by propagating with amdgpu_sync_push_to_job(). With removing the eviction fence from the VM sync at amdgpu_sync_resv(), then the eviction fence can be released properly.
>
> Thanks,
> Prike
>> Thanks,
>> Christian.
>>
>> PS: Please stop calling the eviction fence evf.
>>
>>>>>     return 0;
>>>>>
>>>>>  free_err:
>>>>> @@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct
>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>     dma_fence_put(stub);
>>>>>  }
>>>>>
>>>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo)
>>>> Please name that amdgpu_eviction_fence_remove_all().
>>> Noted.
>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> +{
>>>>> +   struct dma_resv *resv = &bo->tbo.base._resv;
>>>>> +   struct dma_fence *fence, *stub;
>>>>> +   struct dma_resv_iter cursor;
>>>>> +
>>>>> +   dma_resv_assert_held(resv);
>>>>> +
>>>>> +   stub = dma_fence_get_stub();
>>>>> +   dma_resv_for_each_fence(&cursor, resv,
>>>> DMA_RESV_USAGE_BOOKKEEP, fence) {
>>>>> +           struct amdgpu_eviction_fence *ev_fence;
>>>>> +
>>>>> +           ev_fence = fence_to_evf(fence);
>>>>> +           if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
>>>>> +                   continue;
>>>>> +
>>>>> +           dma_resv_replace_fences(resv, fence->context, stub,
>>>>> +                           DMA_RESV_USAGE_BOOKKEEP);
>>>>> +
>>>>> +   }
>>>>> +
>>>>> +   dma_fence_put(stub);
>>>>> +}
>>>>> +
>>>>>  int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr
>>>>> *evf_mgr)  {
>>>>>     /* This needs to be done one time per open */ diff --git
>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>> index fcd867b7147d..da99ac322a2e 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>> @@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct
>>>>> amdgpu_eviction_fence_mgr *evf_mgr,  int
>>>>> amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr
>>>> *evf_mgr,
>>>>>                                 struct drm_exec *exec);
>>>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo);
>>>>>  #endif
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> index 1e73ce30d4d7..f001018a01eb 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>> @@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct
>>>> ttm_buffer_object *bo)
>>>>>     amdgpu_vram_mgr_set_cleared(bo->resource);
>>>>>     dma_resv_add_fence(&bo->base._resv, fence,
>>>> DMA_RESV_USAGE_KERNEL);
>>>>>     dma_fence_put(fence);
>>>>> +   amdgpu_userq_remove_all_eviction_fences(abo);
>>>>>
>>>>>  out:
>>>>>     dma_resv_unlock(&bo->base._resv);

Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Yadav, Arvind]

On 4/22/2025 2:57 PM, Christian König wrote:
> Am 22.04.25 um 11:14 schrieb Liang, Prike:
>> [Public]
>>
>>> -----Original Message-----
>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>> Sent: Thursday, April 17, 2025 3:40 PM
>>> To: Liang, Prike <Prike.Liang@amd.com>; Koenig, Christian
>>> <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>>> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release
>>>
>>> Am 16.04.25 um 16:47 schrieb Liang, Prike:
>>>> [Public]
>>>>
>>>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>>>> Sent: Wednesday, April 16, 2025 7:07 PM
>>>>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>>>>> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached
>>>>> bo release
>>>>>
>>>>> Am 16.04.25 um 10:50 schrieb Prike Liang:
>>>>>> Free the evf when the attached bo released. The evf still be
>>>>>> dependent on and referred to by the attached bo that is scheduled by
>>>>>> the kernel queue SDMA or gfx after the evf signalled.
>>>>>>
>>>>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>>>>> ---
>>>>>>   .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 31 ++++++++++++++++--
>>> -
>>>>>>   .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  1 +
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
>>>>>>   3 files changed, 28 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>>>>> index b34225bbd85d..60be1ac5047d 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>>>>> @@ -27,6 +27,7 @@
>>>>>>
>>>>>>   #define work_to_evf_mgr(w, name) container_of(w, struct
>>>>>> amdgpu_eviction_fence_mgr, name)  #define evf_mgr_to_fpriv(e)
>>>>>> container_of(e, struct amdgpu_fpriv, evf_mgr)
>>>>>> +#define fence_to_evf(f)  container_of(f, struct
>>>>>> +amdgpu_eviction_fence, base)
>>>>>>
>>>>>>   static const char *
>>>>>>   amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence) @@
>>>>>> -47,7 +48,7 @@ int  amdgpu_eviction_fence_replace_fence(struct
>>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>>                                  struct drm_exec *exec)  {
>>>>>> -   struct amdgpu_eviction_fence *old_ef, *new_ef;
>>>>>> +   struct amdgpu_eviction_fence *new_ef;
>>>>>>      struct drm_gem_object *obj;
>>>>>>      unsigned long index;
>>>>>>      int ret;
>>>>>> @@ -72,7 +73,6 @@ amdgpu_eviction_fence_replace_fence(struct
>>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>>
>>>>>>      /* Update the eviction fence now */
>>>>>>      spin_lock(&evf_mgr->ev_fence_lock);
>>>>>> -   old_ef = evf_mgr->ev_fence;
>>>>>>      evf_mgr->ev_fence = new_ef;
>>>>>>      spin_unlock(&evf_mgr->ev_fence_lock);
>>>>>>
>>>>>> @@ -102,9 +102,6 @@ amdgpu_eviction_fence_replace_fence(struct
>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>>              }
>>>>>>      }
>>>>>>
>>>>>> -   /* Free old fence */
>>>>>> -   if (old_ef)
>>>>>> -           dma_fence_put(&old_ef->base);
>>>>> That change looks completely incorrect to me, you will now leak the old fence.
>>>> The eviction fence is attached and shared by all the restored validated VM BOs
>>> during UQ restore, and at this placement the eviction fence is only detached from
>>> one of the BOs. Using amdgpu_userq_remove_all_eviction_fences() will walk over
>>> the resv objects and detach the fence from the resv objs when freeing the BO.
>>>
>>> Yeah, but that doesn't justify this change here. See you're completely messing up
>>> the fence reference count with that.
>>>
>>>> But there's a problem: even though dropping all the evf attached to VM BOs with
>>> this patch, the evf still referred to by the SDMA and GFX kernel queue jobs at the
>>> case when enabling the kq and uq at the same time. Thoughts?
>>>
>>> Mhm, the eviction fence is always added as bookmark isn't it? As long as the GFX
>>> and SDMA jobs are not for evicting something then they should only depend on
>>> fences with usage < bookmark.
>>>
>>> Can you dig up when they are added to the dependencies of the job?
>> When the eviction fence was added to the user queue VM BOs reservation and then updated the BO page table, which will add the eviction fence to the VM sync at amdgpu_sync_resv()
> Stop, wait a second. That shouldn't happen at the first place. Why is the eviction fence considered a dependency for page table updates?
>
> When it is added only as bookkeep then we should never consider that here. Looks like something in the sync obj is messed up.
It is like this. Here, amdgpu_sync_resv is using DMA_RESV_USAGE_BOOKKEEP.

         int amdgpu_sync_resv() {

                 ..

                 /* TODO: Use DMA_RESV_USAGE_READ here */
                 dma_resv_for_each_fence(&cursor, resv, 
DMA_RESV_USAGE_BOOKKEEP, f) {
                     dma_fence_chain_for_each(f, f) {

                 ..

}
during PT update amdgpu_vm_bo_update() is using sync to moving 
fences(Eviction fence) before mapping anything. Because of this Eviction 
fence will act as dependency.

~arvind

> Regards,
> Christian.
>
>> , and then the eviction fence will be added as a dependent fence by propagating with amdgpu_sync_push_to_job(). With removing the eviction fence from the VM sync at amdgpu_sync_resv(), then the eviction fence can be released properly.
>>
>> Thanks,
>> Prike
>>> Thanks,
>>> Christian.
>>>
>>> PS: Please stop calling the eviction fence evf.
>>>
>>>>>>      return 0;
>>>>>>
>>>>>>   free_err:
>>>>>> @@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct
>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>>      dma_fence_put(stub);
>>>>>>   }
>>>>>>
>>>>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo)
>>>>> Please name that amdgpu_eviction_fence_remove_all().
>>>> Noted.
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> +{
>>>>>> +   struct dma_resv *resv = &bo->tbo.base._resv;
>>>>>> +   struct dma_fence *fence, *stub;
>>>>>> +   struct dma_resv_iter cursor;
>>>>>> +
>>>>>> +   dma_resv_assert_held(resv);
>>>>>> +
>>>>>> +   stub = dma_fence_get_stub();
>>>>>> +   dma_resv_for_each_fence(&cursor, resv,
>>>>> DMA_RESV_USAGE_BOOKKEEP, fence) {
>>>>>> +           struct amdgpu_eviction_fence *ev_fence;
>>>>>> +
>>>>>> +           ev_fence = fence_to_evf(fence);
>>>>>> +           if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
>>>>>> +                   continue;
>>>>>> +
>>>>>> +           dma_resv_replace_fences(resv, fence->context, stub,
>>>>>> +                           DMA_RESV_USAGE_BOOKKEEP);
>>>>>> +
>>>>>> +   }
>>>>>> +
>>>>>> +   dma_fence_put(stub);
>>>>>> +}
>>>>>> +
>>>>>>   int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr
>>>>>> *evf_mgr)  {
>>>>>>      /* This needs to be done one time per open */ diff --git
>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>>> index fcd867b7147d..da99ac322a2e 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>>> @@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct
>>>>>> amdgpu_eviction_fence_mgr *evf_mgr,  int
>>>>>> amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr
>>>>> *evf_mgr,
>>>>>>                                  struct drm_exec *exec);
>>>>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo *bo);
>>>>>>   #endif
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> index 1e73ce30d4d7..f001018a01eb 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> @@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct
>>>>> ttm_buffer_object *bo)
>>>>>>      amdgpu_vram_mgr_set_cleared(bo->resource);
>>>>>>      dma_resv_add_fence(&bo->base._resv, fence,
>>>>> DMA_RESV_USAGE_KERNEL);
>>>>>>      dma_fence_put(fence);
>>>>>> +   amdgpu_userq_remove_all_eviction_fences(abo);
>>>>>>
>>>>>>   out:
>>>>>>      dma_resv_unlock(&bo->base._resv);

RE: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Liang, Prike]

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

[AMD Official Use Only - AMD Internal Distribution Only]

> From: Yadav, Arvind <Arvind.Yadav@amd.com>
> Sent: Tuesday, April 22, 2025 8:40 PM
> To: Koenig, Christian <Christian.Koenig@amd.com>; Liang, Prike
> <Prike.Liang@amd.com>
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; amd-
> gfx@lists.freedesktop.org; Christian König <ckoenig.leichtzumerken@gmail.com>
> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release
>
>
> On 4/22/2025 2:57 PM, Christian König wrote:
> > Am 22.04.25 um 11:14 schrieb Liang, Prike:
> >> [Public]
> >>
> >>> -----Original Message-----
> >>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> >>> Sent: Thursday, April 17, 2025 3:40 PM
> >>> To: Liang, Prike <Prike.Liang@amd.com>; Koenig, Christian
> >>> <Christian.Koenig@amd.com>; amd-gfx@lists.freedesktop.org
> >>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> >>> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached
> >>> bo release
> >>>
> >>> Am 16.04.25 um 16:47 schrieb Liang, Prike:
> >>>> [Public]
> >>>>
> >>>>> From: Koenig, Christian <Christian.Koenig@amd.com>
> >>>>> Sent: Wednesday, April 16, 2025 7:07 PM
> >>>>> To: Liang, Prike <Prike.Liang@amd.com>;
> >>>>> amd-gfx@lists.freedesktop.org
> >>>>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> >>>>> Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the
> >>>>> attached bo release
> >>>>>
> >>>>> Am 16.04.25 um 10:50 schrieb Prike Liang:
> >>>>>> Free the evf when the attached bo released. The evf still be
> >>>>>> dependent on and referred to by the attached bo that is scheduled
> >>>>>> by the kernel queue SDMA or gfx after the evf signalled.
> >>>>>>
> >>>>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> >>>>>> ---
> >>>>>>   .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 31
> ++++++++++++++++--
> >>> -
> >>>>>>   .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  1 +
> >>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
> >>>>>>   3 files changed, 28 insertions(+), 5 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> >>>>>> index b34225bbd85d..60be1ac5047d 100644
> >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> >>>>>> @@ -27,6 +27,7 @@
> >>>>>>
> >>>>>>   #define work_to_evf_mgr(w, name) container_of(w, struct
> >>>>>> amdgpu_eviction_fence_mgr, name)  #define evf_mgr_to_fpriv(e)
> >>>>>> container_of(e, struct amdgpu_fpriv, evf_mgr)
> >>>>>> +#define fence_to_evf(f)  container_of(f, struct
> >>>>>> +amdgpu_eviction_fence, base)
> >>>>>>
> >>>>>>   static const char *
> >>>>>>   amdgpu_eviction_fence_get_driver_name(struct dma_fence *fence)
> >>>>>> @@
> >>>>>> -47,7 +48,7 @@ int  amdgpu_eviction_fence_replace_fence(struct
> >>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
> >>>>>>                                  struct drm_exec *exec)  {
> >>>>>> -   struct amdgpu_eviction_fence *old_ef, *new_ef;
> >>>>>> +   struct amdgpu_eviction_fence *new_ef;
> >>>>>>      struct drm_gem_object *obj;
> >>>>>>      unsigned long index;
> >>>>>>      int ret;
> >>>>>> @@ -72,7 +73,6 @@ amdgpu_eviction_fence_replace_fence(struct
> >>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
> >>>>>>
> >>>>>>      /* Update the eviction fence now */
> >>>>>>      spin_lock(&evf_mgr->ev_fence_lock);
> >>>>>> -   old_ef = evf_mgr->ev_fence;
> >>>>>>      evf_mgr->ev_fence = new_ef;
> >>>>>>      spin_unlock(&evf_mgr->ev_fence_lock);
> >>>>>>
> >>>>>> @@ -102,9 +102,6 @@ amdgpu_eviction_fence_replace_fence(struct
> >>>>> amdgpu_eviction_fence_mgr *evf_mgr,
> >>>>>>              }
> >>>>>>      }
> >>>>>>
> >>>>>> -   /* Free old fence */
> >>>>>> -   if (old_ef)
> >>>>>> -           dma_fence_put(&old_ef->base);
> >>>>> That change looks completely incorrect to me, you will now leak the old
> fence.
> >>>> The eviction fence is attached and shared by all the restored
> >>>> validated VM BOs
> >>> during UQ restore, and at this placement the eviction fence is only
> >>> detached from one of the BOs. Using
> >>> amdgpu_userq_remove_all_eviction_fences() will walk over the resv objects
> and detach the fence from the resv objs when freeing the BO.
> >>>
> >>> Yeah, but that doesn't justify this change here. See you're
> >>> completely messing up the fence reference count with that.
> >>>
> >>>> But there's a problem: even though dropping all the evf attached to
> >>>> VM BOs with
> >>> this patch, the evf still referred to by the SDMA and GFX kernel
> >>> queue jobs at the case when enabling the kq and uq at the same time.
> Thoughts?
> >>>
> >>> Mhm, the eviction fence is always added as bookmark isn't it? As
> >>> long as the GFX and SDMA jobs are not for evicting something then
> >>> they should only depend on fences with usage < bookmark.
> >>>
> >>> Can you dig up when they are added to the dependencies of the job?
> >> When the eviction fence was added to the user queue VM BOs
> >> reservation and then updated the BO page table, which will add the
> >> eviction fence to the VM sync at amdgpu_sync_resv()
> > Stop, wait a second. That shouldn't happen at the first place. Why is the eviction
> fence considered a dependency for page table updates?
> >
> > When it is added only as bookkeep then we should never consider that here.
> Looks like something in the sync obj is messed up.
> It is like this. Here, amdgpu_sync_resv is using
> DMA_RESV_USAGE_BOOKKEEP.
>
>          int amdgpu_sync_resv() {
>
>                  ..
>
>                  /* TODO: Use DMA_RESV_USAGE_READ here */
>                  dma_resv_for_each_fence(&cursor, resv,
> DMA_RESV_USAGE_BOOKKEEP, f) {
>                      dma_fence_chain_for_each(f, f) {
>
>                  ..
>
> }
> during PT update amdgpu_vm_bo_update() is using sync to moving
> fences(Eviction fence) before mapping anything. Because of this Eviction fence will
> act as dependency.

Yes, since the amdgpu_sync_resv() uses the bookkeep usage, then all the BOs reservation fences along with the eviction fence will be returned and added to the sync.
And with the attached patch, the eviction fence can be released properly when the kq and uq are enabled.

Thanks,
Prike

> ~arvind
>
> > Regards,
> > Christian.
> >
> >> , and then the eviction fence will be added as a dependent fence by
> propagating with amdgpu_sync_push_to_job(). With removing the eviction fence
> from the VM sync at amdgpu_sync_resv(), then the eviction fence can be released
> properly.
> >>
> >> Thanks,
> >> Prike
> >>> Thanks,
> >>> Christian.
> >>>
> >>> PS: Please stop calling the eviction fence evf.
> >>>
> >>>>>>      return 0;
> >>>>>>
> >>>>>>   free_err:
> >>>>>> @@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct
> >>>>> amdgpu_eviction_fence_mgr *evf_mgr,
> >>>>>>      dma_fence_put(stub);
> >>>>>>   }
> >>>>>>
> >>>>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo
> >>>>>> +*bo)
> >>>>> Please name that amdgpu_eviction_fence_remove_all().
> >>>> Noted.
> >>>>
> >>>>> Regards,
> >>>>> Christian.
> >>>>>
> >>>>>> +{
> >>>>>> +   struct dma_resv *resv = &bo->tbo.base._resv;
> >>>>>> +   struct dma_fence *fence, *stub;
> >>>>>> +   struct dma_resv_iter cursor;
> >>>>>> +
> >>>>>> +   dma_resv_assert_held(resv);
> >>>>>> +
> >>>>>> +   stub = dma_fence_get_stub();
> >>>>>> +   dma_resv_for_each_fence(&cursor, resv,
> >>>>> DMA_RESV_USAGE_BOOKKEEP, fence) {
> >>>>>> +           struct amdgpu_eviction_fence *ev_fence;
> >>>>>> +
> >>>>>> +           ev_fence = fence_to_evf(fence);
> >>>>>> +           if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
> >>>>>> +                   continue;
> >>>>>> +
> >>>>>> +           dma_resv_replace_fences(resv, fence->context, stub,
> >>>>>> +                           DMA_RESV_USAGE_BOOKKEEP);
> >>>>>> +
> >>>>>> +   }
> >>>>>> +
> >>>>>> +   dma_fence_put(stub);
> >>>>>> +}
> >>>>>> +
> >>>>>>   int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr
> >>>>>> *evf_mgr)  {
> >>>>>>      /* This needs to be done one time per open */ diff --git
> >>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> >>>>>> index fcd867b7147d..da99ac322a2e 100644
> >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
> >>>>>> @@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct
> >>>>>> amdgpu_eviction_fence_mgr *evf_mgr,  int
> >>>>>> amdgpu_eviction_fence_replace_fence(struct
> >>>>>> amdgpu_eviction_fence_mgr
> >>>>> *evf_mgr,
> >>>>>>                                  struct drm_exec *exec);
> >>>>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo
> >>>>>> +*bo);
> >>>>>>   #endif
> >>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>>>>> index 1e73ce30d4d7..f001018a01eb 100644
> >>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>>>>> @@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct
> >>>>> ttm_buffer_object *bo)
> >>>>>>      amdgpu_vram_mgr_set_cleared(bo->resource);
> >>>>>>      dma_resv_add_fence(&bo->base._resv, fence,
> >>>>> DMA_RESV_USAGE_KERNEL);
> >>>>>>      dma_fence_put(fence);
> >>>>>> +   amdgpu_userq_remove_all_eviction_fences(abo);
> >>>>>>
> >>>>>>   out:
> >>>>>>      dma_resv_unlock(&bo->base._resv);

[-- Attachment #2: 0003-drm-amdgpu-free-the-evf-when-the-attached-bo-release.patch --]
[-- Type: application/octet-stream, Size: 6072 bytes --]

From 0ddf4223e8b029ce5c17dd8ef23bf07d36a7ced3 Mon Sep 17 00:00:00 2001
From: Prike Liang <Prike.Liang@amd.com>
Date: Tue, 15 Apr 2025 10:42:19 +0800
Subject: [PATCH 3/3] drm/amdgpu: free the evf when the attached bo release

Free the evf when the attached bo released. The evf still
be dependent on and referred to by the attached bo that is
scheduled by the kernel queue SDMA or gfx even signal the
eviction fence.

Signed-off-by: Prike Liang <Prike.Liang@amd.com>
---
 .../drm/amd/amdgpu/amdgpu_eviction_fence.c    | 53 +++++++++++++++----
 .../drm/amd/amdgpu/amdgpu_eviction_fence.h    |  2 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c    |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c      |  7 +++
 4 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
index 610f447ede9b..fc8e509a15f7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
@@ -70,11 +70,6 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 		return -ENOMEM;
 	}
 
-	/* Update the eviction fence now */
-	spin_lock(&evf_mgr->ev_fence_lock);
-	old_ef = evf_mgr->ev_fence;
-	evf_mgr->ev_fence = new_ef;
-	spin_unlock(&evf_mgr->ev_fence_lock);
 
 	/* Attach the new fence */
 	drm_exec_for_each_locked_object(exec, index, obj) {
@@ -82,6 +77,16 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 
 		if (!bo)
 			continue;
+
+		/*detach the old eviction fence first*/
+		amdgpu_eviction_fence_detach(evf_mgr, bo);
+
+		/* Update the eviction fence now */
+		spin_lock(&evf_mgr->ev_fence_lock);
+		old_ef = evf_mgr->ev_fence;
+		evf_mgr->ev_fence = new_ef;
+		spin_unlock(&evf_mgr->ev_fence_lock);
+
 		ret = amdgpu_eviction_fence_attach(evf_mgr, bo);
 		if (ret) {
 			DRM_ERROR("Failed to attch new eviction fence\n");
@@ -89,9 +94,10 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 		}
 	}
 
-	/* Free old fence */
+	/* Free the init eviction fence which was referred by the dma_init*/
 	if (old_ef)
 		dma_fence_put(&old_ef->base);
+
 	return 0;
 
 free_err:
@@ -189,7 +195,6 @@ void amdgpu_eviction_fence_destroy(struct amdgpu_eviction_fence_mgr *evf_mgr)
 int amdgpu_eviction_fence_attach(struct amdgpu_eviction_fence_mgr *evf_mgr,
 				 struct amdgpu_bo *bo)
 {
-	struct dma_fence *ef;
 	struct amdgpu_eviction_fence *ev_fence;
 	struct dma_resv *resv = bo->tbo.base.resv;
 	int ret;
@@ -205,10 +210,12 @@ int amdgpu_eviction_fence_attach(struct amdgpu_eviction_fence_mgr *evf_mgr,
 
 	spin_lock(&evf_mgr->ev_fence_lock);
 	ev_fence = evf_mgr->ev_fence;
-	if (ev_fence) {
-		ef = dma_fence_get(&ev_fence->base);
-		dma_resv_add_fence(resv, ef, DMA_RESV_USAGE_BOOKKEEP);
-	}
+	/*
+	 * The dma_resv_add_fence() already refer to the added fence, hence in this
+	 * placement needn't refer to the armed fence anymore.
+	 */
+	if (ev_fence)
+		dma_resv_add_fence(resv, &ev_fence->base, DMA_RESV_USAGE_BOOKKEEP);
 	spin_unlock(&evf_mgr->ev_fence_lock);
 
 	return 0;
@@ -224,6 +231,30 @@ void amdgpu_eviction_fence_detach(struct amdgpu_eviction_fence_mgr *evf_mgr,
 	dma_fence_put(stub);
 }
 
+void amdgpu_remove_all_eviction_fences(struct amdgpu_bo *bo)
+{
+	struct dma_resv *resv = &bo->tbo.base._resv;
+	struct dma_fence *fence, *stub;
+	struct dma_resv_iter cursor;
+
+	dma_resv_assert_held(resv);
+
+	stub = dma_fence_get_stub();
+	dma_resv_for_each_fence(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP, fence) {
+		struct amdgpu_eviction_fence *ev_fence;
+
+		ev_fence = fence_to_evf(fence);
+		if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
+			continue;
+
+		dma_resv_replace_fences(resv, fence->context, stub,
+				DMA_RESV_USAGE_BOOKKEEP);
+
+	}
+
+	dma_fence_put(stub);
+}
+
 int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr *evf_mgr)
 {
 	/* This needs to be done one time per open */
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
index fcd867b7147d..7e6c55a334e0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
@@ -42,6 +42,7 @@ struct amdgpu_eviction_fence_mgr {
 };
 
 /* Eviction fence helper functions */
+#define fence_to_evf(f)  container_of(f, struct amdgpu_eviction_fence, base)
 struct amdgpu_eviction_fence *
 amdgpu_eviction_fence_create(struct amdgpu_eviction_fence_mgr *evf_mgr);
 
@@ -66,4 +67,5 @@ amdgpu_eviction_fence_signal(struct amdgpu_eviction_fence_mgr *evf_mgr,
 int
 amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
 				    struct drm_exec *exec);
+void amdgpu_remove_all_eviction_fences(struct amdgpu_bo *bo);
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 1e73ce30d4d7..4be75a7cb950 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct ttm_buffer_object *bo)
 	amdgpu_vram_mgr_set_cleared(bo->resource);
 	dma_resv_add_fence(&bo->base._resv, fence, DMA_RESV_USAGE_KERNEL);
 	dma_fence_put(fence);
+	amdgpu_remove_all_eviction_fences(abo);
 
 out:
 	dma_resv_unlock(&bo->base._resv);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index 5576ed0b508f..9aa8cf0984dc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -256,6 +256,13 @@ 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)) {
+				struct amdgpu_eviction_fence *ev_fence;
+
+				ev_fence = fence_to_evf(f);
+				/*remove the eviction fence from the kernel queue sync point*/
+				if (ev_fence)
+					continue;
+
 				r = amdgpu_sync_fence(sync, f, GFP_KERNEL);
 				dma_fence_put(f);
 				if (r)
-- 
2.34.1

Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Christian König]

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

Am 22.04.25 um 15:09 schrieb Liang, Prike:
>>> Stop, wait a second. That shouldn't happen at the first place. Why is the eviction
>> fence considered a dependency for page table updates?
>>> When it is added only as bookkeep then we should never consider that here.
>> Looks like something in the sync obj is messed up.
>> It is like this. Here, amdgpu_sync_resv is using
>> DMA_RESV_USAGE_BOOKKEEP.
>>
>>          int amdgpu_sync_resv() {
>>
>>                  ..
>>
>>                  /* TODO: Use DMA_RESV_USAGE_READ here */

That here is the core of the problem.

I've added this TODO item 4 years ago to switch over to DMA_RESV_USAGE_READ here when moved all TTM use cases to using drm_sched_job_add_resv_dependencies().

That was done, but this TODO here forgotten.

>>                  dma_resv_for_each_fence(&cursor, resv,
>> DMA_RESV_USAGE_BOOKKEEP, f) {
>>                      dma_fence_chain_for_each(f, f) {
>>
>>                  ..
>>
>> }
>> during PT update amdgpu_vm_bo_update() is using sync to moving
>> fences(Eviction fence) before mapping anything. Because of this Eviction fence will
>> act as dependency.
> Yes, since the amdgpu_sync_resv() uses the bookkeep usage, then all the BOs reservation fences along with the eviction fence will be returned and added to the sync.

Yeah, but that is incorrect.

> And with the attached patch, the eviction fence can be released properly when the kq and uq are enabled.

We need to fix the underlying bug first before we can work on the next step.

Regards,
Christian.

>
> Thanks,
> Prike
>
>> ~arvind
>>
>>> Regards,
>>> Christian.
>>>
>>>> , and then the eviction fence will be added as a dependent fence by
>> propagating with amdgpu_sync_push_to_job(). With removing the eviction fence
>> from the VM sync at amdgpu_sync_resv(), then the eviction fence can be released
>> properly.
>>>> Thanks,
>>>> Prike
>>>>> Thanks,
>>>>> Christian.
>>>>>
>>>>> PS: Please stop calling the eviction fence evf.
>>>>>
>>>>>>>>      return 0;
>>>>>>>>
>>>>>>>>   free_err:
>>>>>>>> @@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct
>>>>>>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>>>>>>      dma_fence_put(stub);
>>>>>>>>   }
>>>>>>>>
>>>>>>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo
>>>>>>>> +*bo)
>>>>>>> Please name that amdgpu_eviction_fence_remove_all().
>>>>>> Noted.
>>>>>>
>>>>>>> Regards,
>>>>>>> Christian.
>>>>>>>
>>>>>>>> +{
>>>>>>>> +   struct dma_resv *resv = &bo->tbo.base._resv;
>>>>>>>> +   struct dma_fence *fence, *stub;
>>>>>>>> +   struct dma_resv_iter cursor;
>>>>>>>> +
>>>>>>>> +   dma_resv_assert_held(resv);
>>>>>>>> +
>>>>>>>> +   stub = dma_fence_get_stub();
>>>>>>>> +   dma_resv_for_each_fence(&cursor, resv,
>>>>>>> DMA_RESV_USAGE_BOOKKEEP, fence) {
>>>>>>>> +           struct amdgpu_eviction_fence *ev_fence;
>>>>>>>> +
>>>>>>>> +           ev_fence = fence_to_evf(fence);
>>>>>>>> +           if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))
>>>>>>>> +                   continue;
>>>>>>>> +
>>>>>>>> +           dma_resv_replace_fences(resv, fence->context, stub,
>>>>>>>> +                           DMA_RESV_USAGE_BOOKKEEP);
>>>>>>>> +
>>>>>>>> +   }
>>>>>>>> +
>>>>>>>> +   dma_fence_put(stub);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>>   int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr
>>>>>>>> *evf_mgr)  {
>>>>>>>>      /* This needs to be done one time per open */ diff --git
>>>>>>>> a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>>>>> index fcd867b7147d..da99ac322a2e 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h
>>>>>>>> @@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct
>>>>>>>> amdgpu_eviction_fence_mgr *evf_mgr,  int
>>>>>>>> amdgpu_eviction_fence_replace_fence(struct
>>>>>>>> amdgpu_eviction_fence_mgr
>>>>>>> *evf_mgr,
>>>>>>>>                                  struct drm_exec *exec);
>>>>>>>> +void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo
>>>>>>>> +*bo);
>>>>>>>>   #endif
>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>>> index 1e73ce30d4d7..f001018a01eb 100644
>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>>> @@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct
>>>>>>> ttm_buffer_object *bo)
>>>>>>>>      amdgpu_vram_mgr_set_cleared(bo->resource);
>>>>>>>>      dma_resv_add_fence(&bo->base._resv, fence,
>>>>>>> DMA_RESV_USAGE_KERNEL);
>>>>>>>>      dma_fence_put(fence);
>>>>>>>> +   amdgpu_userq_remove_all_eviction_fences(abo);
>>>>>>>>
>>>>>>>>   out:
>>>>>>>>      dma_resv_unlock(&bo->base._resv);

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

RE: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

[Liang, Prike]

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

[Public]

From: Koenig, Christian <Christian.Koenig@amd.com>
Sent: Tuesday, April 22, 2025 9:26 PM
To: Liang, Prike <Prike.Liang@amd.com>; Yadav, Arvind <Arvind.Yadav@amd.com>
Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org; Christian König <ckoenig.leichtzumerken@gmail.com>
Subject: Re: [PATCH 4/4] drm/amdgpu: free the evf when the attached bo release

Am 22.04.25 um 15:09 schrieb Liang, Prike:


Stop, wait a second. That shouldn't happen at the first place. Why is the eviction

fence considered a dependency for page table updates?



When it is added only as bookkeep then we should never consider that here.

Looks like something in the sync obj is messed up.

It is like this. Here, amdgpu_sync_resv is using

DMA_RESV_USAGE_BOOKKEEP.



         int amdgpu_sync_resv() {



                 ..



                 /* TODO: Use DMA_RESV_USAGE_READ here */

That here is the core of the problem.

I've added this TODO item 4 years ago to switch over to DMA_RESV_USAGE_READ here when moved all TTM use cases to using drm_sched_job_add_resv_dependencies().

That was done, but this TODO here forgotten.

So here the user space submission sync only requires syncing the fences with less than DMA_RESV_USAGE_READ usage in the amdgpu_sync_resv(). If so, then the eviction fence will not be added to the sync with kernel queue submission. I can submit a patch for this change.

Thanks,
Prike



                 dma_resv_for_each_fence(&cursor, resv,

DMA_RESV_USAGE_BOOKKEEP, f) {

                     dma_fence_chain_for_each(f, f) {



                 ..



}

during PT update amdgpu_vm_bo_update() is using sync to moving

fences(Eviction fence) before mapping anything. Because of this Eviction fence will

act as dependency.



Yes, since the amdgpu_sync_resv() uses the bookkeep usage, then all the BOs reservation fences along with the eviction fence will be returned and added to the sync.

Yeah, but that is incorrect.



And with the attached patch, the eviction fence can be released properly when the kq and uq are enabled.

We need to fix the underlying bug first before we can work on the next step.

Regards,
Christian.







Thanks,

Prike



~arvind



Regards,

Christian.



, and then the eviction fence will be added as a dependent fence by

propagating with amdgpu_sync_push_to_job(). With removing the eviction fence

from the VM sync at amdgpu_sync_resv(), then the eviction fence can be released

properly.



Thanks,

Prike

Thanks,

Christian.



PS: Please stop calling the eviction fence evf.



     return 0;



  free_err:

@@ -237,6 +234,30 @@ void amdgpu_eviction_fence_detach(struct

amdgpu_eviction_fence_mgr *evf_mgr,

     dma_fence_put(stub);

  }



+void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo

+*bo)

Please name that amdgpu_eviction_fence_remove_all().

Noted.



Regards,

Christian.



+{

+   struct dma_resv *resv = &bo->tbo.base._resv;

+   struct dma_fence *fence, *stub;

+   struct dma_resv_iter cursor;

+

+   dma_resv_assert_held(resv);

+

+   stub = dma_fence_get_stub();

+   dma_resv_for_each_fence(&cursor, resv,

DMA_RESV_USAGE_BOOKKEEP, fence) {

+           struct amdgpu_eviction_fence *ev_fence;

+

+           ev_fence = fence_to_evf(fence);

+           if (!ev_fence || !dma_fence_is_signaled(&ev_fence->base))

+                   continue;

+

+           dma_resv_replace_fences(resv, fence->context, stub,

+                           DMA_RESV_USAGE_BOOKKEEP);

+

+   }

+

+   dma_fence_put(stub);

+}

+

  int amdgpu_eviction_fence_init(struct amdgpu_eviction_fence_mgr

*evf_mgr)  {

     /* This needs to be done one time per open */ diff --git

a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h

b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h

index fcd867b7147d..da99ac322a2e 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.h

@@ -66,4 +66,5 @@ amdgpu_eviction_fence_signal(struct

amdgpu_eviction_fence_mgr *evf_mgr,  int

amdgpu_eviction_fence_replace_fence(struct

amdgpu_eviction_fence_mgr

*evf_mgr,

                                 struct drm_exec *exec);

+void amdgpu_userq_remove_all_eviction_fences(struct amdgpu_bo

+*bo);

  #endif

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c

b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c

index 1e73ce30d4d7..f001018a01eb 100644

--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c

+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c

@@ -1392,6 +1392,7 @@ void amdgpu_bo_release_notify(struct

ttm_buffer_object *bo)

     amdgpu_vram_mgr_set_cleared(bo->resource);

     dma_resv_add_fence(&bo->base._resv, fence,

DMA_RESV_USAGE_KERNEL);

     dma_fence_put(fence);

+   amdgpu_userq_remove_all_eviction_fences(abo);



  out:

     dma_resv_unlock(&bo->base._resv);


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

Re: [PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump

[Christian König]

Am 16.04.25 um 10:50 schrieb Prike Liang:
> This debug dump will help on debugging the evf attached gem obj fence
> related issue.

That looks like overkill to me and will just massively spam the debug log.

Christian.

>
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 13 +++++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c         |  6 +++++-
>  2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> index 0075469550b0..7030d721196b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> @@ -86,6 +86,19 @@ amdgpu_eviction_fence_replace_fence(struct amdgpu_eviction_fence_mgr *evf_mgr,
>  		if (ret) {
>  			DRM_ERROR("Failed to attch new eviction fence\n");
>  			goto free_err;
> +		} else {
> +			struct dma_resv_iter cursor;
> +			struct dma_fence *fence;
> +			static const char *usage[] = { "kernel", "write", "read", "bookkeep" };
> +
> +			dma_resv_for_each_fence(&cursor, obj->resv, DMA_RESV_USAGE_READ, fence) {
> +				DRM_DEBUG("after attach evf the resv dump usage:%s\n"
> +					   "after attach evf name:%s timeline name:%s seq:%lld %ssingned\n",
> +					usage[dma_resv_iter_usage(&cursor)],
> +					fence->ops->get_driver_name(fence),
> +					fence->ops->get_timeline_name(fence), fence->seqno,
> +					dma_fence_is_signaled(fence) ? "" : "un");
> +			}
>  		}
>  	}
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index d09db052e282..1e73ce30d4d7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1675,7 +1675,11 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo *bo, struct seq_file *m)
>  	amdgpu_bo_print_flag(m, bo, VRAM_CONTIGUOUS);
>  	amdgpu_bo_print_flag(m, bo, VM_ALWAYS_VALID);
>  	amdgpu_bo_print_flag(m, bo, EXPLICIT_SYNC);
> -
> +	/* Add the gem obj resv fence dump*/
> +	if (dma_resv_trylock(bo->tbo.base.resv)) {
> +		dma_resv_describe(bo->tbo.base.resv, m);
> +		dma_resv_unlock(bo->tbo.base.resv);
> +	}
>  	seq_puts(m, "\n");
>  
>  	return size;

RE: [PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump

[Liang, Prike]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, April 16, 2025 7:01 PM
> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
> Subject: Re: [PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump
>
> Am 16.04.25 um 10:50 schrieb Prike Liang:
> > This debug dump will help on debugging the evf attached gem obj fence
> > related issue.
>
> That looks like overkill to me and will just massively spam the debug log.
>
> Christian.
>
How about putting the evf attached resv obj dump in a trace point?

Thanks,
Prike

> > Signed-off-by: Prike Liang <Prike.Liang@amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 13 +++++++++++++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c         |  6 +++++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > index 0075469550b0..7030d721196b 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
> > @@ -86,6 +86,19 @@ amdgpu_eviction_fence_replace_fence(struct
> amdgpu_eviction_fence_mgr *evf_mgr,
> >             if (ret) {
> >                     DRM_ERROR("Failed to attch new eviction fence\n");
> >                     goto free_err;
> > +           } else {
> > +                   struct dma_resv_iter cursor;
> > +                   struct dma_fence *fence;
> > +                   static const char *usage[] = { "kernel", "write", "read",
> > +"bookkeep" };
> > +
> > +                   dma_resv_for_each_fence(&cursor, obj->resv,
> DMA_RESV_USAGE_READ, fence) {
> > +                           DRM_DEBUG("after attach evf the resv dump
> usage:%s\n"
> > +                                      "after attach evf name:%s timeline
> name:%s seq:%lld %ssingned\n",
> > +                                   usage[dma_resv_iter_usage(&cursor)],
> > +                                   fence->ops->get_driver_name(fence),
> > +                                   fence->ops->get_timeline_name(fence),
> fence->seqno,
> > +                                   dma_fence_is_signaled(fence) ? "" : "un");
> > +                   }
> >             }
> >     }
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index d09db052e282..1e73ce30d4d7 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -1675,7 +1675,11 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo
> *bo, struct seq_file *m)
> >     amdgpu_bo_print_flag(m, bo, VRAM_CONTIGUOUS);
> >     amdgpu_bo_print_flag(m, bo, VM_ALWAYS_VALID);
> >     amdgpu_bo_print_flag(m, bo, EXPLICIT_SYNC);
> > -
> > +   /* Add the gem obj resv fence dump*/
> > +   if (dma_resv_trylock(bo->tbo.base.resv)) {
> > +           dma_resv_describe(bo->tbo.base.resv, m);
> > +           dma_resv_unlock(bo->tbo.base.resv);
> > +   }
> >     seq_puts(m, "\n");
> >
> >     return size;

Re: [PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump

[Christian König]

Am 16.04.25 um 14:54 schrieb Liang, Prike:
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, April 16, 2025 7:01 PM
>> To: Liang, Prike <Prike.Liang@amd.com>; amd-gfx@lists.freedesktop.org
>> Cc: Deucher, Alexander <Alexander.Deucher@amd.com>
>> Subject: Re: [PATCH 1/4] drm/amdgpu: add the evf attached gem obj resv dump
>>
>> Am 16.04.25 um 10:50 schrieb Prike Liang:
>>> This debug dump will help on debugging the evf attached gem obj fence
>>> related issue.
>> That looks like overkill to me and will just massively spam the debug log.
>>
>> Christian.
>>
> How about putting the evf attached resv obj dump in a trace point?

Well that's better, but I still don't see the value in it for the eviction fence handling.

See the dma_resv object is just a container for fences. it's completely irrelevant for the eviction fence what other fences are in the resv object.

On the other hand adding the dma_resv_describe() to amdgpu_bo_print_info() is probably quite nice to have and perfectly valid.

Thanks,
Christian.

>
> Thanks,
> Prike
>
>>> Signed-off-by: Prike Liang <Prike.Liang@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c | 13 +++++++++++++
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c         |  6 +++++-
>>>  2 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> index 0075469550b0..7030d721196b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_eviction_fence.c
>>> @@ -86,6 +86,19 @@ amdgpu_eviction_fence_replace_fence(struct
>> amdgpu_eviction_fence_mgr *evf_mgr,
>>>             if (ret) {
>>>                     DRM_ERROR("Failed to attch new eviction fence\n");
>>>                     goto free_err;
>>> +           } else {
>>> +                   struct dma_resv_iter cursor;
>>> +                   struct dma_fence *fence;
>>> +                   static const char *usage[] = { "kernel", "write", "read",
>>> +"bookkeep" };
>>> +
>>> +                   dma_resv_for_each_fence(&cursor, obj->resv,
>> DMA_RESV_USAGE_READ, fence) {
>>> +                           DRM_DEBUG("after attach evf the resv dump
>> usage:%s\n"
>>> +                                      "after attach evf name:%s timeline
>> name:%s seq:%lld %ssingned\n",
>>> +                                   usage[dma_resv_iter_usage(&cursor)],
>>> +                                   fence->ops->get_driver_name(fence),
>>> +                                   fence->ops->get_timeline_name(fence),
>> fence->seqno,
>>> +                                   dma_fence_is_signaled(fence) ? "" : "un");
>>> +                   }
>>>             }
>>>     }
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> index d09db052e282..1e73ce30d4d7 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>> @@ -1675,7 +1675,11 @@ u64 amdgpu_bo_print_info(int id, struct amdgpu_bo
>> *bo, struct seq_file *m)
>>>     amdgpu_bo_print_flag(m, bo, VRAM_CONTIGUOUS);
>>>     amdgpu_bo_print_flag(m, bo, VM_ALWAYS_VALID);
>>>     amdgpu_bo_print_flag(m, bo, EXPLICIT_SYNC);
>>> -
>>> +   /* Add the gem obj resv fence dump*/
>>> +   if (dma_resv_trylock(bo->tbo.base.resv)) {
>>> +           dma_resv_describe(bo->tbo.base.resv, m);
>>> +           dma_resv_unlock(bo->tbo.base.resv);
>>> +   }
>>>     seq_puts(m, "\n");
>>>
>>>     return size;