| Message ID | 20230417155613.4143258-4-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fdinfo alternative memory stats proposal | expand |
Am 17.04.23 um 17:56 schrieb Tvrtko Ursulin: > 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. > - 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] What exactly does size/shared/active mean here? If it means what I think it does I don't see how TTM based drivers should track that in the first place. Christian. > + > +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
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... ;-) (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 >
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? 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 >>
On 17/04/2023 17:20, Christian König wrote: > Am 17.04.23 um 17:56 schrieb Tvrtko Ursulin: >> 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. >> - 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] > > What exactly does size/shared/active mean here? > > If it means what I think it does I don't see how TTM based drivers > should track that in the first place. Size is an analogue to process VM size - maximum reachable/allocated memory belonging to a client. Shared could be IMO viewed as a bit dodgy and either could be dropped or needs to be better defined. For now I simply followed the implementation from Rob's RFC which is: if (obj->handle_count > 1) stats[0].shared += sz; I can see some usefulness to it but haven't thought much about semantics yet. Similar story with active which I think is not very useful. Implementation is like this: if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) stats[0].active += sz; For me it is too transient to bring much value over the resident category. I supposed only advantage is that resident (as does purgeable) needs driver cooperation while active can be done like about from DRM core. Although I am not a big fan of counting these stats from the core to begin with.. Regards, Tvrtko >> +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 >
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"? 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 > >>
On Tue, Apr 18, 2023 at 3:47 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 17/04/2023 17:20, Christian König wrote: > > Am 17.04.23 um 17:56 schrieb Tvrtko Ursulin: > >> 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. > >> - 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] > > > > What exactly does size/shared/active mean here? > > > > If it means what I think it does I don't see how TTM based drivers > > should track that in the first place. > > Size is an analogue to process VM size - maximum reachable/allocated > memory belonging to a client. > > Shared could be IMO viewed as a bit dodgy and either could be dropped or > needs to be better defined. For now I simply followed the implementation > from Rob's RFC which is: > > if (obj->handle_count > 1) > stats[0].shared += sz; > > I can see some usefulness to it but haven't thought much about semantics > yet. > > Similar story with active which I think is not very useful. > Implementation is like this: > > if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) > stats[0].active += sz; > > For me it is too transient to bring much value over the resident > category. I supposed only advantage is that resident (as does purgeable) > needs driver cooperation while active can be done like about from DRM > core. Although I am not a big fan of counting these stats from the core > to begin with.. Maybe there is a better way to track it, like setting an age/time when the buffer is last active (which could be made part of dma_resv to make it automatic). The question I really want to answer is more like "over the last T ms how many buffers were active". This is a useful metric esp when you think about a use-case like the browser where you might have a lot of textures/etc for your 80 different tabs but at any given time only a small subset is active and the rest can be swapped out to zram if needed. BR, -R > > Regards, > > Tvrtko > > >> +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 > >
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? 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 >>>>
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. 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 > >>>>
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 >>>>>>
On Tue, Apr 18, 2023 at 7:46 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > 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. 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 > >>>>>>
On 18/04/2023 17:13, Rob Clark wrote: > On Tue, Apr 18, 2023 at 7:46 AM Tvrtko Ursulin > <tvrtko.ursulin@linux.intel.com> wrote: >> 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. > > 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. I can live with that token reversal, it was not the primary motivation for my RFC as we have discussed that side of things already before I sketched my version up. But I also still don't get what doesn't work with what I described and you did not really address my specific questions with more than a "doesn't work" with not much details. Unless for you it starts and ends with "nonsense entries". If so then it seems there is no option than to disagree and move on. Again, I can accept the drm-$category-memory-$region. Regards, Tvrtko
On Tue, Apr 18, 2023 at 9:44 AM Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> wrote: > > > On 18/04/2023 17:13, Rob Clark wrote: > > On Tue, Apr 18, 2023 at 7:46 AM Tvrtko Ursulin > > <tvrtko.ursulin@linux.intel.com> wrote: > >> 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. > > > > 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. > > I can live with that token reversal, it was not the primary motivation > for my RFC as we have discussed that side of things already before I > sketched my version up. > > But I also still don't get what doesn't work with what I described and > you did not really address my specific questions with more than a > "doesn't work" with not much details. > > Unless for you it starts and ends with "nonsense entries". If so then it > seems there is no option than to disagree and move on. Again, I can > accept the drm-$category-memory-$region. Yeah, it's about "nonsense entries".. and I am perhaps being a bit pedantic about compatibility with old userspace (not like there are 100's of apps parsing drm fdinfo), but it is the principle of the matter ;-) BR, -R
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. - 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