[RFC 3/6] drm: Add fdinfo memory stats
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Tue Apr 18 14:18:46 UTC 2023
On 18/04/2023 14:49, Rob Clark wrote:
> On Tue, Apr 18, 2023 at 2:00 AM Tvrtko Ursulin
> <tvrtko.ursulin at 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 at linux.intel.com> wrote:
>>>>
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> Add support to dump GEM stats to fdinfo.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at 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
>>>>
More information about the dri-devel
mailing list