[PATCH 0/2] fdinfo shared stats

[PATCH 0/2] fdinfo shared stats

[Alex Deucher]

We had a request to add shared buffer stats to fdinfo for amdgpu and
while implementing that, Christian mentioned that just looking at
the GEM handle count doesn't take into account buffers shared with other
subsystems like V4L or RDMA.  Those subsystems don't use GEM, so it
doesn't really matter from a GPU top perspective, but it's more
correct if you actually want to see shared buffers.

Alex Deucher (2):
  drm: update drm_show_memory_stats() for dma-bufs
  drm/amdgpu: add shared fdinfo stats

 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
 drivers/gpu/drm/drm_file.c                 |  2 +-
 4 files changed, 22 insertions(+), 1 deletion(-)

-- 
2.42.0

[PATCH 1/2] drm: update drm_show_memory_stats() for dma-bufs

[Alex Deucher]

Show buffers as shared if they are shared via dma-buf as well
(e.g., shared with v4l or some other subsystem).

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/drm_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 5ddaffd32586..5d5f93b9c263 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -973,7 +973,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
 					DRM_GEM_OBJECT_PURGEABLE;
 		}
 
-		if (obj->handle_count > 1) {
+		if ((obj->handle_count > 1) || obj->dma_buf) {
 			status.shared += obj->size;
 		} else {
 			status.private += obj->size;
-- 
2.42.0

Re: [PATCH 1/2] drm: update drm_show_memory_stats() for dma-bufs

[Felix Kuehling]

On 2023-12-07 13:02, Alex Deucher wrote:
> Show buffers as shared if they are shared via dma-buf as well
> (e.g., shared with v4l or some other subsystem).

You can add KFD to that list. With the in-progress CUDA11 VM changes and 
improved interop between KFD and render nodes, sharing DMABufs between 
KFD and render nodes will become much more common.

Regards,
   Felix


>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Clark <robdclark@gmail.com>
> ---
>   drivers/gpu/drm/drm_file.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 5ddaffd32586..5d5f93b9c263 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -973,7 +973,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>   					DRM_GEM_OBJECT_PURGEABLE;
>   		}
>   
> -		if (obj->handle_count > 1) {
> +		if ((obj->handle_count > 1) || obj->dma_buf) {
>   			status.shared += obj->size;
>   		} else {
>   			status.private += obj->size;

Re: [PATCH 1/2] drm: update drm_show_memory_stats() for dma-bufs

[Rob Clark]

On Thu, Dec 7, 2023 at 10:02 AM Alex Deucher <alexander.deucher@amd.com> wrote:
>
> Show buffers as shared if they are shared via dma-buf as well
> (e.g., shared with v4l or some other subsystem).
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Clark <robdclark@gmail.com>

Reviewed-by: Rob Clark <robdclark@gmail.com>

> ---
>  drivers/gpu/drm/drm_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 5ddaffd32586..5d5f93b9c263 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -973,7 +973,7 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>                                         DRM_GEM_OBJECT_PURGEABLE;
>                 }
>
> -               if (obj->handle_count > 1) {
> +               if ((obj->handle_count > 1) || obj->dma_buf) {
>                         status.shared += obj->size;
>                 } else {
>                         status.private += obj->size;
> --
> 2.42.0
>

[PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Alex Deucher]

Add shared stats.  Useful for seeing shared memory.

v2: take dma-buf into account as well

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
Cc: Rob Clark <robdclark@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
index 5706b282a0c7..c7df7fa3459f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
@@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 		   stats.requested_visible_vram/1024UL);
 	drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
 		   stats.requested_gtt/1024UL);
+	drm_printf(p, "drm-shared-vram:\t%llu KiB\n", stats.vram_shared/1024UL);
+	drm_printf(p, "drm-shared-gtt:\t%llu KiB\n", stats.gtt_shared/1024UL);
+	drm_printf(p, "drm-shared-cpu:\t%llu KiB\n", stats.cpu_shared/1024UL);
+
 	for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
 		if (!usage[hw_ip])
 			continue;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index d79b4ca1ecfc..1b37d95475b8 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
 			  struct amdgpu_mem_stats *stats)
 {
 	uint64_t size = amdgpu_bo_size(bo);
+	struct drm_gem_object *obj;
 	unsigned int domain;
+	bool shared;
 
 	/* Abort if the BO doesn't currently have a backing store */
 	if (!bo->tbo.resource)
 		return;
 
+	obj = &bo->tbo.base;
+	shared = (obj->handle_count > 1) || obj->dma_buf;
+
 	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
 	switch (domain) {
 	case AMDGPU_GEM_DOMAIN_VRAM:
 		stats->vram += size;
 		if (amdgpu_bo_in_cpu_visible_vram(bo))
 			stats->visible_vram += size;
+		if (shared)
+			stats->vram_shared += size;
 		break;
 	case AMDGPU_GEM_DOMAIN_GTT:
 		stats->gtt += size;
+		if (shared)
+			stats->gtt_shared += size;
 		break;
 	case AMDGPU_GEM_DOMAIN_CPU:
 	default:
 		stats->cpu += size;
+		if (shared)
+			stats->cpu_shared += size;
 		break;
 	}
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index d28e21baef16..0503af75dc26 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
 struct amdgpu_mem_stats {
 	/* current VRAM usage, includes visible VRAM */
 	uint64_t vram;
+	/* current shared VRAM usage, includes visible VRAM */
+	uint64_t vram_shared;
 	/* current visible VRAM usage */
 	uint64_t visible_vram;
 	/* current GTT usage */
 	uint64_t gtt;
+	/* current shared GTT usage */
+	uint64_t gtt_shared;
 	/* current system memory usage */
 	uint64_t cpu;
+	/* current shared system memory usage */
+	uint64_t cpu_shared;
 	/* sum of evicted buffers, includes visible VRAM */
 	uint64_t evicted_vram;
 	/* sum of evicted buffers due to CPU access */
-- 
2.42.0

Re: [PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Alex Deucher]

Ping?

Alex

On Thu, Dec 7, 2023 at 1:03 PM Alex Deucher <alexander.deucher@amd.com> wrote:
>
> Add shared stats.  Useful for seeing shared memory.
>
> v2: take dma-buf into account as well
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Clark <robdclark@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
>  3 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> index 5706b282a0c7..c7df7fa3459f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> @@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>                    stats.requested_visible_vram/1024UL);
>         drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
>                    stats.requested_gtt/1024UL);
> +       drm_printf(p, "drm-shared-vram:\t%llu KiB\n", stats.vram_shared/1024UL);
> +       drm_printf(p, "drm-shared-gtt:\t%llu KiB\n", stats.gtt_shared/1024UL);
> +       drm_printf(p, "drm-shared-cpu:\t%llu KiB\n", stats.cpu_shared/1024UL);
> +
>         for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
>                 if (!usage[hw_ip])
>                         continue;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index d79b4ca1ecfc..1b37d95475b8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>                           struct amdgpu_mem_stats *stats)
>  {
>         uint64_t size = amdgpu_bo_size(bo);
> +       struct drm_gem_object *obj;
>         unsigned int domain;
> +       bool shared;
>
>         /* Abort if the BO doesn't currently have a backing store */
>         if (!bo->tbo.resource)
>                 return;
>
> +       obj = &bo->tbo.base;
> +       shared = (obj->handle_count > 1) || obj->dma_buf;
> +
>         domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>         switch (domain) {
>         case AMDGPU_GEM_DOMAIN_VRAM:
>                 stats->vram += size;
>                 if (amdgpu_bo_in_cpu_visible_vram(bo))
>                         stats->visible_vram += size;
> +               if (shared)
> +                       stats->vram_shared += size;
>                 break;
>         case AMDGPU_GEM_DOMAIN_GTT:
>                 stats->gtt += size;
> +               if (shared)
> +                       stats->gtt_shared += size;
>                 break;
>         case AMDGPU_GEM_DOMAIN_CPU:
>         default:
>                 stats->cpu += size;
> +               if (shared)
> +                       stats->cpu_shared += size;
>                 break;
>         }
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index d28e21baef16..0503af75dc26 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
>  struct amdgpu_mem_stats {
>         /* current VRAM usage, includes visible VRAM */
>         uint64_t vram;
> +       /* current shared VRAM usage, includes visible VRAM */
> +       uint64_t vram_shared;
>         /* current visible VRAM usage */
>         uint64_t visible_vram;
>         /* current GTT usage */
>         uint64_t gtt;
> +       /* current shared GTT usage */
> +       uint64_t gtt_shared;
>         /* current system memory usage */
>         uint64_t cpu;
> +       /* current shared system memory usage */
> +       uint64_t cpu_shared;
>         /* sum of evicted buffers, includes visible VRAM */
>         uint64_t evicted_vram;
>         /* sum of evicted buffers due to CPU access */
> --
> 2.42.0
>

Re: [PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Christian König]

Am 07.12.23 um 19:02 schrieb Alex Deucher:
> Add shared stats.  Useful for seeing shared memory.
>
> v2: take dma-buf into account as well
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> Cc: Rob Clark <robdclark@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
>   3 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> index 5706b282a0c7..c7df7fa3459f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> @@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>   		   stats.requested_visible_vram/1024UL);
>   	drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
>   		   stats.requested_gtt/1024UL);
> +	drm_printf(p, "drm-shared-vram:\t%llu KiB\n", stats.vram_shared/1024UL);
> +	drm_printf(p, "drm-shared-gtt:\t%llu KiB\n", stats.gtt_shared/1024UL);
> +	drm_printf(p, "drm-shared-cpu:\t%llu KiB\n", stats.cpu_shared/1024UL);
> +
>   	for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
>   		if (!usage[hw_ip])
>   			continue;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index d79b4ca1ecfc..1b37d95475b8 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>   			  struct amdgpu_mem_stats *stats)
>   {
>   	uint64_t size = amdgpu_bo_size(bo);
> +	struct drm_gem_object *obj;
>   	unsigned int domain;
> +	bool shared;
>   
>   	/* Abort if the BO doesn't currently have a backing store */
>   	if (!bo->tbo.resource)
>   		return;
>   
> +	obj = &bo->tbo.base;
> +	shared = (obj->handle_count > 1) || obj->dma_buf;

I still think that looking at handle_count is the completely wrong 
approach, we should really only look at obj->dma_buf.

Regards,
Christian.

> +
>   	domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>   	switch (domain) {
>   	case AMDGPU_GEM_DOMAIN_VRAM:
>   		stats->vram += size;
>   		if (amdgpu_bo_in_cpu_visible_vram(bo))
>   			stats->visible_vram += size;
> +		if (shared)
> +			stats->vram_shared += size;
>   		break;
>   	case AMDGPU_GEM_DOMAIN_GTT:
>   		stats->gtt += size;
> +		if (shared)
> +			stats->gtt_shared += size;
>   		break;
>   	case AMDGPU_GEM_DOMAIN_CPU:
>   	default:
>   		stats->cpu += size;
> +		if (shared)
> +			stats->cpu_shared += size;
>   		break;
>   	}
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index d28e21baef16..0503af75dc26 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
>   struct amdgpu_mem_stats {
>   	/* current VRAM usage, includes visible VRAM */
>   	uint64_t vram;
> +	/* current shared VRAM usage, includes visible VRAM */
> +	uint64_t vram_shared;
>   	/* current visible VRAM usage */
>   	uint64_t visible_vram;
>   	/* current GTT usage */
>   	uint64_t gtt;
> +	/* current shared GTT usage */
> +	uint64_t gtt_shared;
>   	/* current system memory usage */
>   	uint64_t cpu;
> +	/* current shared system memory usage */
> +	uint64_t cpu_shared;
>   	/* sum of evicted buffers, includes visible VRAM */
>   	uint64_t evicted_vram;
>   	/* sum of evicted buffers due to CPU access */

Re: [PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Tvrtko Ursulin]

On 09/01/2024 07:56, Christian König wrote:
> Am 07.12.23 um 19:02 schrieb Alex Deucher:
>> Add shared stats.  Useful for seeing shared memory.
>>
>> v2: take dma-buf into account as well
>>
>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>> Cc: Rob Clark <robdclark@gmail.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
>>   3 files changed, 21 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>> index 5706b282a0c7..c7df7fa3459f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>> @@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p, 
>> struct drm_file *file)
>>              stats.requested_visible_vram/1024UL);
>>       drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
>>              stats.requested_gtt/1024UL);
>> +    drm_printf(p, "drm-shared-vram:\t%llu KiB\n", 
>> stats.vram_shared/1024UL);
>> +    drm_printf(p, "drm-shared-gtt:\t%llu KiB\n", 
>> stats.gtt_shared/1024UL);
>> +    drm_printf(p, "drm-shared-cpu:\t%llu KiB\n", 
>> stats.cpu_shared/1024UL);
>> +
>>       for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
>>           if (!usage[hw_ip])
>>               continue;
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index d79b4ca1ecfc..1b37d95475b8 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>                 struct amdgpu_mem_stats *stats)
>>   {
>>       uint64_t size = amdgpu_bo_size(bo);
>> +    struct drm_gem_object *obj;
>>       unsigned int domain;
>> +    bool shared;
>>       /* Abort if the BO doesn't currently have a backing store */
>>       if (!bo->tbo.resource)
>>           return;
>> +    obj = &bo->tbo.base;
>> +    shared = (obj->handle_count > 1) || obj->dma_buf;
> 
> I still think that looking at handle_count is the completely wrong 
> approach, we should really only look at obj->dma_buf.

Yeah it is all a bit tricky with the handle table walk. I don't think it 
is even possible to claim it is shared with obj->dma_buf could be the 
same process creating say via udmabuf and importing into drm. It is a 
wild scenario yes, but it could be private memory in that case. Not sure 
where it would leave us if we said this is just a limitation of a BO 
based tracking.

Would adding a new category "imported" help?

Hmm or we simply change drm-usage-stats.rst:

"""
- drm-shared-<region>: <uint> [KiB|MiB]

The total size of buffers that are shared with another file (ie. have 
more than than a single handle).
"""

Changing ie into eg coule be get our of jail free card to allow the 
"(obj->handle_count > 1) || obj->dma_buf;" condition?

Because of the shared with another _file_ wording would cover my wild 
udmabuf self-import case. Unless there are more such creative private 
import options.

Regards,

Tvrtko

> 
> Regards,
> Christian.
> 
>> +
>>       domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>>       switch (domain) {
>>       case AMDGPU_GEM_DOMAIN_VRAM:
>>           stats->vram += size;
>>           if (amdgpu_bo_in_cpu_visible_vram(bo))
>>               stats->visible_vram += size;
>> +        if (shared)
>> +            stats->vram_shared += size;
>>           break;
>>       case AMDGPU_GEM_DOMAIN_GTT:
>>           stats->gtt += size;
>> +        if (shared)
>> +            stats->gtt_shared += size;
>>           break;
>>       case AMDGPU_GEM_DOMAIN_CPU:
>>       default:
>>           stats->cpu += size;
>> +        if (shared)
>> +            stats->cpu_shared += size;
>>           break;
>>       }
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> index d28e21baef16..0503af75dc26 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>> @@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
>>   struct amdgpu_mem_stats {
>>       /* current VRAM usage, includes visible VRAM */
>>       uint64_t vram;
>> +    /* current shared VRAM usage, includes visible VRAM */
>> +    uint64_t vram_shared;
>>       /* current visible VRAM usage */
>>       uint64_t visible_vram;
>>       /* current GTT usage */
>>       uint64_t gtt;
>> +    /* current shared GTT usage */
>> +    uint64_t gtt_shared;
>>       /* current system memory usage */
>>       uint64_t cpu;
>> +    /* current shared system memory usage */
>> +    uint64_t cpu_shared;
>>       /* sum of evicted buffers, includes visible VRAM */
>>       uint64_t evicted_vram;
>>       /* sum of evicted buffers due to CPU access */
> 

Re: [PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Daniel Vetter]

On Tue, Jan 09, 2024 at 09:30:15AM +0000, Tvrtko Ursulin wrote:
> 
> On 09/01/2024 07:56, Christian König wrote:
> > Am 07.12.23 um 19:02 schrieb Alex Deucher:
> > > Add shared stats.  Useful for seeing shared memory.
> > > 
> > > v2: take dma-buf into account as well
> > > 
> > > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
> > >   3 files changed, 21 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > > index 5706b282a0c7..c7df7fa3459f 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > > @@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p,
> > > struct drm_file *file)
> > >              stats.requested_visible_vram/1024UL);
> > >       drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
> > >              stats.requested_gtt/1024UL);
> > > +    drm_printf(p, "drm-shared-vram:\t%llu KiB\n",
> > > stats.vram_shared/1024UL);
> > > +    drm_printf(p, "drm-shared-gtt:\t%llu KiB\n",
> > > stats.gtt_shared/1024UL);
> > > +    drm_printf(p, "drm-shared-cpu:\t%llu KiB\n",
> > > stats.cpu_shared/1024UL);
> > > +
> > >       for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
> > >           if (!usage[hw_ip])
> > >               continue;
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > > index d79b4ca1ecfc..1b37d95475b8 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > > @@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
> > >                 struct amdgpu_mem_stats *stats)
> > >   {
> > >       uint64_t size = amdgpu_bo_size(bo);
> > > +    struct drm_gem_object *obj;
> > >       unsigned int domain;
> > > +    bool shared;
> > >       /* Abort if the BO doesn't currently have a backing store */
> > >       if (!bo->tbo.resource)
> > >           return;
> > > +    obj = &bo->tbo.base;
> > > +    shared = (obj->handle_count > 1) || obj->dma_buf;
> > 
> > I still think that looking at handle_count is the completely wrong
> > approach, we should really only look at obj->dma_buf.
> 
> Yeah it is all a bit tricky with the handle table walk. I don't think it is
> even possible to claim it is shared with obj->dma_buf could be the same
> process creating say via udmabuf and importing into drm. It is a wild
> scenario yes, but it could be private memory in that case. Not sure where it
> would leave us if we said this is just a limitation of a BO based tracking.
> 
> Would adding a new category "imported" help?
> 
> Hmm or we simply change drm-usage-stats.rst:
> 
> """
> - drm-shared-<region>: <uint> [KiB|MiB]
> 
> The total size of buffers that are shared with another file (ie. have more
> than than a single handle).
> """
> 
> Changing ie into eg coule be get our of jail free card to allow the
> "(obj->handle_count > 1) || obj->dma_buf;" condition?
> 
> Because of the shared with another _file_ wording would cover my wild
> udmabuf self-import case. Unless there are more such creative private import
> options.

Yeah I think clarifying that we can only track sharing with other fd and
have no idea whether this means sharing with another process or not is
probably simplest. Maybe not exactly what users want, but still the
roughly best-case approximation we can deliver somewhat cheaply.

Also maybe time for a drm_gem_buffer_object_is_shared() helper so we don't
copypaste this all over and then end up in divergent conditions? I'm
guessing that there's going to be a bunch of drivers which needs this
little helper to add drm-shared-* stats to their fdinfo ...

Cheers, Sima
> 
> Regards,
> 
> Tvrtko
> 
> > 
> > Regards,
> > Christian.
> > 
> > > +
> > >       domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> > >       switch (domain) {
> > >       case AMDGPU_GEM_DOMAIN_VRAM:
> > >           stats->vram += size;
> > >           if (amdgpu_bo_in_cpu_visible_vram(bo))
> > >               stats->visible_vram += size;
> > > +        if (shared)
> > > +            stats->vram_shared += size;
> > >           break;
> > >       case AMDGPU_GEM_DOMAIN_GTT:
> > >           stats->gtt += size;
> > > +        if (shared)
> > > +            stats->gtt_shared += size;
> > >           break;
> > >       case AMDGPU_GEM_DOMAIN_CPU:
> > >       default:
> > >           stats->cpu += size;
> > > +        if (shared)
> > > +            stats->cpu_shared += size;
> > >           break;
> > >       }
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > > index d28e21baef16..0503af75dc26 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > > @@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
> > >   struct amdgpu_mem_stats {
> > >       /* current VRAM usage, includes visible VRAM */
> > >       uint64_t vram;
> > > +    /* current shared VRAM usage, includes visible VRAM */
> > > +    uint64_t vram_shared;
> > >       /* current visible VRAM usage */
> > >       uint64_t visible_vram;
> > >       /* current GTT usage */
> > >       uint64_t gtt;
> > > +    /* current shared GTT usage */
> > > +    uint64_t gtt_shared;
> > >       /* current system memory usage */
> > >       uint64_t cpu;
> > > +    /* current shared system memory usage */
> > > +    uint64_t cpu_shared;
> > >       /* sum of evicted buffers, includes visible VRAM */
> > >       uint64_t evicted_vram;
> > >       /* sum of evicted buffers due to CPU access */
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Tvrtko Ursulin]

On 09/01/2024 12:54, Daniel Vetter wrote:
> On Tue, Jan 09, 2024 at 09:30:15AM +0000, Tvrtko Ursulin wrote:
>>
>> On 09/01/2024 07:56, Christian König wrote:
>>> Am 07.12.23 um 19:02 schrieb Alex Deucher:
>>>> Add shared stats.  Useful for seeing shared memory.
>>>>
>>>> v2: take dma-buf into account as well
>>>>
>>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
>>>>    3 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>> index 5706b282a0c7..c7df7fa3459f 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>> @@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p,
>>>> struct drm_file *file)
>>>>               stats.requested_visible_vram/1024UL);
>>>>        drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
>>>>               stats.requested_gtt/1024UL);
>>>> +    drm_printf(p, "drm-shared-vram:\t%llu KiB\n",
>>>> stats.vram_shared/1024UL);
>>>> +    drm_printf(p, "drm-shared-gtt:\t%llu KiB\n",
>>>> stats.gtt_shared/1024UL);
>>>> +    drm_printf(p, "drm-shared-cpu:\t%llu KiB\n",
>>>> stats.cpu_shared/1024UL);
>>>> +
>>>>        for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
>>>>            if (!usage[hw_ip])
>>>>                continue;
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> index d79b4ca1ecfc..1b37d95475b8 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>> @@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>>>                  struct amdgpu_mem_stats *stats)
>>>>    {
>>>>        uint64_t size = amdgpu_bo_size(bo);
>>>> +    struct drm_gem_object *obj;
>>>>        unsigned int domain;
>>>> +    bool shared;
>>>>        /* Abort if the BO doesn't currently have a backing store */
>>>>        if (!bo->tbo.resource)
>>>>            return;
>>>> +    obj = &bo->tbo.base;
>>>> +    shared = (obj->handle_count > 1) || obj->dma_buf;
>>>
>>> I still think that looking at handle_count is the completely wrong
>>> approach, we should really only look at obj->dma_buf.
>>
>> Yeah it is all a bit tricky with the handle table walk. I don't think it is
>> even possible to claim it is shared with obj->dma_buf could be the same
>> process creating say via udmabuf and importing into drm. It is a wild
>> scenario yes, but it could be private memory in that case. Not sure where it
>> would leave us if we said this is just a limitation of a BO based tracking.
>>
>> Would adding a new category "imported" help?
>>
>> Hmm or we simply change drm-usage-stats.rst:
>>
>> """
>> - drm-shared-<region>: <uint> [KiB|MiB]
>>
>> The total size of buffers that are shared with another file (ie. have more
>> than than a single handle).
>> """
>>
>> Changing ie into eg coule be get our of jail free card to allow the
>> "(obj->handle_count > 1) || obj->dma_buf;" condition?
>>
>> Because of the shared with another _file_ wording would cover my wild
>> udmabuf self-import case. Unless there are more such creative private import
>> options.
> 
> Yeah I think clarifying that we can only track sharing with other fd and
> have no idea whether this means sharing with another process or not is
> probably simplest. Maybe not exactly what users want, but still the
> roughly best-case approximation we can deliver somewhat cheaply.
> 
> Also maybe time for a drm_gem_buffer_object_is_shared() helper so we don't
> copypaste this all over and then end up in divergent conditions? I'm
> guessing that there's going to be a bunch of drivers which needs this
> little helper to add drm-shared-* stats to their fdinfo ...

Yeah I agree that works and i915 would need to use the helper too.

I would only suggest to name it so the meaning of shared is obviously 
only about the fdinfo memory stats and no one gets a more meaningful 
idea about its semantics.

We have drm_show_memory_stats and drm_print_memory_stats exported so 
perhaps something like drm_object_is_shared_for_memory_stats, 
drm_object_is_memory_stats_shared, drm_memory_stats_object_is_shared?

And s/ie/eg/ in the above quoted drm-usage-stats.rst.

Regards,

Tvrtko

> 
> Cheers, Sima
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards,
>>> Christian.
>>>
>>>> +
>>>>        domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>>>>        switch (domain) {
>>>>        case AMDGPU_GEM_DOMAIN_VRAM:
>>>>            stats->vram += size;
>>>>            if (amdgpu_bo_in_cpu_visible_vram(bo))
>>>>                stats->visible_vram += size;
>>>> +        if (shared)
>>>> +            stats->vram_shared += size;
>>>>            break;
>>>>        case AMDGPU_GEM_DOMAIN_GTT:
>>>>            stats->gtt += size;
>>>> +        if (shared)
>>>> +            stats->gtt_shared += size;
>>>>            break;
>>>>        case AMDGPU_GEM_DOMAIN_CPU:
>>>>        default:
>>>>            stats->cpu += size;
>>>> +        if (shared)
>>>> +            stats->cpu_shared += size;
>>>>            break;
>>>>        }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>> index d28e21baef16..0503af75dc26 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>> @@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
>>>>    struct amdgpu_mem_stats {
>>>>        /* current VRAM usage, includes visible VRAM */
>>>>        uint64_t vram;
>>>> +    /* current shared VRAM usage, includes visible VRAM */
>>>> +    uint64_t vram_shared;
>>>>        /* current visible VRAM usage */
>>>>        uint64_t visible_vram;
>>>>        /* current GTT usage */
>>>>        uint64_t gtt;
>>>> +    /* current shared GTT usage */
>>>> +    uint64_t gtt_shared;
>>>>        /* current system memory usage */
>>>>        uint64_t cpu;
>>>> +    /* current shared system memory usage */
>>>> +    uint64_t cpu_shared;
>>>>        /* sum of evicted buffers, includes visible VRAM */
>>>>        uint64_t evicted_vram;
>>>>        /* sum of evicted buffers due to CPU access */
>>>
> 

Re: [PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Daniel Vetter]

On Tue, 9 Jan 2024 at 14:25, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 09/01/2024 12:54, Daniel Vetter wrote:
> > On Tue, Jan 09, 2024 at 09:30:15AM +0000, Tvrtko Ursulin wrote:
> >>
> >> On 09/01/2024 07:56, Christian König wrote:
> >>> Am 07.12.23 um 19:02 schrieb Alex Deucher:
> >>>> Add shared stats.  Useful for seeing shared memory.
> >>>>
> >>>> v2: take dma-buf into account as well
> >>>>
> >>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >>>> Cc: Rob Clark <robdclark@gmail.com>
> >>>> ---
> >>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
> >>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
> >>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
> >>>>    3 files changed, 21 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> >>>> index 5706b282a0c7..c7df7fa3459f 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> >>>> @@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p,
> >>>> struct drm_file *file)
> >>>>               stats.requested_visible_vram/1024UL);
> >>>>        drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
> >>>>               stats.requested_gtt/1024UL);
> >>>> +    drm_printf(p, "drm-shared-vram:\t%llu KiB\n",
> >>>> stats.vram_shared/1024UL);
> >>>> +    drm_printf(p, "drm-shared-gtt:\t%llu KiB\n",
> >>>> stats.gtt_shared/1024UL);
> >>>> +    drm_printf(p, "drm-shared-cpu:\t%llu KiB\n",
> >>>> stats.cpu_shared/1024UL);
> >>>> +
> >>>>        for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
> >>>>            if (!usage[hw_ip])
> >>>>                continue;
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>>> index d79b4ca1ecfc..1b37d95475b8 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> >>>> @@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
> >>>>                  struct amdgpu_mem_stats *stats)
> >>>>    {
> >>>>        uint64_t size = amdgpu_bo_size(bo);
> >>>> +    struct drm_gem_object *obj;
> >>>>        unsigned int domain;
> >>>> +    bool shared;
> >>>>        /* Abort if the BO doesn't currently have a backing store */
> >>>>        if (!bo->tbo.resource)
> >>>>            return;
> >>>> +    obj = &bo->tbo.base;
> >>>> +    shared = (obj->handle_count > 1) || obj->dma_buf;
> >>>
> >>> I still think that looking at handle_count is the completely wrong
> >>> approach, we should really only look at obj->dma_buf.
> >>
> >> Yeah it is all a bit tricky with the handle table walk. I don't think it is
> >> even possible to claim it is shared with obj->dma_buf could be the same
> >> process creating say via udmabuf and importing into drm. It is a wild
> >> scenario yes, but it could be private memory in that case. Not sure where it
> >> would leave us if we said this is just a limitation of a BO based tracking.
> >>
> >> Would adding a new category "imported" help?
> >>
> >> Hmm or we simply change drm-usage-stats.rst:
> >>
> >> """
> >> - drm-shared-<region>: <uint> [KiB|MiB]
> >>
> >> The total size of buffers that are shared with another file (ie. have more
> >> than than a single handle).
> >> """
> >>
> >> Changing ie into eg coule be get our of jail free card to allow the
> >> "(obj->handle_count > 1) || obj->dma_buf;" condition?
> >>
> >> Because of the shared with another _file_ wording would cover my wild
> >> udmabuf self-import case. Unless there are more such creative private import
> >> options.
> >
> > Yeah I think clarifying that we can only track sharing with other fd and
> > have no idea whether this means sharing with another process or not is
> > probably simplest. Maybe not exactly what users want, but still the
> > roughly best-case approximation we can deliver somewhat cheaply.
> >
> > Also maybe time for a drm_gem_buffer_object_is_shared() helper so we don't
> > copypaste this all over and then end up in divergent conditions? I'm
> > guessing that there's going to be a bunch of drivers which needs this
> > little helper to add drm-shared-* stats to their fdinfo ...
>
> Yeah I agree that works and i915 would need to use the helper too.
>
> I would only suggest to name it so the meaning of shared is obviously
> only about the fdinfo memory stats and no one gets a more meaningful
> idea about its semantics.
>
> We have drm_show_memory_stats and drm_print_memory_stats exported so
> perhaps something like drm_object_is_shared_for_memory_stats,
> drm_object_is_memory_stats_shared, drm_memory_stats_object_is_shared?
>
> And s/ie/eg/ in the above quoted drm-usage-stats.rst.

Ack on making it clear this helper would be for fdinfo memory stats
only. Sounds like a good idea to stop people from finding really
creative uses ...
-Sima

>
> Regards,
>
> Tvrtko
>
> >
> > Cheers, Sima
> >>
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>>
> >>> Regards,
> >>> Christian.
> >>>
> >>>> +
> >>>>        domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
> >>>>        switch (domain) {
> >>>>        case AMDGPU_GEM_DOMAIN_VRAM:
> >>>>            stats->vram += size;
> >>>>            if (amdgpu_bo_in_cpu_visible_vram(bo))
> >>>>                stats->visible_vram += size;
> >>>> +        if (shared)
> >>>> +            stats->vram_shared += size;
> >>>>            break;
> >>>>        case AMDGPU_GEM_DOMAIN_GTT:
> >>>>            stats->gtt += size;
> >>>> +        if (shared)
> >>>> +            stats->gtt_shared += size;
> >>>>            break;
> >>>>        case AMDGPU_GEM_DOMAIN_CPU:
> >>>>        default:
> >>>>            stats->cpu += size;
> >>>> +        if (shared)
> >>>> +            stats->cpu_shared += size;
> >>>>            break;
> >>>>        }
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> >>>> index d28e21baef16..0503af75dc26 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> >>>> @@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
> >>>>    struct amdgpu_mem_stats {
> >>>>        /* current VRAM usage, includes visible VRAM */
> >>>>        uint64_t vram;
> >>>> +    /* current shared VRAM usage, includes visible VRAM */
> >>>> +    uint64_t vram_shared;
> >>>>        /* current visible VRAM usage */
> >>>>        uint64_t visible_vram;
> >>>>        /* current GTT usage */
> >>>>        uint64_t gtt;
> >>>> +    /* current shared GTT usage */
> >>>> +    uint64_t gtt_shared;
> >>>>        /* current system memory usage */
> >>>>        uint64_t cpu;
> >>>> +    /* current shared system memory usage */
> >>>> +    uint64_t cpu_shared;
> >>>>        /* sum of evicted buffers, includes visible VRAM */
> >>>>        uint64_t evicted_vram;
> >>>>        /* sum of evicted buffers due to CPU access */
> >>>
> >



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Christian König]

Am 09.01.24 um 15:26 schrieb Daniel Vetter:
> On Tue, 9 Jan 2024 at 14:25, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>> On 09/01/2024 12:54, Daniel Vetter wrote:
>>> On Tue, Jan 09, 2024 at 09:30:15AM +0000, Tvrtko Ursulin wrote:
>>>> On 09/01/2024 07:56, Christian König wrote:
>>>>> Am 07.12.23 um 19:02 schrieb Alex Deucher:
>>>>>> Add shared stats.  Useful for seeing shared memory.
>>>>>>
>>>>>> v2: take dma-buf into account as well
>>>>>>
>>>>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
>>>>>>     3 files changed, 21 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>>>> index 5706b282a0c7..c7df7fa3459f 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>>>> @@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p,
>>>>>> struct drm_file *file)
>>>>>>                stats.requested_visible_vram/1024UL);
>>>>>>         drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
>>>>>>                stats.requested_gtt/1024UL);
>>>>>> +    drm_printf(p, "drm-shared-vram:\t%llu KiB\n",
>>>>>> stats.vram_shared/1024UL);
>>>>>> +    drm_printf(p, "drm-shared-gtt:\t%llu KiB\n",
>>>>>> stats.gtt_shared/1024UL);
>>>>>> +    drm_printf(p, "drm-shared-cpu:\t%llu KiB\n",
>>>>>> stats.cpu_shared/1024UL);
>>>>>> +
>>>>>>         for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
>>>>>>             if (!usage[hw_ip])
>>>>>>                 continue;
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> index d79b4ca1ecfc..1b37d95475b8 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>> @@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct amdgpu_bo *bo,
>>>>>>                   struct amdgpu_mem_stats *stats)
>>>>>>     {
>>>>>>         uint64_t size = amdgpu_bo_size(bo);
>>>>>> +    struct drm_gem_object *obj;
>>>>>>         unsigned int domain;
>>>>>> +    bool shared;
>>>>>>         /* Abort if the BO doesn't currently have a backing store */
>>>>>>         if (!bo->tbo.resource)
>>>>>>             return;
>>>>>> +    obj = &bo->tbo.base;
>>>>>> +    shared = (obj->handle_count > 1) || obj->dma_buf;
>>>>> I still think that looking at handle_count is the completely wrong
>>>>> approach, we should really only look at obj->dma_buf.
>>>> Yeah it is all a bit tricky with the handle table walk. I don't think it is
>>>> even possible to claim it is shared with obj->dma_buf could be the same
>>>> process creating say via udmabuf and importing into drm. It is a wild
>>>> scenario yes, but it could be private memory in that case. Not sure where it
>>>> would leave us if we said this is just a limitation of a BO based tracking.
>>>>
>>>> Would adding a new category "imported" help?
>>>>
>>>> Hmm or we simply change drm-usage-stats.rst:
>>>>
>>>> """
>>>> - drm-shared-<region>: <uint> [KiB|MiB]
>>>>
>>>> The total size of buffers that are shared with another file (ie. have more
>>>> than than a single handle).
>>>> """
>>>>
>>>> Changing ie into eg coule be get our of jail free card to allow the
>>>> "(obj->handle_count > 1) || obj->dma_buf;" condition?
>>>>
>>>> Because of the shared with another _file_ wording would cover my wild
>>>> udmabuf self-import case. Unless there are more such creative private import
>>>> options.
>>> Yeah I think clarifying that we can only track sharing with other fd and
>>> have no idea whether this means sharing with another process or not is
>>> probably simplest. Maybe not exactly what users want, but still the
>>> roughly best-case approximation we can deliver somewhat cheaply.
>>>
>>> Also maybe time for a drm_gem_buffer_object_is_shared() helper so we don't
>>> copypaste this all over and then end up in divergent conditions? I'm
>>> guessing that there's going to be a bunch of drivers which needs this
>>> little helper to add drm-shared-* stats to their fdinfo ...
>> Yeah I agree that works and i915 would need to use the helper too.
>>
>> I would only suggest to name it so the meaning of shared is obviously
>> only about the fdinfo memory stats and no one gets a more meaningful
>> idea about its semantics.
>>
>> We have drm_show_memory_stats and drm_print_memory_stats exported so
>> perhaps something like drm_object_is_shared_for_memory_stats,
>> drm_object_is_memory_stats_shared, drm_memory_stats_object_is_shared?

+ for drm_object_is_shared_for_memory_stats().

>>
>> And s/ie/eg/ in the above quoted drm-usage-stats.rst.
> Ack on making it clear this helper would be for fdinfo memory stats
> only. Sounds like a good idea to stop people from finding really
> creative uses ...

It's astonishing how defensive the development process has become :)

Cheers,
Christian.

> -Sima
>
>> Regards,
>>
>> Tvrtko
>>
>>> Cheers, Sima
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>> +
>>>>>>         domain = amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>>>>>>         switch (domain) {
>>>>>>         case AMDGPU_GEM_DOMAIN_VRAM:
>>>>>>             stats->vram += size;
>>>>>>             if (amdgpu_bo_in_cpu_visible_vram(bo))
>>>>>>                 stats->visible_vram += size;
>>>>>> +        if (shared)
>>>>>> +            stats->vram_shared += size;
>>>>>>             break;
>>>>>>         case AMDGPU_GEM_DOMAIN_GTT:
>>>>>>             stats->gtt += size;
>>>>>> +        if (shared)
>>>>>> +            stats->gtt_shared += size;
>>>>>>             break;
>>>>>>         case AMDGPU_GEM_DOMAIN_CPU:
>>>>>>         default:
>>>>>>             stats->cpu += size;
>>>>>> +        if (shared)
>>>>>> +            stats->cpu_shared += size;
>>>>>>             break;
>>>>>>         }
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>>>> index d28e21baef16..0503af75dc26 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>>>> @@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
>>>>>>     struct amdgpu_mem_stats {
>>>>>>         /* current VRAM usage, includes visible VRAM */
>>>>>>         uint64_t vram;
>>>>>> +    /* current shared VRAM usage, includes visible VRAM */
>>>>>> +    uint64_t vram_shared;
>>>>>>         /* current visible VRAM usage */
>>>>>>         uint64_t visible_vram;
>>>>>>         /* current GTT usage */
>>>>>>         uint64_t gtt;
>>>>>> +    /* current shared GTT usage */
>>>>>> +    uint64_t gtt_shared;
>>>>>>         /* current system memory usage */
>>>>>>         uint64_t cpu;
>>>>>> +    /* current shared system memory usage */
>>>>>> +    uint64_t cpu_shared;
>>>>>>         /* sum of evicted buffers, includes visible VRAM */
>>>>>>         uint64_t evicted_vram;
>>>>>>         /* sum of evicted buffers due to CPU access */
>
>

Re: [PATCH 2/2] drm/amdgpu: add shared fdinfo stats

[Tvrtko Ursulin]

On 09/01/2024 14:57, Christian König wrote:
> Am 09.01.24 um 15:26 schrieb Daniel Vetter:
>> On Tue, 9 Jan 2024 at 14:25, Tvrtko Ursulin
>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>
>>> On 09/01/2024 12:54, Daniel Vetter wrote:
>>>> On Tue, Jan 09, 2024 at 09:30:15AM +0000, Tvrtko Ursulin wrote:
>>>>> On 09/01/2024 07:56, Christian König wrote:
>>>>>> Am 07.12.23 um 19:02 schrieb Alex Deucher:
>>>>>>> Add shared stats.  Useful for seeing shared memory.
>>>>>>>
>>>>>>> v2: take dma-buf into account as well
>>>>>>>
>>>>>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>>>> ---
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
>>>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
>>>>>>>     3 files changed, 21 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>>>>> index 5706b282a0c7..c7df7fa3459f 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
>>>>>>> @@ -97,6 +97,10 @@ void amdgpu_show_fdinfo(struct drm_printer *p,
>>>>>>> struct drm_file *file)
>>>>>>>                stats.requested_visible_vram/1024UL);
>>>>>>>         drm_printf(p, "amd-requested-gtt:\t%llu KiB\n",
>>>>>>>                stats.requested_gtt/1024UL);
>>>>>>> +    drm_printf(p, "drm-shared-vram:\t%llu KiB\n",
>>>>>>> stats.vram_shared/1024UL);
>>>>>>> +    drm_printf(p, "drm-shared-gtt:\t%llu KiB\n",
>>>>>>> stats.gtt_shared/1024UL);
>>>>>>> +    drm_printf(p, "drm-shared-cpu:\t%llu KiB\n",
>>>>>>> stats.cpu_shared/1024UL);
>>>>>>> +
>>>>>>>         for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) {
>>>>>>>             if (!usage[hw_ip])
>>>>>>>                 continue;
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>> index d79b4ca1ecfc..1b37d95475b8 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>>>>>>> @@ -1287,25 +1287,36 @@ void amdgpu_bo_get_memory(struct 
>>>>>>> amdgpu_bo *bo,
>>>>>>>                   struct amdgpu_mem_stats *stats)
>>>>>>>     {
>>>>>>>         uint64_t size = amdgpu_bo_size(bo);
>>>>>>> +    struct drm_gem_object *obj;
>>>>>>>         unsigned int domain;
>>>>>>> +    bool shared;
>>>>>>>         /* Abort if the BO doesn't currently have a backing store */
>>>>>>>         if (!bo->tbo.resource)
>>>>>>>             return;
>>>>>>> +    obj = &bo->tbo.base;
>>>>>>> +    shared = (obj->handle_count > 1) || obj->dma_buf;
>>>>>> I still think that looking at handle_count is the completely wrong
>>>>>> approach, we should really only look at obj->dma_buf.
>>>>> Yeah it is all a bit tricky with the handle table walk. I don't 
>>>>> think it is
>>>>> even possible to claim it is shared with obj->dma_buf could be the 
>>>>> same
>>>>> process creating say via udmabuf and importing into drm. It is a wild
>>>>> scenario yes, but it could be private memory in that case. Not sure 
>>>>> where it
>>>>> would leave us if we said this is just a limitation of a BO based 
>>>>> tracking.
>>>>>
>>>>> Would adding a new category "imported" help?
>>>>>
>>>>> Hmm or we simply change drm-usage-stats.rst:
>>>>>
>>>>> """
>>>>> - drm-shared-<region>: <uint> [KiB|MiB]
>>>>>
>>>>> The total size of buffers that are shared with another file (ie. 
>>>>> have more
>>>>> than than a single handle).
>>>>> """
>>>>>
>>>>> Changing ie into eg coule be get our of jail free card to allow the
>>>>> "(obj->handle_count > 1) || obj->dma_buf;" condition?
>>>>>
>>>>> Because of the shared with another _file_ wording would cover my wild
>>>>> udmabuf self-import case. Unless there are more such creative 
>>>>> private import
>>>>> options.
>>>> Yeah I think clarifying that we can only track sharing with other fd 
>>>> and
>>>> have no idea whether this means sharing with another process or not is
>>>> probably simplest. Maybe not exactly what users want, but still the
>>>> roughly best-case approximation we can deliver somewhat cheaply.
>>>>
>>>> Also maybe time for a drm_gem_buffer_object_is_shared() helper so we 
>>>> don't
>>>> copypaste this all over and then end up in divergent conditions? I'm
>>>> guessing that there's going to be a bunch of drivers which needs this
>>>> little helper to add drm-shared-* stats to their fdinfo ...
>>> Yeah I agree that works and i915 would need to use the helper too.
>>>
>>> I would only suggest to name it so the meaning of shared is obviously
>>> only about the fdinfo memory stats and no one gets a more meaningful
>>> idea about its semantics.
>>>
>>> We have drm_show_memory_stats and drm_print_memory_stats exported so
>>> perhaps something like drm_object_is_shared_for_memory_stats,
>>> drm_object_is_memory_stats_shared, drm_memory_stats_object_is_shared?
> 
> + for drm_object_is_shared_for_memory_stats().

Hmmm although I probably meant to write 
drm_gem_object_is_shared_for_memory_stats. Since drm_mode_object seems 
to have claimed the drm_object_ function name space.

>>> And s/ie/eg/ in the above quoted drm-usage-stats.rst.
>> Ack on making it clear this helper would be for fdinfo memory stats
>> only. Sounds like a good idea to stop people from finding really
>> creative uses ...
> 
> It's astonishing how defensive the development process has become :)

A bit. :) But probably justified in this case.

Regards,

Tvrtko

> 
> Cheers,
> Christian.
> 
>> -Sima
>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>> Cheers, Sima
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>>
>>>>>> Regards,
>>>>>> Christian.
>>>>>>
>>>>>>> +
>>>>>>>         domain = 
>>>>>>> amdgpu_mem_type_to_domain(bo->tbo.resource->mem_type);
>>>>>>>         switch (domain) {
>>>>>>>         case AMDGPU_GEM_DOMAIN_VRAM:
>>>>>>>             stats->vram += size;
>>>>>>>             if (amdgpu_bo_in_cpu_visible_vram(bo))
>>>>>>>                 stats->visible_vram += size;
>>>>>>> +        if (shared)
>>>>>>> +            stats->vram_shared += size;
>>>>>>>             break;
>>>>>>>         case AMDGPU_GEM_DOMAIN_GTT:
>>>>>>>             stats->gtt += size;
>>>>>>> +        if (shared)
>>>>>>> +            stats->gtt_shared += size;
>>>>>>>             break;
>>>>>>>         case AMDGPU_GEM_DOMAIN_CPU:
>>>>>>>         default:
>>>>>>>             stats->cpu += size;
>>>>>>> +        if (shared)
>>>>>>> +            stats->cpu_shared += size;
>>>>>>>             break;
>>>>>>>         }
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>>>>> index d28e21baef16..0503af75dc26 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
>>>>>>> @@ -138,12 +138,18 @@ struct amdgpu_bo_vm {
>>>>>>>     struct amdgpu_mem_stats {
>>>>>>>         /* current VRAM usage, includes visible VRAM */
>>>>>>>         uint64_t vram;
>>>>>>> +    /* current shared VRAM usage, includes visible VRAM */
>>>>>>> +    uint64_t vram_shared;
>>>>>>>         /* current visible VRAM usage */
>>>>>>>         uint64_t visible_vram;
>>>>>>>         /* current GTT usage */
>>>>>>>         uint64_t gtt;
>>>>>>> +    /* current shared GTT usage */
>>>>>>> +    uint64_t gtt_shared;
>>>>>>>         /* current system memory usage */
>>>>>>>         uint64_t cpu;
>>>>>>> +    /* current shared system memory usage */
>>>>>>> +    uint64_t cpu_shared;
>>>>>>>         /* sum of evicted buffers, includes visible VRAM */
>>>>>>>         uint64_t evicted_vram;
>>>>>>>         /* sum of evicted buffers due to CPU access */
>>
>>
> 

Re: [PATCH 0/2] fdinfo shared stats

[Alex Deucher]

On Thu, Dec 7, 2023 at 1:03 PM Alex Deucher <alexander.deucher@amd.com> wrote:
>
> We had a request to add shared buffer stats to fdinfo for amdgpu and
> while implementing that, Christian mentioned that just looking at
> the GEM handle count doesn't take into account buffers shared with other
> subsystems like V4L or RDMA.  Those subsystems don't use GEM, so it
> doesn't really matter from a GPU top perspective, but it's more
> correct if you actually want to see shared buffers.

Any thoughts on this?  Seem reasonable?

Alex

>
> Alex Deucher (2):
>   drm: update drm_show_memory_stats() for dma-bufs
>   drm/amdgpu: add shared fdinfo stats
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
>  drivers/gpu/drm/drm_file.c                 |  2 +-
>  4 files changed, 22 insertions(+), 1 deletion(-)
>
> --
> 2.42.0
>

Re: [PATCH 0/2] fdinfo shared stats

[Alex Deucher]

Ping on this series again?

Alex

On Wed, Dec 13, 2023 at 4:13 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Thu, Dec 7, 2023 at 1:03 PM Alex Deucher <alexander.deucher@amd.com> wrote:
> >
> > We had a request to add shared buffer stats to fdinfo for amdgpu and
> > while implementing that, Christian mentioned that just looking at
> > the GEM handle count doesn't take into account buffers shared with other
> > subsystems like V4L or RDMA.  Those subsystems don't use GEM, so it
> > doesn't really matter from a GPU top perspective, but it's more
> > correct if you actually want to see shared buffers.
>
> Any thoughts on this?  Seem reasonable?
>
> Alex
>
> >
> > Alex Deucher (2):
> >   drm: update drm_show_memory_stats() for dma-bufs
> >   drm/amdgpu: add shared fdinfo stats
> >
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  4 ++++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++++++
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  6 ++++++
> >  drivers/gpu/drm/drm_file.c                 |  2 +-
> >  4 files changed, 22 insertions(+), 1 deletion(-)
> >
> > --
> > 2.42.0
> >