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"? BR, -R > 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 > >>