* [PATCH 1/3] drm/amdgpu: re-work VM syncing
@ 2024-08-21 12:03 Christian König
2024-08-21 12:03 ` [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs Christian König
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Christian König @ 2024-08-21 12:03 UTC (permalink / raw)
To: friedrich.vock, bas, ishitatsuyuki, felix.kuehling; +Cc: amd-gfx
Rework how VM operations synchronize to submissions. Provide an
amdgpu_sync container to the backends instead of an reservation
object and fill in the amdgpu_sync object in the higher layers
of the code.
No intended functional change, just prepares for upcomming changes.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 84 +++++++++++++--------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 11 +--
drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c | 7 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 +---
5 files changed, 65 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index bcb729094521..ba99d428610a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -838,7 +838,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
params.vm = vm;
params.immediate = immediate;
- r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT);
+ r = vm->update_funcs->prepare(¶ms, NULL);
if (r)
goto error;
@@ -933,7 +933,7 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
* @unlocked: unlocked invalidation during MM callback
* @flush_tlb: trigger tlb invalidation after update completed
* @allow_override: change MTYPE for local NUMA nodes
- * @resv: fences we need to sync to
+ * @sync: fences we need to sync to
* @start: start of mapped range
* @last: last mapped entry
* @flags: flags for the entries
@@ -949,16 +949,16 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
* 0 for success, negative erro code for failure.
*/
int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
- bool immediate, bool unlocked, bool flush_tlb, bool allow_override,
- struct dma_resv *resv, uint64_t start, uint64_t last,
- uint64_t flags, uint64_t offset, uint64_t vram_base,
+ bool immediate, bool unlocked, bool flush_tlb,
+ bool allow_override, struct amdgpu_sync *sync,
+ uint64_t start, uint64_t last, uint64_t flags,
+ uint64_t offset, uint64_t vram_base,
struct ttm_resource *res, dma_addr_t *pages_addr,
struct dma_fence **fence)
{
struct amdgpu_vm_tlb_seq_struct *tlb_cb;
struct amdgpu_vm_update_params params;
struct amdgpu_res_cursor cursor;
- enum amdgpu_sync_mode sync_mode;
int r, idx;
if (!drm_dev_enter(adev_to_drm(adev), &idx))
@@ -991,14 +991,6 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
params.allow_override = allow_override;
INIT_LIST_HEAD(¶ms.tlb_flush_waitlist);
- /* Implicitly sync to command submissions in the same VM before
- * unmapping. Sync to moving fences before mapping.
- */
- if (!(flags & AMDGPU_PTE_VALID))
- sync_mode = AMDGPU_SYNC_EQ_OWNER;
- else
- sync_mode = AMDGPU_SYNC_EXPLICIT;
-
amdgpu_vm_eviction_lock(vm);
if (vm->evicting) {
r = -EBUSY;
@@ -1013,7 +1005,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
dma_fence_put(tmp);
}
- r = vm->update_funcs->prepare(¶ms, resv, sync_mode);
+ r = vm->update_funcs->prepare(¶ms, sync);
if (r)
goto error_free;
@@ -1155,23 +1147,30 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
struct amdgpu_bo *bo = bo_va->base.bo;
struct amdgpu_vm *vm = bo_va->base.vm;
struct amdgpu_bo_va_mapping *mapping;
+ struct dma_fence **last_update;
dma_addr_t *pages_addr = NULL;
struct ttm_resource *mem;
- struct dma_fence **last_update;
+ struct amdgpu_sync sync;
bool flush_tlb = clear;
- bool uncached;
- struct dma_resv *resv;
uint64_t vram_base;
uint64_t flags;
+ bool uncached;
int r;
+ amdgpu_sync_create(&sync);
if (clear || !bo) {
mem = NULL;
- resv = vm->root.bo->tbo.base.resv;
+
+ /* Implicitly sync to command submissions in the same VM before
+ * unmapping.
+ */
+ r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
+ AMDGPU_SYNC_EQ_OWNER, vm);
+ if (r)
+ goto error_free;
} else {
struct drm_gem_object *obj = &bo->tbo.base;
- resv = bo->tbo.base.resv;
if (obj->import_attach && bo_va->is_xgmi) {
struct dma_buf *dma_buf = obj->import_attach->dmabuf;
struct drm_gem_object *gobj = dma_buf->priv;
@@ -1185,6 +1184,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
if (mem && (mem->mem_type == TTM_PL_TT ||
mem->mem_type == AMDGPU_PL_PREEMPT))
pages_addr = bo->tbo.ttm->dma_address;
+
+ /* Implicitly sync to moving fences before mapping anything */
+ r = amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv,
+ AMDGPU_SYNC_EXPLICIT, vm);
+ if (r)
+ goto error_free;
}
if (bo) {
@@ -1234,12 +1239,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
trace_amdgpu_vm_bo_update(mapping);
r = amdgpu_vm_update_range(adev, vm, false, false, flush_tlb,
- !uncached, resv, mapping->start, mapping->last,
- update_flags, mapping->offset,
- vram_base, mem, pages_addr,
- last_update);
+ !uncached, &sync, mapping->start,
+ mapping->last, update_flags,
+ mapping->offset, vram_base, mem,
+ pages_addr, last_update);
if (r)
- return r;
+ goto error_free;
}
/* If the BO is not in its preferred location add it back to
@@ -1267,7 +1272,9 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
trace_amdgpu_vm_bo_mapping(mapping);
}
- return 0;
+error_free:
+ amdgpu_sync_free(&sync);
+ return r;
}
/**
@@ -1414,25 +1421,34 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
struct amdgpu_vm *vm,
struct dma_fence **fence)
{
- struct dma_resv *resv = vm->root.bo->tbo.base.resv;
struct amdgpu_bo_va_mapping *mapping;
- uint64_t init_pte_value = 0;
struct dma_fence *f = NULL;
+ struct amdgpu_sync sync;
int r;
+
+ /*
+ * Implicitly sync to command submissions in the same VM before
+ * unmapping.
+ */
+ amdgpu_sync_create(&sync);
+ r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
+ AMDGPU_SYNC_EQ_OWNER, vm);
+ if (r)
+ goto error_free;
+
while (!list_empty(&vm->freed)) {
mapping = list_first_entry(&vm->freed,
struct amdgpu_bo_va_mapping, list);
list_del(&mapping->list);
r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
- resv, mapping->start, mapping->last,
- init_pte_value, 0, 0, NULL, NULL,
- &f);
+ &sync, mapping->start, mapping->last,
+ 0, 0, 0, NULL, NULL, &f);
amdgpu_vm_free_mapping(adev, vm, mapping, f);
if (r) {
dma_fence_put(f);
- return r;
+ goto error_free;
}
}
@@ -1443,7 +1459,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
dma_fence_put(f);
}
- return 0;
+error_free:
+ amdgpu_sync_free(&sync);
+ return r;
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 046949c4b695..1a759012ce93 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -304,8 +304,8 @@ struct amdgpu_vm_update_params {
struct amdgpu_vm_update_funcs {
int (*map_table)(struct amdgpu_bo_vm *bo);
- int (*prepare)(struct amdgpu_vm_update_params *p, struct dma_resv *resv,
- enum amdgpu_sync_mode sync_mode);
+ int (*prepare)(struct amdgpu_vm_update_params *p,
+ struct amdgpu_sync *sync);
int (*update)(struct amdgpu_vm_update_params *p,
struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t addr,
unsigned count, uint32_t incr, uint64_t flags);
@@ -505,9 +505,10 @@ int amdgpu_vm_flush_compute_tlb(struct amdgpu_device *adev,
void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
struct amdgpu_vm *vm, struct amdgpu_bo *bo);
int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
- bool immediate, bool unlocked, bool flush_tlb, bool allow_override,
- struct dma_resv *resv, uint64_t start, uint64_t last,
- uint64_t flags, uint64_t offset, uint64_t vram_base,
+ bool immediate, bool unlocked, bool flush_tlb,
+ bool allow_override, struct amdgpu_sync *sync,
+ uint64_t start, uint64_t last, uint64_t flags,
+ uint64_t offset, uint64_t vram_base,
struct ttm_resource *res, dma_addr_t *pages_addr,
struct dma_fence **fence);
int amdgpu_vm_bo_update(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
index 3895bd7d176a..9ff59a4e6f15 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
@@ -46,13 +46,12 @@ static int amdgpu_vm_cpu_map_table(struct amdgpu_bo_vm *table)
* Negativ errno, 0 for success.
*/
static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
- struct dma_resv *resv,
- enum amdgpu_sync_mode sync_mode)
+ struct amdgpu_sync *sync)
{
- if (!resv)
+ if (!sync)
return 0;
- return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, p->vm, true);
+ return amdgpu_sync_wait(sync, true);
}
/**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index e39d6e7643bf..a076f43097e4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -403,7 +403,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
params.vm = vm;
params.immediate = immediate;
- r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT);
+ r = vm->update_funcs->prepare(¶ms, NULL);
if (r)
goto exit;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
index 9b748d7058b5..4772fba33285 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
@@ -77,32 +77,24 @@ static int amdgpu_vm_sdma_alloc_job(struct amdgpu_vm_update_params *p,
* amdgpu_vm_sdma_prepare - prepare SDMA command submission
*
* @p: see amdgpu_vm_update_params definition
- * @resv: reservation object with embedded fence
- * @sync_mode: synchronization mode
+ * @sync: amdgpu_sync object with fences to wait for
*
* Returns:
* Negativ errno, 0 for success.
*/
static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
- struct dma_resv *resv,
- enum amdgpu_sync_mode sync_mode)
+ struct amdgpu_sync *sync)
{
- struct amdgpu_sync sync;
int r;
r = amdgpu_vm_sdma_alloc_job(p, 0);
if (r)
return r;
- if (!resv)
+ if (!sync)
return 0;
- amdgpu_sync_create(&sync);
- r = amdgpu_sync_resv(p->adev, &sync, resv, sync_mode, p->vm);
- if (!r)
- r = amdgpu_sync_push_to_job(&sync, p->job);
- amdgpu_sync_free(&sync);
-
+ r = amdgpu_sync_push_to_job(sync, p->job);
if (r) {
p->num_dw_left = 0;
amdgpu_job_free(p->job);
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs
2024-08-21 12:03 [PATCH 1/3] drm/amdgpu: re-work VM syncing Christian König
@ 2024-08-21 12:03 ` Christian König
2024-08-21 20:01 ` Felix Kuehling
2024-08-21 12:03 ` [PATCH 3/3] drm/amdgpu: stop syncing PRT map operations Christian König
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2024-08-21 12:03 UTC (permalink / raw)
To: friedrich.vock, bas, ishitatsuyuki, felix.kuehling; +Cc: amd-gfx
This patch tries to solve the basic problem we also need to sync to
the KFD fences of the BO because otherwise it can be that we clear
PTEs while the KFD queues are still running.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 30 ++++++++++++++++++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++++
3 files changed, 37 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
index bdf1ef825d89..c586ab4c911b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
@@ -260,6 +260,36 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
return 0;
}
+/**
+ * amdgpu_sync_kfd - sync to KFD fences
+ *
+ * @sync: sync object to add KFD fences to
+ * @resv: reservation object with KFD fences
+ *
+ * Extract all KFD fences and add them to the sync object.
+ */
+int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv)
+{
+ struct dma_resv_iter cursor;
+ struct dma_fence *f;
+ int r = 0;
+
+ dma_resv_iter_begin(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP);
+ dma_resv_for_each_fence_unlocked(&cursor, f) {
+ void *fence_owner = amdgpu_sync_get_owner(f);
+
+ if (fence_owner != AMDGPU_FENCE_OWNER_KFD)
+ continue;
+
+ r = amdgpu_sync_fence(sync, f);
+ if (r)
+ break;
+ }
+ dma_resv_iter_end(&cursor);
+
+ return r;
+}
+
/* Free the entry back to the slab */
static void amdgpu_sync_entry_free(struct amdgpu_sync_entry *e)
{
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
index cf1e9e858efd..e3272dce798d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
@@ -51,6 +51,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f);
int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
struct dma_resv *resv, enum amdgpu_sync_mode mode,
void *owner);
+int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv);
struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
struct amdgpu_ring *ring);
struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index ba99d428610a..13d429b91327 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1168,6 +1168,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
AMDGPU_SYNC_EQ_OWNER, vm);
if (r)
goto error_free;
+ if (bo) {
+ r = amdgpu_sync_kfd(&sync, bo->tbo.base.resv);
+ if (r)
+ goto error_free;
+ }
+
} else {
struct drm_gem_object *obj = &bo->tbo.base;
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/3] drm/amdgpu: stop syncing PRT map operations
2024-08-21 12:03 [PATCH 1/3] drm/amdgpu: re-work VM syncing Christian König
2024-08-21 12:03 ` [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs Christian König
@ 2024-08-21 12:03 ` Christian König
2024-08-21 17:39 ` [PATCH 1/3] drm/amdgpu: re-work VM syncing Friedrich Vock
2024-08-21 20:46 ` Felix Kuehling
3 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2024-08-21 12:03 UTC (permalink / raw)
To: friedrich.vock, bas, ishitatsuyuki, felix.kuehling; +Cc: amd-gfx
Requested by both Bas and Friedrich. Mapping PTEs as PRT doesn't need to
sync for anything.
Signed-off-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 13d429b91327..a42e172b7dbc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1158,7 +1158,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
int r;
amdgpu_sync_create(&sync);
- if (clear || !bo) {
+ if (clear) {
mem = NULL;
/* Implicitly sync to command submissions in the same VM before
@@ -1173,6 +1173,10 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
if (r)
goto error_free;
}
+ } else if (!bo) {
+ mem = NULL;
+
+ /* PRT map operations don't need to sync to anything. */
} else {
struct drm_gem_object *obj = &bo->tbo.base;
--
2.34.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] drm/amdgpu: re-work VM syncing
2024-08-21 12:03 [PATCH 1/3] drm/amdgpu: re-work VM syncing Christian König
2024-08-21 12:03 ` [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs Christian König
2024-08-21 12:03 ` [PATCH 3/3] drm/amdgpu: stop syncing PRT map operations Christian König
@ 2024-08-21 17:39 ` Friedrich Vock
2024-08-21 20:46 ` Felix Kuehling
3 siblings, 0 replies; 11+ messages in thread
From: Friedrich Vock @ 2024-08-21 17:39 UTC (permalink / raw)
To: Christian König, bas, ishitatsuyuki, felix.kuehling; +Cc: amd-gfx
On 21.08.24 14:03, Christian König wrote:
> Rework how VM operations synchronize to submissions. Provide an
> amdgpu_sync container to the backends instead of an reservation
> object and fill in the amdgpu_sync object in the higher layers
> of the code.
>
> No intended functional change, just prepares for upcomming changes.
This looks like a really nice cleanup! I'm not sure how much it's worth
given that I'm not a maintainer, but this one and patch 3 are
Reviewed-by: Friedrich Vock <friedrich.vock@gmx.de>
Patch 2 looks ok to me as well, but I'm not in the loop as to what
problem this fixes. If the problem already occurs in the wild, it might
make sense to consider backporting?
Regards,
Friedrich
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 84 +++++++++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 11 +--
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c | 7 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 +---
> 5 files changed, 65 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index bcb729094521..ba99d428610a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -838,7 +838,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
> params.vm = vm;
> params.immediate = immediate;
>
> - r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT);
> + r = vm->update_funcs->prepare(¶ms, NULL);
> if (r)
> goto error;
>
> @@ -933,7 +933,7 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
> * @unlocked: unlocked invalidation during MM callback
> * @flush_tlb: trigger tlb invalidation after update completed
> * @allow_override: change MTYPE for local NUMA nodes
> - * @resv: fences we need to sync to
> + * @sync: fences we need to sync to
> * @start: start of mapped range
> * @last: last mapped entry
> * @flags: flags for the entries
> @@ -949,16 +949,16 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
> * 0 for success, negative erro code for failure.
> */
> int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> - bool immediate, bool unlocked, bool flush_tlb, bool allow_override,
> - struct dma_resv *resv, uint64_t start, uint64_t last,
> - uint64_t flags, uint64_t offset, uint64_t vram_base,
> + bool immediate, bool unlocked, bool flush_tlb,
> + bool allow_override, struct amdgpu_sync *sync,
> + uint64_t start, uint64_t last, uint64_t flags,
> + uint64_t offset, uint64_t vram_base,
> struct ttm_resource *res, dma_addr_t *pages_addr,
> struct dma_fence **fence)
> {
> struct amdgpu_vm_tlb_seq_struct *tlb_cb;
> struct amdgpu_vm_update_params params;
> struct amdgpu_res_cursor cursor;
> - enum amdgpu_sync_mode sync_mode;
> int r, idx;
>
> if (!drm_dev_enter(adev_to_drm(adev), &idx))
> @@ -991,14 +991,6 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> params.allow_override = allow_override;
> INIT_LIST_HEAD(¶ms.tlb_flush_waitlist);
>
> - /* Implicitly sync to command submissions in the same VM before
> - * unmapping. Sync to moving fences before mapping.
> - */
> - if (!(flags & AMDGPU_PTE_VALID))
> - sync_mode = AMDGPU_SYNC_EQ_OWNER;
> - else
> - sync_mode = AMDGPU_SYNC_EXPLICIT;
> -
> amdgpu_vm_eviction_lock(vm);
> if (vm->evicting) {
> r = -EBUSY;
> @@ -1013,7 +1005,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> dma_fence_put(tmp);
> }
>
> - r = vm->update_funcs->prepare(¶ms, resv, sync_mode);
> + r = vm->update_funcs->prepare(¶ms, sync);
> if (r)
> goto error_free;
>
> @@ -1155,23 +1147,30 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> struct amdgpu_bo *bo = bo_va->base.bo;
> struct amdgpu_vm *vm = bo_va->base.vm;
> struct amdgpu_bo_va_mapping *mapping;
> + struct dma_fence **last_update;
> dma_addr_t *pages_addr = NULL;
> struct ttm_resource *mem;
> - struct dma_fence **last_update;
> + struct amdgpu_sync sync;
> bool flush_tlb = clear;
> - bool uncached;
> - struct dma_resv *resv;
> uint64_t vram_base;
> uint64_t flags;
> + bool uncached;
> int r;
>
> + amdgpu_sync_create(&sync);
> if (clear || !bo) {
> mem = NULL;
> - resv = vm->root.bo->tbo.base.resv;
> +
> + /* Implicitly sync to command submissions in the same VM before
> + * unmapping.
> + */
> + r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
> + AMDGPU_SYNC_EQ_OWNER, vm);
> + if (r)
> + goto error_free;
> } else {
> struct drm_gem_object *obj = &bo->tbo.base;
>
> - resv = bo->tbo.base.resv;
> if (obj->import_attach && bo_va->is_xgmi) {
> struct dma_buf *dma_buf = obj->import_attach->dmabuf;
> struct drm_gem_object *gobj = dma_buf->priv;
> @@ -1185,6 +1184,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> if (mem && (mem->mem_type == TTM_PL_TT ||
> mem->mem_type == AMDGPU_PL_PREEMPT))
> pages_addr = bo->tbo.ttm->dma_address;
> +
> + /* Implicitly sync to moving fences before mapping anything */
> + r = amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv,
> + AMDGPU_SYNC_EXPLICIT, vm);
> + if (r)
> + goto error_free;
> }
>
> if (bo) {
> @@ -1234,12 +1239,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> trace_amdgpu_vm_bo_update(mapping);
>
> r = amdgpu_vm_update_range(adev, vm, false, false, flush_tlb,
> - !uncached, resv, mapping->start, mapping->last,
> - update_flags, mapping->offset,
> - vram_base, mem, pages_addr,
> - last_update);
> + !uncached, &sync, mapping->start,
> + mapping->last, update_flags,
> + mapping->offset, vram_base, mem,
> + pages_addr, last_update);
> if (r)
> - return r;
> + goto error_free;
> }
>
> /* If the BO is not in its preferred location add it back to
> @@ -1267,7 +1272,9 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> trace_amdgpu_vm_bo_mapping(mapping);
> }
>
> - return 0;
> +error_free:
> + amdgpu_sync_free(&sync);
> + return r;
> }
>
> /**
> @@ -1414,25 +1421,34 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> struct dma_fence **fence)
> {
> - struct dma_resv *resv = vm->root.bo->tbo.base.resv;
> struct amdgpu_bo_va_mapping *mapping;
> - uint64_t init_pte_value = 0;
> struct dma_fence *f = NULL;
> + struct amdgpu_sync sync;
> int r;
>
> +
> + /*
> + * Implicitly sync to command submissions in the same VM before
> + * unmapping.
> + */
> + amdgpu_sync_create(&sync);
> + r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
> + AMDGPU_SYNC_EQ_OWNER, vm);
> + if (r)
> + goto error_free;
> +
> while (!list_empty(&vm->freed)) {
> mapping = list_first_entry(&vm->freed,
> struct amdgpu_bo_va_mapping, list);
> list_del(&mapping->list);
>
> r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
> - resv, mapping->start, mapping->last,
> - init_pte_value, 0, 0, NULL, NULL,
> - &f);
> + &sync, mapping->start, mapping->last,
> + 0, 0, 0, NULL, NULL, &f);
> amdgpu_vm_free_mapping(adev, vm, mapping, f);
> if (r) {
> dma_fence_put(f);
> - return r;
> + goto error_free;
> }
> }
>
> @@ -1443,7 +1459,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> dma_fence_put(f);
> }
>
> - return 0;
> +error_free:
> + amdgpu_sync_free(&sync);
> + return r;
>
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 046949c4b695..1a759012ce93 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -304,8 +304,8 @@ struct amdgpu_vm_update_params {
>
> struct amdgpu_vm_update_funcs {
> int (*map_table)(struct amdgpu_bo_vm *bo);
> - int (*prepare)(struct amdgpu_vm_update_params *p, struct dma_resv *resv,
> - enum amdgpu_sync_mode sync_mode);
> + int (*prepare)(struct amdgpu_vm_update_params *p,
> + struct amdgpu_sync *sync);
> int (*update)(struct amdgpu_vm_update_params *p,
> struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t addr,
> unsigned count, uint32_t incr, uint64_t flags);
> @@ -505,9 +505,10 @@ int amdgpu_vm_flush_compute_tlb(struct amdgpu_device *adev,
> void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
> struct amdgpu_vm *vm, struct amdgpu_bo *bo);
> int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> - bool immediate, bool unlocked, bool flush_tlb, bool allow_override,
> - struct dma_resv *resv, uint64_t start, uint64_t last,
> - uint64_t flags, uint64_t offset, uint64_t vram_base,
> + bool immediate, bool unlocked, bool flush_tlb,
> + bool allow_override, struct amdgpu_sync *sync,
> + uint64_t start, uint64_t last, uint64_t flags,
> + uint64_t offset, uint64_t vram_base,
> struct ttm_resource *res, dma_addr_t *pages_addr,
> struct dma_fence **fence);
> int amdgpu_vm_bo_update(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> index 3895bd7d176a..9ff59a4e6f15 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> @@ -46,13 +46,12 @@ static int amdgpu_vm_cpu_map_table(struct amdgpu_bo_vm *table)
> * Negativ errno, 0 for success.
> */
> static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
> - struct dma_resv *resv,
> - enum amdgpu_sync_mode sync_mode)
> + struct amdgpu_sync *sync)
> {
> - if (!resv)
> + if (!sync)
> return 0;
>
> - return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, p->vm, true);
> + return amdgpu_sync_wait(sync, true);
> }
>
> /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index e39d6e7643bf..a076f43097e4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -403,7 +403,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> params.vm = vm;
> params.immediate = immediate;
>
> - r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT);
> + r = vm->update_funcs->prepare(¶ms, NULL);
> if (r)
> goto exit;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 9b748d7058b5..4772fba33285 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -77,32 +77,24 @@ static int amdgpu_vm_sdma_alloc_job(struct amdgpu_vm_update_params *p,
> * amdgpu_vm_sdma_prepare - prepare SDMA command submission
> *
> * @p: see amdgpu_vm_update_params definition
> - * @resv: reservation object with embedded fence
> - * @sync_mode: synchronization mode
> + * @sync: amdgpu_sync object with fences to wait for
> *
> * Returns:
> * Negativ errno, 0 for success.
> */
> static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
> - struct dma_resv *resv,
> - enum amdgpu_sync_mode sync_mode)
> + struct amdgpu_sync *sync)
> {
> - struct amdgpu_sync sync;
> int r;
>
> r = amdgpu_vm_sdma_alloc_job(p, 0);
> if (r)
> return r;
>
> - if (!resv)
> + if (!sync)
> return 0;
>
> - amdgpu_sync_create(&sync);
> - r = amdgpu_sync_resv(p->adev, &sync, resv, sync_mode, p->vm);
> - if (!r)
> - r = amdgpu_sync_push_to_job(&sync, p->job);
> - amdgpu_sync_free(&sync);
> -
> + r = amdgpu_sync_push_to_job(sync, p->job);
> if (r) {
> p->num_dw_left = 0;
> amdgpu_job_free(p->job);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs
2024-08-21 12:03 ` [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs Christian König
@ 2024-08-21 20:01 ` Felix Kuehling
2024-08-22 9:07 ` Christian König
0 siblings, 1 reply; 11+ messages in thread
From: Felix Kuehling @ 2024-08-21 20:01 UTC (permalink / raw)
To: Christian König, friedrich.vock, bas, ishitatsuyuki; +Cc: amd-gfx
On 2024-08-21 08:03, Christian König wrote:
> This patch tries to solve the basic problem we also need to sync to
> the KFD fences of the BO because otherwise it can be that we clear
> PTEs while the KFD queues are still running.
This is going to trigger a lot of phantom KFD evictions and will tank
performance. It's probably not what you intended.
Regards,
Felix
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 30 ++++++++++++++++++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h | 1 +
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++++
> 3 files changed, 37 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> index bdf1ef825d89..c586ab4c911b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
> @@ -260,6 +260,36 @@ int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
> return 0;
> }
>
> +/**
> + * amdgpu_sync_kfd - sync to KFD fences
> + *
> + * @sync: sync object to add KFD fences to
> + * @resv: reservation object with KFD fences
> + *
> + * Extract all KFD fences and add them to the sync object.
> + */
> +int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv)
> +{
> + struct dma_resv_iter cursor;
> + struct dma_fence *f;
> + int r = 0;
> +
> + dma_resv_iter_begin(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP);
> + dma_resv_for_each_fence_unlocked(&cursor, f) {
> + void *fence_owner = amdgpu_sync_get_owner(f);
> +
> + if (fence_owner != AMDGPU_FENCE_OWNER_KFD)
> + continue;
> +
> + r = amdgpu_sync_fence(sync, f);
> + if (r)
> + break;
> + }
> + dma_resv_iter_end(&cursor);
> +
> + return r;
> +}
> +
> /* Free the entry back to the slab */
> static void amdgpu_sync_entry_free(struct amdgpu_sync_entry *e)
> {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> index cf1e9e858efd..e3272dce798d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
> @@ -51,6 +51,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync, struct dma_fence *f);
> int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync *sync,
> struct dma_resv *resv, enum amdgpu_sync_mode mode,
> void *owner);
> +int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv);
> struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
> struct amdgpu_ring *ring);
> struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index ba99d428610a..13d429b91327 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1168,6 +1168,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> AMDGPU_SYNC_EQ_OWNER, vm);
> if (r)
> goto error_free;
> + if (bo) {
> + r = amdgpu_sync_kfd(&sync, bo->tbo.base.resv);
> + if (r)
> + goto error_free;
> + }
> +
> } else {
> struct drm_gem_object *obj = &bo->tbo.base;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] drm/amdgpu: re-work VM syncing
2024-08-21 12:03 [PATCH 1/3] drm/amdgpu: re-work VM syncing Christian König
` (2 preceding siblings ...)
2024-08-21 17:39 ` [PATCH 1/3] drm/amdgpu: re-work VM syncing Friedrich Vock
@ 2024-08-21 20:46 ` Felix Kuehling
2024-08-22 7:28 ` Friedrich Vock
3 siblings, 1 reply; 11+ messages in thread
From: Felix Kuehling @ 2024-08-21 20:46 UTC (permalink / raw)
To: Christian König, friedrich.vock, bas, ishitatsuyuki; +Cc: amd-gfx
On 2024-08-21 08:03, Christian König wrote:
> Rework how VM operations synchronize to submissions. Provide an
> amdgpu_sync container to the backends instead of an reservation
> object and fill in the amdgpu_sync object in the higher layers
> of the code.
>
> No intended functional change, just prepares for upcomming changes.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 84 +++++++++++++--------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 11 +--
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c | 7 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 +---
There are two calls to amdgpu_vm_update_range in amdkfd/kfd_svm.c that
would need to be updated as well.
Regards,
Felix
> 5 files changed, 65 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index bcb729094521..ba99d428610a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -838,7 +838,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
> params.vm = vm;
> params.immediate = immediate;
>
> - r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT);
> + r = vm->update_funcs->prepare(¶ms, NULL);
> if (r)
> goto error;
>
> @@ -933,7 +933,7 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
> * @unlocked: unlocked invalidation during MM callback
> * @flush_tlb: trigger tlb invalidation after update completed
> * @allow_override: change MTYPE for local NUMA nodes
> - * @resv: fences we need to sync to
> + * @sync: fences we need to sync to
> * @start: start of mapped range
> * @last: last mapped entry
> * @flags: flags for the entries
> @@ -949,16 +949,16 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
> * 0 for success, negative erro code for failure.
> */
> int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> - bool immediate, bool unlocked, bool flush_tlb, bool allow_override,
> - struct dma_resv *resv, uint64_t start, uint64_t last,
> - uint64_t flags, uint64_t offset, uint64_t vram_base,
> + bool immediate, bool unlocked, bool flush_tlb,
> + bool allow_override, struct amdgpu_sync *sync,
> + uint64_t start, uint64_t last, uint64_t flags,
> + uint64_t offset, uint64_t vram_base,
> struct ttm_resource *res, dma_addr_t *pages_addr,
> struct dma_fence **fence)
> {
> struct amdgpu_vm_tlb_seq_struct *tlb_cb;
> struct amdgpu_vm_update_params params;
> struct amdgpu_res_cursor cursor;
> - enum amdgpu_sync_mode sync_mode;
> int r, idx;
>
> if (!drm_dev_enter(adev_to_drm(adev), &idx))
> @@ -991,14 +991,6 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> params.allow_override = allow_override;
> INIT_LIST_HEAD(¶ms.tlb_flush_waitlist);
>
> - /* Implicitly sync to command submissions in the same VM before
> - * unmapping. Sync to moving fences before mapping.
> - */
> - if (!(flags & AMDGPU_PTE_VALID))
> - sync_mode = AMDGPU_SYNC_EQ_OWNER;
> - else
> - sync_mode = AMDGPU_SYNC_EXPLICIT;
> -
> amdgpu_vm_eviction_lock(vm);
> if (vm->evicting) {
> r = -EBUSY;
> @@ -1013,7 +1005,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> dma_fence_put(tmp);
> }
>
> - r = vm->update_funcs->prepare(¶ms, resv, sync_mode);
> + r = vm->update_funcs->prepare(¶ms, sync);
> if (r)
> goto error_free;
>
> @@ -1155,23 +1147,30 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> struct amdgpu_bo *bo = bo_va->base.bo;
> struct amdgpu_vm *vm = bo_va->base.vm;
> struct amdgpu_bo_va_mapping *mapping;
> + struct dma_fence **last_update;
> dma_addr_t *pages_addr = NULL;
> struct ttm_resource *mem;
> - struct dma_fence **last_update;
> + struct amdgpu_sync sync;
> bool flush_tlb = clear;
> - bool uncached;
> - struct dma_resv *resv;
> uint64_t vram_base;
> uint64_t flags;
> + bool uncached;
> int r;
>
> + amdgpu_sync_create(&sync);
> if (clear || !bo) {
> mem = NULL;
> - resv = vm->root.bo->tbo.base.resv;
> +
> + /* Implicitly sync to command submissions in the same VM before
> + * unmapping.
> + */
> + r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
> + AMDGPU_SYNC_EQ_OWNER, vm);
> + if (r)
> + goto error_free;
> } else {
> struct drm_gem_object *obj = &bo->tbo.base;
>
> - resv = bo->tbo.base.resv;
> if (obj->import_attach && bo_va->is_xgmi) {
> struct dma_buf *dma_buf = obj->import_attach->dmabuf;
> struct drm_gem_object *gobj = dma_buf->priv;
> @@ -1185,6 +1184,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> if (mem && (mem->mem_type == TTM_PL_TT ||
> mem->mem_type == AMDGPU_PL_PREEMPT))
> pages_addr = bo->tbo.ttm->dma_address;
> +
> + /* Implicitly sync to moving fences before mapping anything */
> + r = amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv,
> + AMDGPU_SYNC_EXPLICIT, vm);
> + if (r)
> + goto error_free;
> }
>
> if (bo) {
> @@ -1234,12 +1239,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> trace_amdgpu_vm_bo_update(mapping);
>
> r = amdgpu_vm_update_range(adev, vm, false, false, flush_tlb,
> - !uncached, resv, mapping->start, mapping->last,
> - update_flags, mapping->offset,
> - vram_base, mem, pages_addr,
> - last_update);
> + !uncached, &sync, mapping->start,
> + mapping->last, update_flags,
> + mapping->offset, vram_base, mem,
> + pages_addr, last_update);
> if (r)
> - return r;
> + goto error_free;
> }
>
> /* If the BO is not in its preferred location add it back to
> @@ -1267,7 +1272,9 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va,
> trace_amdgpu_vm_bo_mapping(mapping);
> }
>
> - return 0;
> +error_free:
> + amdgpu_sync_free(&sync);
> + return r;
> }
>
> /**
> @@ -1414,25 +1421,34 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> struct dma_fence **fence)
> {
> - struct dma_resv *resv = vm->root.bo->tbo.base.resv;
> struct amdgpu_bo_va_mapping *mapping;
> - uint64_t init_pte_value = 0;
> struct dma_fence *f = NULL;
> + struct amdgpu_sync sync;
> int r;
>
> +
> + /*
> + * Implicitly sync to command submissions in the same VM before
> + * unmapping.
> + */
> + amdgpu_sync_create(&sync);
> + r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
> + AMDGPU_SYNC_EQ_OWNER, vm);
> + if (r)
> + goto error_free;
> +
> while (!list_empty(&vm->freed)) {
> mapping = list_first_entry(&vm->freed,
> struct amdgpu_bo_va_mapping, list);
> list_del(&mapping->list);
>
> r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
> - resv, mapping->start, mapping->last,
> - init_pte_value, 0, 0, NULL, NULL,
> - &f);
> + &sync, mapping->start, mapping->last,
> + 0, 0, 0, NULL, NULL, &f);
> amdgpu_vm_free_mapping(adev, vm, mapping, f);
> if (r) {
> dma_fence_put(f);
> - return r;
> + goto error_free;
> }
> }
>
> @@ -1443,7 +1459,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
> dma_fence_put(f);
> }
>
> - return 0;
> +error_free:
> + amdgpu_sync_free(&sync);
> + return r;
>
> }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 046949c4b695..1a759012ce93 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -304,8 +304,8 @@ struct amdgpu_vm_update_params {
>
> struct amdgpu_vm_update_funcs {
> int (*map_table)(struct amdgpu_bo_vm *bo);
> - int (*prepare)(struct amdgpu_vm_update_params *p, struct dma_resv *resv,
> - enum amdgpu_sync_mode sync_mode);
> + int (*prepare)(struct amdgpu_vm_update_params *p,
> + struct amdgpu_sync *sync);
> int (*update)(struct amdgpu_vm_update_params *p,
> struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t addr,
> unsigned count, uint32_t incr, uint64_t flags);
> @@ -505,9 +505,10 @@ int amdgpu_vm_flush_compute_tlb(struct amdgpu_device *adev,
> void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
> struct amdgpu_vm *vm, struct amdgpu_bo *bo);
> int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> - bool immediate, bool unlocked, bool flush_tlb, bool allow_override,
> - struct dma_resv *resv, uint64_t start, uint64_t last,
> - uint64_t flags, uint64_t offset, uint64_t vram_base,
> + bool immediate, bool unlocked, bool flush_tlb,
> + bool allow_override, struct amdgpu_sync *sync,
> + uint64_t start, uint64_t last, uint64_t flags,
> + uint64_t offset, uint64_t vram_base,
> struct ttm_resource *res, dma_addr_t *pages_addr,
> struct dma_fence **fence);
> int amdgpu_vm_bo_update(struct amdgpu_device *adev,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> index 3895bd7d176a..9ff59a4e6f15 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> @@ -46,13 +46,12 @@ static int amdgpu_vm_cpu_map_table(struct amdgpu_bo_vm *table)
> * Negativ errno, 0 for success.
> */
> static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
> - struct dma_resv *resv,
> - enum amdgpu_sync_mode sync_mode)
> + struct amdgpu_sync *sync)
> {
> - if (!resv)
> + if (!sync)
> return 0;
>
> - return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, p->vm, true);
> + return amdgpu_sync_wait(sync, true);
> }
>
> /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index e39d6e7643bf..a076f43097e4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -403,7 +403,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> params.vm = vm;
> params.immediate = immediate;
>
> - r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT);
> + r = vm->update_funcs->prepare(¶ms, NULL);
> if (r)
> goto exit;
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> index 9b748d7058b5..4772fba33285 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> @@ -77,32 +77,24 @@ static int amdgpu_vm_sdma_alloc_job(struct amdgpu_vm_update_params *p,
> * amdgpu_vm_sdma_prepare - prepare SDMA command submission
> *
> * @p: see amdgpu_vm_update_params definition
> - * @resv: reservation object with embedded fence
> - * @sync_mode: synchronization mode
> + * @sync: amdgpu_sync object with fences to wait for
> *
> * Returns:
> * Negativ errno, 0 for success.
> */
> static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
> - struct dma_resv *resv,
> - enum amdgpu_sync_mode sync_mode)
> + struct amdgpu_sync *sync)
> {
> - struct amdgpu_sync sync;
> int r;
>
> r = amdgpu_vm_sdma_alloc_job(p, 0);
> if (r)
> return r;
>
> - if (!resv)
> + if (!sync)
> return 0;
>
> - amdgpu_sync_create(&sync);
> - r = amdgpu_sync_resv(p->adev, &sync, resv, sync_mode, p->vm);
> - if (!r)
> - r = amdgpu_sync_push_to_job(&sync, p->job);
> - amdgpu_sync_free(&sync);
> -
> + r = amdgpu_sync_push_to_job(sync, p->job);
> if (r) {
> p->num_dw_left = 0;
> amdgpu_job_free(p->job);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] drm/amdgpu: re-work VM syncing
2024-08-21 20:46 ` Felix Kuehling
@ 2024-08-22 7:28 ` Friedrich Vock
2024-08-28 22:29 ` Felix Kuehling
0 siblings, 1 reply; 11+ messages in thread
From: Friedrich Vock @ 2024-08-22 7:28 UTC (permalink / raw)
To: Felix Kuehling, Christian König, bas, ishitatsuyuki; +Cc: amd-gfx
On 21.08.24 22:46, Felix Kuehling wrote:
>
> On 2024-08-21 08:03, Christian König wrote:
>> Rework how VM operations synchronize to submissions. Provide an
>> amdgpu_sync container to the backends instead of an reservation
>> object and fill in the amdgpu_sync object in the higher layers
>> of the code.
>>
>> No intended functional change, just prepares for upcomming changes.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 84 +++++++++++++--------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 11 +--
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c | 7 +-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 +---
>
> There are two calls to amdgpu_vm_update_range in amdkfd/kfd_svm.c that
> would need to be updated as well.
I don't think any change should be needed there? Both calls pass NULL
for the resv. All this patch changes is that we're now passing NULL for
the amdgpu_sync - but the behavior with a NULL amdgpu_sync with this
patch is the same as with a NULL dma_resv without this patch, so nothing
needs to change.
Regards,
Friedrich
>
> Regards,
> Felix
>
>
>> 5 files changed, 65 insertions(+), 55 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index bcb729094521..ba99d428610a 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -838,7 +838,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
>> params.vm = vm;
>> params.immediate = immediate;
>> - r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT);
>> + r = vm->update_funcs->prepare(¶ms, NULL);
>> if (r)
>> goto error;
>> @@ -933,7 +933,7 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params
>> *params,
>> * @unlocked: unlocked invalidation during MM callback
>> * @flush_tlb: trigger tlb invalidation after update completed
>> * @allow_override: change MTYPE for local NUMA nodes
>> - * @resv: fences we need to sync to
>> + * @sync: fences we need to sync to
>> * @start: start of mapped range
>> * @last: last mapped entry
>> * @flags: flags for the entries
>> @@ -949,16 +949,16 @@ amdgpu_vm_tlb_flush(struct
>> amdgpu_vm_update_params *params,
>> * 0 for success, negative erro code for failure.
>> */
>> int amdgpu_vm_update_range(struct amdgpu_device *adev, struct
>> amdgpu_vm *vm,
>> - bool immediate, bool unlocked, bool flush_tlb, bool
>> allow_override,
>> - struct dma_resv *resv, uint64_t start, uint64_t last,
>> - uint64_t flags, uint64_t offset, uint64_t vram_base,
>> + bool immediate, bool unlocked, bool flush_tlb,
>> + bool allow_override, struct amdgpu_sync *sync,
>> + uint64_t start, uint64_t last, uint64_t flags,
>> + uint64_t offset, uint64_t vram_base,
>> struct ttm_resource *res, dma_addr_t *pages_addr,
>> struct dma_fence **fence)
>> {
>> struct amdgpu_vm_tlb_seq_struct *tlb_cb;
>> struct amdgpu_vm_update_params params;
>> struct amdgpu_res_cursor cursor;
>> - enum amdgpu_sync_mode sync_mode;
>> int r, idx;
>> if (!drm_dev_enter(adev_to_drm(adev), &idx))
>> @@ -991,14 +991,6 @@ int amdgpu_vm_update_range(struct amdgpu_device
>> *adev, struct amdgpu_vm *vm,
>> params.allow_override = allow_override;
>> INIT_LIST_HEAD(¶ms.tlb_flush_waitlist);
>> - /* Implicitly sync to command submissions in the same VM before
>> - * unmapping. Sync to moving fences before mapping.
>> - */
>> - if (!(flags & AMDGPU_PTE_VALID))
>> - sync_mode = AMDGPU_SYNC_EQ_OWNER;
>> - else
>> - sync_mode = AMDGPU_SYNC_EXPLICIT;
>> -
>> amdgpu_vm_eviction_lock(vm);
>> if (vm->evicting) {
>> r = -EBUSY;
>> @@ -1013,7 +1005,7 @@ int amdgpu_vm_update_range(struct amdgpu_device
>> *adev, struct amdgpu_vm *vm,
>> dma_fence_put(tmp);
>> }
>> - r = vm->update_funcs->prepare(¶ms, resv, sync_mode);
>> + r = vm->update_funcs->prepare(¶ms, sync);
>> if (r)
>> goto error_free;
>> @@ -1155,23 +1147,30 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>> *adev, struct amdgpu_bo_va *bo_va,
>> struct amdgpu_bo *bo = bo_va->base.bo;
>> struct amdgpu_vm *vm = bo_va->base.vm;
>> struct amdgpu_bo_va_mapping *mapping;
>> + struct dma_fence **last_update;
>> dma_addr_t *pages_addr = NULL;
>> struct ttm_resource *mem;
>> - struct dma_fence **last_update;
>> + struct amdgpu_sync sync;
>> bool flush_tlb = clear;
>> - bool uncached;
>> - struct dma_resv *resv;
>> uint64_t vram_base;
>> uint64_t flags;
>> + bool uncached;
>> int r;
>> + amdgpu_sync_create(&sync);
>> if (clear || !bo) {
>> mem = NULL;
>> - resv = vm->root.bo->tbo.base.resv;
>> +
>> + /* Implicitly sync to command submissions in the same VM before
>> + * unmapping.
>> + */
>> + r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
>> + AMDGPU_SYNC_EQ_OWNER, vm);
>> + if (r)
>> + goto error_free;
>> } else {
>> struct drm_gem_object *obj = &bo->tbo.base;
>> - resv = bo->tbo.base.resv;
>> if (obj->import_attach && bo_va->is_xgmi) {
>> struct dma_buf *dma_buf = obj->import_attach->dmabuf;
>> struct drm_gem_object *gobj = dma_buf->priv;
>> @@ -1185,6 +1184,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>> *adev, struct amdgpu_bo_va *bo_va,
>> if (mem && (mem->mem_type == TTM_PL_TT ||
>> mem->mem_type == AMDGPU_PL_PREEMPT))
>> pages_addr = bo->tbo.ttm->dma_address;
>> +
>> + /* Implicitly sync to moving fences before mapping anything */
>> + r = amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv,
>> + AMDGPU_SYNC_EXPLICIT, vm);
>> + if (r)
>> + goto error_free;
>> }
>> if (bo) {
>> @@ -1234,12 +1239,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>> *adev, struct amdgpu_bo_va *bo_va,
>> trace_amdgpu_vm_bo_update(mapping);
>> r = amdgpu_vm_update_range(adev, vm, false, false, flush_tlb,
>> - !uncached, resv, mapping->start, mapping->last,
>> - update_flags, mapping->offset,
>> - vram_base, mem, pages_addr,
>> - last_update);
>> + !uncached, &sync, mapping->start,
>> + mapping->last, update_flags,
>> + mapping->offset, vram_base, mem,
>> + pages_addr, last_update);
>> if (r)
>> - return r;
>> + goto error_free;
>> }
>> /* If the BO is not in its preferred location add it back to
>> @@ -1267,7 +1272,9 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>> *adev, struct amdgpu_bo_va *bo_va,
>> trace_amdgpu_vm_bo_mapping(mapping);
>> }
>> - return 0;
>> +error_free:
>> + amdgpu_sync_free(&sync);
>> + return r;
>> }
>> /**
>> @@ -1414,25 +1421,34 @@ int amdgpu_vm_clear_freed(struct amdgpu_device
>> *adev,
>> struct amdgpu_vm *vm,
>> struct dma_fence **fence)
>> {
>> - struct dma_resv *resv = vm->root.bo->tbo.base.resv;
>> struct amdgpu_bo_va_mapping *mapping;
>> - uint64_t init_pte_value = 0;
>> struct dma_fence *f = NULL;
>> + struct amdgpu_sync sync;
>> int r;
>> +
>> + /*
>> + * Implicitly sync to command submissions in the same VM before
>> + * unmapping.
>> + */
>> + amdgpu_sync_create(&sync);
>> + r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
>> + AMDGPU_SYNC_EQ_OWNER, vm);
>> + if (r)
>> + goto error_free;
>> +
>> while (!list_empty(&vm->freed)) {
>> mapping = list_first_entry(&vm->freed,
>> struct amdgpu_bo_va_mapping, list);
>> list_del(&mapping->list);
>> r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
>> - resv, mapping->start, mapping->last,
>> - init_pte_value, 0, 0, NULL, NULL,
>> - &f);
>> + &sync, mapping->start, mapping->last,
>> + 0, 0, 0, NULL, NULL, &f);
>> amdgpu_vm_free_mapping(adev, vm, mapping, f);
>> if (r) {
>> dma_fence_put(f);
>> - return r;
>> + goto error_free;
>> }
>> }
>> @@ -1443,7 +1459,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device
>> *adev,
>> dma_fence_put(f);
>> }
>> - return 0;
>> +error_free:
>> + amdgpu_sync_free(&sync);
>> + return r;
>> }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index 046949c4b695..1a759012ce93 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -304,8 +304,8 @@ struct amdgpu_vm_update_params {
>> struct amdgpu_vm_update_funcs {
>> int (*map_table)(struct amdgpu_bo_vm *bo);
>> - int (*prepare)(struct amdgpu_vm_update_params *p, struct dma_resv
>> *resv,
>> - enum amdgpu_sync_mode sync_mode);
>> + int (*prepare)(struct amdgpu_vm_update_params *p,
>> + struct amdgpu_sync *sync);
>> int (*update)(struct amdgpu_vm_update_params *p,
>> struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t addr,
>> unsigned count, uint32_t incr, uint64_t flags);
>> @@ -505,9 +505,10 @@ int amdgpu_vm_flush_compute_tlb(struct
>> amdgpu_device *adev,
>> void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>> struct amdgpu_vm *vm, struct amdgpu_bo *bo);
>> int amdgpu_vm_update_range(struct amdgpu_device *adev, struct
>> amdgpu_vm *vm,
>> - bool immediate, bool unlocked, bool flush_tlb, bool
>> allow_override,
>> - struct dma_resv *resv, uint64_t start, uint64_t last,
>> - uint64_t flags, uint64_t offset, uint64_t vram_base,
>> + bool immediate, bool unlocked, bool flush_tlb,
>> + bool allow_override, struct amdgpu_sync *sync,
>> + uint64_t start, uint64_t last, uint64_t flags,
>> + uint64_t offset, uint64_t vram_base,
>> struct ttm_resource *res, dma_addr_t *pages_addr,
>> struct dma_fence **fence);
>> int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>> index 3895bd7d176a..9ff59a4e6f15 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>> @@ -46,13 +46,12 @@ static int amdgpu_vm_cpu_map_table(struct
>> amdgpu_bo_vm *table)
>> * Negativ errno, 0 for success.
>> */
>> static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
>> - struct dma_resv *resv,
>> - enum amdgpu_sync_mode sync_mode)
>> + struct amdgpu_sync *sync)
>> {
>> - if (!resv)
>> + if (!sync)
>> return 0;
>> - return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, p->vm,
>> true);
>> + return amdgpu_sync_wait(sync, true);
>> }
>> /**
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>> index e39d6e7643bf..a076f43097e4 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>> @@ -403,7 +403,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm,
>> params.vm = vm;
>> params.immediate = immediate;
>> - r = vm->update_funcs->prepare(¶ms, NULL, AMDGPU_SYNC_EXPLICIT);
>> + r = vm->update_funcs->prepare(¶ms, NULL);
>> if (r)
>> goto exit;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> index 9b748d7058b5..4772fba33285 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>> @@ -77,32 +77,24 @@ static int amdgpu_vm_sdma_alloc_job(struct
>> amdgpu_vm_update_params *p,
>> * amdgpu_vm_sdma_prepare - prepare SDMA command submission
>> *
>> * @p: see amdgpu_vm_update_params definition
>> - * @resv: reservation object with embedded fence
>> - * @sync_mode: synchronization mode
>> + * @sync: amdgpu_sync object with fences to wait for
>> *
>> * Returns:
>> * Negativ errno, 0 for success.
>> */
>> static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>> - struct dma_resv *resv,
>> - enum amdgpu_sync_mode sync_mode)
>> + struct amdgpu_sync *sync)
>> {
>> - struct amdgpu_sync sync;
>> int r;
>> r = amdgpu_vm_sdma_alloc_job(p, 0);
>> if (r)
>> return r;
>> - if (!resv)
>> + if (!sync)
>> return 0;
>> - amdgpu_sync_create(&sync);
>> - r = amdgpu_sync_resv(p->adev, &sync, resv, sync_mode, p->vm);
>> - if (!r)
>> - r = amdgpu_sync_push_to_job(&sync, p->job);
>> - amdgpu_sync_free(&sync);
>> -
>> + r = amdgpu_sync_push_to_job(sync, p->job);
>> if (r) {
>> p->num_dw_left = 0;
>> amdgpu_job_free(p->job);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs
2024-08-21 20:01 ` Felix Kuehling
@ 2024-08-22 9:07 ` Christian König
2024-08-28 22:40 ` Felix Kuehling
0 siblings, 1 reply; 11+ messages in thread
From: Christian König @ 2024-08-22 9:07 UTC (permalink / raw)
To: Felix Kuehling, friedrich.vock, bas, ishitatsuyuki; +Cc: amd-gfx
Am 21.08.24 um 22:01 schrieb Felix Kuehling:
> On 2024-08-21 08:03, Christian König wrote:
>> This patch tries to solve the basic problem we also need to sync to
>> the KFD fences of the BO because otherwise it can be that we clear
>> PTEs while the KFD queues are still running.
>
> This is going to trigger a lot of phantom KFD evictions and will tank
> performance. It's probably not what you intended.
I tried to avoid that by only waiting for the KFD fence only in the
particular situation that we can't lock the cleared BO because of
contention.
The only short term alternative I can see is to lock all BOs during CS
and that is a) a really large rework and b) will most likely hurt
performance.
Then there is the alternative to lock the VM during BO eviction, but
that means we need to wait on using the drm_exec object inside TTM as
well. So that won't get this fixed in the next halve year or so.
Regards,
Christian.
>
> Regards,
> Felix
>
>
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 30 ++++++++++++++++++++++++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h | 1 +
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++++
>> 3 files changed, 37 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> index bdf1ef825d89..c586ab4c911b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>> @@ -260,6 +260,36 @@ int amdgpu_sync_resv(struct amdgpu_device *adev,
>> struct amdgpu_sync *sync,
>> return 0;
>> }
>> +/**
>> + * amdgpu_sync_kfd - sync to KFD fences
>> + *
>> + * @sync: sync object to add KFD fences to
>> + * @resv: reservation object with KFD fences
>> + *
>> + * Extract all KFD fences and add them to the sync object.
>> + */
>> +int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv)
>> +{
>> + struct dma_resv_iter cursor;
>> + struct dma_fence *f;
>> + int r = 0;
>> +
>> + dma_resv_iter_begin(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP);
>> + dma_resv_for_each_fence_unlocked(&cursor, f) {
>> + void *fence_owner = amdgpu_sync_get_owner(f);
>> +
>> + if (fence_owner != AMDGPU_FENCE_OWNER_KFD)
>> + continue;
>> +
>> + r = amdgpu_sync_fence(sync, f);
>> + if (r)
>> + break;
>> + }
>> + dma_resv_iter_end(&cursor);
>> +
>> + return r;
>> +}
>> +
>> /* Free the entry back to the slab */
>> static void amdgpu_sync_entry_free(struct amdgpu_sync_entry *e)
>> {
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>> index cf1e9e858efd..e3272dce798d 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>> @@ -51,6 +51,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync,
>> struct dma_fence *f);
>> int amdgpu_sync_resv(struct amdgpu_device *adev, struct amdgpu_sync
>> *sync,
>> struct dma_resv *resv, enum amdgpu_sync_mode mode,
>> void *owner);
>> +int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv);
>> struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
>> struct amdgpu_ring *ring);
>> struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync);
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index ba99d428610a..13d429b91327 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1168,6 +1168,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>> *adev, struct amdgpu_bo_va *bo_va,
>> AMDGPU_SYNC_EQ_OWNER, vm);
>> if (r)
>> goto error_free;
>> + if (bo) {
>> + r = amdgpu_sync_kfd(&sync, bo->tbo.base.resv);
>> + if (r)
>> + goto error_free;
>> + }
>> +
>> } else {
>> struct drm_gem_object *obj = &bo->tbo.base;
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/3] drm/amdgpu: re-work VM syncing
2024-08-22 7:28 ` Friedrich Vock
@ 2024-08-28 22:29 ` Felix Kuehling
0 siblings, 0 replies; 11+ messages in thread
From: Felix Kuehling @ 2024-08-28 22:29 UTC (permalink / raw)
To: Friedrich Vock, Christian König, bas, ishitatsuyuki; +Cc: amd-gfx
On 2024-08-22 03:28, Friedrich Vock wrote:
> On 21.08.24 22:46, Felix Kuehling wrote:
>>
>> On 2024-08-21 08:03, Christian König wrote:
>>> Rework how VM operations synchronize to submissions. Provide an
>>> amdgpu_sync container to the backends instead of an reservation
>>> object and fill in the amdgpu_sync object in the higher layers
>>> of the code.
>>>
>>> No intended functional change, just prepares for upcomming changes.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 84
>>> +++++++++++++--------
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 11 +--
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c | 7 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 2 +-
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 +---
>>
>> There are two calls to amdgpu_vm_update_range in amdkfd/kfd_svm.c
>> that would need to be updated as well.
>
> I don't think any change should be needed there? Both calls pass NULL
> for the resv.
Right, sorry, the change to the function signature looked bigger than it
was due to formatting changes. The patch is
Acked-by: Felix Kuehling <felix.kuehling@amd.com>
> All this patch changes is that we're now passing NULL for the
> amdgpu_sync - but the behavior with a NULL amdgpu_sync with this patch
> is the same as with a NULL dma_resv without this patch, so nothing
> needs to change.
>
> Regards,
> Friedrich
>
>>
>> Regards,
>> Felix
>>
>>
>>> 5 files changed, 65 insertions(+), 55 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index bcb729094521..ba99d428610a 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -838,7 +838,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device
>>> *adev,
>>> params.vm = vm;
>>> params.immediate = immediate;
>>> - r = vm->update_funcs->prepare(¶ms, NULL,
>>> AMDGPU_SYNC_EXPLICIT);
>>> + r = vm->update_funcs->prepare(¶ms, NULL);
>>> if (r)
>>> goto error;
>>> @@ -933,7 +933,7 @@ amdgpu_vm_tlb_flush(struct
>>> amdgpu_vm_update_params *params,
>>> * @unlocked: unlocked invalidation during MM callback
>>> * @flush_tlb: trigger tlb invalidation after update completed
>>> * @allow_override: change MTYPE for local NUMA nodes
>>> - * @resv: fences we need to sync to
>>> + * @sync: fences we need to sync to
>>> * @start: start of mapped range
>>> * @last: last mapped entry
>>> * @flags: flags for the entries
>>> @@ -949,16 +949,16 @@ amdgpu_vm_tlb_flush(struct
>>> amdgpu_vm_update_params *params,
>>> * 0 for success, negative erro code for failure.
>>> */
>>> int amdgpu_vm_update_range(struct amdgpu_device *adev, struct
>>> amdgpu_vm *vm,
>>> - bool immediate, bool unlocked, bool flush_tlb, bool
>>> allow_override,
>>> - struct dma_resv *resv, uint64_t start, uint64_t last,
>>> - uint64_t flags, uint64_t offset, uint64_t vram_base,
>>> + bool immediate, bool unlocked, bool flush_tlb,
>>> + bool allow_override, struct amdgpu_sync *sync,
>>> + uint64_t start, uint64_t last, uint64_t flags,
>>> + uint64_t offset, uint64_t vram_base,
>>> struct ttm_resource *res, dma_addr_t *pages_addr,
>>> struct dma_fence **fence)
>>> {
>>> struct amdgpu_vm_tlb_seq_struct *tlb_cb;
>>> struct amdgpu_vm_update_params params;
>>> struct amdgpu_res_cursor cursor;
>>> - enum amdgpu_sync_mode sync_mode;
>>> int r, idx;
>>> if (!drm_dev_enter(adev_to_drm(adev), &idx))
>>> @@ -991,14 +991,6 @@ int amdgpu_vm_update_range(struct amdgpu_device
>>> *adev, struct amdgpu_vm *vm,
>>> params.allow_override = allow_override;
>>> INIT_LIST_HEAD(¶ms.tlb_flush_waitlist);
>>> - /* Implicitly sync to command submissions in the same VM before
>>> - * unmapping. Sync to moving fences before mapping.
>>> - */
>>> - if (!(flags & AMDGPU_PTE_VALID))
>>> - sync_mode = AMDGPU_SYNC_EQ_OWNER;
>>> - else
>>> - sync_mode = AMDGPU_SYNC_EXPLICIT;
>>> -
>>> amdgpu_vm_eviction_lock(vm);
>>> if (vm->evicting) {
>>> r = -EBUSY;
>>> @@ -1013,7 +1005,7 @@ int amdgpu_vm_update_range(struct
>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>> dma_fence_put(tmp);
>>> }
>>> - r = vm->update_funcs->prepare(¶ms, resv, sync_mode);
>>> + r = vm->update_funcs->prepare(¶ms, sync);
>>> if (r)
>>> goto error_free;
>>> @@ -1155,23 +1147,30 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>> *adev, struct amdgpu_bo_va *bo_va,
>>> struct amdgpu_bo *bo = bo_va->base.bo;
>>> struct amdgpu_vm *vm = bo_va->base.vm;
>>> struct amdgpu_bo_va_mapping *mapping;
>>> + struct dma_fence **last_update;
>>> dma_addr_t *pages_addr = NULL;
>>> struct ttm_resource *mem;
>>> - struct dma_fence **last_update;
>>> + struct amdgpu_sync sync;
>>> bool flush_tlb = clear;
>>> - bool uncached;
>>> - struct dma_resv *resv;
>>> uint64_t vram_base;
>>> uint64_t flags;
>>> + bool uncached;
>>> int r;
>>> + amdgpu_sync_create(&sync);
>>> if (clear || !bo) {
>>> mem = NULL;
>>> - resv = vm->root.bo->tbo.base.resv;
>>> +
>>> + /* Implicitly sync to command submissions in the same VM
>>> before
>>> + * unmapping.
>>> + */
>>> + r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
>>> + AMDGPU_SYNC_EQ_OWNER, vm);
>>> + if (r)
>>> + goto error_free;
>>> } else {
>>> struct drm_gem_object *obj = &bo->tbo.base;
>>> - resv = bo->tbo.base.resv;
>>> if (obj->import_attach && bo_va->is_xgmi) {
>>> struct dma_buf *dma_buf = obj->import_attach->dmabuf;
>>> struct drm_gem_object *gobj = dma_buf->priv;
>>> @@ -1185,6 +1184,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>> *adev, struct amdgpu_bo_va *bo_va,
>>> if (mem && (mem->mem_type == TTM_PL_TT ||
>>> mem->mem_type == AMDGPU_PL_PREEMPT))
>>> pages_addr = bo->tbo.ttm->dma_address;
>>> +
>>> + /* Implicitly sync to moving fences before mapping anything */
>>> + r = amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv,
>>> + AMDGPU_SYNC_EXPLICIT, vm);
>>> + if (r)
>>> + goto error_free;
>>> }
>>> if (bo) {
>>> @@ -1234,12 +1239,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>> *adev, struct amdgpu_bo_va *bo_va,
>>> trace_amdgpu_vm_bo_update(mapping);
>>> r = amdgpu_vm_update_range(adev, vm, false, false, flush_tlb,
>>> - !uncached, resv, mapping->start, mapping->last,
>>> - update_flags, mapping->offset,
>>> - vram_base, mem, pages_addr,
>>> - last_update);
>>> + !uncached, &sync, mapping->start,
>>> + mapping->last, update_flags,
>>> + mapping->offset, vram_base, mem,
>>> + pages_addr, last_update);
>>> if (r)
>>> - return r;
>>> + goto error_free;
>>> }
>>> /* If the BO is not in its preferred location add it back to
>>> @@ -1267,7 +1272,9 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>> *adev, struct amdgpu_bo_va *bo_va,
>>> trace_amdgpu_vm_bo_mapping(mapping);
>>> }
>>> - return 0;
>>> +error_free:
>>> + amdgpu_sync_free(&sync);
>>> + return r;
>>> }
>>> /**
>>> @@ -1414,25 +1421,34 @@ int amdgpu_vm_clear_freed(struct
>>> amdgpu_device *adev,
>>> struct amdgpu_vm *vm,
>>> struct dma_fence **fence)
>>> {
>>> - struct dma_resv *resv = vm->root.bo->tbo.base.resv;
>>> struct amdgpu_bo_va_mapping *mapping;
>>> - uint64_t init_pte_value = 0;
>>> struct dma_fence *f = NULL;
>>> + struct amdgpu_sync sync;
>>> int r;
>>> +
>>> + /*
>>> + * Implicitly sync to command submissions in the same VM before
>>> + * unmapping.
>>> + */
>>> + amdgpu_sync_create(&sync);
>>> + r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
>>> + AMDGPU_SYNC_EQ_OWNER, vm);
>>> + if (r)
>>> + goto error_free;
>>> +
>>> while (!list_empty(&vm->freed)) {
>>> mapping = list_first_entry(&vm->freed,
>>> struct amdgpu_bo_va_mapping, list);
>>> list_del(&mapping->list);
>>> r = amdgpu_vm_update_range(adev, vm, false, false, true,
>>> false,
>>> - resv, mapping->start, mapping->last,
>>> - init_pte_value, 0, 0, NULL, NULL,
>>> - &f);
>>> + &sync, mapping->start, mapping->last,
>>> + 0, 0, 0, NULL, NULL, &f);
>>> amdgpu_vm_free_mapping(adev, vm, mapping, f);
>>> if (r) {
>>> dma_fence_put(f);
>>> - return r;
>>> + goto error_free;
>>> }
>>> }
>>> @@ -1443,7 +1459,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device
>>> *adev,
>>> dma_fence_put(f);
>>> }
>>> - return 0;
>>> +error_free:
>>> + amdgpu_sync_free(&sync);
>>> + return r;
>>> }
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> index 046949c4b695..1a759012ce93 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -304,8 +304,8 @@ struct amdgpu_vm_update_params {
>>> struct amdgpu_vm_update_funcs {
>>> int (*map_table)(struct amdgpu_bo_vm *bo);
>>> - int (*prepare)(struct amdgpu_vm_update_params *p, struct
>>> dma_resv *resv,
>>> - enum amdgpu_sync_mode sync_mode);
>>> + int (*prepare)(struct amdgpu_vm_update_params *p,
>>> + struct amdgpu_sync *sync);
>>> int (*update)(struct amdgpu_vm_update_params *p,
>>> struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t addr,
>>> unsigned count, uint32_t incr, uint64_t flags);
>>> @@ -505,9 +505,10 @@ int amdgpu_vm_flush_compute_tlb(struct
>>> amdgpu_device *adev,
>>> void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
>>> struct amdgpu_vm *vm, struct amdgpu_bo *bo);
>>> int amdgpu_vm_update_range(struct amdgpu_device *adev, struct
>>> amdgpu_vm *vm,
>>> - bool immediate, bool unlocked, bool flush_tlb, bool
>>> allow_override,
>>> - struct dma_resv *resv, uint64_t start, uint64_t last,
>>> - uint64_t flags, uint64_t offset, uint64_t vram_base,
>>> + bool immediate, bool unlocked, bool flush_tlb,
>>> + bool allow_override, struct amdgpu_sync *sync,
>>> + uint64_t start, uint64_t last, uint64_t flags,
>>> + uint64_t offset, uint64_t vram_base,
>>> struct ttm_resource *res, dma_addr_t *pages_addr,
>>> struct dma_fence **fence);
>>> int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>>> index 3895bd7d176a..9ff59a4e6f15 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
>>> @@ -46,13 +46,12 @@ static int amdgpu_vm_cpu_map_table(struct
>>> amdgpu_bo_vm *table)
>>> * Negativ errno, 0 for success.
>>> */
>>> static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
>>> - struct dma_resv *resv,
>>> - enum amdgpu_sync_mode sync_mode)
>>> + struct amdgpu_sync *sync)
>>> {
>>> - if (!resv)
>>> + if (!sync)
>>> return 0;
>>> - return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode,
>>> p->vm, true);
>>> + return amdgpu_sync_wait(sync, true);
>>> }
>>> /**
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>>> index e39d6e7643bf..a076f43097e4 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
>>> @@ -403,7 +403,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device
>>> *adev, struct amdgpu_vm *vm,
>>> params.vm = vm;
>>> params.immediate = immediate;
>>> - r = vm->update_funcs->prepare(¶ms, NULL,
>>> AMDGPU_SYNC_EXPLICIT);
>>> + r = vm->update_funcs->prepare(¶ms, NULL);
>>> if (r)
>>> goto exit;
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> index 9b748d7058b5..4772fba33285 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
>>> @@ -77,32 +77,24 @@ static int amdgpu_vm_sdma_alloc_job(struct
>>> amdgpu_vm_update_params *p,
>>> * amdgpu_vm_sdma_prepare - prepare SDMA command submission
>>> *
>>> * @p: see amdgpu_vm_update_params definition
>>> - * @resv: reservation object with embedded fence
>>> - * @sync_mode: synchronization mode
>>> + * @sync: amdgpu_sync object with fences to wait for
>>> *
>>> * Returns:
>>> * Negativ errno, 0 for success.
>>> */
>>> static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
>>> - struct dma_resv *resv,
>>> - enum amdgpu_sync_mode sync_mode)
>>> + struct amdgpu_sync *sync)
>>> {
>>> - struct amdgpu_sync sync;
>>> int r;
>>> r = amdgpu_vm_sdma_alloc_job(p, 0);
>>> if (r)
>>> return r;
>>> - if (!resv)
>>> + if (!sync)
>>> return 0;
>>> - amdgpu_sync_create(&sync);
>>> - r = amdgpu_sync_resv(p->adev, &sync, resv, sync_mode, p->vm);
>>> - if (!r)
>>> - r = amdgpu_sync_push_to_job(&sync, p->job);
>>> - amdgpu_sync_free(&sync);
>>> -
>>> + r = amdgpu_sync_push_to_job(sync, p->job);
>>> if (r) {
>>> p->num_dw_left = 0;
>>> amdgpu_job_free(p->job);
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs
2024-08-22 9:07 ` Christian König
@ 2024-08-28 22:40 ` Felix Kuehling
2024-08-29 9:48 ` Christian König
0 siblings, 1 reply; 11+ messages in thread
From: Felix Kuehling @ 2024-08-28 22:40 UTC (permalink / raw)
To: Christian König, friedrich.vock, bas, ishitatsuyuki; +Cc: amd-gfx
On 2024-08-22 05:07, Christian König wrote:
> Am 21.08.24 um 22:01 schrieb Felix Kuehling:
>> On 2024-08-21 08:03, Christian König wrote:
>>> This patch tries to solve the basic problem we also need to sync to
>>> the KFD fences of the BO because otherwise it can be that we clear
>>> PTEs while the KFD queues are still running.
>>
>> This is going to trigger a lot of phantom KFD evictions and will tank
>> performance. It's probably not what you intended.
>
> I tried to avoid that by only waiting for the KFD fence only in the
> particular situation that we can't lock the cleared BO because of
> contention.
OK. It's hard to make out where you're adding that call with the small
context in the patch. As far as I can tell it's in the "if (clear ||
!bo)" branch. The "clear" case is as you mention, only used when the BO
cannot be locked. The !bo case is PRT?
Contention would happen, if this runs concurrently with a
restore-from-eviction, in which case we're already on a slow path and
another eviction doesn't matter (as long as we're not getting into a
live-lock situation). Or if a KFD BO is in the middle of being mapped or
unmapped by another thread, which should be unlikely. So maybe this
won't have a huge impact in practice. It's worth a try.
The patch is
Acked-by: Felix Kuehling <felix.kuehling@amd.com>
>
> The only short term alternative I can see is to lock all BOs during CS
> and that is a) a really large rework and b) will most likely hurt
> performance.
>
> Then there is the alternative to lock the VM during BO eviction, but
> that means we need to wait on using the drm_exec object inside TTM as
> well. So that won't get this fixed in the next halve year or so.
>
> Regards,
> Christian.
>
>>
>> Regards,
>> Felix
>>
>>
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 30
>>> ++++++++++++++++++++++++
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h | 1 +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++++
>>> 3 files changed, 37 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>> index bdf1ef825d89..c586ab4c911b 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>> @@ -260,6 +260,36 @@ int amdgpu_sync_resv(struct amdgpu_device
>>> *adev, struct amdgpu_sync *sync,
>>> return 0;
>>> }
>>> +/**
>>> + * amdgpu_sync_kfd - sync to KFD fences
>>> + *
>>> + * @sync: sync object to add KFD fences to
>>> + * @resv: reservation object with KFD fences
>>> + *
>>> + * Extract all KFD fences and add them to the sync object.
>>> + */
>>> +int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv)
>>> +{
>>> + struct dma_resv_iter cursor;
>>> + struct dma_fence *f;
>>> + int r = 0;
>>> +
>>> + dma_resv_iter_begin(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP);
>>> + dma_resv_for_each_fence_unlocked(&cursor, f) {
>>> + void *fence_owner = amdgpu_sync_get_owner(f);
>>> +
>>> + if (fence_owner != AMDGPU_FENCE_OWNER_KFD)
>>> + continue;
>>> +
>>> + r = amdgpu_sync_fence(sync, f);
>>> + if (r)
>>> + break;
>>> + }
>>> + dma_resv_iter_end(&cursor);
>>> +
>>> + return r;
>>> +}
>>> +
>>> /* Free the entry back to the slab */
>>> static void amdgpu_sync_entry_free(struct amdgpu_sync_entry *e)
>>> {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>>> index cf1e9e858efd..e3272dce798d 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>>> @@ -51,6 +51,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync,
>>> struct dma_fence *f);
>>> int amdgpu_sync_resv(struct amdgpu_device *adev, struct
>>> amdgpu_sync *sync,
>>> struct dma_resv *resv, enum amdgpu_sync_mode mode,
>>> void *owner);
>>> +int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv);
>>> struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
>>> struct amdgpu_ring *ring);
>>> struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync);
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index ba99d428610a..13d429b91327 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1168,6 +1168,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>> *adev, struct amdgpu_bo_va *bo_va,
>>> AMDGPU_SYNC_EQ_OWNER, vm);
>>> if (r)
>>> goto error_free;
>>> + if (bo) {
>>> + r = amdgpu_sync_kfd(&sync, bo->tbo.base.resv);
>>> + if (r)
>>> + goto error_free;
>>> + }
>>> +
>>> } else {
>>> struct drm_gem_object *obj = &bo->tbo.base;
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs
2024-08-28 22:40 ` Felix Kuehling
@ 2024-08-29 9:48 ` Christian König
0 siblings, 0 replies; 11+ messages in thread
From: Christian König @ 2024-08-29 9:48 UTC (permalink / raw)
To: Felix Kuehling, friedrich.vock, bas, ishitatsuyuki; +Cc: amd-gfx
Am 29.08.24 um 00:40 schrieb Felix Kuehling:
>
> On 2024-08-22 05:07, Christian König wrote:
>> Am 21.08.24 um 22:01 schrieb Felix Kuehling:
>>> On 2024-08-21 08:03, Christian König wrote:
>>>> This patch tries to solve the basic problem we also need to sync to
>>>> the KFD fences of the BO because otherwise it can be that we clear
>>>> PTEs while the KFD queues are still running.
>>>
>>> This is going to trigger a lot of phantom KFD evictions and will
>>> tank performance. It's probably not what you intended.
>>
>> I tried to avoid that by only waiting for the KFD fence only in the
>> particular situation that we can't lock the cleared BO because of
>> contention.
>
> OK. It's hard to make out where you're adding that call with the small
> context in the patch. As far as I can tell it's in the "if (clear ||
> !bo)" branch. The "clear" case is as you mention, only used when the
> BO cannot be locked. The !bo case is PRT?
Yes, exactly that.
>
> Contention would happen, if this runs concurrently with a
> restore-from-eviction, in which case we're already on a slow path and
> another eviction doesn't matter (as long as we're not getting into a
> live-lock situation). Or if a KFD BO is in the middle of being mapped
> or unmapped by another thread, which should be unlikely. So maybe this
> won't have a huge impact in practice. It's worth a try.
>
> The patch is
>
> Acked-by: Felix Kuehling <felix.kuehling@amd.com>
Let's see if this works or not in practice.
Thanks,
Christian.
>
>
>>
>> The only short term alternative I can see is to lock all BOs during
>> CS and that is a) a really large rework and b) will most likely hurt
>> performance.
>>
>> Then there is the alternative to lock the VM during BO eviction, but
>> that means we need to wait on using the drm_exec object inside TTM as
>> well. So that won't get this fixed in the next halve year or so.
>>
>> Regards,
>> Christian.
>>
>>>
>>> Regards,
>>> Felix
>>>
>>>
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c | 30
>>>> ++++++++++++++++++++++++
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h | 1 +
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 +++++
>>>> 3 files changed, 37 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>>> index bdf1ef825d89..c586ab4c911b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.c
>>>> @@ -260,6 +260,36 @@ int amdgpu_sync_resv(struct amdgpu_device
>>>> *adev, struct amdgpu_sync *sync,
>>>> return 0;
>>>> }
>>>> +/**
>>>> + * amdgpu_sync_kfd - sync to KFD fences
>>>> + *
>>>> + * @sync: sync object to add KFD fences to
>>>> + * @resv: reservation object with KFD fences
>>>> + *
>>>> + * Extract all KFD fences and add them to the sync object.
>>>> + */
>>>> +int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv)
>>>> +{
>>>> + struct dma_resv_iter cursor;
>>>> + struct dma_fence *f;
>>>> + int r = 0;
>>>> +
>>>> + dma_resv_iter_begin(&cursor, resv, DMA_RESV_USAGE_BOOKKEEP);
>>>> + dma_resv_for_each_fence_unlocked(&cursor, f) {
>>>> + void *fence_owner = amdgpu_sync_get_owner(f);
>>>> +
>>>> + if (fence_owner != AMDGPU_FENCE_OWNER_KFD)
>>>> + continue;
>>>> +
>>>> + r = amdgpu_sync_fence(sync, f);
>>>> + if (r)
>>>> + break;
>>>> + }
>>>> + dma_resv_iter_end(&cursor);
>>>> +
>>>> + return r;
>>>> +}
>>>> +
>>>> /* Free the entry back to the slab */
>>>> static void amdgpu_sync_entry_free(struct amdgpu_sync_entry *e)
>>>> {
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>>>> index cf1e9e858efd..e3272dce798d 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sync.h
>>>> @@ -51,6 +51,7 @@ int amdgpu_sync_fence(struct amdgpu_sync *sync,
>>>> struct dma_fence *f);
>>>> int amdgpu_sync_resv(struct amdgpu_device *adev, struct
>>>> amdgpu_sync *sync,
>>>> struct dma_resv *resv, enum amdgpu_sync_mode mode,
>>>> void *owner);
>>>> +int amdgpu_sync_kfd(struct amdgpu_sync *sync, struct dma_resv *resv);
>>>> struct dma_fence *amdgpu_sync_peek_fence(struct amdgpu_sync *sync,
>>>> struct amdgpu_ring *ring);
>>>> struct dma_fence *amdgpu_sync_get_fence(struct amdgpu_sync *sync);
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index ba99d428610a..13d429b91327 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -1168,6 +1168,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device
>>>> *adev, struct amdgpu_bo_va *bo_va,
>>>> AMDGPU_SYNC_EQ_OWNER, vm);
>>>> if (r)
>>>> goto error_free;
>>>> + if (bo) {
>>>> + r = amdgpu_sync_kfd(&sync, bo->tbo.base.resv);
>>>> + if (r)
>>>> + goto error_free;
>>>> + }
>>>> +
>>>> } else {
>>>> struct drm_gem_object *obj = &bo->tbo.base;
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2024-08-29 9:48 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-08-21 12:03 [PATCH 1/3] drm/amdgpu: re-work VM syncing Christian König
2024-08-21 12:03 ` [PATCH 2/3] drm/amdgpu: sync to KFD fences before clearing PTEs Christian König
2024-08-21 20:01 ` Felix Kuehling
2024-08-22 9:07 ` Christian König
2024-08-28 22:40 ` Felix Kuehling
2024-08-29 9:48 ` Christian König
2024-08-21 12:03 ` [PATCH 3/3] drm/amdgpu: stop syncing PRT map operations Christian König
2024-08-21 17:39 ` [PATCH 1/3] drm/amdgpu: re-work VM syncing Friedrich Vock
2024-08-21 20:46 ` Felix Kuehling
2024-08-22 7:28 ` Friedrich Vock
2024-08-28 22:29 ` Felix Kuehling
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox