Re: [Intel-gfx] [RFC 3/6] drm: Add fdinfo memory stats

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/04/2023 15:36, Rob Clark wrote:
> On Tue, Apr 18, 2023 at 7:19 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 18/04/2023 14:49, Rob Clark wrote:
>>> On Tue, Apr 18, 2023 at 2:00 AM Tvrtko Ursulin
>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>>
>>>>
>>>> On 17/04/2023 20:39, Rob Clark wrote:
>>>>> On Mon, Apr 17, 2023 at 8:56 AM Tvrtko Ursulin
>>>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>>>>
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>
>>>>>> Add support to dump GEM stats to fdinfo.
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>> ---
>>>>>>     Documentation/gpu/drm-usage-stats.rst | 12 +++++++
>>>>>>     drivers/gpu/drm/drm_file.c            | 52 +++++++++++++++++++++++++++
>>>>>>     include/drm/drm_drv.h                 |  7 ++++
>>>>>>     include/drm/drm_file.h                |  8 +++++
>>>>>>     4 files changed, 79 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
>>>>>> index 2ab32c40e93c..8273a41b2fb0 100644
>>>>>> --- a/Documentation/gpu/drm-usage-stats.rst
>>>>>> +++ b/Documentation/gpu/drm-usage-stats.rst
>>>>>> @@ -21,6 +21,7 @@ File format specification
>>>>>>
>>>>>>     - File shall contain one key value pair per one line of text.
>>>>>>     - Colon character (`:`) must be used to delimit keys and values.
>>>>>> +- Caret (`^`) is also a reserved character.
>>>>>
>>>>> this doesn't solve the problem that led me to drm-$CATEGORY-memory... ;-)
>>>>
>>>> Could you explain or remind me with a link to a previous explanation?
>>>
>>> How is userspace supposed to know that "drm-memory-foo" is a memory
>>> type "foo" but drm-memory-foo^size is not memory type "foo^size"?
>>
>> Are you referring to nvtop?
> 
> I'm not referring to any particular app.  It could even be some app
> that isn't even written yet but started with an already existing
> kernel without this change.  It is just a general point about forwards
> compatibility of old userspace with new kernel.  And it doesn't really
> matter what special character you use.  You can't retroactively define
> some newly special characters.

What you see does not work if we output both legacy and new key with 
extra category? Userspace which hardcode the name keep working, and 
userspace which parses region names as opaque strings also keeps working 
just shows more entries.

Regards,

Tvrtko

> 
> BR,
> -R
> 
>> Indeed that one hardcodes:
>>
>>     static const char drm_amdgpu_vram[] = "drm-memory-vram";
>>
>> And does brute strcmp on it:
>>
>>     } else if (!strcmp(key, drm_amdgpu_vram_old) || !strcmp(key, drm_amdgpu_vram)) {
>>
>> So okay for that one, if we need to keep it working I just change this in my RFC:
>>
>> """
>> Resident category is identical to the drm-memory-<str> key and two should be
>> mutually exclusive.
>> """
>>
>> Into this:
>>
>> """
>> Resident category is identical to the drm-memory-<str> key and where the categorized one is present the legacy one must be too in order to preserve backward compatibility.
>> """
>>
>> Addition to my RFC:
>>
>> ...
>>          for (i = 0; i < num; i++) {
>>                  if (!regions[i]) /* Allow sparse name arrays. */
>>                          continue;
>>
>>                  print_stat(p, "size", regions[i], stats[i].size);
>>                  print_stat(p, "shared", regions[i], stats[i].shared);
>> +               print_stat(p, "", regions[i], stats[i].resident);
>>                  print_stat(p, "resident", regions[i], stats[i].resident);
>>                  print_stat(p, "purgeable", regions[i], stats[i].purgeable);
>>                  print_stat(p, "active", regions[i], stats[i].active);
>>          }
>> ...
>>
>> Results in output like this (in theory, if/when amdgpu takes on the extended format):
>>
>> drm-memory-vram-size: ... KiB
>> drm-memory-vram: $same KiB
>> drm-memory-vram-resident: $same KiB
>> drm-memory-vram-...:
>>
>> Regards,
>>
>> Tvrtko
>>
>> P.S. Would a slash instead of a caret be prettier?
>>
>>>> What tool barfs on it and how? Spirit of drm-usage-stats.pl is that for
>>>> both drm-engine-<str>: and drm-memory-<str>: generic userspace is
>>>> supposed to take the whole <std> as opaque and just enumerate all it finds.
>>>>
>>>> Gputop manages to do that with engines names it knows _nothing_ about.
>>>> If it worked with memory regions it would just show the list of new
>>>> regions as for example "system^resident", "system^active". Then tools
>>>> can be extended to understand it better and provide a sub-breakdown if
>>>> wanted.
>>>>
>>>> Ugly not, we can change from caret to something nicer, unless I am
>>>> missing something it works, no?
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>>
>>>>> (also, it is IMHO rather ugly)
>>>>>
>>>>> BR,
>>>>> -R
>>>>>
>>>>>>     - All keys shall be prefixed with `drm-`.
>>>>>>     - Whitespace between the delimiter and first non-whitespace character shall be
>>>>>>       ignored when parsing.
>>>>>> @@ -105,6 +106,17 @@ object 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.
>>>>>>
>>>>>> +- drm-memory-<str>^size:      <uint> [KiB|MiB]
>>>>>> +- drm-memory-<str>^shared:    <uint> [KiB|MiB]
>>>>>> +- drm-memory-<str>^resident:  <uint> [KiB|MiB]
>>>>>> +- drm-memory-<str>^purgeable: <uint> [KiB|MiB]
>>>>>> +- drm-memory-<str>^active:    <uint> [KiB|MiB]
>>>>>> +
>>>>>> +Resident category is identical to the drm-memory-<str> key and two should be
>>>>>> +mutually exclusive.
>>>>>> +
>>>>>> +TODO more description text...
>>>>>> +
>>>>>>     - drm-cycles-<str> <uint>
>>>>>>
>>>>>>     Engine identifier string must be the same as the one specified in the
>>>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>>>> index 37b4f76a5191..e202f79e816d 100644
>>>>>> --- a/drivers/gpu/drm/drm_file.c
>>>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>>>> @@ -42,6 +42,7 @@
>>>>>>     #include <drm/drm_client.h>
>>>>>>     #include <drm/drm_drv.h>
>>>>>>     #include <drm/drm_file.h>
>>>>>> +#include <drm/drm_gem.h>
>>>>>>     #include <drm/drm_print.h>
>>>>>>
>>>>>>     #include "drm_crtc_internal.h"
>>>>>> @@ -871,6 +872,54 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
>>>>>>     }
>>>>>>     EXPORT_SYMBOL(drm_send_event);
>>>>>>
>>>>>> +static void
>>>>>> +print_stat(struct drm_printer *p, const char *stat, const char *region, u64 sz)
>>>>>> +{
>>>>>> +       const char *units[] = {"", " KiB", " MiB"};
>>>>>> +       unsigned int u;
>>>>>> +
>>>>>> +       if (sz == ~0ull) /* Not supported by the driver. */
>>>>>> +               return;
>>>>>> +
>>>>>> +       for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
>>>>>> +               if (sz < SZ_1K)
>>>>>> +                       break;
>>>>>> +               sz = div_u64(sz, SZ_1K);
>>>>>> +       }
>>>>>> +
>>>>>> +       drm_printf(p, "drm-memory-%s^%s:\t%llu%s\n",
>>>>>> +                  region, stat, sz, units[u]);
>>>>>> +}
>>>>>> +
>>>>>> +static void print_memory_stats(struct drm_printer *p, struct drm_file *file)
>>>>>> +{
>>>>>> +       struct drm_device *dev = file->minor->dev;
>>>>>> +       struct drm_fdinfo_memory_stat *stats;
>>>>>> +       unsigned int num, i;
>>>>>> +       char **regions;
>>>>>> +
>>>>>> +       regions = dev->driver->query_fdinfo_memory_regions(dev, &num);
>>>>>> +
>>>>>> +       stats = kcalloc(num, sizeof(*stats), GFP_KERNEL);
>>>>>> +       if (!stats)
>>>>>> +               return;
>>>>>> +
>>>>>> +       dev->driver->query_fdinfo_memory_stats(file, stats);
>>>>>> +
>>>>>> +       for (i = 0; i < num; i++) {
>>>>>> +               if (!regions[i]) /* Allow sparse name arrays. */
>>>>>> +                       continue;
>>>>>> +
>>>>>> +               print_stat(p, "size", regions[i], stats[i].size);
>>>>>> +               print_stat(p, "shared", regions[i], stats[i].shared);
>>>>>> +               print_stat(p, "resident", regions[i], stats[i].resident);
>>>>>> +               print_stat(p, "purgeable", regions[i], stats[i].purgeable);
>>>>>> +               print_stat(p, "active", regions[i], stats[i].active);
>>>>>> +       }
>>>>>> +
>>>>>> +       kfree(stats);
>>>>>> +}
>>>>>> +
>>>>>>     /**
>>>>>>      * drm_show_fdinfo - helper for drm file fops
>>>>>>      * @seq_file: output stream
>>>>>> @@ -900,6 +949,9 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
>>>>>>
>>>>>>            if (dev->driver->show_fdinfo)
>>>>>>                    dev->driver->show_fdinfo(&p, file);
>>>>>> +
>>>>>> +       if (dev->driver->query_fdinfo_memory_regions)
>>>>>> +               print_memory_stats(&p, file);
>>>>>>     }
>>>>>>     EXPORT_SYMBOL(drm_show_fdinfo);
>>>>>>
>>>>>> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
>>>>>> index 89e2706cac56..ccc1cd98d2aa 100644
>>>>>> --- a/include/drm/drm_drv.h
>>>>>> +++ b/include/drm/drm_drv.h
>>>>>> @@ -35,6 +35,7 @@
>>>>>>     #include <drm/drm_device.h>
>>>>>>
>>>>>>     struct drm_file;
>>>>>> +struct drm_fdinfo_memory_stat;
>>>>>>     struct drm_gem_object;
>>>>>>     struct drm_master;
>>>>>>     struct drm_minor;
>>>>>> @@ -408,6 +409,12 @@ struct drm_driver {
>>>>>>             */
>>>>>>            void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
>>>>>>
>>>>>> +       char ** (*query_fdinfo_memory_regions)(struct drm_device *dev,
>>>>>> +                                              unsigned int *num);
>>>>>> +
>>>>>> +       void (*query_fdinfo_memory_stats)(struct drm_file *f,
>>>>>> +                                         struct drm_fdinfo_memory_stat *stat);
>>>>>> +
>>>>>>            /** @major: driver major number */
>>>>>>            int major;
>>>>>>            /** @minor: driver minor number */
>>>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>>>> index 7d9b3c65cbc1..00d48beeac5c 100644
>>>>>> --- a/include/drm/drm_file.h
>>>>>> +++ b/include/drm/drm_file.h
>>>>>> @@ -375,6 +375,14 @@ struct drm_file {
>>>>>>     #endif
>>>>>>     };
>>>>>>
>>>>>> +struct drm_fdinfo_memory_stat {
>>>>>> +       u64 size;
>>>>>> +       u64 shared;
>>>>>> +       u64 resident;
>>>>>> +       u64 purgeable;
>>>>>> +       u64 active;
>>>>>> +};
>>>>>> +
>>>>>>     /**
>>>>>>      * drm_is_primary_client - is this an open file of the primary node
>>>>>>      * @file_priv: DRM file
>>>>>> --
>>>>>> 2.37.2
>>>>>>