[PATCH v10 0/6] rework bo mem stats tracking

[PATCH v10 0/6] 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
v8: clearify documentation, minor tweaks, and some bug fixes found
during testing
v9: documentation fix as suggested, no functional change
v10: change how gem objects shared via flink is counted, and fix a race
between fdinfo read and buffer move

Yunxiang Li (6):
  drm: add drm_memory_stats_is_zero
  drm: make drm-active- stats optional
  Documentation/gpu: Clarify drm memory stats definition
  drm: consider GEM object shared when it is exported
  drm/amdgpu: remove unused function parameter
  drm/amdgpu: track bo memory stats at runtime

 Documentation/gpu/drm-usage-stats.rst       |  54 +++---
 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_fdinfo.c  |  17 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c     |  10 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  | 111 ++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h  |   4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h     |   4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 196 +++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  26 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |   1 +
 drivers/gpu/drm/drm_file.c                  |  23 ++-
 drivers/gpu/drm/drm_gem.c                   |   3 +
 drivers/gpu/drm/drm_prime.c                 |   3 +
 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                       |  26 ++-
 18 files changed, 298 insertions(+), 189 deletions(-)

-- 
2.34.1

[PATCH v10 1/6] 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 v10 2/6] 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 with BO's state 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>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.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..da11c16e212aa 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 will 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

[PATCH v10 3/6] Documentation/gpu: Clarify drm memory stats definition

[Yunxiang Li]

Define how to handle buffers with multiple possible placement so we
don't get incompatible implementations. Callout the resident requirement
for drm-purgeable- explicitly. Remove the requirement for there to be
only drm-memory- or only drm-resident-, it's not what's implemented and
having both is better for back-compat. Also re-order the paragraphs to
flow better.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

CC: dri-devel@lists.freedesktop.org
---
 Documentation/gpu/drm-usage-stats.rst | 54 +++++++++++++--------------
 1 file changed, 27 insertions(+), 27 deletions(-)

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index ff964c707754a..19a5323d0e682 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -140,57 +140,57 @@ both.
 Memory
 ^^^^^^
 
-- drm-memory-<region>: <uint> [KiB|MiB]
-
-Each possible memory type which can be used to store buffer objects by the
-GPU in question shall be given a stable and unique name to be returned as the
-string here.
+Each possible memory type which can be used to store buffer objects by the GPU
+in question shall be given a stable and unique name to be used as the "<region>"
+string.
 
 The region name "memory" is reserved to refer to normal system memory.
 
-Value shall reflect the amount of storage currently consumed by the buffer
+The value shall reflect the amount of storage currently consumed by the buffer
 objects belong to this client, in the respective memory region.
 
 Default unit shall be bytes with optional unit specifiers of 'KiB' or 'MiB'
 indicating kibi- or mebi-bytes.
 
-This key is deprecated and is an alias for drm-resident-<region>. Only one of
-the two should be present in the output.
-
-- drm-shared-<region>: <uint> [KiB|MiB]
+- drm-total-<region>: <uint> [KiB|MiB]
 
-The total size of buffers that are shared with another file (e.g., have more
-than a single handle).
+The total size of all requested buffers, including both shared and private
+memory. The backing store for the buffers does not need to be currently
+instantiated to count under this category. To avoid double-counting, if a buffer
+has multiple regions where it can be allocated to, the implementation should
+consistently select a single region for accounting purposes.
 
-- drm-total-<region>: <uint> [KiB|MiB]
+- drm-shared-<region>: <uint> [KiB|MiB]
 
-The total size of all created buffers including shared and private memory. The
-backing store for the buffers does not have to be currently instantiated to be
-counted under this category.
+The total size of buffers that are shared with another file (i.e., have more
+than one handle). The same requirement to avoid double-counting that applies to
+drm-total-<region> also applies here.
 
 - drm-resident-<region>: <uint> [KiB|MiB]
 
-The total size of buffers that are resident (have their backing store present or
-instantiated) in the specified region.
+The total size of buffers that are resident (i.e., have their backing store
+present or instantiated) in the specified region.
+
+- drm-memory-<region>: <uint> [KiB|MiB]
 
-This is an alias for drm-memory-<region> and only one of the two should be
-present in the output.
+This key is deprecated and is only printed by amdgpu; it is an alias for
+drm-resident-<region>.
 
 - drm-purgeable-<region>: <uint> [KiB|MiB]
 
-The total size of buffers that are purgeable.
+The total size of buffers that are resident and purgeable.
 
-For example drivers which implement a form of 'madvise' like functionality can
-here count buffers which have instantiated backing store, but have been marked
-with an equivalent of MADV_DONTNEED.
+For example, drivers that implement functionality similar to 'madvise' can count
+buffers that have instantiated backing stores but have been marked with an
+equivalent of MADV_DONTNEED.
 
 - drm-active-<region>: <uint> [KiB|MiB]
 
 The total size of buffers that are active on one or more engines.
 
-One practical example of this can be presence of unsignaled fences in an GEM
-buffer reservation object. Therefore the active category is a subset of
-resident.
+One practical example of this could be the presence of unsignaled fences in a
+GEM buffer reservation object. Therefore, the active category is a subset of the
+resident category.
 
 Implementation Details
 ======================
-- 
2.34.1

[PATCH v10 4/6] drm: consider GEM object shared when it is exported

[Yunxiang Li]

Tracking the state of a GEM object for shared stats is quite difficult
since the handle_count is managed behind driver's back. So instead
considers GEM object shared the moment it is exported with flink ioctl.
This makes it work the same to the dma_buf case. Add a callback for
drivers to get notified when GEM object is being shared.

Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>

CC: dri-devel@lists.freedesktop.org
---
 drivers/gpu/drm/drm_gem.c   |  3 +++
 drivers/gpu/drm/drm_prime.c |  3 +++
 include/drm/drm_gem.h       | 12 +++++++++++-
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index d4bbc5d109c8b..1ead11de31f6b 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -854,6 +854,9 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
 			goto err;
 
 		obj->name = ret;
+
+		if (obj->funcs->shared)
+			obj->funcs->shared(obj);
 	}
 
 	args->name = (uint64_t) obj->name;
diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
index 0e3f8adf162f6..336d982d69807 100644
--- a/drivers/gpu/drm/drm_prime.c
+++ b/drivers/gpu/drm/drm_prime.c
@@ -406,6 +406,9 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
 	obj->dma_buf = dmabuf;
 	get_dma_buf(obj->dma_buf);
 
+	if (obj->funcs->shared)
+		obj->funcs->shared(obj);
+
 	return dmabuf;
 }
 
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index da11c16e212aa..8c5ffcd485752 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -122,6 +122,16 @@ struct drm_gem_object_funcs {
 	 */
 	struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
 
+	/**
+	 * @shared:
+	 *
+	 * Callback when GEM object becomes shared, see also
+	 * drm_gem_object_is_shared_for_memory_stats
+	 *
+	 * This callback is optional.
+	 */
+	void (*shared)(struct drm_gem_object *obj);
+
 	/**
 	 * @pin:
 	 *
@@ -568,7 +578,7 @@ int drm_gem_evict(struct drm_gem_object *obj);
  */
 static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_object *obj)
 {
-	return (obj->handle_count > 1) || obj->dma_buf;
+	return obj->name || obj->dma_buf;
 }
 
 #ifdef CONFIG_LOCKDEP
-- 
2.34.1

Re: [PATCH v10 4/6] drm: consider GEM object shared when it is exported

[Christian König]

Am 10.12.24 um 18:59 schrieb Yunxiang Li:
> Tracking the state of a GEM object for shared stats is quite difficult
> since the handle_count is managed behind driver's back. So instead
> considers GEM object shared the moment it is exported with flink ioctl.
> This makes it work the same to the dma_buf case. Add a callback for
> drivers to get notified when GEM object is being shared.

First of all GEM flink is pretty much deprecated, we only have it for 
compatibility reasons. So please don't change anything here.

Then flink is not the only way to create multiple handles for a GEM 
object. So this here won't handle all cases.

And finally we already have the .open and .close callbacks, which are 
called whenever a handle for a GEM object is created/destroyed. So it 
shouldn't be necessary in the first place.

Regards,
Christian.

>
> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
>
> CC: dri-devel@lists.freedesktop.org
> ---
>   drivers/gpu/drm/drm_gem.c   |  3 +++
>   drivers/gpu/drm/drm_prime.c |  3 +++
>   include/drm/drm_gem.h       | 12 +++++++++++-
>   3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index d4bbc5d109c8b..1ead11de31f6b 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -854,6 +854,9 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
>   			goto err;
>   
>   		obj->name = ret;
> +
> +		if (obj->funcs->shared)
> +			obj->funcs->shared(obj);
>   	}
>   
>   	args->name = (uint64_t) obj->name;
> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> index 0e3f8adf162f6..336d982d69807 100644
> --- a/drivers/gpu/drm/drm_prime.c
> +++ b/drivers/gpu/drm/drm_prime.c
> @@ -406,6 +406,9 @@ static struct dma_buf *export_and_register_object(struct drm_device *dev,
>   	obj->dma_buf = dmabuf;
>   	get_dma_buf(obj->dma_buf);
>   
> +	if (obj->funcs->shared)
> +		obj->funcs->shared(obj);
> +
>   	return dmabuf;
>   }
>   
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index da11c16e212aa..8c5ffcd485752 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -122,6 +122,16 @@ struct drm_gem_object_funcs {
>   	 */
>   	struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
>   
> +	/**
> +	 * @shared:
> +	 *
> +	 * Callback when GEM object becomes shared, see also
> +	 * drm_gem_object_is_shared_for_memory_stats
> +	 *
> +	 * This callback is optional.
> +	 */
> +	void (*shared)(struct drm_gem_object *obj);
> +
>   	/**
>   	 * @pin:
>   	 *
> @@ -568,7 +578,7 @@ int drm_gem_evict(struct drm_gem_object *obj);
>    */
>   static inline bool drm_gem_object_is_shared_for_memory_stats(struct drm_gem_object *obj)
>   {
> -	return (obj->handle_count > 1) || obj->dma_buf;
> +	return obj->name || obj->dma_buf;
>   }
>   
>   #ifdef CONFIG_LOCKDEP

RE: [PATCH v10 4/6] drm: consider GEM object shared when it is exported

[Li, Yunxiang (Teddy)]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, December 11, 2024 3:16
> Am 10.12.24 um 18:59 schrieb Yunxiang Li:
> > Tracking the state of a GEM object for shared stats is quite difficult
> > since the handle_count is managed behind driver's back. So instead
> > considers GEM object shared the moment it is exported with flink ioctl.
> > This makes it work the same to the dma_buf case. Add a callback for
> > drivers to get notified when GEM object is being shared.
>
> First of all GEM flink is pretty much deprecated, we only have it for compatibility
> reasons. So please don't change anything here.
>
> Then flink is not the only way to create multiple handles for a GEM object. So this
> here won't handle all cases.
>
> And finally we already have the .open and .close callbacks, which are called
> whenever a handle for a GEM object is created/destroyed. So it shouldn't be
> necessary in the first place.

For the importing VM the shared stats is automatically correct by open and close, but for the exporting VM we need to update the shared stat when the buffer gets shared, since it is already counted as private there. As far as I could find, seems like flink ioctl is the only place where the global name is assigned? The importing side have multiple places to get the global name, but the exporter always needs to first call flink to allocate the number right? So hooking into flink and dma-buf should cover the bases?

I could probably make handle_count work somehow, but it looks like it's read in a lot of places without locks so I'm not sure if there will be some race conditions.

> Regards,
> Christian.
>
> >
> > Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> >
> > CC: dri-devel@lists.freedesktop.org
> > ---
> >   drivers/gpu/drm/drm_gem.c   |  3 +++
> >   drivers/gpu/drm/drm_prime.c |  3 +++
> >   include/drm/drm_gem.h       | 12 +++++++++++-
> >   3 files changed, 17 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index d4bbc5d109c8b..1ead11de31f6b 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -854,6 +854,9 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
> >                     goto err;
> >
> >             obj->name = ret;
> > +
> > +           if (obj->funcs->shared)
> > +                   obj->funcs->shared(obj);
> >     }
> >
> >     args->name = (uint64_t) obj->name;
> > diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
> > index 0e3f8adf162f6..336d982d69807 100644
> > --- a/drivers/gpu/drm/drm_prime.c
> > +++ b/drivers/gpu/drm/drm_prime.c
> > @@ -406,6 +406,9 @@ static struct dma_buf *export_and_register_object(struct
> drm_device *dev,
> >     obj->dma_buf = dmabuf;
> >     get_dma_buf(obj->dma_buf);
> >
> > +   if (obj->funcs->shared)
> > +           obj->funcs->shared(obj);
> > +
> >     return dmabuf;
> >   }
> >
> > diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
> > da11c16e212aa..8c5ffcd485752 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -122,6 +122,16 @@ struct drm_gem_object_funcs {
> >      */
> >     struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
> >
> > +   /**
> > +    * @shared:
> > +    *
> > +    * Callback when GEM object becomes shared, see also
> > +    * drm_gem_object_is_shared_for_memory_stats
> > +    *
> > +    * This callback is optional.
> > +    */
> > +   void (*shared)(struct drm_gem_object *obj);
> > +
> >     /**
> >      * @pin:
> >      *
> > @@ -568,7 +578,7 @@ int drm_gem_evict(struct drm_gem_object *obj);
> >    */
> >   static inline bool drm_gem_object_is_shared_for_memory_stats(struct
> drm_gem_object *obj)
> >   {
> > -   return (obj->handle_count > 1) || obj->dma_buf;
> > +   return obj->name || obj->dma_buf;
> >   }
> >
> >   #ifdef CONFIG_LOCKDEP

Re: [PATCH v10 4/6] drm: consider GEM object shared when it is exported

[Christian König]

Am 11.12.24 um 15:02 schrieb Li, Yunxiang (Teddy):
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, December 11, 2024 3:16
>> Am 10.12.24 um 18:59 schrieb Yunxiang Li:
>>> Tracking the state of a GEM object for shared stats is quite difficult
>>> since the handle_count is managed behind driver's back. So instead
>>> considers GEM object shared the moment it is exported with flink ioctl.
>>> This makes it work the same to the dma_buf case. Add a callback for
>>> drivers to get notified when GEM object is being shared.
>> First of all GEM flink is pretty much deprecated, we only have it for compatibility
>> reasons. So please don't change anything here.
>>
>> Then flink is not the only way to create multiple handles for a GEM object. So this
>> here won't handle all cases.
>>
>> And finally we already have the .open and .close callbacks, which are called
>> whenever a handle for a GEM object is created/destroyed. So it shouldn't be
>> necessary in the first place.
> For the importing VM the shared stats is automatically correct by open and close, but for the exporting VM we need to update the shared stat when the buffer gets shared, since it is already counted as private there. As far as I could find, seems like flink ioctl is the only place where the global name is assigned? The importing side have multiple places to get the global name, but the exporter always needs to first call flink to allocate the number right? So hooking into flink and dma-buf should cover the bases?

It's irrelevant where the global name is assigned. The problem is that 
there are more ways to create a new handle for a GEM object than just 
flink and DMA-buf.

For example you can just ask a framebuffer to give you a GEM handle for 
the currently displayed buffer. See the call to drm_gem_handle_create() 
in drm_mode_getfb2_ioctl().

When you make this change here then those GEM handles are not considered 
shared any more even if they are and you sooner or later run into 
warnings on VM destruction.

> I could probably make handle_count work somehow, but it looks like it's read in a lot of places without locks so I'm not sure if there will be some race conditions.

The handle count is protected by the object_name_lock of the device. The 
drm_gem_object_is_shared_for_memory_stats() function is pretty much the 
only case where we read the value without holding the lock since that is 
used only opportunistically.

What you could do is to hook into amdgpu_gem_object_open() and 
amdgpu_gem_object_close(), call 
drm_gem_object_is_shared_for_memory_stats() and go over all the VMs the 
BO belongs to. (See how amdgpu_vm_bo_find() and amdgpu_vm_bo_add are used).

Then have an additional flag inside amdgpu_bo_va who tells you if a BO 
was previously considered shared or private and update the stats 
accordingly when that status changes.

Regards,
Christian.

>
>> Regards,
>> Christian.
>>
>>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
>>>
>>> CC: dri-devel@lists.freedesktop.org
>>> ---
>>>    drivers/gpu/drm/drm_gem.c   |  3 +++
>>>    drivers/gpu/drm/drm_prime.c |  3 +++
>>>    include/drm/drm_gem.h       | 12 +++++++++++-
>>>    3 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index d4bbc5d109c8b..1ead11de31f6b 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -854,6 +854,9 @@ drm_gem_flink_ioctl(struct drm_device *dev, void *data,
>>>                      goto err;
>>>
>>>              obj->name = ret;
>>> +
>>> +           if (obj->funcs->shared)
>>> +                   obj->funcs->shared(obj);
>>>      }
>>>
>>>      args->name = (uint64_t) obj->name;
>>> diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c
>>> index 0e3f8adf162f6..336d982d69807 100644
>>> --- a/drivers/gpu/drm/drm_prime.c
>>> +++ b/drivers/gpu/drm/drm_prime.c
>>> @@ -406,6 +406,9 @@ static struct dma_buf *export_and_register_object(struct
>> drm_device *dev,
>>>      obj->dma_buf = dmabuf;
>>>      get_dma_buf(obj->dma_buf);
>>>
>>> +   if (obj->funcs->shared)
>>> +           obj->funcs->shared(obj);
>>> +
>>>      return dmabuf;
>>>    }
>>>
>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
>>> da11c16e212aa..8c5ffcd485752 100644
>>> --- a/include/drm/drm_gem.h
>>> +++ b/include/drm/drm_gem.h
>>> @@ -122,6 +122,16 @@ struct drm_gem_object_funcs {
>>>       */
>>>      struct dma_buf *(*export)(struct drm_gem_object *obj, int flags);
>>>
>>> +   /**
>>> +    * @shared:
>>> +    *
>>> +    * Callback when GEM object becomes shared, see also
>>> +    * drm_gem_object_is_shared_for_memory_stats
>>> +    *
>>> +    * This callback is optional.
>>> +    */
>>> +   void (*shared)(struct drm_gem_object *obj);
>>> +
>>>      /**
>>>       * @pin:
>>>       *
>>> @@ -568,7 +578,7 @@ int drm_gem_evict(struct drm_gem_object *obj);
>>>     */
>>>    static inline bool drm_gem_object_is_shared_for_memory_stats(struct
>> drm_gem_object *obj)
>>>    {
>>> -   return (obj->handle_count > 1) || obj->dma_buf;
>>> +   return obj->name || obj->dma_buf;
>>>    }
>>>
>>>    #ifdef CONFIG_LOCKDEP

RE: [PATCH v10 4/6] drm: consider GEM object shared when it is exported

[Li, Yunxiang (Teddy)]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Wednesday, December 11, 2024 10:03
> Am 11.12.24 um 15:02 schrieb Li, Yunxiang (Teddy):
> > [Public]
> >
> >> From: Koenig, Christian <Christian.Koenig@amd.com>
> >> Sent: Wednesday, December 11, 2024 3:16 Am 10.12.24 um 18:59 schrieb
> >> Yunxiang Li:
> >>> Tracking the state of a GEM object for shared stats is quite
> >>> difficult since the handle_count is managed behind driver's back. So
> >>> instead considers GEM object shared the moment it is exported with flink ioctl.
> >>> This makes it work the same to the dma_buf case. Add a callback for
> >>> drivers to get notified when GEM object is being shared.
> >> First of all GEM flink is pretty much deprecated, we only have it for
> >> compatibility reasons. So please don't change anything here.
> >>
> >> Then flink is not the only way to create multiple handles for a GEM
> >> object. So this here won't handle all cases.
> >>
> >> And finally we already have the .open and .close callbacks, which are
> >> called whenever a handle for a GEM object is created/destroyed. So it
> >> shouldn't be necessary in the first place.
> > For the importing VM the shared stats is automatically correct by open and close,
> but for the exporting VM we need to update the shared stat when the buffer gets
> shared, since it is already counted as private there. As far as I could find, seems
> like flink ioctl is the only place where the global name is assigned? The importing
> side have multiple places to get the global name, but the exporter always needs to
> first call flink to allocate the number right? So hooking into flink and dma-buf should
> cover the bases?
>
> It's irrelevant where the global name is assigned. The problem is that there are more
> ways to create a new handle for a GEM object than just flink and DMA-buf.
>
> For example you can just ask a framebuffer to give you a GEM handle for the
> currently displayed buffer. See the call to drm_gem_handle_create() in
> drm_mode_getfb2_ioctl().
>
> When you make this change here then those GEM handles are not considered
> shared any more even if they are and you sooner or later run into warnings on VM
> destruction.
>
> > I could probably make handle_count work somehow, but it looks like it's read in a
> lot of places without locks so I'm not sure if there will be some race conditions.
>
> The handle count is protected by the object_name_lock of the device. The
> drm_gem_object_is_shared_for_memory_stats() function is pretty much the only
> case where we read the value without holding the lock since that is used only
> opportunistically.
>
> What you could do is to hook into amdgpu_gem_object_open() and
> amdgpu_gem_object_close(), call
> drm_gem_object_is_shared_for_memory_stats() and go over all the VMs the BO
> belongs to. (See how amdgpu_vm_bo_find() and amdgpu_vm_bo_add are used).
>
> Then have an additional flag inside amdgpu_bo_va who tells you if a BO was
> previously considered shared or private and update the stats accordingly when that
> status changes.

But the open and close functions are called outside the object_name_lock right, so do I regrab the lock in the amdgpu_* functions or I could move the callback into the lock?

> Regards,
> Christian.
>
> >
> >> Regards,
> >> Christian.
> >>
> >>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> >>>
> >>> CC: dri-devel@lists.freedesktop.org
> >>> ---
> >>>    drivers/gpu/drm/drm_gem.c   |  3 +++
> >>>    drivers/gpu/drm/drm_prime.c |  3 +++
> >>>    include/drm/drm_gem.h       | 12 +++++++++++-
> >>>    3 files changed, 17 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>> index d4bbc5d109c8b..1ead11de31f6b 100644
> >>> --- a/drivers/gpu/drm/drm_gem.c
> >>> +++ b/drivers/gpu/drm/drm_gem.c
> >>> @@ -854,6 +854,9 @@ drm_gem_flink_ioctl(struct drm_device *dev, void
> *data,
> >>>                      goto err;
> >>>
> >>>              obj->name = ret;
> >>> +
> >>> +           if (obj->funcs->shared)
> >>> +                   obj->funcs->shared(obj);
> >>>      }
> >>>
> >>>      args->name = (uint64_t) obj->name; diff --git
> >>> a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index
> >>> 0e3f8adf162f6..336d982d69807 100644
> >>> --- a/drivers/gpu/drm/drm_prime.c
> >>> +++ b/drivers/gpu/drm/drm_prime.c
> >>> @@ -406,6 +406,9 @@ static struct dma_buf
> >>> *export_and_register_object(struct
> >> drm_device *dev,
> >>>      obj->dma_buf = dmabuf;
> >>>      get_dma_buf(obj->dma_buf);
> >>>
> >>> +   if (obj->funcs->shared)
> >>> +           obj->funcs->shared(obj);
> >>> +
> >>>      return dmabuf;
> >>>    }
> >>>
> >>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
> >>> da11c16e212aa..8c5ffcd485752 100644
> >>> --- a/include/drm/drm_gem.h
> >>> +++ b/include/drm/drm_gem.h
> >>> @@ -122,6 +122,16 @@ struct drm_gem_object_funcs {
> >>>       */
> >>>      struct dma_buf *(*export)(struct drm_gem_object *obj, int
> >>> flags);
> >>>
> >>> +   /**
> >>> +    * @shared:
> >>> +    *
> >>> +    * Callback when GEM object becomes shared, see also
> >>> +    * drm_gem_object_is_shared_for_memory_stats
> >>> +    *
> >>> +    * This callback is optional.
> >>> +    */
> >>> +   void (*shared)(struct drm_gem_object *obj);
> >>> +
> >>>      /**
> >>>       * @pin:
> >>>       *
> >>> @@ -568,7 +578,7 @@ int drm_gem_evict(struct drm_gem_object *obj);
> >>>     */
> >>>    static inline bool
> >>> drm_gem_object_is_shared_for_memory_stats(struct
> >> drm_gem_object *obj)
> >>>    {
> >>> -   return (obj->handle_count > 1) || obj->dma_buf;
> >>> +   return obj->name || obj->dma_buf;
> >>>    }
> >>>
> >>>    #ifdef CONFIG_LOCKDEP

Re: [PATCH v10 4/6] drm: consider GEM object shared when it is exported

[Christian König]

Am 11.12.24 um 17:14 schrieb Li, Yunxiang (Teddy):
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Wednesday, December 11, 2024 10:03
>> Am 11.12.24 um 15:02 schrieb Li, Yunxiang (Teddy):
>>> [Public]
>>>
>>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>>> Sent: Wednesday, December 11, 2024 3:16 Am 10.12.24 um 18:59 schrieb
>>>> Yunxiang Li:
>>>>> Tracking the state of a GEM object for shared stats is quite
>>>>> difficult since the handle_count is managed behind driver's back. So
>>>>> instead considers GEM object shared the moment it is exported with flink ioctl.
>>>>> This makes it work the same to the dma_buf case. Add a callback for
>>>>> drivers to get notified when GEM object is being shared.
>>>> First of all GEM flink is pretty much deprecated, we only have it for
>>>> compatibility reasons. So please don't change anything here.
>>>>
>>>> Then flink is not the only way to create multiple handles for a GEM
>>>> object. So this here won't handle all cases.
>>>>
>>>> And finally we already have the .open and .close callbacks, which are
>>>> called whenever a handle for a GEM object is created/destroyed. So it
>>>> shouldn't be necessary in the first place.
>>> For the importing VM the shared stats is automatically correct by open and close,
>> but for the exporting VM we need to update the shared stat when the buffer gets
>> shared, since it is already counted as private there. As far as I could find, seems
>> like flink ioctl is the only place where the global name is assigned? The importing
>> side have multiple places to get the global name, but the exporter always needs to
>> first call flink to allocate the number right? So hooking into flink and dma-buf should
>> cover the bases?
>>
>> It's irrelevant where the global name is assigned. The problem is that there are more
>> ways to create a new handle for a GEM object than just flink and DMA-buf.
>>
>> For example you can just ask a framebuffer to give you a GEM handle for the
>> currently displayed buffer. See the call to drm_gem_handle_create() in
>> drm_mode_getfb2_ioctl().
>>
>> When you make this change here then those GEM handles are not considered
>> shared any more even if they are and you sooner or later run into warnings on VM
>> destruction.
>>
>>> I could probably make handle_count work somehow, but it looks like it's read in a
>> lot of places without locks so I'm not sure if there will be some race conditions.
>>
>> The handle count is protected by the object_name_lock of the device. The
>> drm_gem_object_is_shared_for_memory_stats() function is pretty much the only
>> case where we read the value without holding the lock since that is used only
>> opportunistically.
>>
>> What you could do is to hook into amdgpu_gem_object_open() and
>> amdgpu_gem_object_close(), call
>> drm_gem_object_is_shared_for_memory_stats() and go over all the VMs the BO
>> belongs to. (See how amdgpu_vm_bo_find() and amdgpu_vm_bo_add are used).
>>
>> Then have an additional flag inside amdgpu_bo_va who tells you if a BO was
>> previously considered shared or private and update the stats accordingly when that
>> status changes.
> But the open and close functions are called outside the object_name_lock right, so do I regrab the lock in the amdgpu_* functions or I could move the callback into the lock?

You don't need the object_name_lock for this, the update is just 
opportunistically.

E.g. you go over all the VMs a BO belongs to and grab the VM spinlock to 
update the status in the amdgpu_bo_va structure.

It can in theory be that a concurrent process modifies handle_count at 
the same time you update the VM status, but that doesn't matter since 
this modification will update the status once more again.

Regards,
Christian.

>
>> Regards,
>> Christian.
>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
>>>>>
>>>>> CC: dri-devel@lists.freedesktop.org
>>>>> ---
>>>>>     drivers/gpu/drm/drm_gem.c   |  3 +++
>>>>>     drivers/gpu/drm/drm_prime.c |  3 +++
>>>>>     include/drm/drm_gem.h       | 12 +++++++++++-
>>>>>     3 files changed, 17 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>>>> index d4bbc5d109c8b..1ead11de31f6b 100644
>>>>> --- a/drivers/gpu/drm/drm_gem.c
>>>>> +++ b/drivers/gpu/drm/drm_gem.c
>>>>> @@ -854,6 +854,9 @@ drm_gem_flink_ioctl(struct drm_device *dev, void
>> *data,
>>>>>                       goto err;
>>>>>
>>>>>               obj->name = ret;
>>>>> +
>>>>> +           if (obj->funcs->shared)
>>>>> +                   obj->funcs->shared(obj);
>>>>>       }
>>>>>
>>>>>       args->name = (uint64_t) obj->name; diff --git
>>>>> a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index
>>>>> 0e3f8adf162f6..336d982d69807 100644
>>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>>> @@ -406,6 +406,9 @@ static struct dma_buf
>>>>> *export_and_register_object(struct
>>>> drm_device *dev,
>>>>>       obj->dma_buf = dmabuf;
>>>>>       get_dma_buf(obj->dma_buf);
>>>>>
>>>>> +   if (obj->funcs->shared)
>>>>> +           obj->funcs->shared(obj);
>>>>> +
>>>>>       return dmabuf;
>>>>>     }
>>>>>
>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
>>>>> da11c16e212aa..8c5ffcd485752 100644
>>>>> --- a/include/drm/drm_gem.h
>>>>> +++ b/include/drm/drm_gem.h
>>>>> @@ -122,6 +122,16 @@ struct drm_gem_object_funcs {
>>>>>        */
>>>>>       struct dma_buf *(*export)(struct drm_gem_object *obj, int
>>>>> flags);
>>>>>
>>>>> +   /**
>>>>> +    * @shared:
>>>>> +    *
>>>>> +    * Callback when GEM object becomes shared, see also
>>>>> +    * drm_gem_object_is_shared_for_memory_stats
>>>>> +    *
>>>>> +    * This callback is optional.
>>>>> +    */
>>>>> +   void (*shared)(struct drm_gem_object *obj);
>>>>> +
>>>>>       /**
>>>>>        * @pin:
>>>>>        *
>>>>> @@ -568,7 +578,7 @@ int drm_gem_evict(struct drm_gem_object *obj);
>>>>>      */
>>>>>     static inline bool
>>>>> drm_gem_object_is_shared_for_memory_stats(struct
>>>> drm_gem_object *obj)
>>>>>     {
>>>>> -   return (obj->handle_count > 1) || obj->dma_buf;
>>>>> +   return obj->name || obj->dma_buf;
>>>>>     }
>>>>>
>>>>>     #ifdef CONFIG_LOCKDEP

RE: [PATCH v10 4/6] drm: consider GEM object shared when it is exported

[Li, Yunxiang (Teddy)]

[Public]

> From: Koenig, Christian <Christian.Koenig@amd.com>
> Sent: Thursday, December 12, 2024 4:25
> Am 11.12.24 um 17:14 schrieb Li, Yunxiang (Teddy):
> > [Public]
> >
> >> From: Koenig, Christian <Christian.Koenig@amd.com>
> >> Sent: Wednesday, December 11, 2024 10:03 Am 11.12.24 um 15:02 schrieb
> >> Li, Yunxiang (Teddy):
> >>> [Public]
> >>>
> >>>> From: Koenig, Christian <Christian.Koenig@amd.com>
> >>>> Sent: Wednesday, December 11, 2024 3:16 Am 10.12.24 um 18:59
> >>>> schrieb Yunxiang Li:
> >>>>> Tracking the state of a GEM object for shared stats is quite
> >>>>> difficult since the handle_count is managed behind driver's back.
> >>>>> So instead considers GEM object shared the moment it is exported with flink
> ioctl.
> >>>>> This makes it work the same to the dma_buf case. Add a callback
> >>>>> for drivers to get notified when GEM object is being shared.
> >>>> First of all GEM flink is pretty much deprecated, we only have it
> >>>> for compatibility reasons. So please don't change anything here.
> >>>>
> >>>> Then flink is not the only way to create multiple handles for a GEM
> >>>> object. So this here won't handle all cases.
> >>>>
> >>>> And finally we already have the .open and .close callbacks, which
> >>>> are called whenever a handle for a GEM object is created/destroyed.
> >>>> So it shouldn't be necessary in the first place.
> >>> For the importing VM the shared stats is automatically correct by
> >>> open and close,
> >> but for the exporting VM we need to update the shared stat when the
> >> buffer gets shared, since it is already counted as private there. As
> >> far as I could find, seems like flink ioctl is the only place where
> >> the global name is assigned? The importing side have multiple places
> >> to get the global name, but the exporter always needs to first call
> >> flink to allocate the number right? So hooking into flink and dma-buf should cover
> the bases?
> >>
> >> It's irrelevant where the global name is assigned. The problem is
> >> that there are more ways to create a new handle for a GEM object than just flink
> and DMA-buf.
> >>
> >> For example you can just ask a framebuffer to give you a GEM handle
> >> for the currently displayed buffer. See the call to
> >> drm_gem_handle_create() in drm_mode_getfb2_ioctl().
> >>
> >> When you make this change here then those GEM handles are not
> >> considered shared any more even if they are and you sooner or later
> >> run into warnings on VM destruction.
> >>
> >>> I could probably make handle_count work somehow, but it looks like
> >>> it's read in a
> >> lot of places without locks so I'm not sure if there will be some race conditions.
> >>
> >> The handle count is protected by the object_name_lock of the device.
> >> The
> >> drm_gem_object_is_shared_for_memory_stats() function is pretty much
> >> the only case where we read the value without holding the lock since
> >> that is used only opportunistically.
> >>
> >> What you could do is to hook into amdgpu_gem_object_open() and
> >> amdgpu_gem_object_close(), call
> >> drm_gem_object_is_shared_for_memory_stats() and go over all the VMs
> >> the BO belongs to. (See how amdgpu_vm_bo_find() and amdgpu_vm_bo_add
> are used).
> >>
> >> Then have an additional flag inside amdgpu_bo_va who tells you if a
> >> BO was previously considered shared or private and update the stats
> >> accordingly when that status changes.
> > But the open and close functions are called outside the object_name_lock right,
> so do I regrab the lock in the amdgpu_* functions or I could move the callback into
> the lock?
>
> You don't need the object_name_lock for this, the update is just opportunistically.
>
> E.g. you go over all the VMs a BO belongs to and grab the VM spinlock to update
> the status in the amdgpu_bo_va structure.
>
> It can in theory be that a concurrent process modifies handle_count at the same
> time you update the VM status, but that doesn't matter since this modification will
> update the status once more again.

Wouldn't there be an ordering concern? Say the handle count goes to 2 in one thread and another thread drop it down to 1 right after, the two loops run concurrently. Wouldn't it be possible that some VM get updated by the second thread first and then the first thread and be left in the "shared" state?

Teddy

> Regards,
> Christian.
>
> >
> >> Regards,
> >> Christian.
> >>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
> >>>>>
> >>>>> CC: dri-devel@lists.freedesktop.org
> >>>>> ---
> >>>>>     drivers/gpu/drm/drm_gem.c   |  3 +++
> >>>>>     drivers/gpu/drm/drm_prime.c |  3 +++
> >>>>>     include/drm/drm_gem.h       | 12 +++++++++++-
> >>>>>     3 files changed, 17 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>>>> index d4bbc5d109c8b..1ead11de31f6b 100644
> >>>>> --- a/drivers/gpu/drm/drm_gem.c
> >>>>> +++ b/drivers/gpu/drm/drm_gem.c
> >>>>> @@ -854,6 +854,9 @@ drm_gem_flink_ioctl(struct drm_device *dev,
> >>>>> void
> >> *data,
> >>>>>                       goto err;
> >>>>>
> >>>>>               obj->name = ret;
> >>>>> +
> >>>>> +           if (obj->funcs->shared)
> >>>>> +                   obj->funcs->shared(obj);
> >>>>>       }
> >>>>>
> >>>>>       args->name = (uint64_t) obj->name; diff --git
> >>>>> a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index
> >>>>> 0e3f8adf162f6..336d982d69807 100644
> >>>>> --- a/drivers/gpu/drm/drm_prime.c
> >>>>> +++ b/drivers/gpu/drm/drm_prime.c
> >>>>> @@ -406,6 +406,9 @@ static struct dma_buf
> >>>>> *export_and_register_object(struct
> >>>> drm_device *dev,
> >>>>>       obj->dma_buf = dmabuf;
> >>>>>       get_dma_buf(obj->dma_buf);
> >>>>>
> >>>>> +   if (obj->funcs->shared)
> >>>>> +           obj->funcs->shared(obj);
> >>>>> +
> >>>>>       return dmabuf;
> >>>>>     }
> >>>>>
> >>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
> >>>>> da11c16e212aa..8c5ffcd485752 100644
> >>>>> --- a/include/drm/drm_gem.h
> >>>>> +++ b/include/drm/drm_gem.h
> >>>>> @@ -122,6 +122,16 @@ struct drm_gem_object_funcs {
> >>>>>        */
> >>>>>       struct dma_buf *(*export)(struct drm_gem_object *obj, int
> >>>>> flags);
> >>>>>
> >>>>> +   /**
> >>>>> +    * @shared:
> >>>>> +    *
> >>>>> +    * Callback when GEM object becomes shared, see also
> >>>>> +    * drm_gem_object_is_shared_for_memory_stats
> >>>>> +    *
> >>>>> +    * This callback is optional.
> >>>>> +    */
> >>>>> +   void (*shared)(struct drm_gem_object *obj);
> >>>>> +
> >>>>>       /**
> >>>>>        * @pin:
> >>>>>        *
> >>>>> @@ -568,7 +578,7 @@ int drm_gem_evict(struct drm_gem_object *obj);
> >>>>>      */
> >>>>>     static inline bool
> >>>>> drm_gem_object_is_shared_for_memory_stats(struct
> >>>> drm_gem_object *obj)
> >>>>>     {
> >>>>> -   return (obj->handle_count > 1) || obj->dma_buf;
> >>>>> +   return obj->name || obj->dma_buf;
> >>>>>     }
> >>>>>
> >>>>>     #ifdef CONFIG_LOCKDEP

Re: [PATCH v10 4/6] drm: consider GEM object shared when it is exported

[Christian König]

Am 12.12.24 um 15:04 schrieb Li, Yunxiang (Teddy):
> [Public]
>
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Thursday, December 12, 2024 4:25
>> Am 11.12.24 um 17:14 schrieb Li, Yunxiang (Teddy):
>>> [Public]
>>>
>>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>>> Sent: Wednesday, December 11, 2024 10:03 Am 11.12.24 um 15:02 schrieb
>>>> Li, Yunxiang (Teddy):
>>>>> [Public]
>>>>>
>>>>>> From: Koenig, Christian <Christian.Koenig@amd.com>
>>>>>> Sent: Wednesday, December 11, 2024 3:16 Am 10.12.24 um 18:59
>>>>>> schrieb Yunxiang Li:
>>>>>>> Tracking the state of a GEM object for shared stats is quite
>>>>>>> difficult since the handle_count is managed behind driver's back.
>>>>>>> So instead considers GEM object shared the moment it is exported with flink
>> ioctl.
>>>>>>> This makes it work the same to the dma_buf case. Add a callback
>>>>>>> for drivers to get notified when GEM object is being shared.
>>>>>> First of all GEM flink is pretty much deprecated, we only have it
>>>>>> for compatibility reasons. So please don't change anything here.
>>>>>>
>>>>>> Then flink is not the only way to create multiple handles for a GEM
>>>>>> object. So this here won't handle all cases.
>>>>>>
>>>>>> And finally we already have the .open and .close callbacks, which
>>>>>> are called whenever a handle for a GEM object is created/destroyed.
>>>>>> So it shouldn't be necessary in the first place.
>>>>> For the importing VM the shared stats is automatically correct by
>>>>> open and close,
>>>> but for the exporting VM we need to update the shared stat when the
>>>> buffer gets shared, since it is already counted as private there. As
>>>> far as I could find, seems like flink ioctl is the only place where
>>>> the global name is assigned? The importing side have multiple places
>>>> to get the global name, but the exporter always needs to first call
>>>> flink to allocate the number right? So hooking into flink and dma-buf should cover
>> the bases?
>>>> It's irrelevant where the global name is assigned. The problem is
>>>> that there are more ways to create a new handle for a GEM object than just flink
>> and DMA-buf.
>>>> For example you can just ask a framebuffer to give you a GEM handle
>>>> for the currently displayed buffer. See the call to
>>>> drm_gem_handle_create() in drm_mode_getfb2_ioctl().
>>>>
>>>> When you make this change here then those GEM handles are not
>>>> considered shared any more even if they are and you sooner or later
>>>> run into warnings on VM destruction.
>>>>
>>>>> I could probably make handle_count work somehow, but it looks like
>>>>> it's read in a
>>>> lot of places without locks so I'm not sure if there will be some race conditions.
>>>>
>>>> The handle count is protected by the object_name_lock of the device.
>>>> The
>>>> drm_gem_object_is_shared_for_memory_stats() function is pretty much
>>>> the only case where we read the value without holding the lock since
>>>> that is used only opportunistically.
>>>>
>>>> What you could do is to hook into amdgpu_gem_object_open() and
>>>> amdgpu_gem_object_close(), call
>>>> drm_gem_object_is_shared_for_memory_stats() and go over all the VMs
>>>> the BO belongs to. (See how amdgpu_vm_bo_find() and amdgpu_vm_bo_add
>> are used).
>>>> Then have an additional flag inside amdgpu_bo_va who tells you if a
>>>> BO was previously considered shared or private and update the stats
>>>> accordingly when that status changes.
>>> But the open and close functions are called outside the object_name_lock right,
>> so do I regrab the lock in the amdgpu_* functions or I could move the callback into
>> the lock?
>>
>> You don't need the object_name_lock for this, the update is just opportunistically.
>>
>> E.g. you go over all the VMs a BO belongs to and grab the VM spinlock to update
>> the status in the amdgpu_bo_va structure.
>>
>> It can in theory be that a concurrent process modifies handle_count at the same
>> time you update the VM status, but that doesn't matter since this modification will
>> update the status once more again.
> Wouldn't there be an ordering concern? Say the handle count goes to 2 in one thread and another thread drop it down to 1 right after, the two loops run concurrently. Wouldn't it be possible that some VM get updated by the second thread first and then the first thread and be left in the "shared" state?

No, because the update always updates to the current state. E.g. it's 
irrelevant if open or close calls the update, key point is that we grab 
the current state and update to that one.

What in theory can happen is that the current state isn't up to date 
because of missing CPU barriers, but since we are protecting the update 
with a spinlock that should be irrelevant.

Christian.

>
> Teddy
>
>> Regards,
>> Christian.
>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com>
>>>>>>>
>>>>>>> CC: dri-devel@lists.freedesktop.org
>>>>>>> ---
>>>>>>>      drivers/gpu/drm/drm_gem.c   |  3 +++
>>>>>>>      drivers/gpu/drm/drm_prime.c |  3 +++
>>>>>>>      include/drm/drm_gem.h       | 12 +++++++++++-
>>>>>>>      3 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>>>>>> index d4bbc5d109c8b..1ead11de31f6b 100644
>>>>>>> --- a/drivers/gpu/drm/drm_gem.c
>>>>>>> +++ b/drivers/gpu/drm/drm_gem.c
>>>>>>> @@ -854,6 +854,9 @@ drm_gem_flink_ioctl(struct drm_device *dev,
>>>>>>> void
>>>> *data,
>>>>>>>                        goto err;
>>>>>>>
>>>>>>>                obj->name = ret;
>>>>>>> +
>>>>>>> +           if (obj->funcs->shared)
>>>>>>> +                   obj->funcs->shared(obj);
>>>>>>>        }
>>>>>>>
>>>>>>>        args->name = (uint64_t) obj->name; diff --git
>>>>>>> a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index
>>>>>>> 0e3f8adf162f6..336d982d69807 100644
>>>>>>> --- a/drivers/gpu/drm/drm_prime.c
>>>>>>> +++ b/drivers/gpu/drm/drm_prime.c
>>>>>>> @@ -406,6 +406,9 @@ static struct dma_buf
>>>>>>> *export_and_register_object(struct
>>>>>> drm_device *dev,
>>>>>>>        obj->dma_buf = dmabuf;
>>>>>>>        get_dma_buf(obj->dma_buf);
>>>>>>>
>>>>>>> +   if (obj->funcs->shared)
>>>>>>> +           obj->funcs->shared(obj);
>>>>>>> +
>>>>>>>        return dmabuf;
>>>>>>>      }
>>>>>>>
>>>>>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
>>>>>>> da11c16e212aa..8c5ffcd485752 100644
>>>>>>> --- a/include/drm/drm_gem.h
>>>>>>> +++ b/include/drm/drm_gem.h
>>>>>>> @@ -122,6 +122,16 @@ struct drm_gem_object_funcs {
>>>>>>>         */
>>>>>>>        struct dma_buf *(*export)(struct drm_gem_object *obj, int
>>>>>>> flags);
>>>>>>>
>>>>>>> +   /**
>>>>>>> +    * @shared:
>>>>>>> +    *
>>>>>>> +    * Callback when GEM object becomes shared, see also
>>>>>>> +    * drm_gem_object_is_shared_for_memory_stats
>>>>>>> +    *
>>>>>>> +    * This callback is optional.
>>>>>>> +    */
>>>>>>> +   void (*shared)(struct drm_gem_object *obj);
>>>>>>> +
>>>>>>>        /**
>>>>>>>         * @pin:
>>>>>>>         *
>>>>>>> @@ -568,7 +578,7 @@ int drm_gem_evict(struct drm_gem_object *obj);
>>>>>>>       */
>>>>>>>      static inline bool
>>>>>>> drm_gem_object_is_shared_for_memory_stats(struct
>>>>>> drm_gem_object *obj)
>>>>>>>      {
>>>>>>> -   return (obj->handle_count > 1) || obj->dma_buf;
>>>>>>> +   return obj->name || obj->dma_buf;
>>>>>>>      }
>>>>>>>
>>>>>>>      #ifdef CONFIG_LOCKDEP

[PATCH v10 5/6] 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 5df21529b3b13..5cc5f59e30184 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 63e2cb1ab56ea..fe7ae45500639 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -977,7 +977,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;
@@ -1037,7 +1036,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 4f057996ef35b..951b20e40fd35 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1251,7 +1251,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;
 
@@ -1259,7 +1258,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 57636dfd77a8a..88173bd1f9a2c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2144,14 +2144,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 v10 6/6] 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>
---
v10: Keep count of how many ways the buffer is shared so we don't update
the shared stats twice, in the future if buffers can get unshared within
their lifetime we can also use this count to undo the update. Add a
locked version of the normal stats helper so that when buffer moves we
don't drop the lock between the updates.

 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  18 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c    |   7 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 110 +++++-------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |   4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h    |   4 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c     | 192 ++++++++++++++++-----
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h     |  23 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c  |   1 +
 8 files changed, 220 insertions(+), 139 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
index 7717e3e4f05b5..91d638098889d 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_NUM];
 	ktime_t usage[AMDGPU_HW_IP_NUM];
 	const char *pl_name[] = {
 		[TTM_PL_VRAM] = "vram",
@@ -72,15 +72,8 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 		[AMDGPU_PL_DOORBELL] = "doorbell",
 	};
 	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 +90,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]);
@@ -115,9 +107,11 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 	drm_printf(p, "amd-evicted-vram:\t%llu KiB\n",
 		   stats[TTM_PL_VRAM].evicted/1024UL);
 	drm_printf(p, "amd-requested-vram:\t%llu KiB\n",
-		   stats[TTM_PL_VRAM].requested/1024UL);
+		   (stats[TTM_PL_VRAM].drm.shared +
+		    stats[TTM_PL_VRAM].drm.private) / 1024UL);
 	drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
-		   stats[TTM_PL_TT].requested/1024UL);
+		   (stats[TTM_PL_TT].drm.shared +
+		    stats[TTM_PL_TT].drm.private) / 1024UL);
 
 	for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
 		if (!usage[hw_ip])
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index fe7ae45500639..6e899130502f4 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -381,6 +381,12 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 	drm_exec_fini(&exec);
 }
 
+static void amdgpu_gem_object_shared(struct drm_gem_object *obj)
+{
+	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
+	amdgpu_vm_bo_update_shared(bo);
+}
+
 static int amdgpu_gem_object_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma)
 {
 	struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
@@ -407,6 +413,7 @@ const struct drm_gem_object_funcs amdgpu_gem_object_funcs = {
 	.open = amdgpu_gem_object_open,
 	.close = amdgpu_gem_object_close,
 	.export = amdgpu_gem_prime_export,
+	.shared = amdgpu_gem_object_shared,
 	.vmap = drm_gem_ttm_vmap,
 	.vunmap = drm_gem_ttm_vunmap,
 	.mmap = amdgpu_gem_object_mmap,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 951b20e40fd35..96f4b8904e9a6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1258,7 +1258,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);
 
@@ -1271,75 +1271,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
@@ -1554,6 +1485,45 @@ u64 amdgpu_bo_gpu_offset_no_check(struct amdgpu_bo *bo)
 	return amdgpu_gmc_sign_extend(offset);
 }
 
+/**
+ * amdgpu_bo_mem_stats_placement - bo placement for memory accounting
+ * @bo:	the buffer object we should look at
+ *
+ * BO can have multiple preferred placements, to avoid double counting we want
+ * to file it under a single placement for memory stats.
+ * Luckily, if we take the highest set bit in preferred_domains the result is
+ * quite sensible.
+ *
+ * Returns:
+ * Which of the placements should the BO be accounted under.
+ */
+uint32_t amdgpu_bo_mem_stats_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 ce3314152d20f..bdc9a5bc4da46 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -305,9 +305,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_mem_stats_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..461fb8090ae04 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.h
@@ -26,15 +26,15 @@
 
 #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)
 #define AMDGPU_PL_OA		(TTM_PL_PRIV + 2)
 #define AMDGPU_PL_PREEMPT	(TTM_PL_PRIV + 3)
 #define AMDGPU_PL_DOORBELL	(TTM_PL_PRIV + 4)
-#define __AMDGPU_PL_LAST	(TTM_PL_PRIV + 4)
+#define __AMDGPU_PL_NUM	(TTM_PL_PRIV + 5)
 
 #define AMDGPU_GTT_MAX_TRANSFER_SIZE	512
 #define AMDGPU_GTT_NUM_TRANSFER_WINDOWS	2
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 88173bd1f9a2c..955432d9f3e19 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,88 @@ static void amdgpu_vm_bo_reset_state_machine(struct amdgpu_vm *vm)
 	spin_unlock(&vm->status_lock);
 }
 
+/**
+ * amdgpu_vm_update_shared - when BO gets exported for sharing
+ * @base: base structure for tracking BO usage in a VM
+ *
+ * Takes the vm status_lock and updates the memory stat from private to shared.
+ */
+static void amdgpu_vm_update_shared(struct amdgpu_vm_bo_base *base)
+{
+	struct amdgpu_vm *vm = base->vm;
+	struct amdgpu_bo *bo = base->bo;
+	int64_t size = amdgpu_bo_size(bo);
+	uint32_t bo_memtype = amdgpu_bo_mem_stats_placement(bo);
+
+	spin_lock(&vm->status_lock);
+	if (base->share_count++ == 0) {
+		vm->stats[bo_memtype].drm.shared += size;
+		vm->stats[bo_memtype].drm.private -= size;
+	}
+	spin_unlock(&vm->status_lock);
+}
+
+/**
+ * amdgpu_vm_update_stats_locked - helper to update normal memory stat
+ * @base: base structure for tracking BO usage in a VM
+ * @res:  the ttm_resource to use for the purpose of accounting, may or may not
+ *        be bo->tbo.resource
+ * @sign: if we should add (+1) or subtract (-1) from the stat
+ *
+ * Caller need to have the vm status_lock held. Useful for when multiple update
+ * need to happen at the same time.
+ */
+static void amdgpu_vm_update_stats_locked(struct amdgpu_vm_bo_base *base,
+			    struct ttm_resource *res, int sign)
+{
+	struct amdgpu_vm *vm = base->vm;
+	struct amdgpu_bo *bo = base->bo;
+	int64_t size = sign * amdgpu_bo_size(bo);
+	uint32_t bo_memtype = amdgpu_bo_mem_stats_placement(bo);
+	bool shared = drm_gem_object_is_shared_for_memory_stats(&bo->tbo.base);
+
+	/* For drm-total- and drm-shared-, BO are accounted by their preferred
+	 * placement, see also amdgpu_bo_mem_stats_placement.
+	 */
+	if (shared)
+		vm->stats[bo_memtype].drm.shared += size;
+	else
+		vm->stats[bo_memtype].drm.private += size;
+
+	if (res && res->mem_type < __AMDGPU_PL_NUM) {
+		uint32_t res_memtype = res->mem_type;
+
+		vm->stats[res_memtype].drm.resident += size;
+		/* BO only count as purgeable if it is resident,
+		 * since otherwise there's nothing to purge.
+		 */
+		if (bo->flags & AMDGPU_GEM_CREATE_DISCARDABLE)
+			vm->stats[res_memtype].drm.purgeable += size;
+		if (!(bo->preferred_domains & amdgpu_mem_type_to_domain(res_memtype)))
+			vm->stats[bo_memtype].evicted += size;
+	}
+}
+
+/**
+ * amdgpu_vm_update_stats - helper to update normal memory stat
+ * @base: base structure for tracking BO usage in a VM
+ * @res:  the ttm_resource to use for the purpose of accounting, may or may not
+ *        be bo->tbo.resource
+ * @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 buffer was
+ * exported amdgpu_vm_update_shared need to be called instead.
+ */
+void amdgpu_vm_update_stats(struct amdgpu_vm_bo_base *base,
+			    struct ttm_resource *res, int sign)
+{
+	struct amdgpu_vm *vm = base->vm;
+
+	spin_lock(&vm->status_lock);
+	amdgpu_vm_update_stats_locked(base, res, sign);
+	spin_unlock(&vm->status_lock);
+}
+
 /**
  * amdgpu_vm_bo_base_init - Adds bo to the list of bos associated with the vm
  *
@@ -332,6 +415,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, bo->tbo.resource, +1);
 
 	if (!amdgpu_vm_is_bo_always_valid(vm, bo))
 		return;
@@ -1083,53 +1167,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_NUM])
 {
-	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_NUM);
 	spin_unlock(&vm->status_lock);
 }
 
@@ -2076,6 +2118,7 @@ void amdgpu_vm_bo_del(struct amdgpu_device *adev,
 			if (*base != &bo_va->base)
 				continue;
 
+			amdgpu_vm_update_stats(*base, bo->tbo.resource, -1);
 			*base = bo_va->base.next;
 			break;
 		}
@@ -2141,6 +2184,21 @@ bool amdgpu_vm_evictable(struct amdgpu_bo *bo)
 	return true;
 }
 
+/**
+ * amdgpu_vm_bo_update_shared - called when bo gets shared
+ *
+ * @bo: amdgpu buffer object
+ *
+ * Update the per VM stats for all the vm
+ */
+void amdgpu_vm_bo_update_shared(struct amdgpu_bo *bo)
+{
+	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);
+}
+
 /**
  * amdgpu_vm_bo_invalidate - mark the bo as invalid
  *
@@ -2174,6 +2232,32 @@ 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) {
+		struct amdgpu_vm *vm = bo_base->vm;
+
+		spin_lock(&vm->status_lock);
+		amdgpu_vm_update_stats_locked(bo_base, bo->tbo.resource, -1);
+		amdgpu_vm_update_stats_locked(bo_base, new_mem, +1);
+		spin_unlock(&vm->status_lock);
+	}
+
+	amdgpu_vm_bo_invalidate(bo, evicted);
+}
+
 /**
  * amdgpu_vm_get_block_size - calculate VM page table size as power of two
  *
@@ -2590,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_NUM; ++i) {
+		if (!(drm_memory_stats_is_zero(&vm->stats[i].drm) &&
+		      vm->stats[i].evicted == 0))
+			return false;
+	}
+	return true;
+}
+
 /**
  * amdgpu_vm_fini - tear down a vm instance
  *
@@ -2613,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);
@@ -2661,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..46c068ff3d5f3 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;
 
@@ -202,9 +203,13 @@ struct amdgpu_vm_bo_base {
 	/* protected by bo being reserved */
 	struct amdgpu_vm_bo_base	*next;
 
-	/* protected by spinlock */
+	/* protected by vm status_lock */
 	struct list_head		vm_status;
 
+	/* number of ways this bo is shared (e.g. flink, dma-buf)
+	 * protected by vm status_lock */
+	char 				share_count;
+
 	/* protected by the BO being reserved */
 	bool				moved;
 };
@@ -324,10 +329,7 @@ struct amdgpu_vm_fault_info {
 struct amdgpu_mem_stats {
 	struct drm_memory_stats drm;
 
-	/* buffers that requested this placement */
-	uint64_t requested;
-	/* buffers that requested this placement
-	 * but are currently evicted */
+	/* buffers that requested this placement 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 status_lock */
+	struct amdgpu_mem_stats stats[__AMDGPU_PL_NUM];
+
 	/* 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);
+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_NUM]);
 
 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..b0bf216821152 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, entry->bo->tbo.resource, -1);
 	entry->bo->vm_bo = NULL;
 	ttm_bo_set_bulk_move(&entry->bo->tbo, NULL);
 
-- 
2.34.1