On Tue, Apr 18, 2023 at 7:46 AM Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > > > On 18/04/2023 15:36, Rob Clark wrote: > > On Tue, Apr 18, 2023 at 7:19 AM Tvrtko Ursulin > > <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > >> > >> > >> On 18/04/2023 14:49, Rob Clark wrote: > >>> On Tue, Apr 18, 2023 at 2:00 AM Tvrtko Ursulin > >>> <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > >>>> > >>>> > >>>> On 17/04/2023 20:39, Rob Clark wrote: > >>>>> On Mon, Apr 17, 2023 at 8:56 AM Tvrtko Ursulin > >>>>> <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote: > >>>>>> > >>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > >>>>>> > >>>>>> Add support to dump GEM stats to fdinfo. > >>>>>> > >>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx> > >>>>>> --- > >>>>>> 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. well, it shows nonsense entries.. I'd not call that "working" But honestly we are wasting too many words on this.. we just can't re-use the "drm-memory-<anything>" namespace, it is already burnt. Full stop. If you don't like the "drm-$CATEGORY-$REGION" workaround then we can shorten to "drm-mem-$REGION-$CATEGORY" since that won't accidentally match the existing "drm-memory-" pattern. BR, -R > 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 > >>>>>>