On 31/10/2024 13:48, Li, Yunxiang (Teddy) wrote:
[Public]From: Christian König <ckoenig.leichtzumerken@xxxxxxxxx> Sent: Thursday, October 31, 2024 8:54 Am 25.10.24 um 19:41 schrieb Yunxiang Li:Before, every time fdinfo is queried we try to lock all the BOs in the VM and calculate memory usage from scratch. This works okay if the fdinfo is rarely read and the VMs don't have a ton of BOs. If either of these conditions is not true, we get a massive performance hit. In this new revision, we track the BOs as they change states. This way when the fdinfo is queried we only need to take the status lock and copy out the usage stats with minimal impact to the runtime performance. Signed-off-by: Yunxiang Li <Yunxiang.Li@xxxxxxx> --- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 14 +- drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c | 10 +- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 107 +++-------- drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 5 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h | 2 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 189 +++++++++++++++----- drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 12 +- drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 1 + 8 files changed, 199 insertions(+), 141 deletions(-) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index b144404902255..1d8a0ff3c8604 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -36,6 +36,7 @@ #include "amdgpu_gem.h" #include "amdgpu_dma_buf.h" #include "amdgpu_xgmi.h" +#include "amdgpu_vm.h" #include <drm/amdgpu_drm.h> #include <drm/ttm/ttm_tt.h> #include <linux/dma-buf.h> @@ -190,6 +191,13 @@ static void amdgpu_dma_buf_unmap(structdma_buf_attachment *attach,} } +static void amdgpu_dma_buf_release(struct dma_buf *buf) { + struct amdgpu_bo *bo = gem_to_amdgpu_bo(buf->priv); + amdgpu_vm_bo_update_shared(bo, -1); + drm_gem_dmabuf_release(buf);Please run checkpatch.pl on the patch. As far as I can see it would complain about the coding style here (empty line between declaration and code). Not much of an issue but we would like to prevent upstream from complaining about such things.Will do+} + /** * amdgpu_dma_buf_begin_cpu_access - &dma_buf_ops.begin_cpu_accessimplementation* @dma_buf: Shared DMA buffer @@ -237,7 +245,7 @@ const struct dma_buf_ops amdgpu_dmabuf_ops = { .unpin = amdgpu_dma_buf_unpin, .map_dma_buf = amdgpu_dma_buf_map, .unmap_dma_buf = amdgpu_dma_buf_unmap, - .release = drm_gem_dmabuf_release, + .release = amdgpu_dma_buf_release, .begin_cpu_access = amdgpu_dma_buf_begin_cpu_access, .mmap = drm_gem_dmabuf_mmap, .vmap = drm_gem_dmabuf_vmap, @@ -265,8 +273,10 @@ struct dma_buf *amdgpu_gem_prime_export(structdrm_gem_object *gobj,return ERR_PTR(-EPERM); buf = drm_gem_prime_export(gobj, flags); - if (!IS_ERR(buf)) + if (!IS_ERR(buf)) { buf->ops = &amdgpu_dmabuf_ops; + amdgpu_vm_bo_update_shared(bo, +1); + } return buf; } diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c index 7a9573958d87c..e0e09f7b39d10 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c @@ -60,7 +60,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p, structdrm_file *file)struct amdgpu_fpriv *fpriv = file->driver_priv; struct amdgpu_vm *vm = &fpriv->vm; - struct amdgpu_mem_stats stats[__AMDGPU_PL_LAST + 1] = { }; + struct amdgpu_mem_stats stats[__AMDGPU_PL_LAST] = { }; ktime_t usage[AMDGPU_HW_IP_NUM]; const char *pl_name[] = { [TTM_PL_VRAM] = "vram", @@ -70,13 +70,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p, structdrm_file *file)unsigned int hw_ip, i; int ret; - ret = amdgpu_bo_reserve(vm->root.bo, false); - if (ret) - return; - - amdgpu_vm_get_memory(vm, stats, ARRAY_SIZE(stats)); - amdgpu_bo_unreserve(vm->root.bo); - + amdgpu_vm_get_memory(vm, stats); amdgpu_ctx_mgr_usage(&fpriv->ctx_mgr, usage); /* diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 2436b7c9ad12b..98563124ff99c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -1156,7 +1156,7 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object*bo,return; abo = ttm_to_amdgpu_bo(bo); - amdgpu_vm_bo_invalidate(abo, evict); + amdgpu_vm_bo_move(abo, new_mem, evict); amdgpu_bo_kunmap(abo); @@ -1169,86 +1169,6 @@ void amdgpu_bo_move_notify(structttm_buffer_object *bo,old_mem ? old_mem->mem_type : -1); } -void amdgpu_bo_get_memory(struct amdgpu_bo *bo, - struct amdgpu_mem_stats *stats, - unsigned int sz) -{ - const unsigned int domain_to_pl[] = { - [ilog2(AMDGPU_GEM_DOMAIN_CPU)] = TTM_PL_SYSTEM, - [ilog2(AMDGPU_GEM_DOMAIN_GTT)] = TTM_PL_TT, - [ilog2(AMDGPU_GEM_DOMAIN_VRAM)] = TTM_PL_VRAM, - [ilog2(AMDGPU_GEM_DOMAIN_GDS)] =AMDGPU_PL_GDS,- [ilog2(AMDGPU_GEM_DOMAIN_GWS)] =AMDGPU_PL_GWS,- [ilog2(AMDGPU_GEM_DOMAIN_OA)] = AMDGPU_PL_OA, - [ilog2(AMDGPU_GEM_DOMAIN_DOORBELL)] =AMDGPU_PL_DOORBELL,- }; - struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); - struct ttm_resource *res = bo->tbo.resource; - struct drm_gem_object *obj = &bo->tbo.base; - uint64_t size = amdgpu_bo_size(bo); - unsigned int type; - - if (!res) { - /* - * If no backing store use one of the preferred domain for basic - * stats. We take the MSB since that should give a reasonable - * view. - */ - BUILD_BUG_ON(TTM_PL_VRAM < TTM_PL_TT || - TTM_PL_VRAM < TTM_PL_SYSTEM); - type = fls(bo->preferred_domains &AMDGPU_GEM_DOMAIN_MASK);- if (!type) - return; - type--; - if (drm_WARN_ON_ONCE(&adev->ddev, - type >= ARRAY_SIZE(domain_to_pl))) - return; - type = domain_to_pl[type]; - } else { - type = res->mem_type; - } - - /* Squash some into 'cpu' to keep the legacy userspace view. */ - switch (type) { - case TTM_PL_VRAM: - case TTM_PL_TT: - case TTM_PL_SYSTEM: - break; - default: - type = TTM_PL_SYSTEM; - break; - } - - if (drm_WARN_ON_ONCE(&adev->ddev, type >= sz)) - return; - - /* DRM stats common fields: */ - - if (drm_gem_object_is_shared_for_memory_stats(obj)) - stats[type].drm.shared += size; - else - stats[type].drm.private += size; - - if (res) { - stats[type].drm.resident += size; - - if (!dma_resv_test_signaled(obj->resv,DMA_RESV_USAGE_BOOKKEEP))- stats[type].drm.active += size; - else if (bo->flags & AMDGPU_GEM_CREATE_DISCARDABLE) - stats[type].drm.purgeable += size; - } - - /* amdgpu specific stats: */ - - if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) { - stats[TTM_PL_VRAM].requested += size; - if (type != TTM_PL_VRAM) - stats[TTM_PL_VRAM].evicted += size; - } else if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT) { - stats[TTM_PL_TT].requested += size; - } -} - /** * amdgpu_bo_release_notify - notification about a BO being released * @bo: pointer to a buffer object @@ -1463,6 +1383,31 @@ u64 amdgpu_bo_gpu_offset_no_check(structamdgpu_bo *bo)return amdgpu_gmc_sign_extend(offset); } +uint32_t amdgpu_bo_get_preferred_placement(struct amdgpu_bo *bo) { + uint32_t domain = bo->preferred_domains &AMDGPU_GEM_DOMAIN_MASK;+ if (!domain) + return TTM_PL_SYSTEM; + + switch (ilog2(domain)) { + case AMDGPU_GEM_DOMAIN_CPU: + return TTM_PL_SYSTEM; + case AMDGPU_GEM_DOMAIN_GTT: + return TTM_PL_TT; + case AMDGPU_GEM_DOMAIN_VRAM: + return TTM_PL_VRAM; + case AMDGPU_GEM_DOMAIN_GDS: + return AMDGPU_PL_GDS; + case AMDGPU_GEM_DOMAIN_GWS: + return AMDGPU_PL_GWS; + case AMDGPU_GEM_DOMAIN_OA: + return AMDGPU_PL_OA; + case AMDGPU_GEM_DOMAIN_DOORBELL: + return AMDGPU_PL_DOORBELL; + default: + return TTM_PL_SYSTEM;If I'm not completely mistaken that won't work like that. The AMDGPU_GEM_DOMAIN_* defines are masks and not shifts.Yeah you are right, should have been rounddown_pow_of_two+ } +} + /** * amdgpu_bo_get_preferred_domain - get preferred domain * @adev: amdgpu device object diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h index be6769852ece4..bd58a8b0ece66 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h @@ -30,6 +30,7 @@ #include <drm/amdgpu_drm.h> #include "amdgpu.h" +#include "amdgpu_ttm.h" #include "amdgpu_res_cursor.h"Why is that necessary?I got a compile error otherwise for those AMDGPU_PL_*#ifdef CONFIG_MMU_NOTIFIER @@ -300,9 +301,7 @@ int amdgpu_bo_sync_wait_resv(struct amdgpu_device*adev, struct dma_resv *resv,int amdgpu_bo_sync_wait(struct amdgpu_bo *bo, void *owner, bool intr); u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo); u64 amdgpu_bo_gpu_offset_no_check(struct amdgpu_bo *bo); -void amdgpu_bo_get_memory(struct amdgpu_bo *bo, - struct amdgpu_mem_stats *stats, - unsigned int size); +uint32_t amdgpu_bo_get_preferred_placement(struct amdgpu_bo *bo); uint32_t amdgpu_bo_get_preferred_domain(struct amdgpu_device *adev, uint32_t domain); diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h index 2852a6064c9ac..a9088e864fde4 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h @@ -26,8 +26,8 @@ #include <linux/dma-direction.h> #include <drm/gpu_scheduler.h> +#include <drm/ttm/ttm_placement.h> #include "amdgpu_vram_mgr.h" -#include "amdgpu.h"Looks like a valuable cleanup, but should probably a separate patch.Without this there's a circular include that breaks compilation combined with the above#define AMDGPU_PL_GDS (TTM_PL_PRIV + 0) #define AMDGPU_PL_GWS (TTM_PL_PRIV + 1) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 9fab64edd0530..abd35c18ddaa8 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -36,6 +36,7 @@ #include <drm/ttm/ttm_tt.h> #include <drm/drm_exec.h> #include "amdgpu.h" +#include "amdgpu_vm.h" #include "amdgpu_trace.h" #include "amdgpu_amdkfd.h" #include "amdgpu_gmc.h" @@ -310,6 +311,94 @@ static void amdgpu_vm_bo_reset_state_machine(structamdgpu_vm *vm)spin_unlock(&vm->status_lock); } +/** + * amdgpu_vm_update_shared - helper to update shared memory stat + * @base: base structure for tracking BO usage in a VM + * @sign: if we should add (+1) or subtract (-1) from the shared stat + * + * Takes the vm status_lock and updates the shared memory stat. If +the basic + * stat changed (e.g. buffer was moved) amdgpu_vm_update_stats need +to be called + * as well. + */ +static void amdgpu_vm_update_shared(struct amdgpu_vm_bo_base *base, +int sign) { + struct amdgpu_vm *vm = base->vm; + struct amdgpu_bo *bo = base->bo; + struct ttm_resource *res; + int64_t size; + uint32_t type; + + if (!vm || !bo) + return; + + size = sign * amdgpu_bo_size(bo); + if ((res = bo->tbo.resource)) + type = res->mem_type; + else + type = amdgpu_bo_get_preferred_placement(bo);As discussed with Tvrtko that won't work like this. Either use the preferred placement or the actual backing store, but don't use a fallback here.I had a follow up discussion with Tvrtko here https://lists.freedesktop.org/archives/amd-gfx/2024-October/116024.html it seems like this is the intended semantics for the drm-total-* stats. I can see it going both ways, I guess it's just up to which design is most useful for whom ever is reading the stats.
Yeah I think this is correct, unless the allowed mask would perhaps be better than preferred. We don't want drm-total- to show zero if object simply has no current placement.
Current design is for it to mean "all the buffer currently at X" + "all the buffer that wants to be at X but currently don't have a backing" The alternative I guess is for it to mean "all the buffer that wants to be at X"
Alternative is the same, no? But I think it is correct. As I explained before drm-total should be akin to VIRT in top(1) and drm-resident to RES.
Btw, I'm having trouble figuring out where I should account for drm-active-* it's for buffers that are currently being used (e.g. have a fence not signaled) it seems like the work scheduling part is quite far removed from the individual BOs...
Ah that is a fun one. Perhaps we should add DRM_GEM_OBJECT_ACTIVE and use it from drm_print_memory_stats() as with resident and purgeable. Then amdgpu could opt to not display those and it probably wouldn't be a huge loss since drm-active- is very transient and low value.
Regards, Tvrtko
+ if (type >= __AMDGPU_PL_LAST) + return; + + spin_lock(&vm->status_lock); + vm->stats[type].drm.shared += size; + vm->stats[type].drm.private -= size; + spin_unlock(&vm->status_lock); +} + +/** + * amdgpu_vm_update_stats - helper to update normal memory stat + * @base: base structure for tracking BO usage in a VM + * @new_res: if not NULL, the ttm_resource to use for the purpose of +accounting + * (i.e. ignore the one in the BO) + * @sign: if we should add (+1) or subtract (-1) from the stat + * + * Takes the vm status_lock and updates the basic memory stat. If the +shared + * stat changed (e.g. buffer was exported) amdgpu_vm_update_shared +need to be + * called as well. + */ +void amdgpu_vm_update_stats(struct amdgpu_vm_bo_base *base, + struct ttm_resource *new_res, int sign) { + struct amdgpu_vm *vm = base->vm; + struct amdgpu_bo *bo = base->bo; + struct ttm_resource *res; + int64_t size; + uint32_t type; + bool shared; + + if (!vm || !bo) + return; + + size = sign * amdgpu_bo_size(bo); + res = new_res ? new_res : bo->tbo.resource; + type = res ? res->mem_type : amdgpu_bo_get_preferred_placement(bo);Same here. Don't use the preferred domain as fallback.+ shared = drm_gem_object_is_shared_for_memory_stats(&bo->tbo.base); + + if (type >= __AMDGPU_PL_LAST) + return; + + spin_lock(&vm->status_lock); + + if (shared) + vm->stats[type].drm.shared += size; + else + vm->stats[type].drm.private += size; + if (res) + vm->stats[type].drm.resident += size; + if (bo->flags & AMDGPU_GEM_CREATE_DISCARDABLE) + vm->stats[type].drm.purgeable += size; + + if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) { + vm->stats[TTM_PL_VRAM].requested += size; + if (type != TTM_PL_VRAM) + vm->stats[TTM_PL_VRAM].evicted += size;That check is probably not correct. We have BOs which can be placed in both VRAM and GTT.That is true, but does it make sense to count it towards evicted if say our picking order prefers VRAM over GTT?+ } else if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT) { + vm->stats[TTM_PL_TT].requested += size; + } + + spin_unlock(&vm->status_lock); +} + /** * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm * @@ -332,6 +421,7 @@ void amdgpu_vm_bo_base_init(structamdgpu_vm_bo_base *base,return; base->next = bo->vm_bo; bo->vm_bo = base; + amdgpu_vm_update_stats(base, NULL, +1); if (!amdgpu_vm_is_bo_always_valid(vm, bo)) return; @@ -1082,53 +1172,11 @@ int amdgpu_vm_update_range(structamdgpu_device *adev, struct amdgpu_vm *vm,return r; } -static void amdgpu_vm_bo_get_memory(struct amdgpu_bo_va *bo_va, - struct amdgpu_mem_stats *stats, - unsigned int size) -{ - struct amdgpu_vm *vm = bo_va->base.vm; - struct amdgpu_bo *bo = bo_va->base.bo; - - if (!bo) - return; - - /* - * For now ignore BOs which are currently locked and potentially - * changing their location. - */ - if (!amdgpu_vm_is_bo_always_valid(vm, bo) && - !dma_resv_trylock(bo->tbo.base.resv)) - return; - - amdgpu_bo_get_memory(bo, stats, size); - if (!amdgpu_vm_is_bo_always_valid(vm, bo)) - dma_resv_unlock(bo->tbo.base.resv); -} - void amdgpu_vm_get_memory(struct amdgpu_vm *vm, - struct amdgpu_mem_stats *stats, - unsigned int size) + struct amdgpu_mem_stats stats[__AMDGPU_PL_LAST]) { - struct amdgpu_bo_va *bo_va, *tmp; - spin_lock(&vm->status_lock); - list_for_each_entry_safe(bo_va, tmp, &vm->idle, base.vm_status) - amdgpu_vm_bo_get_memory(bo_va, stats, size); - - list_for_each_entry_safe(bo_va, tmp, &vm->evicted, base.vm_status) - amdgpu_vm_bo_get_memory(bo_va, stats, size); - - list_for_each_entry_safe(bo_va, tmp, &vm->relocated, base.vm_status) - amdgpu_vm_bo_get_memory(bo_va, stats, size); - - list_for_each_entry_safe(bo_va, tmp, &vm->moved, base.vm_status) - amdgpu_vm_bo_get_memory(bo_va, stats, size); - - list_for_each_entry_safe(bo_va, tmp, &vm->invalidated, base.vm_status) - amdgpu_vm_bo_get_memory(bo_va, stats, size); - - list_for_each_entry_safe(bo_va, tmp, &vm->done, base.vm_status) - amdgpu_vm_bo_get_memory(bo_va, stats, size); + memcpy(stats, vm->stats, sizeof(*stats) * __AMDGPU_PL_LAST); spin_unlock(&vm->status_lock); } @@ -2071,6 +2119,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device*adev,if (*base != &bo_va->base) continue; + amdgpu_vm_update_stats(*base, NULL, -1); *base = bo_va->base.next; break; } @@ -2136,6 +2185,22 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo) return true; } +/** + * amdgpu_vm_bo_update_shared - called when bo gets shared/unshared + * + * @bo: amdgpu buffer object + * @sign: if we should add (+1) or subtract (-1) the memory stat + * + * Update the per VM stats for all the vm */ void +amdgpu_vm_bo_update_shared(struct amdgpu_bo *bo, int sign) { + struct amdgpu_vm_bo_base *bo_base; + + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) + amdgpu_vm_update_shared(bo_base, sign); } + /** * amdgpu_vm_bo_invalidate - mark the bo as invalid * @@ -2169,6 +2234,28 @@ void amdgpu_vm_bo_invalidate(struct amdgpu_bo*bo, bool evicted)} } +/** + * amdgpu_vm_bo_move - handle BO move + * + * @bo: amdgpu buffer object + * @new_mem: the new placement of the BO move + * @evicted: is the BO evicted + * + * Update the memory stats for the new placement and mark @bo as invalid. + */ +void amdgpu_vm_bo_move(struct amdgpu_bo *bo, struct ttm_resource*new_mem,+ bool evicted) +{ + struct amdgpu_vm_bo_base *bo_base; + + for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { + amdgpu_vm_update_stats(bo_base, bo->tbo.resource, -1); + amdgpu_vm_update_stats(bo_base, new_mem, +1); + } + + amdgpu_vm_bo_invalidate(bo, evicted); } + /** * amdgpu_vm_get_block_size - calculate VM page table size as power of two * @@ -2585,6 +2672,18 @@ void amdgpu_vm_release_compute(structamdgpu_device *adev, struct amdgpu_vm *vm)vm->is_compute_context = false; } +static int amdgpu_vm_stats_is_zero(struct amdgpu_vm *vm) { + int is_zero = 1; + for (int i = 0; i < __AMDGPU_PL_LAST; ++i) { + is_zero = drm_memory_stats_is_zero(&vm->stats[i].drm) && + vm->stats->evicted == 0 && vm->stats->requested == 0; + if (!is_zero) + break;Just make that an "if (...) return false", no need for the local variable. Regards, Christian.D'oh! Teddy
