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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

well, it shows nonsense entries.. I'd not call that "working"

But honestly we are wasting too many words on this.. we just can't
re-use the "drm-memory-<anything>" namespace, it is already burnt.
Full stop.

If you don't like the "drm-$CATEGORY-$REGION" workaround then we can
shorten to "drm-mem-$REGION-$CATEGORY" since that won't accidentally
match the existing "drm-memory-" pattern.

BR,
-R

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux