[PATCH v7 0/4] rework bo mem stats tracking

[PATCH v7 0/4] 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.

The amd-only 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-.
v7: make drm-active- optional


Yunxiang Li (4):
  drm: add drm_memory_stats_is_zero
  drm: make drm-active- stats optional
  drm/amdgpu: remove unused function parameter
  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 |  17 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c  |  10 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c     |   3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  99 +++-------
 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      | 197 +++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  18 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |   1 +
 drivers/gpu/drm/drm_file.c                  |  23 ++-
 drivers/gpu/drm/i915/i915_drm_client.c      |   1 +
 drivers/gpu/drm/xe/xe_drm_client.c          |   1 +
 include/drm/drm_file.h                      |   1 +
 include/drm/drm_gem.h                       |  14 +-
 15 files changed, 241 insertions(+), 155 deletions(-)

-- 
2.34.1

[PATCH v7 1/4] 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>
Reviewed-by: Christian König <christian.koenig@amd.com>
CC: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_file.c | 10 ++++++++++
 include/drm/drm_file.h     |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 714e42b051080..e285fcc28c59c 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -859,6 +859,16 @@ 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 v7 2/4] drm: make drm-active- stats optional

[Yunxiang Li]

Make drm-active- optional just like drm-resident- and drm-purgeable-.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
CC: dri-devel@lists.freedesktop.org
CC: intel-gfx@lists.freedesktop.org
CC: amd-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  1 +
 drivers/gpu/drm/drm_file.c                 | 13 +++++++------
 drivers/gpu/drm/i915/i915_drm_client.c     |  1 +
 drivers/gpu/drm/xe/xe_drm_client.c         |  1 +
 include/drm/drm_gem.h                      | 14 ++++++++------
 5 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
index df2cf5c339255..7717e3e4f05b5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
@@ -97,6 +97,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 
 		drm_print_memory_stats(p,
 				       &stats[i].drm,
+				       DRM_GEM_OBJECT_ACTIVE |
 				       DRM_GEM_OBJECT_RESIDENT |
 				       DRM_GEM_OBJECT_PURGEABLE,
 				       pl_name[i]);
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e285fcc28c59c..fd06671054723 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -884,7 +884,9 @@ void drm_print_memory_stats(struct drm_printer *p,
 {
 	print_size(p, "total", region, stats->private + stats->shared);
 	print_size(p, "shared", region, stats->shared);
-	print_size(p, "active", region, stats->active);
+
+	if (supported_status & DRM_GEM_OBJECT_ACTIVE)
+		print_size(p, "active", region, stats->active);
 
 	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
 		print_size(p, "resident", region, stats->resident);
@@ -917,15 +919,13 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
 
 		if (obj->funcs && obj->funcs->status) {
 			s = obj->funcs->status(obj);
-			supported_status = DRM_GEM_OBJECT_RESIDENT |
-					DRM_GEM_OBJECT_PURGEABLE;
+			supported_status |= s;
 		}
 
-		if (drm_gem_object_is_shared_for_memory_stats(obj)) {
+		if (drm_gem_object_is_shared_for_memory_stats(obj))
 			status.shared += obj->size;
-		} else {
+		else
 			status.private += obj->size;
-		}
 
 		if (s & DRM_GEM_OBJECT_RESIDENT) {
 			status.resident += add_size;
@@ -938,6 +938,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
 
 		if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
 			status.active += add_size;
+			supported_status |= DRM_GEM_OBJECT_ACTIVE;
 
 			/* If still active, don't count as purgeable: */
 			s &= ~DRM_GEM_OBJECT_PURGEABLE;
diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
index f586825054918..168d7375304bc 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.c
+++ b/drivers/gpu/drm/i915/i915_drm_client.c
@@ -102,6 +102,7 @@ static void show_meminfo(struct drm_printer *p, struct drm_file *file)
 	for_each_memory_region(mr, i915, id)
 		drm_print_memory_stats(p,
 				       &stats[id],
+				       DRM_GEM_OBJECT_ACTIVE |
 				       DRM_GEM_OBJECT_RESIDENT |
 				       DRM_GEM_OBJECT_PURGEABLE,
 				       mr->uabi_name);
diff --git a/drivers/gpu/drm/xe/xe_drm_client.c b/drivers/gpu/drm/xe/xe_drm_client.c
index 6a26923fa10e0..54941b4e850c4 100644
--- a/drivers/gpu/drm/xe/xe_drm_client.c
+++ b/drivers/gpu/drm/xe/xe_drm_client.c
@@ -229,6 +229,7 @@ static void show_meminfo(struct drm_printer *p, struct drm_file *file)
 		if (man) {
 			drm_print_memory_stats(p,
 					       &stats[mem_type],
+					       DRM_GEM_OBJECT_ACTIVE |
 					       DRM_GEM_OBJECT_RESIDENT |
 					       (mem_type != XE_PL_SYSTEM ? 0 :
 					       DRM_GEM_OBJECT_PURGEABLE),
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index bae4865b2101a..584ffdf5c2542 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -48,19 +48,21 @@ struct drm_gem_object;
  * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
  * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
  * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
+ * @DRM_GEM_OBJECT_ACTIVE: object is currently used by an active submission
  *
  * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
- * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
- * it still active or not resident, in which case drm_show_fdinfo() will not
+ * and drm_show_fdinfo().  Note that an object can report DRM_GEM_OBJECT_PURGEABLE
+ * and be active or not resident, in which case drm_show_fdinfo() will not
  * account for it as purgeable.  So drivers do not need to check if the buffer
- * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
- * as purgeable even while it is still busy on the GPU.. it does not _actually_
- * become puregeable until it becomes idle.  The status gem object func does
- * not need to consider this.)
+ * is idle and resident to return this bit, i.e. userspace can mark a buffer as
+ * purgeable even while it is still busy on the GPU. It whill not get reported
+ * in the puregeable stats until it becomes idle.  The status gem object func
+ * does not need to consider this.
  */
 enum drm_gem_object_status {
 	DRM_GEM_OBJECT_RESIDENT  = BIT(0),
 	DRM_GEM_OBJECT_PURGEABLE = BIT(1),
+	DRM_GEM_OBJECT_ACTIVE = BIT(2),
 };
 
 /**
-- 
2.34.1

Re: [PATCH v7 2/4] drm: make drm-active- stats optional

[Jani Nikula]

On Sun, 10 Nov 2024, Yunxiang Li <Yunxiang.Li@amd.com> wrote:
> Make drm-active- optional just like drm-resident- and drm-purgeable-.

Why? What does it mean?

This is what the commit message should answer.

>
> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> CC: dri-devel@lists.freedesktop.org
> CC: intel-gfx@lists.freedesktop.org
> CC: amd-gfx@lists.freedesktop.org
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  1 +
>  drivers/gpu/drm/drm_file.c                 | 13 +++++++------
>  drivers/gpu/drm/i915/i915_drm_client.c     |  1 +
>  drivers/gpu/drm/xe/xe_drm_client.c         |  1 +
>  include/drm/drm_gem.h                      | 14 ++++++++------
>  5 files changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> index df2cf5c339255..7717e3e4f05b5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> @@ -97,6 +97,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>  
>  		drm_print_memory_stats(p,
>  				       &stats[i].drm,
> +				       DRM_GEM_OBJECT_ACTIVE |
>  				       DRM_GEM_OBJECT_RESIDENT |
>  				       DRM_GEM_OBJECT_PURGEABLE,
>  				       pl_name[i]);
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index e285fcc28c59c..fd06671054723 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -884,7 +884,9 @@ void drm_print_memory_stats(struct drm_printer *p,
>  {
>  	print_size(p, "total", region, stats->private + stats->shared);
>  	print_size(p, "shared", region, stats->shared);
> -	print_size(p, "active", region, stats->active);
> +
> +	if (supported_status & DRM_GEM_OBJECT_ACTIVE)
> +		print_size(p, "active", region, stats->active);
>  
>  	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
>  		print_size(p, "resident", region, stats->resident);
> @@ -917,15 +919,13 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>  
>  		if (obj->funcs && obj->funcs->status) {
>  			s = obj->funcs->status(obj);
> -			supported_status = DRM_GEM_OBJECT_RESIDENT |
> -					DRM_GEM_OBJECT_PURGEABLE;
> +			supported_status |= s;
>  		}
>  
> -		if (drm_gem_object_is_shared_for_memory_stats(obj)) {
> +		if (drm_gem_object_is_shared_for_memory_stats(obj))
>  			status.shared += obj->size;
> -		} else {
> +		else
>  			status.private += obj->size;
> -		}
>  
>  		if (s & DRM_GEM_OBJECT_RESIDENT) {
>  			status.resident += add_size;
> @@ -938,6 +938,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>  
>  		if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
>  			status.active += add_size;
> +			supported_status |= DRM_GEM_OBJECT_ACTIVE;
>  
>  			/* If still active, don't count as purgeable: */
>  			s &= ~DRM_GEM_OBJECT_PURGEABLE;
> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> index f586825054918..168d7375304bc 100644
> --- a/drivers/gpu/drm/i915/i915_drm_client.c
> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> @@ -102,6 +102,7 @@ static void show_meminfo(struct drm_printer *p, struct drm_file *file)
>  	for_each_memory_region(mr, i915, id)
>  		drm_print_memory_stats(p,
>  				       &stats[id],
> +				       DRM_GEM_OBJECT_ACTIVE |
>  				       DRM_GEM_OBJECT_RESIDENT |
>  				       DRM_GEM_OBJECT_PURGEABLE,
>  				       mr->uabi_name);
> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c b/drivers/gpu/drm/xe/xe_drm_client.c
> index 6a26923fa10e0..54941b4e850c4 100644
> --- a/drivers/gpu/drm/xe/xe_drm_client.c
> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> @@ -229,6 +229,7 @@ static void show_meminfo(struct drm_printer *p, struct drm_file *file)
>  		if (man) {
>  			drm_print_memory_stats(p,
>  					       &stats[mem_type],
> +					       DRM_GEM_OBJECT_ACTIVE |
>  					       DRM_GEM_OBJECT_RESIDENT |
>  					       (mem_type != XE_PL_SYSTEM ? 0 :
>  					       DRM_GEM_OBJECT_PURGEABLE),
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index bae4865b2101a..584ffdf5c2542 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -48,19 +48,21 @@ struct drm_gem_object;
>   * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
>   * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
>   * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> + * @DRM_GEM_OBJECT_ACTIVE: object is currently used by an active submission
>   *
>   * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
> - * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
> - * it still active or not resident, in which case drm_show_fdinfo() will not
> + * and drm_show_fdinfo().  Note that an object can report DRM_GEM_OBJECT_PURGEABLE
> + * and be active or not resident, in which case drm_show_fdinfo() will not
>   * account for it as purgeable.  So drivers do not need to check if the buffer
> - * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> - * as purgeable even while it is still busy on the GPU.. it does not _actually_
> - * become puregeable until it becomes idle.  The status gem object func does
> - * not need to consider this.)
> + * is idle and resident to return this bit, i.e. userspace can mark a buffer as
> + * purgeable even while it is still busy on the GPU. It whill not get reported
> + * in the puregeable stats until it becomes idle.  The status gem object func
> + * does not need to consider this.
>   */
>  enum drm_gem_object_status {
>  	DRM_GEM_OBJECT_RESIDENT  = BIT(0),
>  	DRM_GEM_OBJECT_PURGEABLE = BIT(1),
> +	DRM_GEM_OBJECT_ACTIVE = BIT(2),
>  };
>  
>  /**

-- 
Jani Nikula, Intel

[PATCH v8 2/4] drm: make drm-active- stats optional

[Yunxiang Li]

When memory stats is generated fresh everytime by going though all the
BOs, their active information is quite easy to get. But if the stats are
tracked alongside BO's state changes this becomes harder since the job
scheduling part doesn't really deal with individual buffers.

Make drm-active- optional to enable amdgpu to switch to the second
method.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
CC: dri-devel@lists.freedesktop.org
CC: intel-gfx@lists.freedesktop.org
CC: amd-gfx@lists.freedesktop.org
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  1 +
 drivers/gpu/drm/drm_file.c                 | 13 +++++++------
 drivers/gpu/drm/i915/i915_drm_client.c     |  1 +
 drivers/gpu/drm/xe/xe_drm_client.c         |  1 +
 include/drm/drm_gem.h                      | 14 ++++++++------
 5 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
index df2cf5c339255..7717e3e4f05b5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
@@ -97,6 +97,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 
 		drm_print_memory_stats(p,
 				       &stats[i].drm,
+				       DRM_GEM_OBJECT_ACTIVE |
 				       DRM_GEM_OBJECT_RESIDENT |
 				       DRM_GEM_OBJECT_PURGEABLE,
 				       pl_name[i]);
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e285fcc28c59c..fd06671054723 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -884,7 +884,9 @@ void drm_print_memory_stats(struct drm_printer *p,
 {
 	print_size(p, "total", region, stats->private + stats->shared);
 	print_size(p, "shared", region, stats->shared);
-	print_size(p, "active", region, stats->active);
+
+	if (supported_status & DRM_GEM_OBJECT_ACTIVE)
+		print_size(p, "active", region, stats->active);
 
 	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
 		print_size(p, "resident", region, stats->resident);
@@ -917,15 +919,13 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
 
 		if (obj->funcs && obj->funcs->status) {
 			s = obj->funcs->status(obj);
-			supported_status = DRM_GEM_OBJECT_RESIDENT |
-					DRM_GEM_OBJECT_PURGEABLE;
+			supported_status |= s;
 		}
 
-		if (drm_gem_object_is_shared_for_memory_stats(obj)) {
+		if (drm_gem_object_is_shared_for_memory_stats(obj))
 			status.shared += obj->size;
-		} else {
+		else
 			status.private += obj->size;
-		}
 
 		if (s & DRM_GEM_OBJECT_RESIDENT) {
 			status.resident += add_size;
@@ -938,6 +938,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
 
 		if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
 			status.active += add_size;
+			supported_status |= DRM_GEM_OBJECT_ACTIVE;
 
 			/* If still active, don't count as purgeable: */
 			s &= ~DRM_GEM_OBJECT_PURGEABLE;
diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
index f586825054918..168d7375304bc 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.c
+++ b/drivers/gpu/drm/i915/i915_drm_client.c
@@ -102,6 +102,7 @@ static void show_meminfo(struct drm_printer *p, struct drm_file *file)
 	for_each_memory_region(mr, i915, id)
 		drm_print_memory_stats(p,
 				       &stats[id],
+				       DRM_GEM_OBJECT_ACTIVE |
 				       DRM_GEM_OBJECT_RESIDENT |
 				       DRM_GEM_OBJECT_PURGEABLE,
 				       mr->uabi_name);
diff --git a/drivers/gpu/drm/xe/xe_drm_client.c b/drivers/gpu/drm/xe/xe_drm_client.c
index 6a26923fa10e0..54941b4e850c4 100644
--- a/drivers/gpu/drm/xe/xe_drm_client.c
+++ b/drivers/gpu/drm/xe/xe_drm_client.c
@@ -229,6 +229,7 @@ static void show_meminfo(struct drm_printer *p, struct drm_file *file)
 		if (man) {
 			drm_print_memory_stats(p,
 					       &stats[mem_type],
+					       DRM_GEM_OBJECT_ACTIVE |
 					       DRM_GEM_OBJECT_RESIDENT |
 					       (mem_type != XE_PL_SYSTEM ? 0 :
 					       DRM_GEM_OBJECT_PURGEABLE),
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index bae4865b2101a..584ffdf5c2542 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -48,19 +48,21 @@ struct drm_gem_object;
  * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
  * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
  * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
+ * @DRM_GEM_OBJECT_ACTIVE: object is currently used by an active submission
  *
  * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
- * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
- * it still active or not resident, in which case drm_show_fdinfo() will not
+ * and drm_show_fdinfo().  Note that an object can report DRM_GEM_OBJECT_PURGEABLE
+ * and be active or not resident, in which case drm_show_fdinfo() will not
  * account for it as purgeable.  So drivers do not need to check if the buffer
- * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
- * as purgeable even while it is still busy on the GPU.. it does not _actually_
- * become puregeable until it becomes idle.  The status gem object func does
- * not need to consider this.)
+ * is idle and resident to return this bit, i.e. userspace can mark a buffer as
+ * purgeable even while it is still busy on the GPU. It whill not get reported
+ * in the puregeable stats until it becomes idle.  The status gem object func
+ * does not need to consider this.
  */
 enum drm_gem_object_status {
 	DRM_GEM_OBJECT_RESIDENT  = BIT(0),
 	DRM_GEM_OBJECT_PURGEABLE = BIT(1),
+	DRM_GEM_OBJECT_ACTIVE = BIT(2),
 };
 
 /**
-- 
2.34.1

Re: [PATCH v7 2/4] drm: make drm-active- stats optional

[Tvrtko Ursulin]

On 10/11/2024 15:41, Yunxiang Li wrote:
> Make drm-active- optional just like drm-resident- and drm-purgeable-.

As Jani has already commented the commit message needs some work.

> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> CC: dri-devel@lists.freedesktop.org
> CC: intel-gfx@lists.freedesktop.org
> CC: amd-gfx@lists.freedesktop.org
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  1 +
>   drivers/gpu/drm/drm_file.c                 | 13 +++++++------
>   drivers/gpu/drm/i915/i915_drm_client.c     |  1 +
>   drivers/gpu/drm/xe/xe_drm_client.c         |  1 +
>   include/drm/drm_gem.h                      | 14 ++++++++------
>   5 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> index df2cf5c339255..7717e3e4f05b5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> @@ -97,6 +97,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>   
>   		drm_print_memory_stats(p,
>   				       &stats[i].drm,
> +				       DRM_GEM_OBJECT_ACTIVE |
>   				       DRM_GEM_OBJECT_RESIDENT |
>   				       DRM_GEM_OBJECT_PURGEABLE,
>   				       pl_name[i]);
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index e285fcc28c59c..fd06671054723 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -884,7 +884,9 @@ void drm_print_memory_stats(struct drm_printer *p,
>   {
>   	print_size(p, "total", region, stats->private + stats->shared);
>   	print_size(p, "shared", region, stats->shared);
> -	print_size(p, "active", region, stats->active);
> +
> +	if (supported_status & DRM_GEM_OBJECT_ACTIVE)
> +		print_size(p, "active", region, stats->active);
>   
>   	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
>   		print_size(p, "resident", region, stats->resident);
> @@ -917,15 +919,13 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>   
>   		if (obj->funcs && obj->funcs->status) {
>   			s = obj->funcs->status(obj);
> -			supported_status = DRM_GEM_OBJECT_RESIDENT |
> -					DRM_GEM_OBJECT_PURGEABLE;
> +			supported_status |= s;

I think this is correct and I think I've raised that it should be like 
this when the code was originally added. I only don't remember what was 
the argument to keep it hardcoded, if there was any. Adding Rob in case 
he can remember.

>   		}
>   
> -		if (drm_gem_object_is_shared_for_memory_stats(obj)) {
> +		if (drm_gem_object_is_shared_for_memory_stats(obj))
>   			status.shared += obj->size;
> -		} else {
> +		else
>   			status.private += obj->size;
> -		}

Drive by cleanup, okay.

>   
>   		if (s & DRM_GEM_OBJECT_RESIDENT) {
>   			status.resident += add_size;
> @@ -938,6 +938,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>   
>   		if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
>   			status.active += add_size;
> +			supported_status |= DRM_GEM_OBJECT_ACTIVE;

I wonder what behaviour we should have here if the driver has reported 
DRM_GEM_OBJECT_ACTIVE via its status vfunc. Like should it be like this:

    if ((s & DRM_GEM_OBJECT_ACTIVE) ||
        !dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
	  ...

?

So if some driver starts reporting this flag _and_ is still calling 
drm_show_memory_stats(), it's version of activity tracking is used 
instead of the the dma_resv based test.

>   
>   			/* If still active, don't count as purgeable: */
>   			s &= ~DRM_GEM_OBJECT_PURGEABLE;
> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> index f586825054918..168d7375304bc 100644
> --- a/drivers/gpu/drm/i915/i915_drm_client.c
> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> @@ -102,6 +102,7 @@ static void show_meminfo(struct drm_printer *p, struct drm_file *file)
>   	for_each_memory_region(mr, i915, id)
>   		drm_print_memory_stats(p,
>   				       &stats[id],
> +				       DRM_GEM_OBJECT_ACTIVE |
>   				       DRM_GEM_OBJECT_RESIDENT |
>   				       DRM_GEM_OBJECT_PURGEABLE,
>   				       mr->uabi_name);
> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c b/drivers/gpu/drm/xe/xe_drm_client.c
> index 6a26923fa10e0..54941b4e850c4 100644
> --- a/drivers/gpu/drm/xe/xe_drm_client.c
> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> @@ -229,6 +229,7 @@ static void show_meminfo(struct drm_printer *p, struct drm_file *file)
>   		if (man) {
>   			drm_print_memory_stats(p,
>   					       &stats[mem_type],
> +					       DRM_GEM_OBJECT_ACTIVE |
>   					       DRM_GEM_OBJECT_RESIDENT |
>   					       (mem_type != XE_PL_SYSTEM ? 0 :
>   					       DRM_GEM_OBJECT_PURGEABLE),
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index bae4865b2101a..584ffdf5c2542 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -48,19 +48,21 @@ struct drm_gem_object;
>    * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
>    * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
>    * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> + * @DRM_GEM_OBJECT_ACTIVE: object is currently used by an active submission
>    *
>    * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
> - * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
> - * it still active or not resident, in which case drm_show_fdinfo() will not
> + * and drm_show_fdinfo().  Note that an object can report DRM_GEM_OBJECT_PURGEABLE
> + * and be active or not resident, in which case drm_show_fdinfo() will not
>    * account for it as purgeable.  So drivers do not need to check if the buffer
> - * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> - * as purgeable even while it is still busy on the GPU.. it does not _actually_
> - * become puregeable until it becomes idle.  The status gem object func does
> - * not need to consider this.)
> + * is idle and resident to return this bit, i.e. userspace can mark a buffer as
> + * purgeable even while it is still busy on the GPU. It whill not get reported

Good cleanup.

s/whill/will/

> + * in the puregeable stats until it becomes idle.  The status gem object func
> + * does not need to consider this.
>    */
>   enum drm_gem_object_status {
>   	DRM_GEM_OBJECT_RESIDENT  = BIT(0),
>   	DRM_GEM_OBJECT_PURGEABLE = BIT(1),
> +	DRM_GEM_OBJECT_ACTIVE = BIT(2),
>   };
>   
>   /**

Regards,

Tvrtko

RE: [PATCH v7 2/4] drm: make drm-active- stats optional

[Li, Yunxiang (Teddy)]

[Public]

> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Sent: Monday, November 11, 2024 5:30
> On 10/11/2024 15:41, Yunxiang Li wrote:
> > Make drm-active- optional just like drm-resident- and drm-purgeable-.
>
> As Jani has already commented the commit message needs some work.
>
> > Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> > CC: dri-devel@lists.freedesktop.org
> > CC: intel-gfx@lists.freedesktop.org
> > CC: amd-gfx@lists.freedesktop.org
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  1 +
> >   drivers/gpu/drm/drm_file.c                 | 13 +++++++------
> >   drivers/gpu/drm/i915/i915_drm_client.c     |  1 +
> >   drivers/gpu/drm/xe/xe_drm_client.c         |  1 +
> >   include/drm/drm_gem.h                      | 14 ++++++++------
> >   5 files changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > index df2cf5c339255..7717e3e4f05b5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > @@ -97,6 +97,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p,
> > struct drm_file *file)
> >
> >             drm_print_memory_stats(p,
> >                                    &stats[i].drm,
> > +                                  DRM_GEM_OBJECT_ACTIVE |
> >                                    DRM_GEM_OBJECT_RESIDENT |
> >                                    DRM_GEM_OBJECT_PURGEABLE,
> >                                    pl_name[i]);
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index e285fcc28c59c..fd06671054723 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -884,7 +884,9 @@ void drm_print_memory_stats(struct drm_printer *p,
> >   {
> >     print_size(p, "total", region, stats->private + stats->shared);
> >     print_size(p, "shared", region, stats->shared);
> > -   print_size(p, "active", region, stats->active);
> > +
> > +   if (supported_status & DRM_GEM_OBJECT_ACTIVE)
> > +           print_size(p, "active", region, stats->active);
> >
> >     if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> >             print_size(p, "resident", region, stats->resident); @@ -917,15
> > +919,13 @@ void drm_show_memory_stats(struct drm_printer *p, struct
> > drm_file *file)
> >
> >             if (obj->funcs && obj->funcs->status) {
> >                     s = obj->funcs->status(obj);
> > -                   supported_status = DRM_GEM_OBJECT_RESIDENT |
> > -                                   DRM_GEM_OBJECT_PURGEABLE;
> > +                   supported_status |= s;
>
> I think this is correct and I think I've raised that it should be like this when the code
> was originally added. I only don't remember what was the argument to keep it
> hardcoded, if there was any. Adding Rob in case he can remember.
>
> >             }
> >
> > -           if (drm_gem_object_is_shared_for_memory_stats(obj)) {
> > +           if (drm_gem_object_is_shared_for_memory_stats(obj))
> >                     status.shared += obj->size;
> > -           } else {
> > +           else
> >                     status.private += obj->size;
> > -           }
>
> Drive by cleanup, okay.
>
> >
> >             if (s & DRM_GEM_OBJECT_RESIDENT) {
> >                     status.resident += add_size;
> > @@ -938,6 +938,7 @@ void drm_show_memory_stats(struct drm_printer *p,
> > struct drm_file *file)
> >
> >             if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
> >                     status.active += add_size;
> > +                   supported_status |= DRM_GEM_OBJECT_ACTIVE;
>
> I wonder what behaviour we should have here if the driver has reported
> DRM_GEM_OBJECT_ACTIVE via its status vfunc. Like should it be like this:
>
>     if ((s & DRM_GEM_OBJECT_ACTIVE) ||
>         !dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
>         ...
>
> ?
>
> So if some driver starts reporting this flag _and_ is still calling
> drm_show_memory_stats(), it's version of activity tracking is used instead of the
> the dma_resv based test.

I don't think that is feasible with the current API, because there's no way to differentiate "driver thinks a BO is not active" and "driver does not implement activity tracking". I think it's probably okay to keep it like this until someone wants to do it differently and refactor then.

Teddy

> >
> >                     /* If still active, don't count as purgeable: */
> >                     s &= ~DRM_GEM_OBJECT_PURGEABLE;
> > diff --git a/drivers/gpu/drm/i915/i915_drm_client.c
> > b/drivers/gpu/drm/i915/i915_drm_client.c
> > index f586825054918..168d7375304bc 100644
> > --- a/drivers/gpu/drm/i915/i915_drm_client.c
> > +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> > @@ -102,6 +102,7 @@ static void show_meminfo(struct drm_printer *p, struct
> drm_file *file)
> >     for_each_memory_region(mr, i915, id)
> >             drm_print_memory_stats(p,
> >                                    &stats[id],
> > +                                  DRM_GEM_OBJECT_ACTIVE |
> >                                    DRM_GEM_OBJECT_RESIDENT |
> >                                    DRM_GEM_OBJECT_PURGEABLE,
> >                                    mr->uabi_name);
> > diff --git a/drivers/gpu/drm/xe/xe_drm_client.c
> > b/drivers/gpu/drm/xe/xe_drm_client.c
> > index 6a26923fa10e0..54941b4e850c4 100644
> > --- a/drivers/gpu/drm/xe/xe_drm_client.c
> > +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> > @@ -229,6 +229,7 @@ static void show_meminfo(struct drm_printer *p, struct
> drm_file *file)
> >             if (man) {
> >                     drm_print_memory_stats(p,
> >                                            &stats[mem_type],
> > +                                          DRM_GEM_OBJECT_ACTIVE |
> >                                            DRM_GEM_OBJECT_RESIDENT |
> >                                            (mem_type != XE_PL_SYSTEM ? 0 :
> >                                            DRM_GEM_OBJECT_PURGEABLE),
> diff --git
> > a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
> > bae4865b2101a..584ffdf5c2542 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -48,19 +48,21 @@ struct drm_gem_object;
> >    * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> >    * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not
> unpinned)
> >    * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by
> > userspace
> > + * @DRM_GEM_OBJECT_ACTIVE: object is currently used by an active
> > + submission
> >    *
> >    * Bitmask of status used for fdinfo memory stats, see
> > &drm_gem_object_funcs.status
> > - * and drm_show_fdinfo().  Note that an object can
> > DRM_GEM_OBJECT_PURGEABLE if
> > - * it still active or not resident, in which case drm_show_fdinfo()
> > will not
> > + * and drm_show_fdinfo().  Note that an object can report
> > + DRM_GEM_OBJECT_PURGEABLE
> > + * and be active or not resident, in which case drm_show_fdinfo()
> > + will not
> >    * account for it as purgeable.  So drivers do not need to check if
> > the buffer
> > - * is idle and resident to return this bit.  (Ie. userspace can mark
> > a buffer
> > - * as purgeable even while it is still busy on the GPU.. it does not
> > _actually_
> > - * become puregeable until it becomes idle.  The status gem object
> > func does
> > - * not need to consider this.)
> > + * is idle and resident to return this bit, i.e. userspace can mark a
> > + buffer as
> > + * purgeable even while it is still busy on the GPU. It whill not get
> > + reported
>
> Good cleanup.
>
> s/whill/will/
>
> > + * in the puregeable stats until it becomes idle.  The status gem
> > + object func
> > + * does not need to consider this.
> >    */
> >   enum drm_gem_object_status {
> >     DRM_GEM_OBJECT_RESIDENT  = BIT(0),
> >     DRM_GEM_OBJECT_PURGEABLE = BIT(1),
> > +   DRM_GEM_OBJECT_ACTIVE = BIT(2),
> >   };
> >
> >   /**
>
> Regards,
>
> Tvrtko

Re: [PATCH v7 2/4] drm: make drm-active- stats optional

[Tvrtko Ursulin]

On 18/11/2024 15:17, Li, Yunxiang (Teddy) wrote:
> [Public]
> 
>> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Sent: Monday, November 11, 2024 5:30
>> On 10/11/2024 15:41, Yunxiang Li wrote:
>>> Make drm-active- optional just like drm-resident- and drm-purgeable-.
>>
>> As Jani has already commented the commit message needs some work.
>>
>>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
>>> CC: dri-devel@lists.freedesktop.org
>>> CC: intel-gfx@lists.freedesktop.org
>>> CC: amd-gfx@lists.freedesktop.org
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  1 +
>>>    drivers/gpu/drm/drm_file.c                 | 13 +++++++------
>>>    drivers/gpu/drm/i915/i915_drm_client.c     |  1 +
>>>    drivers/gpu/drm/xe/xe_drm_client.c         |  1 +
>>>    include/drm/drm_gem.h                      | 14 ++++++++------
>>>    5 files changed, 18 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>> index df2cf5c339255..7717e3e4f05b5 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>> @@ -97,6 +97,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p,
>>> struct drm_file *file)
>>>
>>>              drm_print_memory_stats(p,
>>>                                     &stats[i].drm,
>>> +                                  DRM_GEM_OBJECT_ACTIVE |
>>>                                     DRM_GEM_OBJECT_RESIDENT |
>>>                                     DRM_GEM_OBJECT_PURGEABLE,
>>>                                     pl_name[i]);
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index e285fcc28c59c..fd06671054723 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -884,7 +884,9 @@ void drm_print_memory_stats(struct drm_printer *p,
>>>    {
>>>      print_size(p, "total", region, stats->private + stats->shared);
>>>      print_size(p, "shared", region, stats->shared);
>>> -   print_size(p, "active", region, stats->active);
>>> +
>>> +   if (supported_status & DRM_GEM_OBJECT_ACTIVE)
>>> +           print_size(p, "active", region, stats->active);
>>>
>>>      if (supported_status & DRM_GEM_OBJECT_RESIDENT)
>>>              print_size(p, "resident", region, stats->resident); @@ -917,15
>>> +919,13 @@ void drm_show_memory_stats(struct drm_printer *p, struct
>>> drm_file *file)
>>>
>>>              if (obj->funcs && obj->funcs->status) {
>>>                      s = obj->funcs->status(obj);
>>> -                   supported_status = DRM_GEM_OBJECT_RESIDENT |
>>> -                                   DRM_GEM_OBJECT_PURGEABLE;
>>> +                   supported_status |= s;
>>
>> I think this is correct and I think I've raised that it should be like this when the code
>> was originally added. I only don't remember what was the argument to keep it
>> hardcoded, if there was any. Adding Rob in case he can remember.
>>
>>>              }
>>>
>>> -           if (drm_gem_object_is_shared_for_memory_stats(obj)) {
>>> +           if (drm_gem_object_is_shared_for_memory_stats(obj))
>>>                      status.shared += obj->size;
>>> -           } else {
>>> +           else
>>>                      status.private += obj->size;
>>> -           }
>>
>> Drive by cleanup, okay.
>>
>>>
>>>              if (s & DRM_GEM_OBJECT_RESIDENT) {
>>>                      status.resident += add_size;
>>> @@ -938,6 +938,7 @@ void drm_show_memory_stats(struct drm_printer *p,
>>> struct drm_file *file)
>>>
>>>              if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
>>>                      status.active += add_size;
>>> +                   supported_status |= DRM_GEM_OBJECT_ACTIVE;
>>
>> I wonder what behaviour we should have here if the driver has reported
>> DRM_GEM_OBJECT_ACTIVE via its status vfunc. Like should it be like this:
>>
>>      if ((s & DRM_GEM_OBJECT_ACTIVE) ||
>>          !dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
>>          ...
>>
>> ?
>>
>> So if some driver starts reporting this flag _and_ is still calling
>> drm_show_memory_stats(), it's version of activity tracking is used instead of the
>> the dma_resv based test.
> 
> I don't think that is feasible with the current API, because there's no way to differentiate "driver thinks a BO is not active" and "driver does not implement activity tracking". I think it's probably okay to keep it like this until someone wants to do it differently and refactor then.

Ah yes, good point. I actually initially thought the same (that we would 
need additional "supports active reporting" flag) but then for some 
reason convinced myself it is possible without it. I agree it works as is.

Regards,

Tvrtko

> 
> Teddy
> 
>>>
>>>                      /* If still active, don't count as purgeable: */
>>>                      s &= ~DRM_GEM_OBJECT_PURGEABLE;
>>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c
>>> b/drivers/gpu/drm/i915/i915_drm_client.c
>>> index f586825054918..168d7375304bc 100644
>>> --- a/drivers/gpu/drm/i915/i915_drm_client.c
>>> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
>>> @@ -102,6 +102,7 @@ static void show_meminfo(struct drm_printer *p, struct
>> drm_file *file)
>>>      for_each_memory_region(mr, i915, id)
>>>              drm_print_memory_stats(p,
>>>                                     &stats[id],
>>> +                                  DRM_GEM_OBJECT_ACTIVE |
>>>                                     DRM_GEM_OBJECT_RESIDENT |
>>>                                     DRM_GEM_OBJECT_PURGEABLE,
>>>                                     mr->uabi_name);
>>> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c
>>> b/drivers/gpu/drm/xe/xe_drm_client.c
>>> index 6a26923fa10e0..54941b4e850c4 100644
>>> --- a/drivers/gpu/drm/xe/xe_drm_client.c
>>> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
>>> @@ -229,6 +229,7 @@ static void show_meminfo(struct drm_printer *p, struct
>> drm_file *file)
>>>              if (man) {
>>>                      drm_print_memory_stats(p,
>>>                                             &stats[mem_type],
>>> +                                          DRM_GEM_OBJECT_ACTIVE |
>>>                                             DRM_GEM_OBJECT_RESIDENT |
>>>                                             (mem_type != XE_PL_SYSTEM ? 0 :
>>>                                             DRM_GEM_OBJECT_PURGEABLE),
>> diff --git
>>> a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
>>> bae4865b2101a..584ffdf5c2542 100644
>>> --- a/include/drm/drm_gem.h
>>> +++ b/include/drm/drm_gem.h
>>> @@ -48,19 +48,21 @@ struct drm_gem_object;
>>>     * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
>>>     * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not
>> unpinned)
>>>     * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by
>>> userspace
>>> + * @DRM_GEM_OBJECT_ACTIVE: object is currently used by an active
>>> + submission
>>>     *
>>>     * Bitmask of status used for fdinfo memory stats, see
>>> &drm_gem_object_funcs.status
>>> - * and drm_show_fdinfo().  Note that an object can
>>> DRM_GEM_OBJECT_PURGEABLE if
>>> - * it still active or not resident, in which case drm_show_fdinfo()
>>> will not
>>> + * and drm_show_fdinfo().  Note that an object can report
>>> + DRM_GEM_OBJECT_PURGEABLE
>>> + * and be active or not resident, in which case drm_show_fdinfo()
>>> + will not
>>>     * account for it as purgeable.  So drivers do not need to check if
>>> the buffer
>>> - * is idle and resident to return this bit.  (Ie. userspace can mark
>>> a buffer
>>> - * as purgeable even while it is still busy on the GPU.. it does not
>>> _actually_
>>> - * become puregeable until it becomes idle.  The status gem object
>>> func does
>>> - * not need to consider this.)
>>> + * is idle and resident to return this bit, i.e. userspace can mark a
>>> + buffer as
>>> + * purgeable even while it is still busy on the GPU. It whill not get
>>> + reported
>>
>> Good cleanup.
>>
>> s/whill/will/
>>
>>> + * in the puregeable stats until it becomes idle.  The status gem
>>> + object func
>>> + * does not need to consider this.
>>>     */
>>>    enum drm_gem_object_status {
>>>      DRM_GEM_OBJECT_RESIDENT  = BIT(0),
>>>      DRM_GEM_OBJECT_PURGEABLE = BIT(1),
>>> +   DRM_GEM_OBJECT_ACTIVE = BIT(2),
>>>    };
>>>
>>>    /**
>>
>> Regards,
>>
>> Tvrtko

[PATCH v7 3/4] 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 13e049cbc9300..9bac5dd4cd1c2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -980,7 +980,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;
@@ -1040,7 +1039,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 b5f65ef1efcde..f0486519bee84 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1149,7 +1149,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;
 
@@ -1157,7 +1156,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 68dd4130b5ad9..d0db155a9ab7c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2143,14 +2143,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 5d119ac26c4fe..6a1b344e15e1b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -524,8 +524,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 v7 4/4] 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. With
this new approach however, we would no longer be able to track active
buffers.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
---
v7: fix style and other minor issues, the best definition of drm-total-
is still an open question but that feel like a seperate patch series
since it would also affect other drivers

 drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  15 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c  |  11 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  98 +++-------
 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      | 193 +++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  15 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |   1 +
 8 files changed, 207 insertions(+), 133 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..423cea4e571ee 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,14 @@ 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 +246,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 +274,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 7717e3e4f05b5..1a74d8d9dedb7 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",
@@ -74,13 +74,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);
 
 	/*
@@ -97,7 +91,6 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 
 		drm_print_memory_stats(p,
 				       &stats[i].drm,
-				       DRM_GEM_OBJECT_ACTIVE |
 				       DRM_GEM_OBJECT_RESIDENT |
 				       DRM_GEM_OBJECT_PURGEABLE,
 				       pl_name[i]);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index f0486519bee84..ec9b013a15d81 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,75 +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;
-	}
-
-	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
@@ -1452,6 +1383,33 @@ 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 (rounddown_pow_of_two(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 d0db155a9ab7c..032e672b1299f 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,92 @@ 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);
+	res = bo->tbo.resource;
+	type = res ? res->mem_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 +419,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 +1170,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);
 }
 
@@ -2075,6 +2121,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;
 		}
@@ -2140,6 +2187,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
  *
@@ -2173,6 +2236,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
  *
@@ -2589,6 +2674,16 @@ 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)
+{
+	for (int i = 0; i < __AMDGPU_PL_LAST; ++i) {
+		if (!(drm_memory_stats_is_zero(&vm->stats[i].drm) &&
+		      vm->stats->evicted == 0 && vm->stats->requested == 0))
+			return false;
+	}
+	return true;
+}
+
 /**
  * amdgpu_vm_fini - tear down a vm instance
  *
@@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 
 	root = amdgpu_bo_ref(vm->root.bo);
 	amdgpu_bo_reserve(root, true);
-	amdgpu_vm_put_task_info(vm->task_info);
 	amdgpu_vm_set_pasid(adev, vm, 0);
 	dma_fence_wait(vm->last_unlocked, false);
 	dma_fence_put(vm->last_unlocked);
@@ -2660,6 +2754,15 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 		}
 	}
 
+	if (!amdgpu_vm_stats_is_zero(vm)) {
+		struct amdgpu_task_info *ti = vm->task_info;
+
+		dev_warn(adev->dev,
+			 "VM memory stats for proc %s(%d) task %s(%d) is non-zero when fini\n",
+			 ti->process_name, ti->pid, ti->task_name, ti->tgid);
+	}
+
+	amdgpu_vm_put_task_info(vm->task_info);
 }
 
 /**
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 6a1b344e15e1b..30efe9c9c08ef 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;
 
@@ -327,7 +328,8 @@ struct amdgpu_mem_stats {
 	/* buffers that requested this placement */
 	uint64_t requested;
 	/* buffers that requested this placement
-	 * but are currently evicted */
+	 * but are currently evicted
+	 */
 	uint64_t evicted;
 };
 
@@ -345,6 +347,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 +530,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 +585,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 v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Christian König]

Am 10.11.24 um 16: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. With
> this new approach however, we would no longer be able to track active
> buffers.
>
> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> ---
> v7: fix style and other minor issues, the best definition of drm-total-
> is still an open question but that feel like a seperate patch series
> since it would also affect other drivers
>
>   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  15 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c  |  11 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  98 +++-------
>   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      | 193 +++++++++++++++-----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  15 +-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |   1 +
>   8 files changed, 207 insertions(+), 133 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..423cea4e571ee 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,14 @@ 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 +246,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 +274,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 7717e3e4f05b5..1a74d8d9dedb7 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",
> @@ -74,13 +74,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);
>   
>   	/*
> @@ -97,7 +91,6 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>   
>   		drm_print_memory_stats(p,
>   				       &stats[i].drm,
> -				       DRM_GEM_OBJECT_ACTIVE |
>   				       DRM_GEM_OBJECT_RESIDENT |
>   				       DRM_GEM_OBJECT_PURGEABLE,
>   				       pl_name[i]);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index f0486519bee84..ec9b013a15d81 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,75 +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;
> -	}
> -
> -	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
> @@ -1452,6 +1383,33 @@ 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 (rounddown_pow_of_two(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 d0db155a9ab7c..032e672b1299f 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,92 @@ 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);
> +	res = bo->tbo.resource;
> +	type = res ? res->mem_type : amdgpu_bo_get_preferred_placement(bo);

Again, it's a clear NAK from my side to do stuff like that.

When there isn't any backing store the BO should *not* be accounted to 
anything.

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

Please drop those checks.

> +
> +	size = sign * amdgpu_bo_size(bo);
> +	res = new_res ? new_res : bo->tbo.resource;

That is basically broken logic. What could be is that bo->tbo.resource 
is given as a parameter here, but we shouldn't have such logic inside 
the function.

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

Again that is incorrect. BOs can be created with VRAM|GTT as their 
placement.

If such a BO is placed into GTT that doesn't mean it is evicted.

> +	} 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 +419,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 +1170,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);
>   }
>   
> @@ -2075,6 +2121,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;
>   		}
> @@ -2140,6 +2187,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
>    *
> @@ -2173,6 +2236,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
>    *
> @@ -2589,6 +2674,16 @@ 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)
> +{
> +	for (int i = 0; i < __AMDGPU_PL_LAST; ++i) {
> +		if (!(drm_memory_stats_is_zero(&vm->stats[i].drm) &&
> +		      vm->stats->evicted == 0 && vm->stats->requested == 0))
> +			return false;
> +	}
> +	return true;
> +}
> +
>   /**
>    * amdgpu_vm_fini - tear down a vm instance
>    *
> @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   
>   	root = amdgpu_bo_ref(vm->root.bo);
>   	amdgpu_bo_reserve(root, true);
> -	amdgpu_vm_put_task_info(vm->task_info);
>   	amdgpu_vm_set_pasid(adev, vm, 0);
>   	dma_fence_wait(vm->last_unlocked, false);
>   	dma_fence_put(vm->last_unlocked);
> @@ -2660,6 +2754,15 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   		}
>   	}
>   
> +	if (!amdgpu_vm_stats_is_zero(vm)) {
> +		struct amdgpu_task_info *ti = vm->task_info;
> +
> +		dev_warn(adev->dev,
> +			 "VM memory stats for proc %s(%d) task %s(%d) is non-zero when fini\n",
> +			 ti->process_name, ti->pid, ti->task_name, ti->tgid);
> +	}
> +
> +	amdgpu_vm_put_task_info(vm->task_info);

Please don't move the call to amdgpu_vm_put_task_info().



>   }
>   
>   /**
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 6a1b344e15e1b..30efe9c9c08ef 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;
>   
> @@ -327,7 +328,8 @@ struct amdgpu_mem_stats {
>   	/* buffers that requested this placement */
>   	uint64_t requested;
>   	/* buffers that requested this placement
> -	 * but are currently evicted */
> +	 * but are currently evicted
> +	 */
>   	uint64_t evicted;
>   };
>   
> @@ -345,6 +347,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 +530,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 +585,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 v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Li, Yunxiang (Teddy)]

[Public]

> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> Sent: Tuesday, November 12, 2024 5:54
> Am 10.11.24 um 16:41 schrieb Yunxiang Li:
> > @@ -310,6 +311,92 @@ 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);
> > +   res = bo->tbo.resource;
> > +   type = res ? res->mem_type : amdgpu_bo_get_preferred_placement(bo);
>
> Again, it's a clear NAK from my side to do stuff like that.
>
> When there isn't any backing store the BO should *not* be accounted to anything.

I don't have a preference either way, but I think it should be a separate discussion to properly define what drm-total- means.

> > +   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;
>
> Again that is incorrect. BOs can be created with VRAM|GTT as their placement.
>
> If such a BO is placed into GTT that doesn't mean it is evicted.

In that case, do we count BO with VRAM|GTT in both VRAM and GTT's .requested field? and if they are not in either, they go in both .evicted field?

> > @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
> > struct amdgpu_vm *vm)
> >
> >     root = amdgpu_bo_ref(vm->root.bo);
> >     amdgpu_bo_reserve(root, true);
> > -   amdgpu_vm_put_task_info(vm->task_info);
> >     amdgpu_vm_set_pasid(adev, vm, 0);
> >     dma_fence_wait(vm->last_unlocked, false);
> >     dma_fence_put(vm->last_unlocked);
> > @@ -2660,6 +2754,15 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
> struct amdgpu_vm *vm)
> >             }
> >     }
> >
> > +   if (!amdgpu_vm_stats_is_zero(vm)) {
> > +           struct amdgpu_task_info *ti = vm->task_info;
> > +
> > +           dev_warn(adev->dev,
> > +                    "VM memory stats for proc %s(%d) task %s(%d) is non-zero
> when fini\n",
> > +                    ti->process_name, ti->pid, ti->task_name, ti->tgid);
> > +   }
> > +
> > +   amdgpu_vm_put_task_info(vm->task_info);
>
> Please don't move the call to amdgpu_vm_put_task_info().

Is keeping the task_info alive a hazard here? I could copy out the info, it just seemed a bit wasteful.

Regards,
Teddy

Re: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Christian König]

Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
> [Public]
>
>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>> Sent: Tuesday, November 12, 2024 5:54
>> Am 10.11.24 um 16:41 schrieb Yunxiang Li:
>>> @@ -310,6 +311,92 @@ 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);
>>> +   res = bo->tbo.resource;
>>> +   type = res ? res->mem_type : amdgpu_bo_get_preferred_placement(bo);
>> Again, it's a clear NAK from my side to do stuff like that.
>>
>> When there isn't any backing store the BO should *not* be accounted to anything.
> I don't have a preference either way, but I think it should be a separate discussion to properly define what drm-total- means.
>
>>> +   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;
>> Again that is incorrect. BOs can be created with VRAM|GTT as their placement.
>>
>> If such a BO is placed into GTT that doesn't mean it is evicted.
> In that case, do we count BO with VRAM|GTT in both VRAM and GTT's .requested field? and if they are not in either, they go in both .evicted field?

Oh, good question depends on the definition of the requested field.

Accounting it to VRAM.evicted while GTT placement is desirable as well 
is certainly not correct.

 From my understanding they should go into the VRAM request, but not 
account to evicted. But Tvrtko might see that differently.

>
>>> @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
>>> struct amdgpu_vm *vm)
>>>
>>>      root = amdgpu_bo_ref(vm->root.bo);
>>>      amdgpu_bo_reserve(root, true);
>>> -   amdgpu_vm_put_task_info(vm->task_info);
>>>      amdgpu_vm_set_pasid(adev, vm, 0);
>>>      dma_fence_wait(vm->last_unlocked, false);
>>>      dma_fence_put(vm->last_unlocked);
>>> @@ -2660,6 +2754,15 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm)
>>>              }
>>>      }
>>>
>>> +   if (!amdgpu_vm_stats_is_zero(vm)) {
>>> +           struct amdgpu_task_info *ti = vm->task_info;
>>> +
>>> +           dev_warn(adev->dev,
>>> +                    "VM memory stats for proc %s(%d) task %s(%d) is non-zero
>> when fini\n",
>>> +                    ti->process_name, ti->pid, ti->task_name, ti->tgid);
>>> +   }
>>> +
>>> +   amdgpu_vm_put_task_info(vm->task_info);
>> Please don't move the call to amdgpu_vm_put_task_info().
> Is keeping the task_info alive a hazard here? I could copy out the info, it just seemed a bit wasteful.

Ah, now I see why you have moved that.

IIRC we need to free up the task info before releasing the PASID, but 
that info might be outdated. Need to check the code.

Does it work if you move the message further up or does the root PD then 
break your neck because it isn't released yet?

Thanks,
Christian.

>
> Regards,
> Teddy

Re: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Tvrtko Ursulin]

On 13/11/2024 08:49, Christian König wrote:
> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
>> [Public]
>>
>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>> Sent: Tuesday, November 12, 2024 5:54
>>> Am 10.11.24 um 16:41 schrieb Yunxiang Li:
>>>> @@ -310,6 +311,92 @@ 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);
>>>> +   res = bo->tbo.resource;
>>>> +   type = res ? res->mem_type : amdgpu_bo_get_preferred_placement(bo);
>>> Again, it's a clear NAK from my side to do stuff like that.
>>>
>>> When there isn't any backing store the BO should *not* be accounted 
>>> to anything.
>> I don't have a preference either way, but I think it should be a 
>> separate discussion to properly define what drm-total- means.

Total must show the total size of all BOs which exist even if they don't 
currently have a backing store. That's how drm-usage-stats.rst defines 
the field and that is how all the other drivers work.

>>>> +   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;
>>> Again that is incorrect. BOs can be created with VRAM|GTT as their 
>>> placement.
>>>
>>> If such a BO is placed into GTT that doesn't mean it is evicted.
>> In that case, do we count BO with VRAM|GTT in both VRAM and GTT's 
>> .requested field? and if they are not in either, they go in both 
>> .evicted field?
> 
> Oh, good question depends on the definition of the requested field.
> 
> Accounting it to VRAM.evicted while GTT placement is desirable as well 
> is certainly not correct.
> 
>  From my understanding they should go into the VRAM request, but not 
> account to evicted. But Tvrtko might see that differently.

Semantics of requested and evicted are kind of amdgpu 'legacy' thing. So 
the question is whether or not they should keep matching. Originally 
they were like this (I will edit out parts which deal with CPU visible 
for ease of comparison, and which have since been removed anyway):

        if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) {
                stats->requested_vram += size;
                if (res->mem_type != TTM_PL_VRAM)
                         stats->evicted_vram += size;
         } else if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT) {
                stats->requested_gtt += size;
         }

So the part about accounting as evicted with dual preferred placement 
was there from the start.

Then after my changes:

         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;
         }

I mostly kept the same semantics.

Teddy's version keeps it the same:

	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;
	}

If no AMD tools depend on the legacy semantics for evicted/requested we 
can change them. There is some overlap with the standard keys anyway and 
the fact preferred mask is unordered made the original behaviour a bit 
presumptuous to begin with. In summary I think it depends on whether we 
need to keep the legacy semantics, or even the keys themselves.

Regards,

Tvrtko

>>>> @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
>>>> struct amdgpu_vm *vm)
>>>>
>>>>      root = amdgpu_bo_ref(vm->root.bo);
>>>>      amdgpu_bo_reserve(root, true);
>>>> -   amdgpu_vm_put_task_info(vm->task_info);
>>>>      amdgpu_vm_set_pasid(adev, vm, 0);
>>>>      dma_fence_wait(vm->last_unlocked, false);
>>>>      dma_fence_put(vm->last_unlocked);
>>>> @@ -2660,6 +2754,15 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
>>> struct amdgpu_vm *vm)
>>>>              }
>>>>      }
>>>>
>>>> +   if (!amdgpu_vm_stats_is_zero(vm)) {
>>>> +           struct amdgpu_task_info *ti = vm->task_info;
>>>> +
>>>> +           dev_warn(adev->dev,
>>>> +                    "VM memory stats for proc %s(%d) task %s(%d) is 
>>>> non-zero
>>> when fini\n",
>>>> +                    ti->process_name, ti->pid, ti->task_name, 
>>>> ti->tgid);
>>>> +   }
>>>> +
>>>> +   amdgpu_vm_put_task_info(vm->task_info);
>>> Please don't move the call to amdgpu_vm_put_task_info().
>> Is keeping the task_info alive a hazard here? I could copy out the 
>> info, it just seemed a bit wasteful.
> 
> Ah, now I see why you have moved that.
> 
> IIRC we need to free up the task info before releasing the PASID, but 
> that info might be outdated. Need to check the code.
> 
> Does it work if you move the message further up or does the root PD then 
> break your neck because it isn't released yet?
> 
> Thanks,
> Christian.
> 
>>
>> Regards,
>> Teddy
> 

Re: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Christian König]

Am 13.11.24 um 11:25 schrieb Tvrtko Ursulin:
> On 13/11/2024 08:49, Christian König wrote:
>> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
>>> [SNIP]
>>>>> +   size = sign * amdgpu_bo_size(bo);
>>>>> +   res = bo->tbo.resource;
>>>>> +   type = res ? res->mem_type : 
>>>>> amdgpu_bo_get_preferred_placement(bo);
>>>> Again, it's a clear NAK from my side to do stuff like that.
>>>>
>>>> When there isn't any backing store the BO should *not* be accounted 
>>>> to anything.
>>> I don't have a preference either way, but I think it should be a 
>>> separate discussion to properly define what drm-total- means.
>
> Total must show the total size of all BOs which exist even if they 
> don't currently have a backing store. That's how drm-usage-stats.rst 
> defines the field and that is how all the other drivers work.

In that case we should only look at the preferred placement and not the 
backing store at all.

But that makes the total identical to the requested value, doesn't it?

>
>>>>> +   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;
>>>> Again that is incorrect. BOs can be created with VRAM|GTT as their 
>>>> placement.
>>>>
>>>> If such a BO is placed into GTT that doesn't mean it is evicted.
>>> In that case, do we count BO with VRAM|GTT in both VRAM and GTT's 
>>> .requested field? and if they are not in either, they go in both 
>>> .evicted field?
>>
>> Oh, good question depends on the definition of the requested field.
>>
>> Accounting it to VRAM.evicted while GTT placement is desirable as 
>> well is certainly not correct.
>>
>>  From my understanding they should go into the VRAM request, but not 
>> account to evicted. But Tvrtko might see that differently.
>
> Semantics of requested and evicted are kind of amdgpu 'legacy' thing. 
> So the question is whether or not they should keep matching. 
> Originally they were like this (I will edit out parts which deal with 
> CPU visible for ease of comparison, and which have since been removed 
> anyway):
>
>        if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) {
>                stats->requested_vram += size;
>                if (res->mem_type != TTM_PL_VRAM)
>                         stats->evicted_vram += size;
>         } else if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT) {
>                stats->requested_gtt += size;
>         }
>
> So the part about accounting as evicted with dual preferred placement 
> was there from the start.
>
> Then after my changes:
>
>         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;
>         }
>
> I mostly kept the same semantics.
>
> Teddy's version keeps it the same:
>
>     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;
>     }
>
> If no AMD tools depend on the legacy semantics for evicted/requested 
> we can change them. There is some overlap with the standard keys 
> anyway and the fact preferred mask is unordered made the original 
> behaviour a bit presumptuous to begin with. In summary I think it 
> depends on whether we need to keep the legacy semantics, or even the 
> keys themselves.

As far as I know we don't have any dependency on the amdgpu specific keys.

But I need to double check what drm-usage-stats.rst actually defines. 
Maybe that doesn't really match what we have in amdgpu and other TTM 
drivers as information.

Thanks,
Christian.



>
>
> Regards,
>
> Tvrtko
>
>>>>> @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
>>>>> struct amdgpu_vm *vm)
>>>>>
>>>>>      root = amdgpu_bo_ref(vm->root.bo);
>>>>>      amdgpu_bo_reserve(root, true);
>>>>> -   amdgpu_vm_put_task_info(vm->task_info);
>>>>>      amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>      dma_fence_wait(vm->last_unlocked, false);
>>>>>      dma_fence_put(vm->last_unlocked);
>>>>> @@ -2660,6 +2754,15 @@ void amdgpu_vm_fini(struct amdgpu_device 
>>>>> *adev,
>>>> struct amdgpu_vm *vm)
>>>>>              }
>>>>>      }
>>>>>
>>>>> +   if (!amdgpu_vm_stats_is_zero(vm)) {
>>>>> +           struct amdgpu_task_info *ti = vm->task_info;
>>>>> +
>>>>> +           dev_warn(adev->dev,
>>>>> +                    "VM memory stats for proc %s(%d) task %s(%d) 
>>>>> is non-zero
>>>> when fini\n",
>>>>> + ti->process_name, ti->pid, ti->task_name, ti->tgid);
>>>>> +   }
>>>>> +
>>>>> +   amdgpu_vm_put_task_info(vm->task_info);
>>>> Please don't move the call to amdgpu_vm_put_task_info().
>>> Is keeping the task_info alive a hazard here? I could copy out the 
>>> info, it just seemed a bit wasteful.
>>
>> Ah, now I see why you have moved that.
>>
>> IIRC we need to free up the task info before releasing the PASID, but 
>> that info might be outdated. Need to check the code.
>>
>> Does it work if you move the message further up or does the root PD 
>> then break your neck because it isn't released yet?
>>
>> Thanks,
>> Christian.
>>
>>>
>>> Regards,
>>> Teddy
>>

RE: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Li, Yunxiang (Teddy)]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, November 13, 2024 6:39
> Am 13.11.24 um 11:25 schrieb Tvrtko Ursulin:
> > On 13/11/2024 08:49, Christian König wrote:
> >> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
> >>> [SNIP]
> >>>>> +   size = sign * amdgpu_bo_size(bo);
> >>>>> +   res = bo->tbo.resource;
> >>>>> +   type = res ? res->mem_type :
> >>>>> amdgpu_bo_get_preferred_placement(bo);
> >>>> Again, it's a clear NAK from my side to do stuff like that.
> >>>>
> >>>> When there isn't any backing store the BO should *not* be accounted
> >>>> to anything.
> >>> I don't have a preference either way, but I think it should be a
> >>> separate discussion to properly define what drm-total- means.
> >
> > Total must show the total size of all BOs which exist even if they
> > don't currently have a backing store. That's how drm-usage-stats.rst
> > defines the field and that is how all the other drivers work.
>
> In that case we should only look at the preferred placement and not the backing
> store at all.
>
> But that makes the total identical to the requested value, doesn't it?

Yes, the issue is not which BO needs to be counted but where they should be counted. This gets more complicated if we consider BOs to prefer multiple placements.

IMO it makes sense to have drm-total- to work like the legacy amd-requested- where we look at BO's preferred placement. For multiple preferred placements we say that the implementation needs to pick one of them to avoid double counting, but which one is up to the implementation as long as it's done in a consistent manner. Does that sound reasonable?

> >
> >>>>> +   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;
> >>>> Again that is incorrect. BOs can be created with VRAM|GTT as their
> >>>> placement.
> >>>>
> >>>> If such a BO is placed into GTT that doesn't mean it is evicted.
> >>> In that case, do we count BO with VRAM|GTT in both VRAM and GTT's
> >>> .requested field? and if they are not in either, they go in both
> >>> .evicted field?
> >>
> >> Oh, good question depends on the definition of the requested field.
> >>
> >> Accounting it to VRAM.evicted while GTT placement is desirable as
> >> well is certainly not correct.
> >>
> >>  From my understanding they should go into the VRAM request, but not
> >> account to evicted. But Tvrtko might see that differently.
> >
> > Semantics of requested and evicted are kind of amdgpu 'legacy' thing.
> > So the question is whether or not they should keep matching.
> > Originally they were like this (I will edit out parts which deal with
> > CPU visible for ease of comparison, and which have since been removed
> > anyway):
> >
> >        if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) {
> >                stats->requested_vram += size;
> >                if (res->mem_type != TTM_PL_VRAM)
> >                         stats->evicted_vram += size;
> >         } else if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT) {
> >                stats->requested_gtt += size;
> >         }
> >
> > So the part about accounting as evicted with dual preferred placement
> > was there from the start.
> >
> > Then after my changes:
> >
> >         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;
> >         }
> >
> > I mostly kept the same semantics.
> >
> > Teddy's version keeps it the same:
> >
> >     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;
> >     }
> >
> > If no AMD tools depend on the legacy semantics for evicted/requested
> > we can change them. There is some overlap with the standard keys
> > anyway and the fact preferred mask is unordered made the original
> > behaviour a bit presumptuous to begin with. In summary I think it
> > depends on whether we need to keep the legacy semantics, or even the
> > keys themselves.
>
> As far as I know we don't have any dependency on the amdgpu specific keys.
>
> But I need to double check what drm-usage-stats.rst actually defines.
> Maybe that doesn't really match what we have in amdgpu and other TTM drivers as
> information.
>
> Thanks,
> Christian.

If we go with the above definition for drm-total-, I can change the amd-requested- fields to just mirror drm-total-, and have amd-evicted-vram check "preferred_domains & amdgpu_mem_type_to_domain(memtype) == 0", that should cover it?

Teddy

> >
> >
> > Regards,
> >
> > Tvrtko
> >
> >>>>> @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device
> >>>>> *adev, struct amdgpu_vm *vm)
> >>>>>
> >>>>>      root = amdgpu_bo_ref(vm->root.bo);
> >>>>>      amdgpu_bo_reserve(root, true);
> >>>>> -   amdgpu_vm_put_task_info(vm->task_info);
> >>>>>      amdgpu_vm_set_pasid(adev, vm, 0);
> >>>>>      dma_fence_wait(vm->last_unlocked, false);
> >>>>>      dma_fence_put(vm->last_unlocked); @@ -2660,6 +2754,15 @@ void
> >>>>> amdgpu_vm_fini(struct amdgpu_device *adev,
> >>>> struct amdgpu_vm *vm)
> >>>>>              }
> >>>>>      }
> >>>>>
> >>>>> +   if (!amdgpu_vm_stats_is_zero(vm)) {
> >>>>> +           struct amdgpu_task_info *ti = vm->task_info;
> >>>>> +
> >>>>> +           dev_warn(adev->dev,
> >>>>> +                    "VM memory stats for proc %s(%d) task %s(%d)
> >>>>> is non-zero
> >>>> when fini\n",
> >>>>> + ti->process_name, ti->pid, ti->task_name, ti->tgid);
> >>>>> +   }
> >>>>> +
> >>>>> +   amdgpu_vm_put_task_info(vm->task_info);
> >>>> Please don't move the call to amdgpu_vm_put_task_info().
> >>> Is keeping the task_info alive a hazard here? I could copy out the
> >>> info, it just seemed a bit wasteful.
> >>
> >> Ah, now I see why you have moved that.
> >>
> >> IIRC we need to free up the task info before releasing the PASID, but
> >> that info might be outdated. Need to check the code.
> >>
> >> Does it work if you move the message further up or does the root PD
> >> then break your neck because it isn't released yet?
> >>
> >> Thanks,
> >> Christian.
> >>
> >>>
> >>> Regards,
> >>> Teddy
> >>

Re: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Christian König]

Am 13.11.24 um 14:53 schrieb Li, Yunxiang (Teddy):
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, November 13, 2024 6:39
>> Am 13.11.24 um 11:25 schrieb Tvrtko Ursulin:
>>> On 13/11/2024 08:49, Christian König wrote:
>>>> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
>>>>> [SNIP]
>>>>>>> +   size = sign * amdgpu_bo_size(bo);
>>>>>>> +   res = bo->tbo.resource;
>>>>>>> +   type = res ? res->mem_type :
>>>>>>> amdgpu_bo_get_preferred_placement(bo);
>>>>>> Again, it's a clear NAK from my side to do stuff like that.
>>>>>>
>>>>>> When there isn't any backing store the BO should *not* be accounted
>>>>>> to anything.
>>>>> I don't have a preference either way, but I think it should be a
>>>>> separate discussion to properly define what drm-total- means.
>>> Total must show the total size of all BOs which exist even if they
>>> don't currently have a backing store. That's how drm-usage-stats.rst
>>> defines the field and that is how all the other drivers work.
>> In that case we should only look at the preferred placement and not the backing
>> store at all.
>>
>> But that makes the total identical to the requested value, doesn't it?
> Yes, the issue is not which BO needs to be counted but where they should be counted. This gets more complicated if we consider BOs to prefer multiple placements.
>
> IMO it makes sense to have drm-total- to work like the legacy amd-requested- where we look at BO's preferred placement. For multiple preferred placements we say that the implementation needs to pick one of them to avoid double counting, but which one is up to the implementation as long as it's done in a consistent manner. Does that sound reasonable?

Yeah that works for me. Just don't look at both bo->preferred_placement 
and bo->tbo.resource because that will certainly be inconsistent in some 
use cases.

>
>>>>>>> +   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;
>>>>>> Again that is incorrect. BOs can be created with VRAM|GTT as their
>>>>>> placement.
>>>>>>
>>>>>> If such a BO is placed into GTT that doesn't mean it is evicted.
>>>>> In that case, do we count BO with VRAM|GTT in both VRAM and GTT's
>>>>> .requested field? and if they are not in either, they go in both
>>>>> .evicted field?
>>>> Oh, good question depends on the definition of the requested field.
>>>>
>>>> Accounting it to VRAM.evicted while GTT placement is desirable as
>>>> well is certainly not correct.
>>>>
>>>>   From my understanding they should go into the VRAM request, but not
>>>> account to evicted. But Tvrtko might see that differently.
>>> Semantics of requested and evicted are kind of amdgpu 'legacy' thing.
>>> So the question is whether or not they should keep matching.
>>> Originally they were like this (I will edit out parts which deal with
>>> CPU visible for ease of comparison, and which have since been removed
>>> anyway):
>>>
>>>         if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM) {
>>>                 stats->requested_vram += size;
>>>                 if (res->mem_type != TTM_PL_VRAM)
>>>                          stats->evicted_vram += size;
>>>          } else if (bo->preferred_domains & AMDGPU_GEM_DOMAIN_GTT) {
>>>                 stats->requested_gtt += size;
>>>          }
>>>
>>> So the part about accounting as evicted with dual preferred placement
>>> was there from the start.
>>>
>>> Then after my changes:
>>>
>>>          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;
>>>          }
>>>
>>> I mostly kept the same semantics.
>>>
>>> Teddy's version keeps it the same:
>>>
>>>      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;
>>>      }
>>>
>>> If no AMD tools depend on the legacy semantics for evicted/requested
>>> we can change them. There is some overlap with the standard keys
>>> anyway and the fact preferred mask is unordered made the original
>>> behaviour a bit presumptuous to begin with. In summary I think it
>>> depends on whether we need to keep the legacy semantics, or even the
>>> keys themselves.
>> As far as I know we don't have any dependency on the amdgpu specific keys.
>>
>> But I need to double check what drm-usage-stats.rst actually defines.
>> Maybe that doesn't really match what we have in amdgpu and other TTM drivers as
>> information.
>>
>> Thanks,
>> Christian.
> If we go with the above definition for drm-total-, I can change the amd-requested- fields to just mirror drm-total-, and have amd-evicted-vram check "preferred_domains & amdgpu_mem_type_to_domain(memtype) == 0", that should cover it?

Yeah that should work, especially "preferred_domains & 
amdgpu_mem_type_to_domain(memtype) == 0" looks like a really good idea 
to me since that is generic.

Regards,
Christian.


>
> Teddy
>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>>>>> @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device
>>>>>>> *adev, struct amdgpu_vm *vm)
>>>>>>>
>>>>>>>       root = amdgpu_bo_ref(vm->root.bo);
>>>>>>>       amdgpu_bo_reserve(root, true);
>>>>>>> -   amdgpu_vm_put_task_info(vm->task_info);
>>>>>>>       amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>>>       dma_fence_wait(vm->last_unlocked, false);
>>>>>>>       dma_fence_put(vm->last_unlocked); @@ -2660,6 +2754,15 @@ void
>>>>>>> amdgpu_vm_fini(struct amdgpu_device *adev,
>>>>>> struct amdgpu_vm *vm)
>>>>>>>               }
>>>>>>>       }
>>>>>>>
>>>>>>> +   if (!amdgpu_vm_stats_is_zero(vm)) {
>>>>>>> +           struct amdgpu_task_info *ti = vm->task_info;
>>>>>>> +
>>>>>>> +           dev_warn(adev->dev,
>>>>>>> +                    "VM memory stats for proc %s(%d) task %s(%d)
>>>>>>> is non-zero
>>>>>> when fini\n",
>>>>>>> + ti->process_name, ti->pid, ti->task_name, ti->tgid);
>>>>>>> +   }
>>>>>>> +
>>>>>>> +   amdgpu_vm_put_task_info(vm->task_info);
>>>>>> Please don't move the call to amdgpu_vm_put_task_info().
>>>>> Is keeping the task_info alive a hazard here? I could copy out the
>>>>> info, it just seemed a bit wasteful.
>>>> Ah, now I see why you have moved that.
>>>>
>>>> IIRC we need to free up the task info before releasing the PASID, but
>>>> that info might be outdated. Need to check the code.
>>>>
>>>> Does it work if you move the message further up or does the root PD
>>>> then break your neck because it isn't released yet?
>>>>
>>>> Thanks,
>>>> Christian.
>>>>
>>>>> Regards,
>>>>> Teddy

RE: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Li, Yunxiang (Teddy)]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, November 13, 2024 9:22
> Am 13.11.24 um 14:53 schrieb Li, Yunxiang (Teddy):
> > [Public]
> >
> >> From: Koenig, Christian <Christian.Koenig@amd.com>
> >> Sent: Wednesday, November 13, 2024 6:39 Am 13.11.24 um 11:25 schrieb
> >> Tvrtko Ursulin:
> >>> On 13/11/2024 08:49, Christian König wrote:
> >>>> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
> >>>>> [SNIP]
> >>>>>>> +   size = sign * amdgpu_bo_size(bo);
> >>>>>>> +   res = bo->tbo.resource;
> >>>>>>> +   type = res ? res->mem_type :
> >>>>>>> amdgpu_bo_get_preferred_placement(bo);
> >>>>>> Again, it's a clear NAK from my side to do stuff like that.
> >>>>>>
> >>>>>> When there isn't any backing store the BO should *not* be
> >>>>>> accounted to anything.
> >>>>> I don't have a preference either way, but I think it should be a
> >>>>> separate discussion to properly define what drm-total- means.
> >>> Total must show the total size of all BOs which exist even if they
> >>> don't currently have a backing store. That's how drm-usage-stats.rst
> >>> defines the field and that is how all the other drivers work.
> >> In that case we should only look at the preferred placement and not
> >> the backing store at all.
> >>
> >> But that makes the total identical to the requested value, doesn't it?
> > Yes, the issue is not which BO needs to be counted but where they should be
> counted. This gets more complicated if we consider BOs to prefer multiple
> placements.
> >
> > IMO it makes sense to have drm-total- to work like the legacy amd-requested-
> where we look at BO's preferred placement. For multiple preferred placements we
> say that the implementation needs to pick one of them to avoid double counting, but
> which one is up to the implementation as long as it's done in a consistent manner.
> Does that sound reasonable?
>
> Yeah that works for me. Just don't look at both bo->preferred_placement and bo-
> >tbo.resource because that will certainly be inconsistent in some use cases.

oof, from the commit message i915/xe is doing the exact opposite, BO gets counted in the totals for all the possible(preferred?) regions.

Teddy

Re: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Tvrtko Ursulin]

On 13/11/2024 17:01, Li, Yunxiang (Teddy) wrote:
> [Public]
> 
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, November 13, 2024 9:22
>> Am 13.11.24 um 14:53 schrieb Li, Yunxiang (Teddy):
>>> [Public]
>>>
>>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>>> Sent: Wednesday, November 13, 2024 6:39 Am 13.11.24 um 11:25 schrieb
>>>> Tvrtko Ursulin:
>>>>> On 13/11/2024 08:49, Christian König wrote:
>>>>>> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
>>>>>>> [SNIP]
>>>>>>>>> +   size = sign * amdgpu_bo_size(bo);
>>>>>>>>> +   res = bo->tbo.resource;
>>>>>>>>> +   type = res ? res->mem_type :
>>>>>>>>> amdgpu_bo_get_preferred_placement(bo);
>>>>>>>> Again, it's a clear NAK from my side to do stuff like that.
>>>>>>>>
>>>>>>>> When there isn't any backing store the BO should *not* be
>>>>>>>> accounted to anything.
>>>>>>> I don't have a preference either way, but I think it should be a
>>>>>>> separate discussion to properly define what drm-total- means.
>>>>> Total must show the total size of all BOs which exist even if they
>>>>> don't currently have a backing store. That's how drm-usage-stats.rst
>>>>> defines the field and that is how all the other drivers work.
>>>> In that case we should only look at the preferred placement and not
>>>> the backing store at all.
>>>>
>>>> But that makes the total identical to the requested value, doesn't it?
>>> Yes, the issue is not which BO needs to be counted but where they should be
>> counted. This gets more complicated if we consider BOs to prefer multiple
>> placements.
>>>
>>> IMO it makes sense to have drm-total- to work like the legacy amd-requested-
>> where we look at BO's preferred placement. For multiple preferred placements we
>> say that the implementation needs to pick one of them to avoid double counting, but
>> which one is up to the implementation as long as it's done in a consistent manner.
>> Does that sound reasonable?
>>
>> Yeah that works for me. Just don't look at both bo->preferred_placement and bo-
>>> tbo.resource because that will certainly be inconsistent in some use cases.
> 
> oof, from the commit message i915/xe is doing the exact opposite, BO gets counted in the totals for all the possible(preferred?) regions.

Which commit message? I was doing that early during i915 patch 
development but stopped in v2:

commit 968853033d8aa4dbb80fbafa6f5d9b6a0ea21272
Author: Tvrtko Ursulin <tursulin@ursulin.net>
Date:   Tue Nov 7 10:18:06 2023 +0000

     drm/i915: Implement fdinfo memory stats printing

     Use the newly added drm_print_memory_stats helper to show memory
     utilisation of our objects in drm/driver specific fdinfo output.

     To collect the stats we walk the per memory regions object lists
     and accumulate object size into the respective drm_memory_stats
     categories.

     v2:
      * Only account against the active region.

^^^ THIS ^^^

      * Use DMA_RESV_USAGE_BOOKKEEP when testing for active. (Tejas)

     v3:
      * Update commit text. (Aravind)
      * Update to use memory regions uabi names.

In code that would be here:

static void
obj_meminfo(struct drm_i915_gem_object *obj,
	    struct drm_memory_stats stats[INTEL_REGION_UNKNOWN])
{
	const enum intel_region_id id = obj->mm.region ?
					obj->mm.region->id : INTEL_REGION_SMEM;

So either active region or SMEM if no backing store. Maybe that should 
be improved too. Grr (to myself).

I don't see xe is counting total against all regions either, apart that 
maybe it has potential null ptr deref?

static void bo_meminfo(struct xe_bo *bo,
		       struct drm_memory_stats stats[TTM_NUM_MEM_TYPES])
{
	u64 sz = bo->size;
	u32 mem_type = bo->ttm.resource->mem_type;

Or is bo->ttm.resource always present in xe? Adding Matt according to 
git blame.

Regards,

Tvrtko

> 
> Teddy

Re: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Matthew Auld]

Hi,

On 13/11/2024 17:30, Tvrtko Ursulin wrote:
> 
> On 13/11/2024 17:01, Li, Yunxiang (Teddy) wrote:
>> [Public]
>>
>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>> Sent: Wednesday, November 13, 2024 9:22
>>> Am 13.11.24 um 14:53 schrieb Li, Yunxiang (Teddy):
>>>> [Public]
>>>>
>>>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>>>> Sent: Wednesday, November 13, 2024 6:39 Am 13.11.24 um 11:25 schrieb
>>>>> Tvrtko Ursulin:
>>>>>> On 13/11/2024 08:49, Christian König wrote:
>>>>>>> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
>>>>>>>> [SNIP]
>>>>>>>>>> +   size = sign * amdgpu_bo_size(bo);
>>>>>>>>>> +   res = bo->tbo.resource;
>>>>>>>>>> +   type = res ? res->mem_type :
>>>>>>>>>> amdgpu_bo_get_preferred_placement(bo);
>>>>>>>>> Again, it's a clear NAK from my side to do stuff like that.
>>>>>>>>>
>>>>>>>>> When there isn't any backing store the BO should *not* be
>>>>>>>>> accounted to anything.
>>>>>>>> I don't have a preference either way, but I think it should be a
>>>>>>>> separate discussion to properly define what drm-total- means.
>>>>>> Total must show the total size of all BOs which exist even if they
>>>>>> don't currently have a backing store. That's how drm-usage-stats.rst
>>>>>> defines the field and that is how all the other drivers work.
>>>>> In that case we should only look at the preferred placement and not
>>>>> the backing store at all.
>>>>>
>>>>> But that makes the total identical to the requested value, doesn't it?
>>>> Yes, the issue is not which BO needs to be counted but where they 
>>>> should be
>>> counted. This gets more complicated if we consider BOs to prefer 
>>> multiple
>>> placements.
>>>>
>>>> IMO it makes sense to have drm-total- to work like the legacy amd- 
>>>> requested-
>>> where we look at BO's preferred placement. For multiple preferred 
>>> placements we
>>> say that the implementation needs to pick one of them to avoid double 
>>> counting, but
>>> which one is up to the implementation as long as it's done in a 
>>> consistent manner.
>>> Does that sound reasonable?
>>>
>>> Yeah that works for me. Just don't look at both bo- 
>>> >preferred_placement and bo-
>>>> tbo.resource because that will certainly be inconsistent in some use 
>>>> cases.
>>
>> oof, from the commit message i915/xe is doing the exact opposite, BO 
>> gets counted in the totals for all the possible(preferred?) regions.
> 
> Which commit message? I was doing that early during i915 patch 
> development but stopped in v2:
> 
> commit 968853033d8aa4dbb80fbafa6f5d9b6a0ea21272
> Author: Tvrtko Ursulin <tursulin@ursulin.net>
> Date:   Tue Nov 7 10:18:06 2023 +0000
> 
>      drm/i915: Implement fdinfo memory stats printing
> 
>      Use the newly added drm_print_memory_stats helper to show memory
>      utilisation of our objects in drm/driver specific fdinfo output.
> 
>      To collect the stats we walk the per memory regions object lists
>      and accumulate object size into the respective drm_memory_stats
>      categories.
> 
>      v2:
>       * Only account against the active region.
> 
> ^^^ THIS ^^^
> 
>       * Use DMA_RESV_USAGE_BOOKKEEP when testing for active. (Tejas)
> 
>      v3:
>       * Update commit text. (Aravind)
>       * Update to use memory regions uabi names.
> 
> In code that would be here:
> 
> static void
> obj_meminfo(struct drm_i915_gem_object *obj,
>          struct drm_memory_stats stats[INTEL_REGION_UNKNOWN])
> {
>      const enum intel_region_id id = obj->mm.region ?
>                      obj->mm.region->id : INTEL_REGION_SMEM;
> 
> So either active region or SMEM if no backing store. Maybe that should 
> be improved too. Grr (to myself).
> 
> I don't see xe is counting total against all regions either, apart that 
> maybe it has potential null ptr deref?
> 
> static void bo_meminfo(struct xe_bo *bo,
>                 struct drm_memory_stats stats[TTM_NUM_MEM_TYPES])
> {
>      u64 sz = bo->size;
>      u32 mem_type = bo->ttm.resource->mem_type;
> 
> Or is bo->ttm.resource always present in xe? Adding Matt according to 
> git blame.

Right, we shouldn't currently have a way of seeing NULL resource here in 
xe, at least if we are holding the bo lock.

> 
> Regards,
> 
> Tvrtko
> 
>>
>> Teddy

RE: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Li, Yunxiang (Teddy)]

[AMD Official Use Only - AMD Internal Distribution Only]

> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> Sent: Wednesday, November 13, 2024 12:31
> On 13/11/2024 17:01, Li, Yunxiang (Teddy) wrote:
> > [Public]
> >
> >> From: Koenig, Christian <Christian.Koenig@amd.com>
> >> Sent: Wednesday, November 13, 2024 9:22 Am 13.11.24 um 14:53 schrieb
> >> Li, Yunxiang (Teddy):
> >>> [Public]
> >>>
> >>>> From: Koenig, Christian <Christian.Koenig@amd.com>
> >>>> Sent: Wednesday, November 13, 2024 6:39 Am 13.11.24 um 11:25
> >>>> schrieb Tvrtko Ursulin:
> >>>>> On 13/11/2024 08:49, Christian König wrote:
> >>>>>> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
> >>>>>>> [SNIP]
> >>>>>>>>> +   size = sign * amdgpu_bo_size(bo);
> >>>>>>>>> +   res = bo->tbo.resource;
> >>>>>>>>> +   type = res ? res->mem_type :
> >>>>>>>>> amdgpu_bo_get_preferred_placement(bo);
> >>>>>>>> Again, it's a clear NAK from my side to do stuff like that.
> >>>>>>>>
> >>>>>>>> When there isn't any backing store the BO should *not* be
> >>>>>>>> accounted to anything.
> >>>>>>> I don't have a preference either way, but I think it should be a
> >>>>>>> separate discussion to properly define what drm-total- means.
> >>>>> Total must show the total size of all BOs which exist even if they
> >>>>> don't currently have a backing store. That's how
> >>>>> drm-usage-stats.rst defines the field and that is how all the other drivers
> work.
> >>>> In that case we should only look at the preferred placement and not
> >>>> the backing store at all.
> >>>>
> >>>> But that makes the total identical to the requested value, doesn't it?
> >>> Yes, the issue is not which BO needs to be counted but where they
> >>> should be
> >> counted. This gets more complicated if we consider BOs to prefer
> >> multiple placements.
> >>>
> >>> IMO it makes sense to have drm-total- to work like the legacy
> >>> amd-requested-
> >> where we look at BO's preferred placement. For multiple preferred
> >> placements we say that the implementation needs to pick one of them
> >> to avoid double counting, but which one is up to the implementation as long as
> it's done in a consistent manner.
> >> Does that sound reasonable?
> >>
> >> Yeah that works for me. Just don't look at both
> >> bo->preferred_placement and bo-
> >>> tbo.resource because that will certainly be inconsistent in some use cases.
> >
> > oof, from the commit message i915/xe is doing the exact opposite, BO gets
> counted in the totals for all the possible(preferred?) regions.
>
> Which commit message? I was doing that early during i915 patch development but
> stopped in v2:
>
> commit 968853033d8aa4dbb80fbafa6f5d9b6a0ea21272
> Author: Tvrtko Ursulin <tursulin@ursulin.net>
> Date:   Tue Nov 7 10:18:06 2023 +0000
>
>      drm/i915: Implement fdinfo memory stats printing
>
>      Use the newly added drm_print_memory_stats helper to show memory
>      utilisation of our objects in drm/driver specific fdinfo output.
>
>      To collect the stats we walk the per memory regions object lists
>      and accumulate object size into the respective drm_memory_stats
>      categories.
>
>      v2:
>       * Only account against the active region.
>
> ^^^ THIS ^^^
>
>       * Use DMA_RESV_USAGE_BOOKKEEP when testing for active. (Tejas)
>
>      v3:
>       * Update commit text. (Aravind)
>       * Update to use memory regions uabi names.
>
> In code that would be here:
>
> static void
> obj_meminfo(struct drm_i915_gem_object *obj,
>           struct drm_memory_stats stats[INTEL_REGION_UNKNOWN]) {
>       const enum intel_region_id id = obj->mm.region ?
>                                       obj->mm.region->id :
> INTEL_REGION_SMEM;
>
> So either active region or SMEM if no backing store. Maybe that should be
> improved too. Grr (to myself).
>
> I don't see xe is counting total against all regions either, apart that maybe it has
> potential null ptr deref?
>
> static void bo_meminfo(struct xe_bo *bo,
>                      struct drm_memory_stats stats[TTM_NUM_MEM_TYPES]) {
>       u64 sz = bo->size;
>       u32 mem_type = bo->ttm.resource->mem_type;
>
> Or is bo->ttm.resource always present in xe? Adding Matt according to git blame.
>
> Regards,
>
> Tvrtko

Thanks for the background, yeah it was mentioned in the xe commit, and the i915 changes looked very similar so I thought I should probably mention here. Now I dug around the mailing list and seems there was some confusion and the wrong commit message got merged.
https://lists.freedesktop.org/archives/intel-xe/2023-August/010885.html

RE: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Li, Yunxiang (Teddy)]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, November 13, 2024 3:49
> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
> >> From: Christian König <ckoenig.leichtzumerken@gmail.com>
> >> Sent: Tuesday, November 12, 2024 5:54
> >> Am 10.11.24 um 16:41 schrieb Yunxiang Li:
> >>> @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device
> >>> *adev, struct amdgpu_vm *vm)
> >>>
> >>>      root = amdgpu_bo_ref(vm->root.bo);
> >>>      amdgpu_bo_reserve(root, true);
> >>> -   amdgpu_vm_put_task_info(vm->task_info);
> >>>      amdgpu_vm_set_pasid(adev, vm, 0);
> >>>      dma_fence_wait(vm->last_unlocked, false);
> >>>      dma_fence_put(vm->last_unlocked); @@ -2660,6 +2754,15 @@ void
> >>> amdgpu_vm_fini(struct amdgpu_device *adev,
> >> struct amdgpu_vm *vm)
> >>>              }
> >>>      }
> >>>
> >>> +   if (!amdgpu_vm_stats_is_zero(vm)) {
> >>> +           struct amdgpu_task_info *ti = vm->task_info;
> >>> +
> >>> +           dev_warn(adev->dev,
> >>> +                    "VM memory stats for proc %s(%d) task %s(%d) is
> >>> + non-zero
> >> when fini\n",
> >>> +                    ti->process_name, ti->pid, ti->task_name, ti->tgid);
> >>> +   }
> >>> +
> >>> +   amdgpu_vm_put_task_info(vm->task_info);
> >> Please don't move the call to amdgpu_vm_put_task_info().
> > Is keeping the task_info alive a hazard here? I could copy out the info, it just
> seemed a bit wasteful.
>
> Ah, now I see why you have moved that.
>
> IIRC we need to free up the task info before releasing the PASID, but that info might
> be outdated. Need to check the code.
>
> Does it work if you move the message further up or does the root PD then break
> your neck because it isn't released yet?
>
> Thanks,
> Christian.

It needs to be after root BO is deleted. I think there's a way to go from pasid to task_info but not the other way around, so it should be safe? It's okay if the pasid/pid/etc gets recycled before we get here and we print outdated info since it's just so we know which application we should use to try to reproduce the bug.

Teddy

Re: [PATCH v7 4/4] drm/amdgpu: track bo memory stats at runtime

[Christian König]

Am 13.11.24 um 15:09 schrieb Li, Yunxiang (Teddy):
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, November 13, 2024 3:49
>> Am 12.11.24 um 19:16 schrieb Li, Yunxiang (Teddy):
>>>> From: Christian König <ckoenig.leichtzumerken@gmail.com>
>>>> Sent: Tuesday, November 12, 2024 5:54
>>>> Am 10.11.24 um 16:41 schrieb Yunxiang Li:
>>>>> @@ -2612,7 +2707,6 @@ void amdgpu_vm_fini(struct amdgpu_device
>>>>> *adev, struct amdgpu_vm *vm)
>>>>>
>>>>>       root = amdgpu_bo_ref(vm->root.bo);
>>>>>       amdgpu_bo_reserve(root, true);
>>>>> -   amdgpu_vm_put_task_info(vm->task_info);
>>>>>       amdgpu_vm_set_pasid(adev, vm, 0);
>>>>>       dma_fence_wait(vm->last_unlocked, false);
>>>>>       dma_fence_put(vm->last_unlocked); @@ -2660,6 +2754,15 @@ void
>>>>> amdgpu_vm_fini(struct amdgpu_device *adev,
>>>> struct amdgpu_vm *vm)
>>>>>               }
>>>>>       }
>>>>>
>>>>> +   if (!amdgpu_vm_stats_is_zero(vm)) {
>>>>> +           struct amdgpu_task_info *ti = vm->task_info;
>>>>> +
>>>>> +           dev_warn(adev->dev,
>>>>> +                    "VM memory stats for proc %s(%d) task %s(%d) is
>>>>> + non-zero
>>>> when fini\n",
>>>>> +                    ti->process_name, ti->pid, ti->task_name, ti->tgid);
>>>>> +   }
>>>>> +
>>>>> +   amdgpu_vm_put_task_info(vm->task_info);
>>>> Please don't move the call to amdgpu_vm_put_task_info().
>>> Is keeping the task_info alive a hazard here? I could copy out the info, it just
>> seemed a bit wasteful.
>>
>> Ah, now I see why you have moved that.
>>
>> IIRC we need to free up the task info before releasing the PASID, but that info might
>> be outdated. Need to check the code.
>>
>> Does it work if you move the message further up or does the root PD then break
>> your neck because it isn't released yet?
>>
>> Thanks,
>> Christian.
> It needs to be after root BO is deleted. I think there's a way to go from pasid to task_info but not the other way around, so it should be safe? It's okay if the pasid/pid/etc gets recycled before we get here and we print outdated info since it's just so we know which application we should use to try to reproduce the bug.

Double checked that and the order is actually incorrect right now and 
your patch here is fixing it!

The call to amdgpu_vm_put_task_info() needs to be after the call to 
amdgpu_vm_set_pasid(adev, vm, 0) or otherwise fault handling might use a 
freed up task info.

So feel free to completely ignore my comment, you're actually fixing things.

Regards,
Christian.


>
> Teddy