[PATCH] drm/amdgpu: add shared fdinfo stats

[Christian König]

Am 28.11.23 um 18:52 schrieb Rob Clark:
> On Tue, Nov 28, 2023 at 6:28 AM Alex Deucher <alexdeucher at gmail.com> wrote:
>> On Tue, Nov 28, 2023 at 9:17 AM Christian König
>> <ckoenig.leichtzumerken at gmail.com> wrote:
>>> Am 17.11.23 um 20:56 schrieb Alex Deucher:
>>>> Add shared stats.  Useful for seeing shared memory.
>>>>
>>>> Signed-off-by: Alex Deucher <alexander.deucher at amd.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..c24f7b2c04c1 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;
>>> Interesting approach but I don't think that this is correct.
>>>
>>> The handle_count is basically how many GEM handles are there for BO, so
>>> for example it doesn't catch sharing things with V4L.
>>>
>>> What we should probably rather do is to take a look if
>>> bo->tbo.base.dma_buf is NULL or not.
>> +Rob, dri-devel
>>
>> This is what the generic drm helper code does.  See
>> drm_show_memory_stats().  If that is not correct that code should
>> probably be fixed too.
> OTOH, v4l doesn't expose fdinfo.  What "shared" is telling you is
> whether the BO is counted multiple times when you look at all
> processes fdinfo.

Oh, then that's not fully correct either.

You can have multiple handles for the same GEM object in a single client 
as well.

This for example happens when you interact with KMS to get an handle for 
a displayed BO.

DRM flink was one of the major other reasons, but I hope we are not 
using that widely any more.

What exactly is the purpose? To avoid counting a BO multiple times 
because you go over the handles in the common code?

If yes than I would say use obj->handle_count in the common code and 
ob->dma_buf in amdgpu because that is certainly unique.

Regards,
Christian.

>
> But I guess it would be ok to look for obj->handle_count > 1 || obj->dma_buf
>
> BR,
> -R
>
>> Alex
>>
>>> 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 */