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

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

 




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?

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]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux