[PATCH v6 0/5] rework bo mem stats tracking

[PATCH v6 0/5] rework bo mem stats tracking

[Yunxiang Li]

Right now every time the fdinfo is read, we go through the vm lists and
lock all the BOs to calcuate the statistics. This causes a lot of lock
contention when the VM is actively used. It gets worse if there is a lot
of shared BOs or if there's a lot of submissions. We have seen
submissions lock-up for seconds due to fdinfo for some workload.
Therefore, rework the implementation to track the BOs as they get moved
around.

Also the visible memory stat is removed to simplify implementation, it's
unclear how useful this stat is since kernel map/unmap BOs whenever it
wants to and on a modern system all of VRAM can be mapped if needed.

v5: rebase on top of the drm_print_memory_stats refactor
v6: split the drm changes into a seperate patch for drm-devel review,
fix handling of drm-total- vs drm-resident- and handle drm-purgable-.

I'm currently stuck on drm-active-, I don't know where would be a good
place to add such info, especially how I could remove a BO's stat when
it's fence is signaled.

Yunxiang Li (5):
  drm/amdgpu: remove unused function parameter
  drm/amdgpu: make drm-memory-* report resident memory
  drm/amdgpu: stop tracking visible memory stats
  drm: add drm_memory_stats_is_zero
  drm/amdgpu: track bo memory stats at runtime

 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |   4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  16 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c  |  23 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c     |   3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 119 +++---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |  16 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |   2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 193 +++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  26 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |   1 +
 drivers/gpu/drm/drm_file.c                  |   9 +
 include/drm/drm_file.h                      |   1 +
 12 files changed, 230 insertions(+), 183 deletions(-)

-- 
2.34.1

[PATCH v6 1/5] drm/amdgpu: remove unused function parameter

[Yunxiang Li]

amdgpu_vm_bo_invalidate doesn't use the adev parameter and not all
callers have a reference to adev handy, so remove it for cleanliness.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      | 4 ++--
 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c     | 3 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 3 +--
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 4 +---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 3 +--
 6 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index d891ab779ca7f..471f3dc81e8db 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1105,7 +1105,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 	 * We can't use gang submit on with reserved VMIDs when the VM changes
 	 * can't be invalidated by more than one engine at the same time.
 	 */
-	if (p->gang_size > 1 && !p->adev->vm_manager.concurrent_flush) {
+	if (p->gang_size > 1 && !adev->vm_manager.concurrent_flush) {
 		for (i = 0; i < p->gang_size; ++i) {
 			struct drm_sched_entity *entity = p->entities[i];
 			struct drm_gpu_scheduler *sched = entity->rq->sched;
@@ -1189,7 +1189,7 @@ static int amdgpu_cs_vm_handling(struct amdgpu_cs_parser *p)
 			if (!bo)
 				continue;
 
-			amdgpu_vm_bo_invalidate(adev, bo, false);
+			amdgpu_vm_bo_invalidate(bo, false);
 		}
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 8e81a83d37d84..b144404902255 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -345,7 +345,7 @@ amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach)
 	/* FIXME: This should be after the "if", but needs a fix to make sure
 	 * DMABuf imports are initialized in the right VM list.
 	 */
-	amdgpu_vm_bo_invalidate(adev, bo, false);
+	amdgpu_vm_bo_invalidate(bo, false);
 	if (!bo->tbo.resource || bo->tbo.resource->mem_type == TTM_PL_SYSTEM)
 		return;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 1a5df8b946616..bc1ad6cdf0364 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -839,7 +839,6 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void *data,
 int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *filp)
 {
-	struct amdgpu_device *adev = drm_to_adev(dev);
 	struct drm_amdgpu_gem_op *args = data;
 	struct drm_gem_object *gobj;
 	struct amdgpu_vm_bo_base *base;
@@ -899,7 +898,7 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
 			robj->allowed_domains |= AMDGPU_GEM_DOMAIN_GTT;
 
 		if (robj->flags & AMDGPU_GEM_CREATE_VM_ALWAYS_VALID)
-			amdgpu_vm_bo_invalidate(adev, robj, true);
+			amdgpu_vm_bo_invalidate(robj, true);
 
 		amdgpu_bo_unreserve(robj);
 		break;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 1e6a044e3143b..045222b6bd049 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1148,7 +1148,6 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
 			   bool evict,
 			   struct ttm_resource *new_mem)
 {
-	struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
 	struct ttm_resource *old_mem = bo->resource;
 	struct amdgpu_bo *abo;
 
@@ -1156,7 +1155,7 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
 		return;
 
 	abo = ttm_to_amdgpu_bo(bo);
-	amdgpu_vm_bo_invalidate(adev, abo, evict);
+	amdgpu_vm_bo_invalidate(abo, evict);
 
 	amdgpu_bo_kunmap(abo);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 9d6ffe38b48ad..9fab64edd0530 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2139,14 +2139,12 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
 /**
  * amdgpu_vm_bo_invalidate - mark the bo as invalid
  *
- * @adev: amdgpu_device pointer
  * @bo: amdgpu buffer object
  * @evicted: is the BO evicted
  *
  * Mark @bo as invalid.
  */
-void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
-			     struct amdgpu_bo *bo, bool evicted)
+void amdgpu_vm_bo_invalidate(struct amdgpu_bo *bo, bool evicted)
 {
 	struct amdgpu_vm_bo_base *bo_base;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index c5b41e3ed14f2..428f7379bd759 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -515,8 +515,7 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			struct amdgpu_bo_va *bo_va,
 			bool clear);
 bool amdgpu_vm_evictable(struct amdgpu_bo *bo);
-void amdgpu_vm_bo_invalidate(struct amdgpu_device *adev,
-			     struct amdgpu_bo *bo, bool evicted);
+void amdgpu_vm_bo_invalidate(struct amdgpu_bo *bo, bool evicted);
 uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr);
 struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
 				       struct amdgpu_bo *bo);
-- 
2.34.1

[PATCH v6 2/5] drm/amdgpu: make drm-memory-* report resident memory

[Yunxiang Li]

The old behavior reports the resident memory usage for this key and the
documentation say so as well. However this was accidentally changed to
include buffers that was evicted.

Fixes: a2529f67e2ed ("drm/amdgpu: Use drm_print_memory_stats helper from fdinfo")
Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
Acked-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c | 7 ++++---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 1 -
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 1 -
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
index 00a4ab082459f..8281dd45faaa0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
@@ -33,6 +33,7 @@
 #include <drm/amdgpu_drm.h>
 #include <drm/drm_debugfs.h>
 #include <drm/drm_drv.h>
+#include <drm/drm_file.h>
 
 #include "amdgpu.h"
 #include "amdgpu_vm.h"
@@ -95,11 +96,11 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 
 	/* Legacy amdgpu keys, alias to drm-resident-memory-: */
 	drm_printf(p, "drm-memory-vram:\t%llu KiB\n",
-		   stats[TTM_PL_VRAM].total/1024UL);
+		   stats[TTM_PL_VRAM].drm.resident/1024UL);
 	drm_printf(p, "drm-memory-gtt: \t%llu KiB\n",
-		   stats[TTM_PL_TT].total/1024UL);
+		   stats[TTM_PL_TT].drm.resident/1024UL);
 	drm_printf(p, "drm-memory-cpu: \t%llu KiB\n",
-		   stats[TTM_PL_SYSTEM].total/1024UL);
+		   stats[TTM_PL_SYSTEM].drm.resident/1024UL);
 
 	/* Amdgpu specific memory accounting keys: */
 	drm_printf(p, "amd-memory-visible-vram:\t%llu KiB\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 045222b6bd049..2a53e72f3964f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1223,7 +1223,6 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 
 	/* DRM stats common fields: */
 
-	stats[type].total += size;
 	if (drm_gem_object_is_shared_for_memory_stats(obj))
 		stats[type].drm.shared += size;
 	else
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 7260349917ef0..a5653f474f85c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -142,7 +142,6 @@ struct amdgpu_bo_vm {
 struct amdgpu_mem_stats {
 	struct drm_memory_stats drm;
 
-	uint64_t total;
 	uint64_t visible;
 	uint64_t evicted;
 	uint64_t evicted_visible;
-- 
2.34.1

[PATCH v6 3/5] drm/amdgpu: stop tracking visible memory stats

[Yunxiang Li]

Since on modern systems all of vram can be made visible anyways, to
simplify the new implementation, drops tracking how much memory is
visible for now. If this is really needed we can add it back on top of
the new implementation.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  6 ------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 12 ++----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 10 ----------
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     | 11 ++++++++++-
 4 files changed, 12 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
index 8281dd45faaa0..7a9573958d87c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
@@ -103,16 +103,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 		   stats[TTM_PL_SYSTEM].drm.resident/1024UL);
 
 	/* Amdgpu specific memory accounting keys: */
-	drm_printf(p, "amd-memory-visible-vram:\t%llu KiB\n",
-		   stats[TTM_PL_VRAM].visible/1024UL);
 	drm_printf(p, "amd-evicted-vram:\t%llu KiB\n",
 		   stats[TTM_PL_VRAM].evicted/1024UL);
-	drm_printf(p, "amd-evicted-visible-vram:\t%llu KiB\n",
-		   stats[TTM_PL_VRAM].evicted_visible/1024UL);
 	drm_printf(p, "amd-requested-vram:\t%llu KiB\n",
 		   stats[TTM_PL_VRAM].requested/1024UL);
-	drm_printf(p, "amd-requested-visible-vram:\t%llu KiB\n",
-		   stats[TTM_PL_VRAM].requested_visible/1024UL);
 	drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
 		   stats[TTM_PL_TT].requested/1024UL);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 2a53e72f3964f..2436b7c9ad12b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -40,6 +40,7 @@
 #include "amdgpu_trace.h"
 #include "amdgpu_amdkfd.h"
 #include "amdgpu_vram_mgr.h"
+#include "amdgpu_vm.h"
 
 /**
  * DOC: amdgpu_object
@@ -1235,23 +1236,14 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 			stats[type].drm.active += size;
 		else if (bo->flags & AMDGPU_GEM_CREATE_DISCARDABLE)
 			stats[type].drm.purgeable += size;
-
-		if (type == TTM_PL_VRAM && amdgpu_res_cpu_visible(adev, res))
-			stats[type].visible += size;
 	}
 
 	/* amdgpu specific stats: */
 
 	if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) {
 		stats[TTM_PL_VRAM].requested += size;
-		if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
-			stats[TTM_PL_VRAM].requested_visible += size;
-
-		if (type != TTM_PL_VRAM) {
+		if (type != TTM_PL_VRAM)
 			stats[TTM_PL_VRAM].evicted += size;
-			if (bo->flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
-				stats[TTM_PL_VRAM].evicted_visible += size;
-		}
 	} else if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT) {
 		stats[TTM_PL_TT].requested += size;
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index a5653f474f85c..be6769852ece4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -139,16 +139,6 @@ struct amdgpu_bo_vm {
 	struct amdgpu_vm_bo_base        entries[];
 };
 
-struct amdgpu_mem_stats {
-	struct drm_memory_stats drm;
-
-	uint64_t visible;
-	uint64_t evicted;
-	uint64_t evicted_visible;
-	uint64_t requested;
-	uint64_t requested_visible;
-};
-
 static inline struct amdgpu_bo *ttm_to_amdgpu_bo(struct ttm_buffer_object *tbo)
 {
 	return container_of(tbo, struct amdgpu_bo, tbo);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 428f7379bd759..6a1b344e15e1b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -42,7 +42,6 @@ struct amdgpu_bo_va;
 struct amdgpu_job;
 struct amdgpu_bo_list_entry;
 struct amdgpu_bo_vm;
-struct amdgpu_mem_stats;
 
 /*
  * GPUVM handling
@@ -322,6 +321,16 @@ struct amdgpu_vm_fault_info {
 	unsigned int	vmhub;
 };
 
+struct amdgpu_mem_stats {
+	struct drm_memory_stats drm;
+
+	/* buffers that requested this placement */
+	uint64_t requested;
+	/* buffers that requested this placement
+	 * but are currently evicted */
+	uint64_t evicted;
+};
+
 struct amdgpu_vm {
 	/* tree of virtual addresses mapped */
 	struct rb_root_cached	va;
-- 
2.34.1

[PATCH v6 4/5] drm: add drm_memory_stats_is_zero

[Yunxiang Li]

Add a helper to check if the memory stats is zero, this will be used to
check for memory accounting errors.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
---
 drivers/gpu/drm/drm_file.c | 9 +++++++++
 include/drm/drm_file.h     | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 714e42b051080..75ed701d80f74 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -859,6 +859,15 @@ static void print_size(struct drm_printer *p, const char *stat,
 	drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
 }
 
+int drm_memory_stats_is_zero(const struct drm_memory_stats *stats) {
+	return (stats->shared == 0 &&
+		stats->private == 0 &&
+		stats->resident == 0 &&
+		stats->purgeable == 0 &&
+		stats->active == 0);
+}
+EXPORT_SYMBOL(drm_memory_stats_is_zero);
+
 /**
  * drm_print_memory_stats - A helper to print memory stats
  * @p: The printer to print output to
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index ab230d3af138d..7f91e35d027d9 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -477,6 +477,7 @@ struct drm_memory_stats {
 
 enum drm_gem_object_status;
 
+int drm_memory_stats_is_zero(const struct drm_memory_stats *stats);
 void drm_print_memory_stats(struct drm_printer *p,
 			    const struct drm_memory_stats *stats,
 			    enum drm_gem_object_status supported_status,
-- 
2.34.1

[PATCH v6 5/5] drm/amdgpu: track bo memory stats at runtime

[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@amd.com>
---
 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(struct dma_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);
+}
+
 /**
  * amdgpu_dma_buf_begin_cpu_access - &dma_buf_ops.begin_cpu_access implementation
  * @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(struct drm_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, struct drm_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, struct drm_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(struct ttm_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(struct amdgpu_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;
+	}
+}
+
 /**
  * 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"
 
 #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"
 
 #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(struct amdgpu_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);
+	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);
+	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;
+	} 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(struct amdgpu_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(struct amdgpu_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(struct amdgpu_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;
+	}
+	return is_zero;
+}
+
 /**
  * amdgpu_vm_fini - tear down a vm instance
  *
@@ -2656,6 +2755,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		}
 	}
 
+	if (!amdgpu_vm_stats_is_zero(vm))
+		dev_err(adev->dev, "VM memory stats is non-zero when fini\n");
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 6a1b344e15e1b..03589559641c4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -35,6 +35,7 @@
 #include "amdgpu_sync.h"
 #include "amdgpu_ring.h"
 #include "amdgpu_ids.h"
+#include "amdgpu_ttm.h"
 
 struct drm_exec;
 
@@ -345,6 +346,9 @@ struct amdgpu_vm {
 	/* Lock to protect vm_bo add/del/move on all lists of vm */
 	spinlock_t		status_lock;
 
+	/* Memory statistics for this vm, protected by the status_lock */
+	struct amdgpu_mem_stats stats[__AMDGPU_PL_LAST];
+
 	/* Per-VM and PT BOs who needs a validation */
 	struct list_head	evicted;
 
@@ -525,6 +529,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
 			bool clear);
 bool amdgpu_vm_evictable(struct amdgpu_bo *bo);
 void amdgpu_vm_bo_invalidate(struct amdgpu_bo *bo, bool evicted);
+void amdgpu_vm_update_stats(struct amdgpu_vm_bo_base *base,
+			    struct ttm_resource *new_res, int sign);
+void amdgpu_vm_bo_update_shared(struct amdgpu_bo *bo, int sign);
+void amdgpu_vm_bo_move(struct amdgpu_bo *bo, struct ttm_resource *new_mem,
+		       bool evicted);
 uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr);
 struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
 				       struct amdgpu_bo *bo);
@@ -575,8 +584,7 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
 void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
 				struct amdgpu_vm *vm);
 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]);
 
 int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 		       struct amdgpu_bo_vm *vmbo, bool immediate);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index f78a0434a48fa..384526d10a3bc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -537,6 +537,7 @@ static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry)
 	if (!entry->bo)
 		return;
 
+	amdgpu_vm_update_stats(entry, NULL, -1);
 	entry->bo->vm_bo = NULL;
 	ttm_bo_set_bulk_move(&entry->bo->tbo, NULL);
 
-- 
2.34.1

Re: [PATCH v6 4/5] drm: add drm_memory_stats_is_zero

[Christian König]

Am 25.10.24 um 19:41 schrieb Yunxiang Li:
> Add a helper to check if the memory stats is zero, this will be used to
> check for memory accounting errors.
>
> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

In theory I would need to upstream that through the drm-misc-next 
channel, but I think that's simply enough that we can take it through 
amd-staging-drm-next.

Regards,
Christian.

> ---
>   drivers/gpu/drm/drm_file.c | 9 +++++++++
>   include/drm/drm_file.h     | 1 +
>   2 files changed, 10 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 714e42b051080..75ed701d80f74 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -859,6 +859,15 @@ static void print_size(struct drm_printer *p, const char *stat,
>   	drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
>   }
>   
> +int drm_memory_stats_is_zero(const struct drm_memory_stats *stats) {
> +	return (stats->shared == 0 &&
> +		stats->private == 0 &&
> +		stats->resident == 0 &&
> +		stats->purgeable == 0 &&
> +		stats->active == 0);
> +}
> +EXPORT_SYMBOL(drm_memory_stats_is_zero);
> +
>   /**
>    * drm_print_memory_stats - A helper to print memory stats
>    * @p: The printer to print output to
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index ab230d3af138d..7f91e35d027d9 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -477,6 +477,7 @@ struct drm_memory_stats {
>   
>   enum drm_gem_object_status;
>   
> +int drm_memory_stats_is_zero(const struct drm_memory_stats *stats);
>   void drm_print_memory_stats(struct drm_printer *p,
>   			    const struct drm_memory_stats *stats,
>   			    enum drm_gem_object_status supported_status,

Re: [PATCH v6 5/5] drm/amdgpu: track bo memory stats at runtime

[Christian König]

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@amd.com>
> ---
>   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(struct dma_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.

> +}
> +
>   /**
>    * amdgpu_dma_buf_begin_cpu_access - &dma_buf_ops.begin_cpu_access implementation
>    * @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(struct drm_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, struct drm_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, struct drm_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(struct ttm_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(struct amdgpu_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.

> +	}
> +}
> +
>   /**
>    * 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?

>   
>   #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.

>   
>   #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(struct amdgpu_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.

> +	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.

> +	} 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(struct amdgpu_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(struct amdgpu_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(struct amdgpu_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.

> +	}
> +	return is_zero;
> +}
> +
>   /**
>    * amdgpu_vm_fini - tear down a vm instance
>    *
> @@ -2656,6 +2755,8 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   		}
>   	}
>   
> +	if (!amdgpu_vm_stats_is_zero(vm))
> +		dev_err(adev->dev, "VM memory stats is non-zero when fini\n");
>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 6a1b344e15e1b..03589559641c4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -35,6 +35,7 @@
>   #include "amdgpu_sync.h"
>   #include "amdgpu_ring.h"
>   #include "amdgpu_ids.h"
> +#include "amdgpu_ttm.h"
>   
>   struct drm_exec;
>   
> @@ -345,6 +346,9 @@ struct amdgpu_vm {
>   	/* Lock to protect vm_bo add/del/move on all lists of vm */
>   	spinlock_t		status_lock;
>   
> +	/* Memory statistics for this vm, protected by the status_lock */
> +	struct amdgpu_mem_stats stats[__AMDGPU_PL_LAST];
> +
>   	/* Per-VM and PT BOs who needs a validation */
>   	struct list_head	evicted;
>   
> @@ -525,6 +529,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev,
>   			bool clear);
>   bool amdgpu_vm_evictable(struct amdgpu_bo *bo);
>   void amdgpu_vm_bo_invalidate(struct amdgpu_bo *bo, bool evicted);
> +void amdgpu_vm_update_stats(struct amdgpu_vm_bo_base *base,
> +			    struct ttm_resource *new_res, int sign);
> +void amdgpu_vm_bo_update_shared(struct amdgpu_bo *bo, int sign);
> +void amdgpu_vm_bo_move(struct amdgpu_bo *bo, struct ttm_resource *new_mem,
> +		       bool evicted);
>   uint64_t amdgpu_vm_map_gart(const dma_addr_t *pages_addr, uint64_t addr);
>   struct amdgpu_bo_va *amdgpu_vm_bo_find(struct amdgpu_vm *vm,
>   				       struct amdgpu_bo *bo);
> @@ -575,8 +584,7 @@ void amdgpu_vm_set_task_info(struct amdgpu_vm *vm);
>   void amdgpu_vm_move_to_lru_tail(struct amdgpu_device *adev,
>   				struct amdgpu_vm *vm);
>   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]);
>   
>   int amdgpu_vm_pt_clear(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>   		       struct amdgpu_bo_vm *vmbo, bool immediate);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> index f78a0434a48fa..384526d10a3bc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> @@ -537,6 +537,7 @@ static void amdgpu_vm_pt_free(struct amdgpu_vm_bo_base *entry)
>   	if (!entry->bo)
>   		return;
>   
> +	amdgpu_vm_update_stats(entry, NULL, -1);
>   	entry->bo->vm_bo = NULL;
>   	ttm_bo_set_bulk_move(&entry->bo->tbo, NULL);
>   

RE: [PATCH v6 5/5] drm/amdgpu: track bo memory stats at runtime

[Li, Yunxiang (Teddy)]

[Public]

> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> 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@amd.com>
> > ---
> >   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(struct
> dma_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_access
> implementation
> >    * @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(struct
> drm_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, struct
> drm_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, struct
> drm_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(struct
> ttm_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(struct
> amdgpu_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(struct
> amdgpu_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.

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"

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...

> > +   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(struct
> amdgpu_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(struct
> amdgpu_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(struct
> amdgpu_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

Re: [PATCH v6 4/5] drm: add drm_memory_stats_is_zero

[Tvrtko Ursulin]

On 25/10/2024 18:41, Yunxiang Li wrote:
> Add a helper to check if the memory stats is zero, this will be used to
> check for memory accounting errors.
> 
> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> ---
>   drivers/gpu/drm/drm_file.c | 9 +++++++++
>   include/drm/drm_file.h     | 1 +
>   2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 714e42b051080..75ed701d80f74 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -859,6 +859,15 @@ static void print_size(struct drm_printer *p, const char *stat,
>   	drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
>   }
>   
> +int drm_memory_stats_is_zero(const struct drm_memory_stats *stats) {
> +	return (stats->shared == 0 &&
> +		stats->private == 0 &&
> +		stats->resident == 0 &&
> +		stats->purgeable == 0 &&
> +		stats->active == 0);
> +}

Could use mem_is_zero() for some value of source/binary compactness.

> +EXPORT_SYMBOL(drm_memory_stats_is_zero);
> +

I am not a huge fan of adding this as an interface as the only caller 
appears to be a sanity check in amdgpu_vm_fini():

	if (!amdgpu_vm_stats_is_zero(vm))
		dev_err(adev->dev, "VM memory stats is non-zero when fini\n");

But I guess there is some value in sanity checking since amdgpu does not 
have a notion of debug only code (compiled at production and exercised 
via a test suite).

I do suggest to demote the dev_err to notice log level would suffice and 
be more accurate.

Regards,

Tvrtko

>   /**
>    * drm_print_memory_stats - A helper to print memory stats
>    * @p: The printer to print output to
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index ab230d3af138d..7f91e35d027d9 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -477,6 +477,7 @@ struct drm_memory_stats {
>   
>   enum drm_gem_object_status;
>   
> +int drm_memory_stats_is_zero(const struct drm_memory_stats *stats);
>   void drm_print_memory_stats(struct drm_printer *p,
>   			    const struct drm_memory_stats *stats,
>   			    enum drm_gem_object_status supported_status,

Re: [PATCH v6 5/5] drm/amdgpu: track bo memory stats at runtime

[Tvrtko Ursulin]

On 31/10/2024 13:48, Li, Yunxiang (Teddy) wrote:
> [Public]
> 
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> 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@amd.com>
>>> ---
>>>    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(struct
>> dma_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_access
>> implementation
>>>     * @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(struct
>> drm_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, struct
>> drm_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, struct
>> drm_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(struct
>> ttm_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(struct
>> amdgpu_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(struct
>> amdgpu_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(struct
>> amdgpu_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(struct
>> amdgpu_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(struct
>> amdgpu_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

RE: [PATCH v6 4/5] drm: add drm_memory_stats_is_zero

[Li, Yunxiang (Teddy)]

[AMD Official Use Only - AMD Internal Distribution Only]

> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Sent: Thursday, November 7, 2024 5:41
> On 25/10/2024 18:41, Yunxiang Li wrote:
> > Add a helper to check if the memory stats is zero, this will be used
> > to check for memory accounting errors.
> >
> > Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> > ---
> >   drivers/gpu/drm/drm_file.c | 9 +++++++++
> >   include/drm/drm_file.h     | 1 +
> >   2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 714e42b051080..75ed701d80f74 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -859,6 +859,15 @@ static void print_size(struct drm_printer *p, const char
> *stat,
> >     drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
> >   }
> >
> > +int drm_memory_stats_is_zero(const struct drm_memory_stats *stats) {
> > +   return (stats->shared == 0 &&
> > +           stats->private == 0 &&
> > +           stats->resident == 0 &&
> > +           stats->purgeable == 0 &&
> > +           stats->active == 0);
> > +}
>
> Could use mem_is_zero() for some value of source/binary compactness.

Yeah, the patch set started out with that when it's just a function in amdgpu, but Christ didn't like it.

> > +EXPORT_SYMBOL(drm_memory_stats_is_zero);
> > +
>
> I am not a huge fan of adding this as an interface as the only caller appears to be a
> sanity check in amdgpu_vm_fini():
>
>       if (!amdgpu_vm_stats_is_zero(vm))
>               dev_err(adev->dev, "VM memory stats is non-zero when fini\n");
>
> But I guess there is some value in sanity checking since amdgpu does not have a
> notion of debug only code (compiled at production and exercised via a test suite).
>
> I do suggest to demote the dev_err to notice log level would suffice and be more
> accurate.

I think it's very important to have a check like this when we have a known invariant, especially in this case where there's stat tracking code spread out everywhere and we have very little chance of catching a bug right when it happened. And since whenever this check fails we know for sure there is a bug, I don't see the harm of keeping it as an error.

Now that I think about it, I probably want to have the process & task name in here to aid in reproduction.

Teddy

RE: [PATCH v6 5/5] drm/amdgpu: track bo memory stats at runtime

[Li, Yunxiang (Teddy)]

[Public]

> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Sent: Thursday, November 7, 2024 5:48
> On 31/10/2024 13:48, Li, Yunxiang (Teddy) wrote:
> > [Public]
> >
> >> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> >> 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@amd.com>
> >>> ---
> >>>    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(struct
> >> dma_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_access
> >> implementation
> >>>     * @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(struct
> >> drm_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,
> >>> struct
> >> drm_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, struct
> >> drm_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(struct
> >> ttm_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(struct
> >> amdgpu_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(struct
> >> amdgpu_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.

The two differ in how "evicted" buffers are counted. In the first case the evicted buffers adds to the total of where they happens to be at, second case the evicted buffers counts towards their preferred placement's total. Since we already have drm-resident that takes eviction into account, there might be some value to have drm-total- track a orthogonal statistics, aka where buffers want to be at?

> > 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.

That would be fine with me, it's also a quite inflated metric since the kernel doesn't actually know which buffers are used in each submission so it just fence all of them.

>
> 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(struct
> >> amdgpu_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(struct
> >> amdgpu_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(struct
> >> amdgpu_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

Re: [PATCH v6 4/5] drm: add drm_memory_stats_is_zero

[Tvrtko Ursulin]

On 07/11/2024 14:17, Li, Yunxiang (Teddy) wrote:
> [AMD Official Use Only - AMD Internal Distribution Only]
> 
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Sent: Thursday, November 7, 2024 5:41
>> On 25/10/2024 18:41, Yunxiang Li wrote:
>>> Add a helper to check if the memory stats is zero, this will be used
>>> to check for memory accounting errors.
>>>
>>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
>>> ---
>>>    drivers/gpu/drm/drm_file.c | 9 +++++++++
>>>    include/drm/drm_file.h     | 1 +
>>>    2 files changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 714e42b051080..75ed701d80f74 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -859,6 +859,15 @@ static void print_size(struct drm_printer *p, const char
>> *stat,
>>>      drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
>>>    }
>>>
>>> +int drm_memory_stats_is_zero(const struct drm_memory_stats *stats) {
>>> +   return (stats->shared == 0 &&
>>> +           stats->private == 0 &&
>>> +           stats->resident == 0 &&
>>> +           stats->purgeable == 0 &&
>>> +           stats->active == 0);
>>> +}
>>
>> Could use mem_is_zero() for some value of source/binary compactness.
> 
> Yeah, the patch set started out with that when it's just a function in amdgpu, but Christ didn't like it.

Okay, I don't feel so strongly about the implementation details.

>>> +EXPORT_SYMBOL(drm_memory_stats_is_zero);
>>> +
>>
>> I am not a huge fan of adding this as an interface as the only caller appears to be a
>> sanity check in amdgpu_vm_fini():
>>
>>        if (!amdgpu_vm_stats_is_zero(vm))
>>                dev_err(adev->dev, "VM memory stats is non-zero when fini\n");
>>
>> But I guess there is some value in sanity checking since amdgpu does not have a
>> notion of debug only code (compiled at production and exercised via a test suite).
>>
>> I do suggest to demote the dev_err to notice log level would suffice and be more
>> accurate.
> 
> I think it's very important to have a check like this when we have a known invariant, especially in this case where there's stat tracking code spread out everywhere and we have very little chance of catching a bug right when it happened. And since whenever this check fails we know for sure there is a bug, I don't see the harm of keeping it as an error.
It would indeed be a programming error if it can happen, but from the 
point of view of a driver and system log I think a warning is actually 
right.

Regards,

Tvrtko

> 
> Now that I think about it, I probably want to have the process & task name in here to aid in reproduction.
> 
> Teddy

Re: [PATCH v6 4/5] drm: add drm_memory_stats_is_zero

[Christian König]

Am 07.11.24 um 15:43 schrieb Tvrtko Ursulin:
> On 07/11/2024 14:17, Li, Yunxiang (Teddy) wrote:
>> [AMD Official Use Only - AMD Internal Distribution Only]
>>
>>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>>> Sent: Thursday, November 7, 2024 5:41
>>> On 25/10/2024 18:41, Yunxiang Li wrote:
>>>> Add a helper to check if the memory stats is zero, this will be used
>>>> to check for memory accounting errors.
>>>>
>>>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_file.c | 9 +++++++++
>>>>    include/drm/drm_file.h     | 1 +
>>>>    2 files changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>> index 714e42b051080..75ed701d80f74 100644
>>>> --- a/drivers/gpu/drm/drm_file.c
>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>> @@ -859,6 +859,15 @@ static void print_size(struct drm_printer *p, 
>>>> const char
>>> *stat,
>>>>      drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, 
>>>> units[u]);
>>>>    }
>>>>
>>>> +int drm_memory_stats_is_zero(const struct drm_memory_stats *stats) {
>>>> +   return (stats->shared == 0 &&
>>>> +           stats->private == 0 &&
>>>> +           stats->resident == 0 &&
>>>> +           stats->purgeable == 0 &&
>>>> +           stats->active == 0);
>>>> +}
>>>
>>> Could use mem_is_zero() for some value of source/binary compactness.
>>
>> Yeah, the patch set started out with that when it's just a function 
>> in amdgpu, but Christ didn't like it.
>
> Okay, I don't feel so strongly about the implementation details.

mem_is_zero() just has the tendency to randomly fail when the compiler 
adds padding in between fields.

>>>> +EXPORT_SYMBOL(drm_memory_stats_is_zero);
>>>> +
>>>
>>> I am not a huge fan of adding this as an interface as the only 
>>> caller appears to be a
>>> sanity check in amdgpu_vm_fini():
>>>
>>>        if (!amdgpu_vm_stats_is_zero(vm))
>>>                dev_err(adev->dev, "VM memory stats is non-zero when 
>>> fini\n");
>>>
>>> But I guess there is some value in sanity checking since amdgpu does 
>>> not have a
>>> notion of debug only code (compiled at production and exercised via 
>>> a test suite).
>>>
>>> I do suggest to demote the dev_err to notice log level would suffice 
>>> and be more
>>> accurate.
>>
>> I think it's very important to have a check like this when we have a 
>> known invariant, especially in this case where there's stat tracking 
>> code spread out everywhere and we have very little chance of catching 
>> a bug right when it happened. And since whenever this check fails we 
>> know for sure there is a bug, I don't see the harm of keeping it as 
>> an error.
> It would indeed be a programming error if it can happen, but from the 
> point of view of a driver and system log I think a warning is actually 
> right.

Yeah agree, an error usually means you have either done something wrong 
or your data is corrupted because something bad happened (failed disk 
etc...).

The the stats are nonsense that is annoying but not fatal, so not really 
an error.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>
>> Now that I think about it, I probably want to have the process & task 
>> name in here to aid in reproduction.
>>
>> Teddy

Re: [PATCH v6 5/5] drm/amdgpu: track bo memory stats at runtime

[Tvrtko Ursulin]

On 07/11/2024 14:24, Li, Yunxiang (Teddy) wrote:
> [Public]
> 
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Sent: Thursday, November 7, 2024 5:48
>> On 31/10/2024 13:48, Li, Yunxiang (Teddy) wrote:
>>> [Public]
>>>
>>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>>> 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@amd.com>
>>>>> ---
>>>>>     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(struct
>>>> dma_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_access
>>>> implementation
>>>>>      * @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(struct
>>>> drm_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,
>>>>> struct
>>>> drm_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, struct
>>>> drm_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(struct
>>>> ttm_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(struct
>>>> amdgpu_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(struct
>>>> amdgpu_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.
> 
> The two differ in how "evicted" buffers are counted. In the first case the evicted buffers adds to the total of where they happens to be at, second case the evicted buffers counts towards their preferred placement's total. Since we already have drm-resident that takes eviction into account, there might be some value to have drm-total- track a orthogonal statistics, aka where buffers want to be at?

Oh I see. Hm... Both options appear can result with some confusing 
runtime behaviour so I am not sure TBH.

For me the main thing is that drm-total- is not shown as zero (in all 
regions) when there is no backing store but the BO exists, and that 
drm-resident- reflects the current placement. Don't think I currently 
have a strong opinion on the details of whether total follows placement, 
or sticks to the placement from the BO allowed/preferred mask.

>>> 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.
> 
> That would be fine with me, it's also a quite inflated metric since the kernel doesn't actually know which buffers are used in each submission so it just fence all of them.

Okay lets go with that, as a new patch in the series I guess, and see if 
someone spots a problem with that approach.

Regards,

Tvrtko

> 
>>
>> 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(struct
>>>> amdgpu_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(struct
>>>> amdgpu_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(struct
>>>> amdgpu_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