[Intel-gfx] [RFC 0/6] fdinfo alternative memory stats proposal

[Intel-gfx] [RFC 0/6] fdinfo alternative memory stats proposal

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

As discussed in the Rob's thread here is a slightly alternative idea on what to
expose and how.

DRM core is still defining a list of common memory categories but it is now up
to drivers to fill in the data and opt into the feature.

There is also no aggregated category and memory regions are always specified in
key names.

Two driver vfuncs are added where DRM core queries the number and names of
memory regions supported by the driver instance, and second where the driver
fills in the usage statistics for centrally defined memory categories.

I think this is a more future proof option since by moving the stat filling to
drivers they are able to show not only the GEM handles but all used memory. For
instance in case of i915 we have contexts, ring buffers, status pages and page
tables all backed by GEM objects too.

It also opens up a route for reporting sub-object size backing store granularity
and allows for not traversing under the file_table lock for drivers which are
able to do it in a more light-weight manner. For the former one example could be
simply adding TTM region helpers.

Not having aggregated counters means we do not need to add a second set of keys
as soon as the first driver wants to provide a more detailed view. And userspace
can trivially aggregate itself anyway.

At the same time two trivial helpers are provided who want to show just the
basic stats.

I have also tried to preserve the drm-memory-$region naming by reserving a
special character ('^') as a suffix ie. drm-memory-$region^$category. Unless I
am missing something this should be compatible with any existing parsers which
would just see more memory regions with more specific names. And they can be
updated to support the format extension.

Series is a bit rough so for discussion only.

Rob Clark (1):
  drm: Add common fdinfo helper

Tvrtko Ursulin (5):
  drm/i915: Use the fdinfo helper
  drm: Add fdinfo memory stats
  drm: Add simple fdinfo memory helpers
  drm/msm: Add basic memory stats
  drm/i915: Implement fdinfo memory stats printing

 Documentation/gpu/drm-usage-stats.rst  |  22 +++-
 drivers/gpu/drm/drm_file.c             | 132 +++++++++++++++++++
 drivers/gpu/drm/i915/i915_driver.c     |  11 +-
 drivers/gpu/drm/i915/i915_drm_client.c | 167 +++++++++++++++++--------
 drivers/gpu/drm/i915/i915_drm_client.h |  30 ++---
 drivers/gpu/drm/i915/i915_drv.h        |   4 +-
 drivers/gpu/drm/i915/i915_gem.c        |   6 +-
 drivers/gpu/drm/msm/msm_drv.c          |   4 +
 include/drm/drm_drv.h                  |  14 +++
 include/drm/drm_file.h                 |  18 +++
 10 files changed, 324 insertions(+), 84 deletions(-)

-- 
2.37.2

[Intel-gfx] [RFC 1/6] drm: Add common fdinfo helper

[Tvrtko Ursulin]

From: Rob Clark <robdclark@chromium.org>

Handle a bit of the boiler-plate in a single case, and make it easier to
add some core tracked stats.

v2: Update drm-usage-stats.rst, 64b client-id, rename drm_show_fdinfo

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 Documentation/gpu/drm-usage-stats.rst | 10 +++++++-
 drivers/gpu/drm/drm_file.c            | 35 +++++++++++++++++++++++++++
 include/drm/drm_drv.h                 |  7 ++++++
 include/drm/drm_file.h                |  4 +++
 4 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index b46327356e80..2ab32c40e93c 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -126,7 +126,15 @@ percentage utilization of the engine, whereas drm-engine-<str> only reflects
 time active without considering what frequency the engine is operating as a
 percentage of it's maximum frequency.
 
+Implementation Details
+======================
+
+Drivers should use drm_show_fdinfo() in their `struct file_operations`, and
+implement &drm_driver.show_fdinfo if they wish to provide any stats which
+are not provided by drm_show_fdinfo().  But even driver specific stats should
+be documented above and where possible, aligned with other drivers.
+
 Driver specific implementations
-===============================
+-------------------------------
 
 :ref:`i915-usage-stats`
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index c1018c470047..37b4f76a5191 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -148,6 +148,7 @@ bool drm_dev_needs_global_mutex(struct drm_device *dev)
  */
 struct drm_file *drm_file_alloc(struct drm_minor *minor)
 {
+	static atomic64_t ident = ATOMIC_INIT(0);
 	struct drm_device *dev = minor->dev;
 	struct drm_file *file;
 	int ret;
@@ -156,6 +157,8 @@ struct drm_file *drm_file_alloc(struct drm_minor *minor)
 	if (!file)
 		return ERR_PTR(-ENOMEM);
 
+	/* Get a unique identifier for fdinfo: */
+	file->client_id = atomic64_inc_return(&ident);
 	file->pid = get_pid(task_tgid(current));
 	file->minor = minor;
 
@@ -868,6 +871,38 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
 }
 EXPORT_SYMBOL(drm_send_event);
 
+/**
+ * drm_show_fdinfo - helper for drm file fops
+ * @seq_file: output stream
+ * @f: the device file instance
+ *
+ * Helper to implement fdinfo, for userspace to query usage stats, etc, of a
+ * process using the GPU.  See also &drm_driver.show_fdinfo.
+ *
+ * For text output format description please see Documentation/gpu/drm-usage-stats.rst
+ */
+void drm_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	struct drm_file *file = f->private_data;
+	struct drm_device *dev = file->minor->dev;
+	struct drm_printer p = drm_seq_file_printer(m);
+
+	drm_printf(&p, "drm-driver:\t%s\n", dev->driver->name);
+	drm_printf(&p, "drm-client-id:\t%llu\n", file->client_id);
+
+	if (dev_is_pci(dev->dev)) {
+		struct pci_dev *pdev = to_pci_dev(dev->dev);
+
+		drm_printf(&p, "drm-pdev:\t%04x:%02x:%02x.%d\n",
+			   pci_domain_nr(pdev->bus), pdev->bus->number,
+			   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
+	}
+
+	if (dev->driver->show_fdinfo)
+		dev->driver->show_fdinfo(&p, file);
+}
+EXPORT_SYMBOL(drm_show_fdinfo);
+
 /**
  * mock_drm_getfile - Create a new struct file for the drm device
  * @minor: drm minor to wrap (e.g. #drm_device.primary)
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index b419c59c4bef..89e2706cac56 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -401,6 +401,13 @@ struct drm_driver {
 			       struct drm_device *dev, uint32_t handle,
 			       uint64_t *offset);
 
+	/**
+	 * @show_fdinfo:
+	 *
+	 * Print device specific fdinfo.  See Documentation/gpu/drm-usage-stats.rst.
+	 */
+	void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
+
 	/** @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 ecffe24e2b1b..7d9b3c65cbc1 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -258,6 +258,9 @@ struct drm_file {
 	/** @pid: Process that opened this file. */
 	struct pid *pid;
 
+	/** @client_id: A unique id for fdinfo */
+	u64 client_id;
+
 	/** @magic: Authentication magic, see @authenticated. */
 	drm_magic_t magic;
 
@@ -438,6 +441,7 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e);
 void drm_send_event_timestamp_locked(struct drm_device *dev,
 				     struct drm_pending_event *e,
 				     ktime_t timestamp);
+void drm_show_fdinfo(struct seq_file *m, struct file *f);
 
 struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);
 
-- 
2.37.2

[Intel-gfx] [RFC 2/6] drm/i915: Use the fdinfo helper

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Use the common fdinfo helper for printing the basics. Remove now unused
client id allocation code.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/i915/i915_driver.c     |  6 +--
 drivers/gpu/drm/i915/i915_drm_client.c | 65 ++++----------------------
 drivers/gpu/drm/i915/i915_drm_client.h | 22 ++-------
 drivers/gpu/drm/i915/i915_drv.h        |  2 -
 drivers/gpu/drm/i915/i915_gem.c        |  6 +--
 5 files changed, 18 insertions(+), 83 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index a52db8a80900..6493548c69bf 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -244,8 +244,6 @@ static int i915_driver_early_probe(struct drm_i915_private *dev_priv)
 	if (ret < 0)
 		goto err_rootgt;
 
-	i915_drm_clients_init(&dev_priv->clients, dev_priv);
-
 	i915_gem_init_early(dev_priv);
 
 	/* This must be called before any calls to HAS_PCH_* */
@@ -279,7 +277,6 @@ static void i915_driver_late_release(struct drm_i915_private *dev_priv)
 	intel_power_domains_cleanup(dev_priv);
 	i915_gem_cleanup_early(dev_priv);
 	intel_gt_driver_late_release_all(dev_priv);
-	i915_drm_clients_fini(&dev_priv->clients);
 	intel_region_ttm_device_fini(dev_priv);
 	vlv_suspend_cleanup(dev_priv);
 	i915_workqueues_cleanup(dev_priv);
@@ -1700,7 +1697,7 @@ static const struct file_operations i915_driver_fops = {
 	.compat_ioctl = i915_ioc32_compat_ioctl,
 	.llseek = noop_llseek,
 #ifdef CONFIG_PROC_FS
-	.show_fdinfo = i915_drm_client_fdinfo,
+	.show_fdinfo = drm_show_fdinfo,
 #endif
 };
 
@@ -1800,6 +1797,7 @@ static const struct drm_driver i915_drm_driver = {
 	.open = i915_driver_open,
 	.lastclose = i915_driver_lastclose,
 	.postclose = i915_driver_postclose,
+	.show_fdinfo = i915_drm_client_fdinfo,
 
 	.prime_handle_to_fd = drm_gem_prime_handle_to_fd,
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
index e8fa172ebe5e..c654984189f7 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.c
+++ b/drivers/gpu/drm/i915/i915_drm_client.c
@@ -17,64 +17,29 @@
 #include "i915_gem.h"
 #include "i915_utils.h"
 
-void i915_drm_clients_init(struct i915_drm_clients *clients,
-			   struct drm_i915_private *i915)
-{
-	clients->i915 = i915;
-	clients->next_id = 0;
-
-	xa_init_flags(&clients->xarray, XA_FLAGS_ALLOC | XA_FLAGS_LOCK_IRQ);
-}
-
-struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients)
+struct i915_drm_client *i915_drm_client_alloc(void)
 {
 	struct i915_drm_client *client;
-	struct xarray *xa = &clients->xarray;
-	int ret;
 
 	client = kzalloc(sizeof(*client), GFP_KERNEL);
 	if (!client)
-		return ERR_PTR(-ENOMEM);
-
-	xa_lock_irq(xa);
-	ret = __xa_alloc_cyclic(xa, &client->id, client, xa_limit_32b,
-				&clients->next_id, GFP_KERNEL);
-	xa_unlock_irq(xa);
-	if (ret < 0)
-		goto err;
+		return NULL;
 
 	kref_init(&client->kref);
 	spin_lock_init(&client->ctx_lock);
 	INIT_LIST_HEAD(&client->ctx_list);
-	client->clients = clients;
 
 	return client;
-
-err:
-	kfree(client);
-
-	return ERR_PTR(ret);
 }
 
 void __i915_drm_client_free(struct kref *kref)
 {
 	struct i915_drm_client *client =
 		container_of(kref, typeof(*client), kref);
-	struct xarray *xa = &client->clients->xarray;
-	unsigned long flags;
 
-	xa_lock_irqsave(xa, flags);
-	__xa_erase(xa, client->id);
-	xa_unlock_irqrestore(xa, flags);
 	kfree(client);
 }
 
-void i915_drm_clients_fini(struct i915_drm_clients *clients)
-{
-	GEM_BUG_ON(!xa_empty(&clients->xarray));
-	xa_destroy(&clients->xarray);
-}
-
 #ifdef CONFIG_PROC_FS
 static const char * const uabi_class_names[] = {
 	[I915_ENGINE_CLASS_RENDER] = "render",
@@ -101,38 +66,34 @@ static u64 busy_add(struct i915_gem_context *ctx, unsigned int class)
 }
 
 static void
-show_client_class(struct seq_file *m,
+show_client_class(struct drm_printer *p,
+		  struct drm_i915_private *i915,
 		  struct i915_drm_client *client,
 		  unsigned int class)
 {
-	const struct list_head *list = &client->ctx_list;
+	const unsigned int capacity = i915->engine_uabi_class_count[class];
 	u64 total = atomic64_read(&client->past_runtime[class]);
-	const unsigned int capacity =
-		client->clients->i915->engine_uabi_class_count[class];
 	struct i915_gem_context *ctx;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(ctx, list, client_link)
+	list_for_each_entry_rcu(ctx, &client->ctx_list, client_link)
 		total += busy_add(ctx, class);
 	rcu_read_unlock();
 
 	if (capacity)
-		seq_printf(m, "drm-engine-%s:\t%llu ns\n",
+		drm_printf(p, "drm-engine-%s:\t%llu ns\n",
 			   uabi_class_names[class], total);
 
 	if (capacity > 1)
-		seq_printf(m, "drm-engine-capacity-%s:\t%u\n",
+		drm_printf(p, "drm-engine-capacity-%s:\t%u\n",
 			   uabi_class_names[class],
 			   capacity);
 }
 
-void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
+void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
 {
-	struct drm_file *file = f->private_data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct drm_i915_private *i915 = file_priv->i915;
-	struct i915_drm_client *client = file_priv->client;
-	struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
 	unsigned int i;
 
 	/*
@@ -141,12 +102,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
 	 * ******************************************************************
 	 */
 
-	seq_printf(m, "drm-driver:\t%s\n", i915->drm.driver->name);
-	seq_printf(m, "drm-pdev:\t%04x:%02x:%02x.%d\n",
-		   pci_domain_nr(pdev->bus), pdev->bus->number,
-		   PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
-	seq_printf(m, "drm-client-id:\t%u\n", client->id);
-
 	/*
 	 * Temporarily skip showing client engine information with GuC submission till
 	 * fetching engine busyness is implemented in the GuC submission backend
@@ -155,6 +110,6 @@ void i915_drm_client_fdinfo(struct seq_file *m, struct file *f)
 		return;
 
 	for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
-		show_client_class(m, client, i);
+		show_client_class(p, i915, file_priv->client, i);
 }
 #endif
diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
index 69496af996d9..4c18b99e10a4 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.h
+++ b/drivers/gpu/drm/i915/i915_drm_client.h
@@ -9,20 +9,13 @@
 #include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/spinlock.h>
-#include <linux/xarray.h>
 
 #include <uapi/drm/i915_drm.h>
 
 #define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE
 
-struct drm_i915_private;
-
-struct i915_drm_clients {
-	struct drm_i915_private *i915;
-
-	struct xarray xarray;
-	u32 next_id;
-};
+struct drm_file;
+struct drm_printer;
 
 struct i915_drm_client {
 	struct kref kref;
@@ -32,17 +25,12 @@ struct i915_drm_client {
 	spinlock_t ctx_lock; /* For add/remove from ctx_list. */
 	struct list_head ctx_list; /* List of contexts belonging to client. */
 
-	struct i915_drm_clients *clients;
-
 	/**
 	 * @past_runtime: Accumulation of pphwsp runtimes from closed contexts.
 	 */
 	atomic64_t past_runtime[I915_LAST_UABI_ENGINE_CLASS + 1];
 };
 
-void i915_drm_clients_init(struct i915_drm_clients *clients,
-			   struct drm_i915_private *i915);
-
 static inline struct i915_drm_client *
 i915_drm_client_get(struct i915_drm_client *client)
 {
@@ -57,12 +45,10 @@ static inline void i915_drm_client_put(struct i915_drm_client *client)
 	kref_put(&client->kref, __i915_drm_client_free);
 }
 
-struct i915_drm_client *i915_drm_client_add(struct i915_drm_clients *clients);
+struct i915_drm_client *i915_drm_client_alloc(void);
 
 #ifdef CONFIG_PROC_FS
-void i915_drm_client_fdinfo(struct seq_file *m, struct file *f);
+void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file);
 #endif
 
-void i915_drm_clients_fini(struct i915_drm_clients *clients);
-
 #endif /* !__I915_DRM_CLIENT_H__ */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index fe7eeafe9cff..eb739fb9cdbb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -347,8 +347,6 @@ struct drm_i915_private {
 
 	struct i915_pmu pmu;
 
-	struct i915_drm_clients clients;
-
 	/* The TTM device structure. */
 	struct ttm_device bdev;
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0a78bdbd36b1..db4c4af5f4b7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1306,11 +1306,9 @@ int i915_gem_open(struct drm_i915_private *i915, struct drm_file *file)
 	if (!file_priv)
 		goto err_alloc;
 
-	client = i915_drm_client_add(&i915->clients);
-	if (IS_ERR(client)) {
-		ret = PTR_ERR(client);
+	client = i915_drm_client_alloc();
+	if (!client)
 		goto err_client;
-	}
 
 	file->driver_priv = file_priv;
 	file_priv->i915 = i915;
-- 
2.37.2

[Intel-gfx] [RFC 3/6] drm: Add fdinfo memory stats

[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]
+
+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

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

[Christian König]

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

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

[Tvrtko Ursulin]

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
> 

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

[Rob Clark]

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
> >

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

[Rob Clark]

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
>

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

[Tvrtko Ursulin]

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
>>

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

[Rob Clark]

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
> >>

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

[Tvrtko Ursulin]

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
>>>>

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

[Rob Clark]

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
> >>>>

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

[Tvrtko Ursulin]

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
>>>>>>

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

[Rob Clark]

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
> >>>>>>

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

[Tvrtko Ursulin]

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

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

[Rob Clark]

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

[Intel-gfx] [RFC 4/6] drm: Add simple fdinfo memory helpers

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

For drivers who only wish to show one memory region called 'system,
and only account the GEM buffer object handles under it.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/drm_file.c | 45 ++++++++++++++++++++++++++++++++++++++
 include/drm/drm_file.h     |  6 +++++
 2 files changed, 51 insertions(+)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index e202f79e816d..1e70669dddf7 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -872,6 +872,51 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
 }
 EXPORT_SYMBOL(drm_send_event);
 
+static void
+add_obj(struct drm_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
+{
+	u64 sz = obj->size;
+
+	stats[0].size += sz;
+
+	if (obj->handle_count > 1)
+		stats[0].shared += sz;
+
+	if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
+		stats[0].active += sz;
+
+	/* Not supported. */
+	stats[0].resident = ~0ull;
+	stats[0].purgeable = ~0ull;
+}
+
+char **
+drm_query_fdinfo_system_region(struct drm_device *dev, unsigned int *num)
+{
+	static char *region[] = {
+		"system",
+	};
+
+	*num = 1;
+
+	return region;
+}
+EXPORT_SYMBOL(drm_query_fdinfo_system_region);
+
+void
+drm_query_fdinfo_system_memory(struct drm_file *file,
+			       struct drm_fdinfo_memory_stat *stats)
+{
+	struct drm_gem_object *obj;
+	int id;
+
+	spin_lock(&file->table_lock);
+	idr_for_each_entry(&file->object_idr, obj, id)
+		add_obj(obj, stats);
+	spin_unlock(&file->table_lock);
+}
+EXPORT_SYMBOL(drm_query_fdinfo_system_memory);
+
 static void
 print_stat(struct drm_printer *p, const char *stat, const char *region, u64 sz)
 {
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 00d48beeac5c..dd7c6fb2c975 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -383,6 +383,12 @@ struct drm_fdinfo_memory_stat {
 	u64 active;
 };
 
+char **drm_query_fdinfo_system_region(struct drm_device *dev,
+				      unsigned int *num);
+void drm_query_fdinfo_system_memory(struct drm_file *file,
+				    struct drm_fdinfo_memory_stat *stats);
+
+
 /**
  * drm_is_primary_client - is this an open file of the primary node
  * @file_priv: DRM file
-- 
2.37.2

Re: [Intel-gfx] [RFC 4/6] drm: Add simple fdinfo memory helpers

[Rob Clark]

On Mon, Apr 17, 2023 at 8:56 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> For drivers who only wish to show one memory region called 'system,
> and only account the GEM buffer object handles under it.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/drm_file.c | 45 ++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_file.h     |  6 +++++
>  2 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index e202f79e816d..1e70669dddf7 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -872,6 +872,51 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
>  }
>  EXPORT_SYMBOL(drm_send_event);
>
> +static void
> +add_obj(struct drm_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
> +{
> +       u64 sz = obj->size;
> +
> +       stats[0].size += sz;
> +
> +       if (obj->handle_count > 1)
> +               stats[0].shared += sz;
> +
> +       if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
> +               stats[0].active += sz;
> +
> +       /* Not supported. */
> +       stats[0].resident = ~0ull;
> +       stats[0].purgeable = ~0ull;

Hmm, this kinda makes the simple helper not very useful.  In my
version, you get something that is useful for all UMA drivers (which
all, IIRC, have some form of madv ioctl).  I was kinda imagining that
for ttm drivers, my print_memory_stats() would just become a helper
and that TTM (or "multi-region") drivers would have their own thing.

BR,
-R

> +}
> +
> +char **
> +drm_query_fdinfo_system_region(struct drm_device *dev, unsigned int *num)
> +{
> +       static char *region[] = {
> +               "system",
> +       };
> +
> +       *num = 1;
> +
> +       return region;
> +}
> +EXPORT_SYMBOL(drm_query_fdinfo_system_region);
> +
> +void
> +drm_query_fdinfo_system_memory(struct drm_file *file,
> +                              struct drm_fdinfo_memory_stat *stats)
> +{
> +       struct drm_gem_object *obj;
> +       int id;
> +
> +       spin_lock(&file->table_lock);
> +       idr_for_each_entry(&file->object_idr, obj, id)
> +               add_obj(obj, stats);
> +       spin_unlock(&file->table_lock);
> +}
> +EXPORT_SYMBOL(drm_query_fdinfo_system_memory);
> +
>  static void
>  print_stat(struct drm_printer *p, const char *stat, const char *region, u64 sz)
>  {
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 00d48beeac5c..dd7c6fb2c975 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -383,6 +383,12 @@ struct drm_fdinfo_memory_stat {
>         u64 active;
>  };
>
> +char **drm_query_fdinfo_system_region(struct drm_device *dev,
> +                                     unsigned int *num);
> +void drm_query_fdinfo_system_memory(struct drm_file *file,
> +                                   struct drm_fdinfo_memory_stat *stats);
> +
> +
>  /**
>   * drm_is_primary_client - is this an open file of the primary node
>   * @file_priv: DRM file
> --
> 2.37.2
>

Re: [Intel-gfx] [RFC 4/6] drm: Add simple fdinfo memory helpers

[Tvrtko Ursulin]

On 18/04/2023 18:18, 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>
>>
>> For drivers who only wish to show one memory region called 'system,
>> and only account the GEM buffer object handles under it.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/drm_file.c | 45 ++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_file.h     |  6 +++++
>>   2 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index e202f79e816d..1e70669dddf7 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -872,6 +872,51 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
>>   }
>>   EXPORT_SYMBOL(drm_send_event);
>>
>> +static void
>> +add_obj(struct drm_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
>> +{
>> +       u64 sz = obj->size;
>> +
>> +       stats[0].size += sz;
>> +
>> +       if (obj->handle_count > 1)
>> +               stats[0].shared += sz;
>> +
>> +       if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
>> +               stats[0].active += sz;
>> +
>> +       /* Not supported. */
>> +       stats[0].resident = ~0ull;
>> +       stats[0].purgeable = ~0ull;
> 
> Hmm, this kinda makes the simple helper not very useful.  In my
> version, you get something that is useful for all UMA drivers (which
> all, IIRC, have some form of madv ioctl).  I was kinda imagining that
> for ttm drivers, my print_memory_stats() would just become a helper
> and that TTM (or "multi-region") drivers would have their own thing.

Hm how? Your version also needed a driver specific vfunc:

+static enum drm_gem_object_status msm_gem_status(struct drm_gem_object *obj)
+{
+	struct msm_gem_object *msm_obj = to_msm_bo(obj);
+	enum drm_gem_object_status status = 0;
+
+	if (msm_obj->pages)
+		status |= DRM_GEM_OBJECT_RESIDENT;
+
+	if (msm_obj->madv == MSM_MADV_DONTNEED)
+		status |= DRM_GEM_OBJECT_PURGEABLE;
+
+	return status;
+}

Regards,

Tvrtko

> 
> BR,
> -R
> 
>> +}
>> +
>> +char **
>> +drm_query_fdinfo_system_region(struct drm_device *dev, unsigned int *num)
>> +{
>> +       static char *region[] = {
>> +               "system",
>> +       };
>> +
>> +       *num = 1;
>> +
>> +       return region;
>> +}
>> +EXPORT_SYMBOL(drm_query_fdinfo_system_region);
>> +
>> +void
>> +drm_query_fdinfo_system_memory(struct drm_file *file,
>> +                              struct drm_fdinfo_memory_stat *stats)
>> +{
>> +       struct drm_gem_object *obj;
>> +       int id;
>> +
>> +       spin_lock(&file->table_lock);
>> +       idr_for_each_entry(&file->object_idr, obj, id)
>> +               add_obj(obj, stats);
>> +       spin_unlock(&file->table_lock);
>> +}
>> +EXPORT_SYMBOL(drm_query_fdinfo_system_memory);
>> +
>>   static void
>>   print_stat(struct drm_printer *p, const char *stat, const char *region, u64 sz)
>>   {
>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>> index 00d48beeac5c..dd7c6fb2c975 100644
>> --- a/include/drm/drm_file.h
>> +++ b/include/drm/drm_file.h
>> @@ -383,6 +383,12 @@ struct drm_fdinfo_memory_stat {
>>          u64 active;
>>   };
>>
>> +char **drm_query_fdinfo_system_region(struct drm_device *dev,
>> +                                     unsigned int *num);
>> +void drm_query_fdinfo_system_memory(struct drm_file *file,
>> +                                   struct drm_fdinfo_memory_stat *stats);
>> +
>> +
>>   /**
>>    * drm_is_primary_client - is this an open file of the primary node
>>    * @file_priv: DRM file
>> --
>> 2.37.2
>>

Re: [Intel-gfx] [RFC 4/6] drm: Add simple fdinfo memory helpers

[Rob Clark]

On Wed, Apr 19, 2023 at 6:16 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 18/04/2023 18:18, 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>
> >>
> >> For drivers who only wish to show one memory region called 'system,
> >> and only account the GEM buffer object handles under it.
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_file.c | 45 ++++++++++++++++++++++++++++++++++++++
> >>   include/drm/drm_file.h     |  6 +++++
> >>   2 files changed, 51 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> >> index e202f79e816d..1e70669dddf7 100644
> >> --- a/drivers/gpu/drm/drm_file.c
> >> +++ b/drivers/gpu/drm/drm_file.c
> >> @@ -872,6 +872,51 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
> >>   }
> >>   EXPORT_SYMBOL(drm_send_event);
> >>
> >> +static void
> >> +add_obj(struct drm_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
> >> +{
> >> +       u64 sz = obj->size;
> >> +
> >> +       stats[0].size += sz;
> >> +
> >> +       if (obj->handle_count > 1)
> >> +               stats[0].shared += sz;
> >> +
> >> +       if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
> >> +               stats[0].active += sz;
> >> +
> >> +       /* Not supported. */
> >> +       stats[0].resident = ~0ull;
> >> +       stats[0].purgeable = ~0ull;
> >
> > Hmm, this kinda makes the simple helper not very useful.  In my
> > version, you get something that is useful for all UMA drivers (which
> > all, IIRC, have some form of madv ioctl).  I was kinda imagining that
> > for ttm drivers, my print_memory_stats() would just become a helper
> > and that TTM (or "multi-region") drivers would have their own thing.
>
> Hm how? Your version also needed a driver specific vfunc:

Sure, but there is a possibility for a driver specific vfunc ;-)

And arguably we could move madv to drm_gem_object, along with get/put
pages tracking.. since all the UMA drivers pretty much do the same
thing.  So maybe the vfunc is a temporary thing.

Anyways, I could go back to my original version where it was a helper
called from the driver to print mem stats.  That way, TTM drivers
could do their own thing.

BR,
-R

> +static enum drm_gem_object_status msm_gem_status(struct drm_gem_object *obj)
> +{
> +       struct msm_gem_object *msm_obj = to_msm_bo(obj);
> +       enum drm_gem_object_status status = 0;
> +
> +       if (msm_obj->pages)
> +               status |= DRM_GEM_OBJECT_RESIDENT;
> +
> +       if (msm_obj->madv == MSM_MADV_DONTNEED)
> +               status |= DRM_GEM_OBJECT_PURGEABLE;
> +
> +       return status;
> +}
>
> Regards,
>
> Tvrtko
>
> >
> > BR,
> > -R
> >
> >> +}
> >> +
> >> +char **
> >> +drm_query_fdinfo_system_region(struct drm_device *dev, unsigned int *num)
> >> +{
> >> +       static char *region[] = {
> >> +               "system",
> >> +       };
> >> +
> >> +       *num = 1;
> >> +
> >> +       return region;
> >> +}
> >> +EXPORT_SYMBOL(drm_query_fdinfo_system_region);
> >> +
> >> +void
> >> +drm_query_fdinfo_system_memory(struct drm_file *file,
> >> +                              struct drm_fdinfo_memory_stat *stats)
> >> +{
> >> +       struct drm_gem_object *obj;
> >> +       int id;
> >> +
> >> +       spin_lock(&file->table_lock);
> >> +       idr_for_each_entry(&file->object_idr, obj, id)
> >> +               add_obj(obj, stats);
> >> +       spin_unlock(&file->table_lock);
> >> +}
> >> +EXPORT_SYMBOL(drm_query_fdinfo_system_memory);
> >> +
> >>   static void
> >>   print_stat(struct drm_printer *p, const char *stat, const char *region, u64 sz)
> >>   {
> >> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> >> index 00d48beeac5c..dd7c6fb2c975 100644
> >> --- a/include/drm/drm_file.h
> >> +++ b/include/drm/drm_file.h
> >> @@ -383,6 +383,12 @@ struct drm_fdinfo_memory_stat {
> >>          u64 active;
> >>   };
> >>
> >> +char **drm_query_fdinfo_system_region(struct drm_device *dev,
> >> +                                     unsigned int *num);
> >> +void drm_query_fdinfo_system_memory(struct drm_file *file,
> >> +                                   struct drm_fdinfo_memory_stat *stats);
> >> +
> >> +
> >>   /**
> >>    * drm_is_primary_client - is this an open file of the primary node
> >>    * @file_priv: DRM file
> >> --
> >> 2.37.2
> >>

Re: [Intel-gfx] [RFC 4/6] drm: Add simple fdinfo memory helpers

[Tvrtko Ursulin]

On 19/04/2023 15:32, Rob Clark wrote:
> On Wed, Apr 19, 2023 at 6:16 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 18/04/2023 18:18, 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>
>>>>
>>>> For drivers who only wish to show one memory region called 'system,
>>>> and only account the GEM buffer object handles under it.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_file.c | 45 ++++++++++++++++++++++++++++++++++++++
>>>>    include/drm/drm_file.h     |  6 +++++
>>>>    2 files changed, 51 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>> index e202f79e816d..1e70669dddf7 100644
>>>> --- a/drivers/gpu/drm/drm_file.c
>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>> @@ -872,6 +872,51 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
>>>>    }
>>>>    EXPORT_SYMBOL(drm_send_event);
>>>>
>>>> +static void
>>>> +add_obj(struct drm_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
>>>> +{
>>>> +       u64 sz = obj->size;
>>>> +
>>>> +       stats[0].size += sz;
>>>> +
>>>> +       if (obj->handle_count > 1)
>>>> +               stats[0].shared += sz;
>>>> +
>>>> +       if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
>>>> +               stats[0].active += sz;
>>>> +
>>>> +       /* Not supported. */
>>>> +       stats[0].resident = ~0ull;
>>>> +       stats[0].purgeable = ~0ull;
>>>
>>> Hmm, this kinda makes the simple helper not very useful.  In my
>>> version, you get something that is useful for all UMA drivers (which
>>> all, IIRC, have some form of madv ioctl).  I was kinda imagining that
>>> for ttm drivers, my print_memory_stats() would just become a helper
>>> and that TTM (or "multi-region") drivers would have their own thing.
>>
>> Hm how? Your version also needed a driver specific vfunc:
> 
> Sure, but there is a possibility for a driver specific vfunc ;-)

Indeed, they are there in both proposals! :)

> And arguably we could move madv to drm_gem_object, along with get/put
> pages tracking.. since all the UMA drivers pretty much do the same
> thing.  So maybe the vfunc is a temporary thing.
> 
> Anyways, I could go back to my original version where it was a helper
> called from the driver to print mem stats.  That way, TTM drivers
> could do their own thing.

I must have missed that. So it was the same idea as in my RFC?

Regards,

Tvrtko
  
> BR,
> -R
> 
>> +static enum drm_gem_object_status msm_gem_status(struct drm_gem_object *obj)
>> +{
>> +       struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> +       enum drm_gem_object_status status = 0;
>> +
>> +       if (msm_obj->pages)
>> +               status |= DRM_GEM_OBJECT_RESIDENT;
>> +
>> +       if (msm_obj->madv == MSM_MADV_DONTNEED)
>> +               status |= DRM_GEM_OBJECT_PURGEABLE;
>> +
>> +       return status;
>> +}
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> BR,
>>> -R
>>>
>>>> +}
>>>> +
>>>> +char **
>>>> +drm_query_fdinfo_system_region(struct drm_device *dev, unsigned int *num)
>>>> +{
>>>> +       static char *region[] = {
>>>> +               "system",
>>>> +       };
>>>> +
>>>> +       *num = 1;
>>>> +
>>>> +       return region;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_query_fdinfo_system_region);
>>>> +
>>>> +void
>>>> +drm_query_fdinfo_system_memory(struct drm_file *file,
>>>> +                              struct drm_fdinfo_memory_stat *stats)
>>>> +{
>>>> +       struct drm_gem_object *obj;
>>>> +       int id;
>>>> +
>>>> +       spin_lock(&file->table_lock);
>>>> +       idr_for_each_entry(&file->object_idr, obj, id)
>>>> +               add_obj(obj, stats);
>>>> +       spin_unlock(&file->table_lock);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_query_fdinfo_system_memory);
>>>> +
>>>>    static void
>>>>    print_stat(struct drm_printer *p, const char *stat, const char *region, u64 sz)
>>>>    {
>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>> index 00d48beeac5c..dd7c6fb2c975 100644
>>>> --- a/include/drm/drm_file.h
>>>> +++ b/include/drm/drm_file.h
>>>> @@ -383,6 +383,12 @@ struct drm_fdinfo_memory_stat {
>>>>           u64 active;
>>>>    };
>>>>
>>>> +char **drm_query_fdinfo_system_region(struct drm_device *dev,
>>>> +                                     unsigned int *num);
>>>> +void drm_query_fdinfo_system_memory(struct drm_file *file,
>>>> +                                   struct drm_fdinfo_memory_stat *stats);
>>>> +
>>>> +
>>>>    /**
>>>>     * drm_is_primary_client - is this an open file of the primary node
>>>>     * @file_priv: DRM file
>>>> --
>>>> 2.37.2
>>>>

Re: [Intel-gfx] [RFC 4/6] drm: Add simple fdinfo memory helpers

[Rob Clark]

On Thu, Apr 20, 2023 at 6:14 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 19/04/2023 15:32, Rob Clark wrote:
> > On Wed, Apr 19, 2023 at 6:16 AM Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> >>
> >>
> >> On 18/04/2023 18:18, 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>
> >>>>
> >>>> For drivers who only wish to show one memory region called 'system,
> >>>> and only account the GEM buffer object handles under it.
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/drm_file.c | 45 ++++++++++++++++++++++++++++++++++++++
> >>>>    include/drm/drm_file.h     |  6 +++++
> >>>>    2 files changed, 51 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> >>>> index e202f79e816d..1e70669dddf7 100644
> >>>> --- a/drivers/gpu/drm/drm_file.c
> >>>> +++ b/drivers/gpu/drm/drm_file.c
> >>>> @@ -872,6 +872,51 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
> >>>>    }
> >>>>    EXPORT_SYMBOL(drm_send_event);
> >>>>
> >>>> +static void
> >>>> +add_obj(struct drm_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
> >>>> +{
> >>>> +       u64 sz = obj->size;
> >>>> +
> >>>> +       stats[0].size += sz;
> >>>> +
> >>>> +       if (obj->handle_count > 1)
> >>>> +               stats[0].shared += sz;
> >>>> +
> >>>> +       if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
> >>>> +               stats[0].active += sz;
> >>>> +
> >>>> +       /* Not supported. */
> >>>> +       stats[0].resident = ~0ull;
> >>>> +       stats[0].purgeable = ~0ull;
> >>>
> >>> Hmm, this kinda makes the simple helper not very useful.  In my
> >>> version, you get something that is useful for all UMA drivers (which
> >>> all, IIRC, have some form of madv ioctl).  I was kinda imagining that
> >>> for ttm drivers, my print_memory_stats() would just become a helper
> >>> and that TTM (or "multi-region") drivers would have their own thing.
> >>
> >> Hm how? Your version also needed a driver specific vfunc:
> >
> > Sure, but there is a possibility for a driver specific vfunc ;-)
>
> Indeed, they are there in both proposals! :)
>
> > And arguably we could move madv to drm_gem_object, along with get/put
> > pages tracking.. since all the UMA drivers pretty much do the same
> > thing.  So maybe the vfunc is a temporary thing.
> >
> > Anyways, I could go back to my original version where it was a helper
> > called from the driver to print mem stats.  That way, TTM drivers
> > could do their own thing.
>
> I must have missed that. So it was the same idea as in my RFC?

similar, danvet asked for something "more core" ;-)

https://patchwork.freedesktop.org/patch/531403/?series=116216&rev=1

BR,
-R

> Regards,
>
> Tvrtko
>
> > BR,
> > -R
> >
> >> +static enum drm_gem_object_status msm_gem_status(struct drm_gem_object *obj)
> >> +{
> >> +       struct msm_gem_object *msm_obj = to_msm_bo(obj);
> >> +       enum drm_gem_object_status status = 0;
> >> +
> >> +       if (msm_obj->pages)
> >> +               status |= DRM_GEM_OBJECT_RESIDENT;
> >> +
> >> +       if (msm_obj->madv == MSM_MADV_DONTNEED)
> >> +               status |= DRM_GEM_OBJECT_PURGEABLE;
> >> +
> >> +       return status;
> >> +}
> >>
> >> Regards,
> >>
> >> Tvrtko
> >>
> >>>
> >>> BR,
> >>> -R
> >>>
> >>>> +}
> >>>> +
> >>>> +char **
> >>>> +drm_query_fdinfo_system_region(struct drm_device *dev, unsigned int *num)
> >>>> +{
> >>>> +       static char *region[] = {
> >>>> +               "system",
> >>>> +       };
> >>>> +
> >>>> +       *num = 1;
> >>>> +
> >>>> +       return region;
> >>>> +}
> >>>> +EXPORT_SYMBOL(drm_query_fdinfo_system_region);
> >>>> +
> >>>> +void
> >>>> +drm_query_fdinfo_system_memory(struct drm_file *file,
> >>>> +                              struct drm_fdinfo_memory_stat *stats)
> >>>> +{
> >>>> +       struct drm_gem_object *obj;
> >>>> +       int id;
> >>>> +
> >>>> +       spin_lock(&file->table_lock);
> >>>> +       idr_for_each_entry(&file->object_idr, obj, id)
> >>>> +               add_obj(obj, stats);
> >>>> +       spin_unlock(&file->table_lock);
> >>>> +}
> >>>> +EXPORT_SYMBOL(drm_query_fdinfo_system_memory);
> >>>> +
> >>>>    static void
> >>>>    print_stat(struct drm_printer *p, const char *stat, const char *region, u64 sz)
> >>>>    {
> >>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> >>>> index 00d48beeac5c..dd7c6fb2c975 100644
> >>>> --- a/include/drm/drm_file.h
> >>>> +++ b/include/drm/drm_file.h
> >>>> @@ -383,6 +383,12 @@ struct drm_fdinfo_memory_stat {
> >>>>           u64 active;
> >>>>    };
> >>>>
> >>>> +char **drm_query_fdinfo_system_region(struct drm_device *dev,
> >>>> +                                     unsigned int *num);
> >>>> +void drm_query_fdinfo_system_memory(struct drm_file *file,
> >>>> +                                   struct drm_fdinfo_memory_stat *stats);
> >>>> +
> >>>> +
> >>>>    /**
> >>>>     * drm_is_primary_client - is this an open file of the primary node
> >>>>     * @file_priv: DRM file
> >>>> --
> >>>> 2.37.2
> >>>>

[Intel-gfx] [RFC 5/6] drm/msm: Add basic memory stats

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Use DRM helpers for implementing basic memory stats.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_drv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 060c7689a739..7662103b5999 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1090,6 +1090,10 @@ static const struct drm_driver msm_driver = {
 	.prime_fd_to_handle = drm_gem_prime_fd_to_handle,
 	.gem_prime_import_sg_table = msm_gem_prime_import_sg_table,
 	.gem_prime_mmap     = msm_gem_prime_mmap,
+#ifdef CONFIG_PROC_FS
+	.query_fdinfo_memory_regions = drm_query_fdinfo_system_region,
+	.query_fdinfo_memory_stats = drm_query_fdinfo_system_memory,
+#endif
 #ifdef CONFIG_DEBUG_FS
 	.debugfs_init       = msm_debugfs_init,
 #endif
-- 
2.37.2

[Intel-gfx] [RFC 6/6] drm/i915: Implement fdinfo memory stats printing

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Show how more driver specific set of memory stats could be shown,
more specifically where object can reside in multiple regions, showing all
the supported stats, and where there is more to show than just user visible
objects.

WIP...

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/i915_driver.c     |   5 ++
 drivers/gpu/drm/i915/i915_drm_client.c | 102 +++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drm_client.h |   8 ++
 drivers/gpu/drm/i915/i915_drv.h        |   2 +
 4 files changed, 117 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
index 6493548c69bf..4c70206cbc27 100644
--- a/drivers/gpu/drm/i915/i915_driver.c
+++ b/drivers/gpu/drm/i915/i915_driver.c
@@ -1806,6 +1806,11 @@ static const struct drm_driver i915_drm_driver = {
 	.dumb_create = i915_gem_dumb_create,
 	.dumb_map_offset = i915_gem_dumb_mmap_offset,
 
+#ifdef CONFIG_PROC_FS
+	.query_fdinfo_memory_regions = i915_query_fdinfo_memory_regions,
+	.query_fdinfo_memory_stats = i915_query_fdinfo_memory_stats,
+#endif
+
 	.ioctls = i915_ioctls,
 	.num_ioctls = ARRAY_SIZE(i915_ioctls),
 	.fops = &i915_driver_fops,
diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
index c654984189f7..65857c68bdb3 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.c
+++ b/drivers/gpu/drm/i915/i915_drm_client.c
@@ -12,6 +12,7 @@
 #include <drm/drm_print.h>
 
 #include "gem/i915_gem_context.h"
+#include "intel_memory_region.h"
 #include "i915_drm_client.h"
 #include "i915_file_private.h"
 #include "i915_gem.h"
@@ -112,4 +113,105 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
 	for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
 		show_client_class(p, i915, file_priv->client, i);
 }
+
+char **
+i915_query_fdinfo_memory_regions(struct drm_device *dev, unsigned int *num)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct intel_memory_region *mr;
+	enum intel_region_id id;
+
+	/* FIXME move to init */
+	for_each_memory_region(mr, i915, id) {
+		if (!i915->mm.region_names[id])
+			i915->mm.region_names[id] = mr->name;
+	}
+
+	*num = id;
+
+	return i915->mm.region_names;
+}
+
+static void
+add_obj(struct drm_i915_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
+{
+        struct intel_memory_region *mr;
+	u64 sz = obj->base.size;
+        enum intel_region_id id;
+	unsigned int i;
+
+	if (!obj)
+		return;
+
+	/* Attribute size and shared to all possible memory regions. */
+	for (i = 0; i < obj->mm.n_placements; i++) {
+		mr = obj->mm.placements[i];
+		id = mr->id;
+
+		stats[id].size += sz;
+		if (obj->base.handle_count > 1)
+			stats[id].shared += sz;
+	}
+
+	/* Attribute other categories to only the current region. */
+	mr = obj->mm.region;
+	if (mr)
+		id = mr->id;
+	else
+		id = INTEL_REGION_SMEM;
+
+	if (!i915_gem_object_has_pages(obj))
+		return;
+
+	stats[id].resident += sz;
+
+	if (!dma_resv_test_signaled(obj->base.resv, dma_resv_usage_rw(true)))
+		stats[id].active += sz;
+	else if (i915_gem_object_is_shrinkable(obj) &&
+		obj->mm.madv == I915_MADV_DONTNEED)
+		stats[id].purgeable += sz;
+}
+
+void
+i915_query_fdinfo_memory_stats(struct drm_file *file,
+			       struct drm_fdinfo_memory_stat *stats)
+{
+	struct drm_i915_file_private *file_priv = file->driver_priv;
+	struct i915_drm_client *client = file_priv->client;
+	struct drm_gem_object *drm_obj;
+	struct i915_gem_context *ctx;
+	int id;
+
+	/*
+	 * FIXME - we can do this better and in fewer passes if we are to start
+	 * exporting proper memory stats.
+	 */
+
+	/* User created objects */
+	spin_lock(&file->table_lock);
+	idr_for_each_entry(&file->object_idr, drm_obj, id)
+		add_obj(to_intel_bo(drm_obj), stats);
+	spin_unlock(&file->table_lock);
+
+	/* Contexts, rings, timelines, page tables, ... */
+	rcu_read_lock();
+	list_for_each_entry_rcu(ctx, &client->ctx_list, client_link) {
+		struct i915_gem_engines_iter it;
+		struct intel_context *ce;
+
+		for_each_gem_engine(ce, rcu_dereference(ctx->engines), it) {
+			/* FIXME races?! */
+			if (ce->state)
+				add_obj(ce->state->obj, stats);
+			if (ce->timeline && ce->timeline->hwsp_ggtt)
+				add_obj(ce->timeline->hwsp_ggtt->obj, stats);
+			if (ce->ring && ce->ring->vma)
+				add_obj(ce->ring->vma->obj, stats);
+		}
+
+		/* TODO  vtx->vm page table backing objects */
+	}
+	rcu_read_unlock();
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
index 4c18b99e10a4..622936c51903 100644
--- a/drivers/gpu/drm/i915/i915_drm_client.h
+++ b/drivers/gpu/drm/i915/i915_drm_client.h
@@ -14,7 +14,10 @@
 
 #define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE
 
+struct drm_device;
 struct drm_file;
+struct drm_fdinfo_memory_stat;
+struct drm_gem_object;
 struct drm_printer;
 
 struct i915_drm_client {
@@ -49,6 +52,11 @@ struct i915_drm_client *i915_drm_client_alloc(void);
 
 #ifdef CONFIG_PROC_FS
 void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file);
+
+char **i915_query_fdinfo_memory_regions(struct drm_device *dev,
+					unsigned int *num);
+void i915_query_fdinfo_memory_stats(struct drm_file *file,
+				    struct drm_fdinfo_memory_stat *stats);
 #endif
 
 #endif /* !__I915_DRM_CLIENT_H__ */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index eb739fb9cdbb..b84d2f0ed2cb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -161,6 +161,8 @@ struct i915_gem_mm {
 
 	struct intel_memory_region *regions[INTEL_REGION_UNKNOWN];
 
+	char *region_names[INTEL_REGION_UNKNOWN];
+
 	struct notifier_block oom_notifier;
 	struct notifier_block vmap_notifier;
 	struct shrinker shrinker;
-- 
2.37.2

Re: [Intel-gfx] [RFC 6/6] drm/i915: Implement fdinfo memory stats printing

[Rob Clark]

On Mon, Apr 17, 2023 at 8:56 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> Show how more driver specific set of memory stats could be shown,
> more specifically where object can reside in multiple regions, showing all
> the supported stats, and where there is more to show than just user visible
> objects.
>
> WIP...
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_driver.c     |   5 ++
>  drivers/gpu/drm/i915/i915_drm_client.c | 102 +++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drm_client.h |   8 ++
>  drivers/gpu/drm/i915/i915_drv.h        |   2 +
>  4 files changed, 117 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> index 6493548c69bf..4c70206cbc27 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -1806,6 +1806,11 @@ static const struct drm_driver i915_drm_driver = {
>         .dumb_create = i915_gem_dumb_create,
>         .dumb_map_offset = i915_gem_dumb_mmap_offset,
>
> +#ifdef CONFIG_PROC_FS
> +       .query_fdinfo_memory_regions = i915_query_fdinfo_memory_regions,
> +       .query_fdinfo_memory_stats = i915_query_fdinfo_memory_stats,
> +#endif
> +
>         .ioctls = i915_ioctls,
>         .num_ioctls = ARRAY_SIZE(i915_ioctls),
>         .fops = &i915_driver_fops,
> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> index c654984189f7..65857c68bdb3 100644
> --- a/drivers/gpu/drm/i915/i915_drm_client.c
> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> @@ -12,6 +12,7 @@
>  #include <drm/drm_print.h>
>
>  #include "gem/i915_gem_context.h"
> +#include "intel_memory_region.h"
>  #include "i915_drm_client.h"
>  #include "i915_file_private.h"
>  #include "i915_gem.h"
> @@ -112,4 +113,105 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
>         for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
>                 show_client_class(p, i915, file_priv->client, i);
>  }
> +
> +char **
> +i915_query_fdinfo_memory_regions(struct drm_device *dev, unsigned int *num)
> +{
> +       struct drm_i915_private *i915 = to_i915(dev);
> +       struct intel_memory_region *mr;
> +       enum intel_region_id id;
> +
> +       /* FIXME move to init */
> +       for_each_memory_region(mr, i915, id) {
> +               if (!i915->mm.region_names[id])
> +                       i915->mm.region_names[id] = mr->name;
> +       }
> +
> +       *num = id;
> +
> +       return i915->mm.region_names;
> +}
> +
> +static void
> +add_obj(struct drm_i915_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
> +{
> +        struct intel_memory_region *mr;
> +       u64 sz = obj->base.size;
> +        enum intel_region_id id;
> +       unsigned int i;
> +
> +       if (!obj)
> +               return;
> +
> +       /* Attribute size and shared to all possible memory regions. */
> +       for (i = 0; i < obj->mm.n_placements; i++) {
> +               mr = obj->mm.placements[i];
> +               id = mr->id;
> +
> +               stats[id].size += sz;

This implies that summing up all of the categories is not the same as
the toplevel stats that I was proposing

BR,
-R

> +               if (obj->base.handle_count > 1)
> +                       stats[id].shared += sz;
> +       }
> +
> +       /* Attribute other categories to only the current region. */
> +       mr = obj->mm.region;
> +       if (mr)
> +               id = mr->id;
> +       else
> +               id = INTEL_REGION_SMEM;
> +
> +       if (!i915_gem_object_has_pages(obj))
> +               return;
> +
> +       stats[id].resident += sz;
> +
> +       if (!dma_resv_test_signaled(obj->base.resv, dma_resv_usage_rw(true)))
> +               stats[id].active += sz;
> +       else if (i915_gem_object_is_shrinkable(obj) &&
> +               obj->mm.madv == I915_MADV_DONTNEED)
> +               stats[id].purgeable += sz;
> +}
> +
> +void
> +i915_query_fdinfo_memory_stats(struct drm_file *file,
> +                              struct drm_fdinfo_memory_stat *stats)
> +{
> +       struct drm_i915_file_private *file_priv = file->driver_priv;
> +       struct i915_drm_client *client = file_priv->client;
> +       struct drm_gem_object *drm_obj;
> +       struct i915_gem_context *ctx;
> +       int id;
> +
> +       /*
> +        * FIXME - we can do this better and in fewer passes if we are to start
> +        * exporting proper memory stats.
> +        */
> +
> +       /* User created objects */
> +       spin_lock(&file->table_lock);
> +       idr_for_each_entry(&file->object_idr, drm_obj, id)
> +               add_obj(to_intel_bo(drm_obj), stats);
> +       spin_unlock(&file->table_lock);
> +
> +       /* Contexts, rings, timelines, page tables, ... */
> +       rcu_read_lock();
> +       list_for_each_entry_rcu(ctx, &client->ctx_list, client_link) {
> +               struct i915_gem_engines_iter it;
> +               struct intel_context *ce;
> +
> +               for_each_gem_engine(ce, rcu_dereference(ctx->engines), it) {
> +                       /* FIXME races?! */
> +                       if (ce->state)
> +                               add_obj(ce->state->obj, stats);
> +                       if (ce->timeline && ce->timeline->hwsp_ggtt)
> +                               add_obj(ce->timeline->hwsp_ggtt->obj, stats);
> +                       if (ce->ring && ce->ring->vma)
> +                               add_obj(ce->ring->vma->obj, stats);
> +               }
> +
> +               /* TODO  vtx->vm page table backing objects */
> +       }
> +       rcu_read_unlock();
> +}
> +
>  #endif
> diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
> index 4c18b99e10a4..622936c51903 100644
> --- a/drivers/gpu/drm/i915/i915_drm_client.h
> +++ b/drivers/gpu/drm/i915/i915_drm_client.h
> @@ -14,7 +14,10 @@
>
>  #define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE
>
> +struct drm_device;
>  struct drm_file;
> +struct drm_fdinfo_memory_stat;
> +struct drm_gem_object;
>  struct drm_printer;
>
>  struct i915_drm_client {
> @@ -49,6 +52,11 @@ struct i915_drm_client *i915_drm_client_alloc(void);
>
>  #ifdef CONFIG_PROC_FS
>  void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file);
> +
> +char **i915_query_fdinfo_memory_regions(struct drm_device *dev,
> +                                       unsigned int *num);
> +void i915_query_fdinfo_memory_stats(struct drm_file *file,
> +                                   struct drm_fdinfo_memory_stat *stats);
>  #endif
>
>  #endif /* !__I915_DRM_CLIENT_H__ */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index eb739fb9cdbb..b84d2f0ed2cb 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -161,6 +161,8 @@ struct i915_gem_mm {
>
>         struct intel_memory_region *regions[INTEL_REGION_UNKNOWN];
>
> +       char *region_names[INTEL_REGION_UNKNOWN];
> +
>         struct notifier_block oom_notifier;
>         struct notifier_block vmap_notifier;
>         struct shrinker shrinker;
> --
> 2.37.2
>

Re: [Intel-gfx] [RFC 6/6] drm/i915: Implement fdinfo memory stats printing

[Tvrtko Ursulin]

On 18/04/2023 15: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>
>>
>> Show how more driver specific set of memory stats could be shown,
>> more specifically where object can reside in multiple regions, showing all
>> the supported stats, and where there is more to show than just user visible
>> objects.
>>
>> WIP...
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_driver.c     |   5 ++
>>   drivers/gpu/drm/i915/i915_drm_client.c | 102 +++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/i915_drm_client.h |   8 ++
>>   drivers/gpu/drm/i915/i915_drv.h        |   2 +
>>   4 files changed, 117 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
>> index 6493548c69bf..4c70206cbc27 100644
>> --- a/drivers/gpu/drm/i915/i915_driver.c
>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>> @@ -1806,6 +1806,11 @@ static const struct drm_driver i915_drm_driver = {
>>          .dumb_create = i915_gem_dumb_create,
>>          .dumb_map_offset = i915_gem_dumb_mmap_offset,
>>
>> +#ifdef CONFIG_PROC_FS
>> +       .query_fdinfo_memory_regions = i915_query_fdinfo_memory_regions,
>> +       .query_fdinfo_memory_stats = i915_query_fdinfo_memory_stats,
>> +#endif
>> +
>>          .ioctls = i915_ioctls,
>>          .num_ioctls = ARRAY_SIZE(i915_ioctls),
>>          .fops = &i915_driver_fops,
>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
>> index c654984189f7..65857c68bdb3 100644
>> --- a/drivers/gpu/drm/i915/i915_drm_client.c
>> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
>> @@ -12,6 +12,7 @@
>>   #include <drm/drm_print.h>
>>
>>   #include "gem/i915_gem_context.h"
>> +#include "intel_memory_region.h"
>>   #include "i915_drm_client.h"
>>   #include "i915_file_private.h"
>>   #include "i915_gem.h"
>> @@ -112,4 +113,105 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
>>          for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
>>                  show_client_class(p, i915, file_priv->client, i);
>>   }
>> +
>> +char **
>> +i915_query_fdinfo_memory_regions(struct drm_device *dev, unsigned int *num)
>> +{
>> +       struct drm_i915_private *i915 = to_i915(dev);
>> +       struct intel_memory_region *mr;
>> +       enum intel_region_id id;
>> +
>> +       /* FIXME move to init */
>> +       for_each_memory_region(mr, i915, id) {
>> +               if (!i915->mm.region_names[id])
>> +                       i915->mm.region_names[id] = mr->name;
>> +       }
>> +
>> +       *num = id;
>> +
>> +       return i915->mm.region_names;
>> +}
>> +
>> +static void
>> +add_obj(struct drm_i915_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
>> +{
>> +        struct intel_memory_region *mr;
>> +       u64 sz = obj->base.size;
>> +        enum intel_region_id id;
>> +       unsigned int i;
>> +
>> +       if (!obj)
>> +               return;
>> +
>> +       /* Attribute size and shared to all possible memory regions. */
>> +       for (i = 0; i < obj->mm.n_placements; i++) {
>> +               mr = obj->mm.placements[i];
>> +               id = mr->id;
>> +
>> +               stats[id].size += sz;
> 
> This implies that summing up all of the categories is not the same as
> the toplevel stats that I was proposing

Correct, my categories are a bit different. You had private and shared as two mutually exclusive buckets, and then resident as subset of either/both. I have size as analogue to VmSize and resident as a subset of that, analogue to VmRss.

Shared is a bit wishy-washy, not sure about that one in either proposals. It can be either imported or exported buffers, but in essence I think it fits better as a subset of total size.

Regards,

Tvrtko

>> +               if (obj->base.handle_count > 1)
>> +                       stats[id].shared += sz;
>> +       }
>> +
>> +       /* Attribute other categories to only the current region. */
>> +       mr = obj->mm.region;
>> +       if (mr)
>> +               id = mr->id;
>> +       else
>> +               id = INTEL_REGION_SMEM;
>> +
>> +       if (!i915_gem_object_has_pages(obj))
>> +               return;
>> +
>> +       stats[id].resident += sz;
>> +
>> +       if (!dma_resv_test_signaled(obj->base.resv, dma_resv_usage_rw(true)))
>> +               stats[id].active += sz;
>> +       else if (i915_gem_object_is_shrinkable(obj) &&
>> +               obj->mm.madv == I915_MADV_DONTNEED)
>> +               stats[id].purgeable += sz;
>> +}
>> +
>> +void
>> +i915_query_fdinfo_memory_stats(struct drm_file *file,
>> +                              struct drm_fdinfo_memory_stat *stats)
>> +{
>> +       struct drm_i915_file_private *file_priv = file->driver_priv;
>> +       struct i915_drm_client *client = file_priv->client;
>> +       struct drm_gem_object *drm_obj;
>> +       struct i915_gem_context *ctx;
>> +       int id;
>> +
>> +       /*
>> +        * FIXME - we can do this better and in fewer passes if we are to start
>> +        * exporting proper memory stats.
>> +        */
>> +
>> +       /* User created objects */
>> +       spin_lock(&file->table_lock);
>> +       idr_for_each_entry(&file->object_idr, drm_obj, id)
>> +               add_obj(to_intel_bo(drm_obj), stats);
>> +       spin_unlock(&file->table_lock);
>> +
>> +       /* Contexts, rings, timelines, page tables, ... */
>> +       rcu_read_lock();
>> +       list_for_each_entry_rcu(ctx, &client->ctx_list, client_link) {
>> +               struct i915_gem_engines_iter it;
>> +               struct intel_context *ce;
>> +
>> +               for_each_gem_engine(ce, rcu_dereference(ctx->engines), it) {
>> +                       /* FIXME races?! */
>> +                       if (ce->state)
>> +                               add_obj(ce->state->obj, stats);
>> +                       if (ce->timeline && ce->timeline->hwsp_ggtt)
>> +                               add_obj(ce->timeline->hwsp_ggtt->obj, stats);
>> +                       if (ce->ring && ce->ring->vma)
>> +                               add_obj(ce->ring->vma->obj, stats);
>> +               }
>> +
>> +               /* TODO  vtx->vm page table backing objects */
>> +       }
>> +       rcu_read_unlock();
>> +}
>> +
>>   #endif
>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
>> index 4c18b99e10a4..622936c51903 100644
>> --- a/drivers/gpu/drm/i915/i915_drm_client.h
>> +++ b/drivers/gpu/drm/i915/i915_drm_client.h
>> @@ -14,7 +14,10 @@
>>
>>   #define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE
>>
>> +struct drm_device;
>>   struct drm_file;
>> +struct drm_fdinfo_memory_stat;
>> +struct drm_gem_object;
>>   struct drm_printer;
>>
>>   struct i915_drm_client {
>> @@ -49,6 +52,11 @@ struct i915_drm_client *i915_drm_client_alloc(void);
>>
>>   #ifdef CONFIG_PROC_FS
>>   void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file);
>> +
>> +char **i915_query_fdinfo_memory_regions(struct drm_device *dev,
>> +                                       unsigned int *num);
>> +void i915_query_fdinfo_memory_stats(struct drm_file *file,
>> +                                   struct drm_fdinfo_memory_stat *stats);
>>   #endif
>>
>>   #endif /* !__I915_DRM_CLIENT_H__ */
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index eb739fb9cdbb..b84d2f0ed2cb 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -161,6 +161,8 @@ struct i915_gem_mm {
>>
>>          struct intel_memory_region *regions[INTEL_REGION_UNKNOWN];
>>
>> +       char *region_names[INTEL_REGION_UNKNOWN];
>> +
>>          struct notifier_block oom_notifier;
>>          struct notifier_block vmap_notifier;
>>          struct shrinker shrinker;
>> --
>> 2.37.2
>>

Re: [Intel-gfx] [RFC 6/6] drm/i915: Implement fdinfo memory stats printing

[Rob Clark]

On Tue, Apr 18, 2023 at 7:58 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 18/04/2023 15: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>
> >>
> >> Show how more driver specific set of memory stats could be shown,
> >> more specifically where object can reside in multiple regions, showing all
> >> the supported stats, and where there is more to show than just user visible
> >> objects.
> >>
> >> WIP...
> >>
> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_driver.c     |   5 ++
> >>   drivers/gpu/drm/i915/i915_drm_client.c | 102 +++++++++++++++++++++++++
> >>   drivers/gpu/drm/i915/i915_drm_client.h |   8 ++
> >>   drivers/gpu/drm/i915/i915_drv.h        |   2 +
> >>   4 files changed, 117 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> >> index 6493548c69bf..4c70206cbc27 100644
> >> --- a/drivers/gpu/drm/i915/i915_driver.c
> >> +++ b/drivers/gpu/drm/i915/i915_driver.c
> >> @@ -1806,6 +1806,11 @@ static const struct drm_driver i915_drm_driver = {
> >>          .dumb_create = i915_gem_dumb_create,
> >>          .dumb_map_offset = i915_gem_dumb_mmap_offset,
> >>
> >> +#ifdef CONFIG_PROC_FS
> >> +       .query_fdinfo_memory_regions = i915_query_fdinfo_memory_regions,
> >> +       .query_fdinfo_memory_stats = i915_query_fdinfo_memory_stats,
> >> +#endif
> >> +
> >>          .ioctls = i915_ioctls,
> >>          .num_ioctls = ARRAY_SIZE(i915_ioctls),
> >>          .fops = &i915_driver_fops,
> >> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> >> index c654984189f7..65857c68bdb3 100644
> >> --- a/drivers/gpu/drm/i915/i915_drm_client.c
> >> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> >> @@ -12,6 +12,7 @@
> >>   #include <drm/drm_print.h>
> >>
> >>   #include "gem/i915_gem_context.h"
> >> +#include "intel_memory_region.h"
> >>   #include "i915_drm_client.h"
> >>   #include "i915_file_private.h"
> >>   #include "i915_gem.h"
> >> @@ -112,4 +113,105 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
> >>          for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
> >>                  show_client_class(p, i915, file_priv->client, i);
> >>   }
> >> +
> >> +char **
> >> +i915_query_fdinfo_memory_regions(struct drm_device *dev, unsigned int *num)
> >> +{
> >> +       struct drm_i915_private *i915 = to_i915(dev);
> >> +       struct intel_memory_region *mr;
> >> +       enum intel_region_id id;
> >> +
> >> +       /* FIXME move to init */
> >> +       for_each_memory_region(mr, i915, id) {
> >> +               if (!i915->mm.region_names[id])
> >> +                       i915->mm.region_names[id] = mr->name;
> >> +       }
> >> +
> >> +       *num = id;
> >> +
> >> +       return i915->mm.region_names;
> >> +}
> >> +
> >> +static void
> >> +add_obj(struct drm_i915_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
> >> +{
> >> +        struct intel_memory_region *mr;
> >> +       u64 sz = obj->base.size;
> >> +        enum intel_region_id id;
> >> +       unsigned int i;
> >> +
> >> +       if (!obj)
> >> +               return;
> >> +
> >> +       /* Attribute size and shared to all possible memory regions. */
> >> +       for (i = 0; i < obj->mm.n_placements; i++) {
> >> +               mr = obj->mm.placements[i];
> >> +               id = mr->id;
> >> +
> >> +               stats[id].size += sz;
> >
> > This implies that summing up all of the categories is not the same as
> > the toplevel stats that I was proposing

Sorry, I mis-spoke, I meant "summing up all of the regions is not..."

> Correct, my categories are a bit different. You had private and shared as two mutually exclusive buckets, and then resident as subset of either/both. I have size as analogue to VmSize and resident as a subset of that, analogue to VmRss.
>

I split shared because by definition shared buffers can be counted
against multiple drm_file's, whereas private is only counted against
the single drm_file.  Driver or app changes are unlikely to change the
shared size, whereas private footprint is a thing you can optimize to
some degree.

> Shared is a bit wishy-washy, not sure about that one in either proposals. It can be either imported or exported buffers, but in essence I think it fits better as a subset of total size.

Imported vs exported doesn't really matter.. it is just an
implementation detail of the winsys.  But I think it is useful to know
how much of an app's footprint is shared vs private.  You could
express it different ways, but my proposal had private and shared,
from which you can calculate total:

   total = private + shared

but you could flip the path around and advertise just total and
shared, and calculate private from that.

BR,
-R

> Regards,
>
> Tvrtko
>
> >> +               if (obj->base.handle_count > 1)
> >> +                       stats[id].shared += sz;
> >> +       }
> >> +
> >> +       /* Attribute other categories to only the current region. */
> >> +       mr = obj->mm.region;
> >> +       if (mr)
> >> +               id = mr->id;
> >> +       else
> >> +               id = INTEL_REGION_SMEM;
> >> +
> >> +       if (!i915_gem_object_has_pages(obj))
> >> +               return;
> >> +
> >> +       stats[id].resident += sz;
> >> +
> >> +       if (!dma_resv_test_signaled(obj->base.resv, dma_resv_usage_rw(true)))
> >> +               stats[id].active += sz;
> >> +       else if (i915_gem_object_is_shrinkable(obj) &&
> >> +               obj->mm.madv == I915_MADV_DONTNEED)
> >> +               stats[id].purgeable += sz;
> >> +}
> >> +
> >> +void
> >> +i915_query_fdinfo_memory_stats(struct drm_file *file,
> >> +                              struct drm_fdinfo_memory_stat *stats)
> >> +{
> >> +       struct drm_i915_file_private *file_priv = file->driver_priv;
> >> +       struct i915_drm_client *client = file_priv->client;
> >> +       struct drm_gem_object *drm_obj;
> >> +       struct i915_gem_context *ctx;
> >> +       int id;
> >> +
> >> +       /*
> >> +        * FIXME - we can do this better and in fewer passes if we are to start
> >> +        * exporting proper memory stats.
> >> +        */
> >> +
> >> +       /* User created objects */
> >> +       spin_lock(&file->table_lock);
> >> +       idr_for_each_entry(&file->object_idr, drm_obj, id)
> >> +               add_obj(to_intel_bo(drm_obj), stats);
> >> +       spin_unlock(&file->table_lock);
> >> +
> >> +       /* Contexts, rings, timelines, page tables, ... */
> >> +       rcu_read_lock();
> >> +       list_for_each_entry_rcu(ctx, &client->ctx_list, client_link) {
> >> +               struct i915_gem_engines_iter it;
> >> +               struct intel_context *ce;
> >> +
> >> +               for_each_gem_engine(ce, rcu_dereference(ctx->engines), it) {
> >> +                       /* FIXME races?! */
> >> +                       if (ce->state)
> >> +                               add_obj(ce->state->obj, stats);
> >> +                       if (ce->timeline && ce->timeline->hwsp_ggtt)
> >> +                               add_obj(ce->timeline->hwsp_ggtt->obj, stats);
> >> +                       if (ce->ring && ce->ring->vma)
> >> +                               add_obj(ce->ring->vma->obj, stats);
> >> +               }
> >> +
> >> +               /* TODO  vtx->vm page table backing objects */
> >> +       }
> >> +       rcu_read_unlock();
> >> +}
> >> +
> >>   #endif
> >> diff --git a/drivers/gpu/drm/i915/i915_drm_client.h b/drivers/gpu/drm/i915/i915_drm_client.h
> >> index 4c18b99e10a4..622936c51903 100644
> >> --- a/drivers/gpu/drm/i915/i915_drm_client.h
> >> +++ b/drivers/gpu/drm/i915/i915_drm_client.h
> >> @@ -14,7 +14,10 @@
> >>
> >>   #define I915_LAST_UABI_ENGINE_CLASS I915_ENGINE_CLASS_COMPUTE
> >>
> >> +struct drm_device;
> >>   struct drm_file;
> >> +struct drm_fdinfo_memory_stat;
> >> +struct drm_gem_object;
> >>   struct drm_printer;
> >>
> >>   struct i915_drm_client {
> >> @@ -49,6 +52,11 @@ struct i915_drm_client *i915_drm_client_alloc(void);
> >>
> >>   #ifdef CONFIG_PROC_FS
> >>   void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file);
> >> +
> >> +char **i915_query_fdinfo_memory_regions(struct drm_device *dev,
> >> +                                       unsigned int *num);
> >> +void i915_query_fdinfo_memory_stats(struct drm_file *file,
> >> +                                   struct drm_fdinfo_memory_stat *stats);
> >>   #endif
> >>
> >>   #endif /* !__I915_DRM_CLIENT_H__ */
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index eb739fb9cdbb..b84d2f0ed2cb 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -161,6 +161,8 @@ struct i915_gem_mm {
> >>
> >>          struct intel_memory_region *regions[INTEL_REGION_UNKNOWN];
> >>
> >> +       char *region_names[INTEL_REGION_UNKNOWN];
> >> +
> >>          struct notifier_block oom_notifier;
> >>          struct notifier_block vmap_notifier;
> >>          struct shrinker shrinker;
> >> --
> >> 2.37.2
> >>

Re: [Intel-gfx] [RFC 6/6] drm/i915: Implement fdinfo memory stats printing

[Tvrtko Ursulin]

On 18/04/2023 17:08, Rob Clark wrote:
> On Tue, Apr 18, 2023 at 7:58 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>> On 18/04/2023 15: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>
>>>>
>>>> Show how more driver specific set of memory stats could be shown,
>>>> more specifically where object can reside in multiple regions, showing all
>>>> the supported stats, and where there is more to show than just user visible
>>>> objects.
>>>>
>>>> WIP...
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_driver.c     |   5 ++
>>>>    drivers/gpu/drm/i915/i915_drm_client.c | 102 +++++++++++++++++++++++++
>>>>    drivers/gpu/drm/i915/i915_drm_client.h |   8 ++
>>>>    drivers/gpu/drm/i915/i915_drv.h        |   2 +
>>>>    4 files changed, 117 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
>>>> index 6493548c69bf..4c70206cbc27 100644
>>>> --- a/drivers/gpu/drm/i915/i915_driver.c
>>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>>>> @@ -1806,6 +1806,11 @@ static const struct drm_driver i915_drm_driver = {
>>>>           .dumb_create = i915_gem_dumb_create,
>>>>           .dumb_map_offset = i915_gem_dumb_mmap_offset,
>>>>
>>>> +#ifdef CONFIG_PROC_FS
>>>> +       .query_fdinfo_memory_regions = i915_query_fdinfo_memory_regions,
>>>> +       .query_fdinfo_memory_stats = i915_query_fdinfo_memory_stats,
>>>> +#endif
>>>> +
>>>>           .ioctls = i915_ioctls,
>>>>           .num_ioctls = ARRAY_SIZE(i915_ioctls),
>>>>           .fops = &i915_driver_fops,
>>>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
>>>> index c654984189f7..65857c68bdb3 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drm_client.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
>>>> @@ -12,6 +12,7 @@
>>>>    #include <drm/drm_print.h>
>>>>
>>>>    #include "gem/i915_gem_context.h"
>>>> +#include "intel_memory_region.h"
>>>>    #include "i915_drm_client.h"
>>>>    #include "i915_file_private.h"
>>>>    #include "i915_gem.h"
>>>> @@ -112,4 +113,105 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>>           for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
>>>>                   show_client_class(p, i915, file_priv->client, i);
>>>>    }
>>>> +
>>>> +char **
>>>> +i915_query_fdinfo_memory_regions(struct drm_device *dev, unsigned int *num)
>>>> +{
>>>> +       struct drm_i915_private *i915 = to_i915(dev);
>>>> +       struct intel_memory_region *mr;
>>>> +       enum intel_region_id id;
>>>> +
>>>> +       /* FIXME move to init */
>>>> +       for_each_memory_region(mr, i915, id) {
>>>> +               if (!i915->mm.region_names[id])
>>>> +                       i915->mm.region_names[id] = mr->name;
>>>> +       }
>>>> +
>>>> +       *num = id;
>>>> +
>>>> +       return i915->mm.region_names;
>>>> +}
>>>> +
>>>> +static void
>>>> +add_obj(struct drm_i915_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
>>>> +{
>>>> +        struct intel_memory_region *mr;
>>>> +       u64 sz = obj->base.size;
>>>> +        enum intel_region_id id;
>>>> +       unsigned int i;
>>>> +
>>>> +       if (!obj)
>>>> +               return;
>>>> +
>>>> +       /* Attribute size and shared to all possible memory regions. */
>>>> +       for (i = 0; i < obj->mm.n_placements; i++) {
>>>> +               mr = obj->mm.placements[i];
>>>> +               id = mr->id;
>>>> +
>>>> +               stats[id].size += sz;
>>>
>>> This implies that summing up all of the categories is not the same as
>>> the toplevel stats that I was proposing
> 
> Sorry, I mis-spoke, I meant "summing up all of the regions is not..."

Ah okay. It could be made like that yes.

I wasn't sure what would be more useful for drivers which support memory 
regions. To see how much memory file could be using worst case, or 
strictly how much it is currently using. So for buffer objects where 
userspace allows kernel to choose the region from a supplied list, I 
thought it would be useful to show that in total size against all 
possible regions.

In a way you see this driver /could/ be using 1G in vram and 1G in 
system, but currently it only has resident 1G in vram. Or you see 
another file which has 1G vram size and 1G resident size and you can 
infer some things.

Perhaps that can be confusing and it would be better to let total size 
migrate between regions at runtime as does resident and other 
categories. But then the total size per region would change at runtime 
influenced by other app activity (as driver is transparently migrating 
buffers between regions). Which can also be very confusing, it would 
appear as if the app is creating/freeing objects when it isn't.
>> Correct, my categories are a bit different. You had private and shared as two mutually exclusive buckets, and then resident as subset of either/both. I have size as analogue to VmSize and resident as a subset of that, analogue to VmRss.
>>
> 
> I split shared because by definition shared buffers can be counted
> against multiple drm_file's, whereas private is only counted against
> the single drm_file.  Driver or app changes are unlikely to change the
> shared size, whereas private footprint is a thing you can optimize to
> some degree.
 >
>> Shared is a bit wishy-washy, not sure about that one in either proposals. It can be either imported or exported buffers, but in essence I think it fits better as a subset of total size.
> 
> Imported vs exported doesn't really matter.. it is just an
> implementation detail of the winsys.  But I think it is useful to know
> how much of an app's footprint is shared vs private.  You could
> express it different ways, but my proposal had private and shared,
> from which you can calculate total:
> 
>     total = private + shared
> 
> but you could flip the path around and advertise just total and
> shared, and calculate private from that.

Yeah I am not sure. My gut feeling was that stable "top level" size is 
the best option. Aka "this is how much this file could be using worst case".

If shared for file A can drop once file B closes the object it 
previously imported from A, I think that could be confusing. Because A 
did nothing - it is not suddenly using more private memory (hasn't 
allocated anything) nor has closed any shared memory objects.

And on a tangent, but what about shared vs private stats when we have 
userptr object created from shared memory? Core cannot really untangle 
those. Or the memory allocated for other than buffer objects as I argue 
in the cover letter.

Regards,

Tvrtko

Re: [Intel-gfx] [RFC 6/6] drm/i915: Implement fdinfo memory stats printing

[Rob Clark]

On Wed, Apr 19, 2023 at 7:06 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 18/04/2023 17:08, Rob Clark wrote:
> > On Tue, Apr 18, 2023 at 7:58 AM Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> >> On 18/04/2023 15: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>
> >>>>
> >>>> Show how more driver specific set of memory stats could be shown,
> >>>> more specifically where object can reside in multiple regions, showing all
> >>>> the supported stats, and where there is more to show than just user visible
> >>>> objects.
> >>>>
> >>>> WIP...
> >>>>
> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/i915_driver.c     |   5 ++
> >>>>    drivers/gpu/drm/i915/i915_drm_client.c | 102 +++++++++++++++++++++++++
> >>>>    drivers/gpu/drm/i915/i915_drm_client.h |   8 ++
> >>>>    drivers/gpu/drm/i915/i915_drv.h        |   2 +
> >>>>    4 files changed, 117 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> >>>> index 6493548c69bf..4c70206cbc27 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_driver.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
> >>>> @@ -1806,6 +1806,11 @@ static const struct drm_driver i915_drm_driver = {
> >>>>           .dumb_create = i915_gem_dumb_create,
> >>>>           .dumb_map_offset = i915_gem_dumb_mmap_offset,
> >>>>
> >>>> +#ifdef CONFIG_PROC_FS
> >>>> +       .query_fdinfo_memory_regions = i915_query_fdinfo_memory_regions,
> >>>> +       .query_fdinfo_memory_stats = i915_query_fdinfo_memory_stats,
> >>>> +#endif
> >>>> +
> >>>>           .ioctls = i915_ioctls,
> >>>>           .num_ioctls = ARRAY_SIZE(i915_ioctls),
> >>>>           .fops = &i915_driver_fops,
> >>>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> >>>> index c654984189f7..65857c68bdb3 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_drm_client.c
> >>>> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> >>>> @@ -12,6 +12,7 @@
> >>>>    #include <drm/drm_print.h>
> >>>>
> >>>>    #include "gem/i915_gem_context.h"
> >>>> +#include "intel_memory_region.h"
> >>>>    #include "i915_drm_client.h"
> >>>>    #include "i915_file_private.h"
> >>>>    #include "i915_gem.h"
> >>>> @@ -112,4 +113,105 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
> >>>>           for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
> >>>>                   show_client_class(p, i915, file_priv->client, i);
> >>>>    }
> >>>> +
> >>>> +char **
> >>>> +i915_query_fdinfo_memory_regions(struct drm_device *dev, unsigned int *num)
> >>>> +{
> >>>> +       struct drm_i915_private *i915 = to_i915(dev);
> >>>> +       struct intel_memory_region *mr;
> >>>> +       enum intel_region_id id;
> >>>> +
> >>>> +       /* FIXME move to init */
> >>>> +       for_each_memory_region(mr, i915, id) {
> >>>> +               if (!i915->mm.region_names[id])
> >>>> +                       i915->mm.region_names[id] = mr->name;
> >>>> +       }
> >>>> +
> >>>> +       *num = id;
> >>>> +
> >>>> +       return i915->mm.region_names;
> >>>> +}
> >>>> +
> >>>> +static void
> >>>> +add_obj(struct drm_i915_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
> >>>> +{
> >>>> +        struct intel_memory_region *mr;
> >>>> +       u64 sz = obj->base.size;
> >>>> +        enum intel_region_id id;
> >>>> +       unsigned int i;
> >>>> +
> >>>> +       if (!obj)
> >>>> +               return;
> >>>> +
> >>>> +       /* Attribute size and shared to all possible memory regions. */
> >>>> +       for (i = 0; i < obj->mm.n_placements; i++) {
> >>>> +               mr = obj->mm.placements[i];
> >>>> +               id = mr->id;
> >>>> +
> >>>> +               stats[id].size += sz;
> >>>
> >>> This implies that summing up all of the categories is not the same as
> >>> the toplevel stats that I was proposing
> >
> > Sorry, I mis-spoke, I meant "summing up all of the regions is not..."
>
> Ah okay. It could be made like that yes.
>
> I wasn't sure what would be more useful for drivers which support memory
> regions. To see how much memory file could be using worst case, or
> strictly how much it is currently using. So for buffer objects where
> userspace allows kernel to choose the region from a supplied list, I
> thought it would be useful to show that in total size against all
> possible regions.
>
> In a way you see this driver /could/ be using 1G in vram and 1G in
> system, but currently it only has resident 1G in vram. Or you see
> another file which has 1G vram size and 1G resident size and you can
> infer some things.

AFAIU all the buffers could exist in system memory at some point in
time, and vram is more like an explicitly managed fast cache.  Like,
what happens on suspend to ram or hibernate, I assume you don't keep
vram powered?

> Perhaps that can be confusing and it would be better to let total size
> migrate between regions at runtime as does resident and other
> categories. But then the total size per region would change at runtime
> influenced by other app activity (as driver is transparently migrating
> buffers between regions). Which can also be very confusing, it would
> appear as if the app is creating/freeing objects when it isn't.
> >> Correct, my categories are a bit different. You had private and shared as two mutually exclusive buckets, and then resident as subset of either/both. I have size as analogue to VmSize and resident as a subset of that, analogue to VmRss.
> >>
> >
> > I split shared because by definition shared buffers can be counted
> > against multiple drm_file's, whereas private is only counted against
> > the single drm_file.  Driver or app changes are unlikely to change the
> > shared size, whereas private footprint is a thing you can optimize to
> > some degree.
>  >
> >> Shared is a bit wishy-washy, not sure about that one in either proposals. It can be either imported or exported buffers, but in essence I think it fits better as a subset of total size.
> >
> > Imported vs exported doesn't really matter.. it is just an
> > implementation detail of the winsys.  But I think it is useful to know
> > how much of an app's footprint is shared vs private.  You could
> > express it different ways, but my proposal had private and shared,
> > from which you can calculate total:
> >
> >     total = private + shared
> >
> > but you could flip the path around and advertise just total and
> > shared, and calculate private from that.
>
> Yeah I am not sure. My gut feeling was that stable "top level" size is
> the best option. Aka "this is how much this file could be using worst case".
>
> If shared for file A can drop once file B closes the object it
> previously imported from A, I think that could be confusing. Because A
> did nothing - it is not suddenly using more private memory (hasn't
> allocated anything) nor has closed any shared memory objects.

ok, fair

> And on a tangent, but what about shared vs private stats when we have
> userptr object created from shared memory? Core cannot really untangle
> those. Or the memory allocated for other than buffer objects as I argue
> in the cover letter.

hmm, not sure.. I'd be inclined to just count them as private.  Are
you allowed to dma-buf export a userptr buffer?  That seems like it
could go pretty badly..

BR,
-R

> Regards,
>
> Tvrtko

Re: [Intel-gfx] [RFC 6/6] drm/i915: Implement fdinfo memory stats printing

[Tvrtko Ursulin]

On 19/04/2023 15:38, Rob Clark wrote:
> On Wed, Apr 19, 2023 at 7:06 AM Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
>>
>>
>> On 18/04/2023 17:08, Rob Clark wrote:
>>> On Tue, Apr 18, 2023 at 7:58 AM Tvrtko Ursulin
>>> <tvrtko.ursulin@linux.intel.com> wrote:
>>>> On 18/04/2023 15: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>
>>>>>>
>>>>>> Show how more driver specific set of memory stats could be shown,
>>>>>> more specifically where object can reside in multiple regions, showing all
>>>>>> the supported stats, and where there is more to show than just user visible
>>>>>> objects.
>>>>>>
>>>>>> WIP...
>>>>>>
>>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/i915_driver.c     |   5 ++
>>>>>>     drivers/gpu/drm/i915/i915_drm_client.c | 102 +++++++++++++++++++++++++
>>>>>>     drivers/gpu/drm/i915/i915_drm_client.h |   8 ++
>>>>>>     drivers/gpu/drm/i915/i915_drv.h        |   2 +
>>>>>>     4 files changed, 117 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
>>>>>> index 6493548c69bf..4c70206cbc27 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_driver.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
>>>>>> @@ -1806,6 +1806,11 @@ static const struct drm_driver i915_drm_driver = {
>>>>>>            .dumb_create = i915_gem_dumb_create,
>>>>>>            .dumb_map_offset = i915_gem_dumb_mmap_offset,
>>>>>>
>>>>>> +#ifdef CONFIG_PROC_FS
>>>>>> +       .query_fdinfo_memory_regions = i915_query_fdinfo_memory_regions,
>>>>>> +       .query_fdinfo_memory_stats = i915_query_fdinfo_memory_stats,
>>>>>> +#endif
>>>>>> +
>>>>>>            .ioctls = i915_ioctls,
>>>>>>            .num_ioctls = ARRAY_SIZE(i915_ioctls),
>>>>>>            .fops = &i915_driver_fops,
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
>>>>>> index c654984189f7..65857c68bdb3 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_drm_client.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
>>>>>> @@ -12,6 +12,7 @@
>>>>>>     #include <drm/drm_print.h>
>>>>>>
>>>>>>     #include "gem/i915_gem_context.h"
>>>>>> +#include "intel_memory_region.h"
>>>>>>     #include "i915_drm_client.h"
>>>>>>     #include "i915_file_private.h"
>>>>>>     #include "i915_gem.h"
>>>>>> @@ -112,4 +113,105 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>>>>            for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
>>>>>>                    show_client_class(p, i915, file_priv->client, i);
>>>>>>     }
>>>>>> +
>>>>>> +char **
>>>>>> +i915_query_fdinfo_memory_regions(struct drm_device *dev, unsigned int *num)
>>>>>> +{
>>>>>> +       struct drm_i915_private *i915 = to_i915(dev);
>>>>>> +       struct intel_memory_region *mr;
>>>>>> +       enum intel_region_id id;
>>>>>> +
>>>>>> +       /* FIXME move to init */
>>>>>> +       for_each_memory_region(mr, i915, id) {
>>>>>> +               if (!i915->mm.region_names[id])
>>>>>> +                       i915->mm.region_names[id] = mr->name;
>>>>>> +       }
>>>>>> +
>>>>>> +       *num = id;
>>>>>> +
>>>>>> +       return i915->mm.region_names;
>>>>>> +}
>>>>>> +
>>>>>> +static void
>>>>>> +add_obj(struct drm_i915_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
>>>>>> +{
>>>>>> +        struct intel_memory_region *mr;
>>>>>> +       u64 sz = obj->base.size;
>>>>>> +        enum intel_region_id id;
>>>>>> +       unsigned int i;
>>>>>> +
>>>>>> +       if (!obj)
>>>>>> +               return;
>>>>>> +
>>>>>> +       /* Attribute size and shared to all possible memory regions. */
>>>>>> +       for (i = 0; i < obj->mm.n_placements; i++) {
>>>>>> +               mr = obj->mm.placements[i];
>>>>>> +               id = mr->id;
>>>>>> +
>>>>>> +               stats[id].size += sz;
>>>>>
>>>>> This implies that summing up all of the categories is not the same as
>>>>> the toplevel stats that I was proposing
>>>
>>> Sorry, I mis-spoke, I meant "summing up all of the regions is not..."
>>
>> Ah okay. It could be made like that yes.
>>
>> I wasn't sure what would be more useful for drivers which support memory
>> regions. To see how much memory file could be using worst case, or
>> strictly how much it is currently using. So for buffer objects where
>> userspace allows kernel to choose the region from a supplied list, I
>> thought it would be useful to show that in total size against all
>> possible regions.
>>
>> In a way you see this driver /could/ be using 1G in vram and 1G in
>> system, but currently it only has resident 1G in vram. Or you see
>> another file which has 1G vram size and 1G resident size and you can
>> infer some things.
> 
> AFAIU all the buffers could exist in system memory at some point in
> time, and vram is more like an explicitly managed fast cache.  Like,
> what happens on suspend to ram or hibernate, I assume you don't keep
> vram powered?

Yeah they can be swapped out on suspend, but that's different that 
buffers which are explicitly marked as being allowed to exist in either 
region at runtime.

>> Perhaps that can be confusing and it would be better to let total size
>> migrate between regions at runtime as does resident and other
>> categories. But then the total size per region would change at runtime
>> influenced by other app activity (as driver is transparently migrating
>> buffers between regions). Which can also be very confusing, it would
>> appear as if the app is creating/freeing objects when it isn't.
>>>> Correct, my categories are a bit different. You had private and shared as two mutually exclusive buckets, and then resident as subset of either/both. I have size as analogue to VmSize and resident as a subset of that, analogue to VmRss.
>>>>
>>>
>>> I split shared because by definition shared buffers can be counted
>>> against multiple drm_file's, whereas private is only counted against
>>> the single drm_file.  Driver or app changes are unlikely to change the
>>> shared size, whereas private footprint is a thing you can optimize to
>>> some degree.
>>   >
>>>> Shared is a bit wishy-washy, not sure about that one in either proposals. It can be either imported or exported buffers, but in essence I think it fits better as a subset of total size.
>>>
>>> Imported vs exported doesn't really matter.. it is just an
>>> implementation detail of the winsys.  But I think it is useful to know
>>> how much of an app's footprint is shared vs private.  You could
>>> express it different ways, but my proposal had private and shared,
>>> from which you can calculate total:
>>>
>>>      total = private + shared
>>>
>>> but you could flip the path around and advertise just total and
>>> shared, and calculate private from that.
>>
>> Yeah I am not sure. My gut feeling was that stable "top level" size is
>> the best option. Aka "this is how much this file could be using worst case".
>>
>> If shared for file A can drop once file B closes the object it
>> previously imported from A, I think that could be confusing. Because A
>> did nothing - it is not suddenly using more private memory (hasn't
>> allocated anything) nor has closed any shared memory objects.
> 
> ok, fair
> 
>> And on a tangent, but what about shared vs private stats when we have
>> userptr object created from shared memory? Core cannot really untangle
>> those. Or the memory allocated for other than buffer objects as I argue
>> in the cover letter.
> 
> hmm, not sure.. I'd be inclined to just count them as private.  Are
> you allowed to dma-buf export a userptr buffer?  That seems like it
> could go pretty badly..

AFAIR we forbid that, but my point was more that there is shared memory 
and shared memory, not related to dma-buf I mean. Just that two 
processes could create two userptr objects from the same shared memory 
block. Memory accounting is as always complicated.

Regards,

Tvrtko

Re: [Intel-gfx] [RFC 6/6] drm/i915: Implement fdinfo memory stats printing

[Rob Clark]

On Thu, Apr 20, 2023 at 6:11 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 19/04/2023 15:38, Rob Clark wrote:
> > On Wed, Apr 19, 2023 at 7:06 AM Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> >>
> >>
> >> On 18/04/2023 17:08, Rob Clark wrote:
> >>> On Tue, Apr 18, 2023 at 7:58 AM Tvrtko Ursulin
> >>> <tvrtko.ursulin@linux.intel.com> wrote:
> >>>> On 18/04/2023 15: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>
> >>>>>>
> >>>>>> Show how more driver specific set of memory stats could be shown,
> >>>>>> more specifically where object can reside in multiple regions, showing all
> >>>>>> the supported stats, and where there is more to show than just user visible
> >>>>>> objects.
> >>>>>>
> >>>>>> WIP...
> >>>>>>
> >>>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>> ---
> >>>>>>     drivers/gpu/drm/i915/i915_driver.c     |   5 ++
> >>>>>>     drivers/gpu/drm/i915/i915_drm_client.c | 102 +++++++++++++++++++++++++
> >>>>>>     drivers/gpu/drm/i915/i915_drm_client.h |   8 ++
> >>>>>>     drivers/gpu/drm/i915/i915_drv.h        |   2 +
> >>>>>>     4 files changed, 117 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_driver.c b/drivers/gpu/drm/i915/i915_driver.c
> >>>>>> index 6493548c69bf..4c70206cbc27 100644
> >>>>>> --- a/drivers/gpu/drm/i915/i915_driver.c
> >>>>>> +++ b/drivers/gpu/drm/i915/i915_driver.c
> >>>>>> @@ -1806,6 +1806,11 @@ static const struct drm_driver i915_drm_driver = {
> >>>>>>            .dumb_create = i915_gem_dumb_create,
> >>>>>>            .dumb_map_offset = i915_gem_dumb_mmap_offset,
> >>>>>>
> >>>>>> +#ifdef CONFIG_PROC_FS
> >>>>>> +       .query_fdinfo_memory_regions = i915_query_fdinfo_memory_regions,
> >>>>>> +       .query_fdinfo_memory_stats = i915_query_fdinfo_memory_stats,
> >>>>>> +#endif
> >>>>>> +
> >>>>>>            .ioctls = i915_ioctls,
> >>>>>>            .num_ioctls = ARRAY_SIZE(i915_ioctls),
> >>>>>>            .fops = &i915_driver_fops,
> >>>>>> diff --git a/drivers/gpu/drm/i915/i915_drm_client.c b/drivers/gpu/drm/i915/i915_drm_client.c
> >>>>>> index c654984189f7..65857c68bdb3 100644
> >>>>>> --- a/drivers/gpu/drm/i915/i915_drm_client.c
> >>>>>> +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> >>>>>> @@ -12,6 +12,7 @@
> >>>>>>     #include <drm/drm_print.h>
> >>>>>>
> >>>>>>     #include "gem/i915_gem_context.h"
> >>>>>> +#include "intel_memory_region.h"
> >>>>>>     #include "i915_drm_client.h"
> >>>>>>     #include "i915_file_private.h"
> >>>>>>     #include "i915_gem.h"
> >>>>>> @@ -112,4 +113,105 @@ void i915_drm_client_fdinfo(struct drm_printer *p, struct drm_file *file)
> >>>>>>            for (i = 0; i < ARRAY_SIZE(uabi_class_names); i++)
> >>>>>>                    show_client_class(p, i915, file_priv->client, i);
> >>>>>>     }
> >>>>>> +
> >>>>>> +char **
> >>>>>> +i915_query_fdinfo_memory_regions(struct drm_device *dev, unsigned int *num)
> >>>>>> +{
> >>>>>> +       struct drm_i915_private *i915 = to_i915(dev);
> >>>>>> +       struct intel_memory_region *mr;
> >>>>>> +       enum intel_region_id id;
> >>>>>> +
> >>>>>> +       /* FIXME move to init */
> >>>>>> +       for_each_memory_region(mr, i915, id) {
> >>>>>> +               if (!i915->mm.region_names[id])
> >>>>>> +                       i915->mm.region_names[id] = mr->name;
> >>>>>> +       }
> >>>>>> +
> >>>>>> +       *num = id;
> >>>>>> +
> >>>>>> +       return i915->mm.region_names;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void
> >>>>>> +add_obj(struct drm_i915_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
> >>>>>> +{
> >>>>>> +        struct intel_memory_region *mr;
> >>>>>> +       u64 sz = obj->base.size;
> >>>>>> +        enum intel_region_id id;
> >>>>>> +       unsigned int i;
> >>>>>> +
> >>>>>> +       if (!obj)
> >>>>>> +               return;
> >>>>>> +
> >>>>>> +       /* Attribute size and shared to all possible memory regions. */
> >>>>>> +       for (i = 0; i < obj->mm.n_placements; i++) {
> >>>>>> +               mr = obj->mm.placements[i];
> >>>>>> +               id = mr->id;
> >>>>>> +
> >>>>>> +               stats[id].size += sz;
> >>>>>
> >>>>> This implies that summing up all of the categories is not the same as
> >>>>> the toplevel stats that I was proposing
> >>>
> >>> Sorry, I mis-spoke, I meant "summing up all of the regions is not..."
> >>
> >> Ah okay. It could be made like that yes.
> >>
> >> I wasn't sure what would be more useful for drivers which support memory
> >> regions. To see how much memory file could be using worst case, or
> >> strictly how much it is currently using. So for buffer objects where
> >> userspace allows kernel to choose the region from a supplied list, I
> >> thought it would be useful to show that in total size against all
> >> possible regions.
> >>
> >> In a way you see this driver /could/ be using 1G in vram and 1G in
> >> system, but currently it only has resident 1G in vram. Or you see
> >> another file which has 1G vram size and 1G resident size and you can
> >> infer some things.
> >
> > AFAIU all the buffers could exist in system memory at some point in
> > time, and vram is more like an explicitly managed fast cache.  Like,
> > what happens on suspend to ram or hibernate, I assume you don't keep
> > vram powered?
>
> Yeah they can be swapped out on suspend, but that's different that
> buffers which are explicitly marked as being allowed to exist in either
> region at runtime.

it sounds, at least from the PoV of system memory usage, they still
need pages in system memory so they have somewhere to live when
swapped out of vram?

> >> Perhaps that can be confusing and it would be better to let total size
> >> migrate between regions at runtime as does resident and other
> >> categories. But then the total size per region would change at runtime
> >> influenced by other app activity (as driver is transparently migrating
> >> buffers between regions). Which can also be very confusing, it would
> >> appear as if the app is creating/freeing objects when it isn't.
> >>>> Correct, my categories are a bit different. You had private and shared as two mutually exclusive buckets, and then resident as subset of either/both. I have size as analogue to VmSize and resident as a subset of that, analogue to VmRss.
> >>>>
> >>>
> >>> I split shared because by definition shared buffers can be counted
> >>> against multiple drm_file's, whereas private is only counted against
> >>> the single drm_file.  Driver or app changes are unlikely to change the
> >>> shared size, whereas private footprint is a thing you can optimize to
> >>> some degree.
> >>   >
> >>>> Shared is a bit wishy-washy, not sure about that one in either proposals. It can be either imported or exported buffers, but in essence I think it fits better as a subset of total size.
> >>>
> >>> Imported vs exported doesn't really matter.. it is just an
> >>> implementation detail of the winsys.  But I think it is useful to know
> >>> how much of an app's footprint is shared vs private.  You could
> >>> express it different ways, but my proposal had private and shared,
> >>> from which you can calculate total:
> >>>
> >>>      total = private + shared
> >>>
> >>> but you could flip the path around and advertise just total and
> >>> shared, and calculate private from that.
> >>
> >> Yeah I am not sure. My gut feeling was that stable "top level" size is
> >> the best option. Aka "this is how much this file could be using worst case".
> >>
> >> If shared for file A can drop once file B closes the object it
> >> previously imported from A, I think that could be confusing. Because A
> >> did nothing - it is not suddenly using more private memory (hasn't
> >> allocated anything) nor has closed any shared memory objects.
> >
> > ok, fair
> >
> >> And on a tangent, but what about shared vs private stats when we have
> >> userptr object created from shared memory? Core cannot really untangle
> >> those. Or the memory allocated for other than buffer objects as I argue
> >> in the cover letter.
> >
> > hmm, not sure.. I'd be inclined to just count them as private.  Are
> > you allowed to dma-buf export a userptr buffer?  That seems like it
> > could go pretty badly..
>
> AFAIR we forbid that, but my point was more that there is shared memory
> and shared memory, not related to dma-buf I mean. Just that two
> processes could create two userptr objects from the same shared memory
> block. Memory accounting is as always complicated.

don't let "perfect" be the enemy of "good", especially in this case
when it sounds like "perfect" isn't even possible ;-)

ie, we should just count as private vs shared based on handle count

BR,
-R

> Regards,
>
> Tvrtko

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for fdinfo alternative memory stats proposal

[Patchwork]

== Series Details ==

Series: fdinfo alternative memory stats proposal
URL   : https://patchwork.freedesktop.org/series/116581/
State : warning

== Summary ==

Error: dim checkpatch failed
92bb2d834df8 drm: Add common fdinfo helper
1e36b9258ec6 drm/i915: Use the fdinfo helper
368bec1f61d6 drm: Add fdinfo memory stats
-:59: WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be better as static const
#59: FILE: drivers/gpu/drm/drm_file.c:878:
+	const char *units[] = {"", " KiB", " MiB"};

total: 0 errors, 1 warnings, 0 checks, 127 lines checked
31ff5fcacf6d drm: Add simple fdinfo memory helpers
-:40: WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be better as static const
#40: FILE: drivers/gpu/drm/drm_file.c:896:
+	static char *region[] = {

-:80: CHECK:LINE_SPACING: Please don't use multiple blank lines
#80: FILE: include/drm/drm_file.h:391:
+
+

total: 0 errors, 1 warnings, 1 checks, 63 lines checked
6db9de4ab15b drm/msm: Add basic memory stats
dd1f311a52b7 drm/i915: Implement fdinfo memory stats printing
-:69: ERROR:CODE_INDENT: code indent should use tabs where possible
#69: FILE: drivers/gpu/drm/i915/i915_drm_client.c:138:
+        struct intel_memory_region *mr;$

-:69: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#69: FILE: drivers/gpu/drm/i915/i915_drm_client.c:138:
+        struct intel_memory_region *mr;$

-:71: ERROR:CODE_INDENT: code indent should use tabs where possible
#71: FILE: drivers/gpu/drm/i915/i915_drm_client.c:140:
+        enum intel_region_id id;$

-:71: WARNING:LEADING_SPACE: please, no spaces at the start of a line
#71: FILE: drivers/gpu/drm/i915/i915_drm_client.c:140:
+        enum intel_region_id id;$

-:102: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#102: FILE: drivers/gpu/drm/i915/i915_drm_client.c:171:
+	else if (i915_gem_object_is_shrinkable(obj) &&
+		obj->mm.madv == I915_MADV_DONTNEED)

total: 2 errors, 2 warnings, 1 checks, 152 lines checked

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for fdinfo alternative memory stats proposal

[Patchwork]

== Series Details ==

Series: fdinfo alternative memory stats proposal
URL   : https://patchwork.freedesktop.org/series/116581/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✓ Fi.CI.BAT: success for fdinfo alternative memory stats proposal

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4651 bytes --]

== Series Details ==

Series: fdinfo alternative memory stats proposal
URL   : https://patchwork.freedesktop.org/series/116581/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13022 -> Patchwork_116581v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/index.html

Participating hosts (37 -> 36)
------------------------------

  Missing    (1): fi-snb-2520m 

Known issues
------------

  Here are the changes found in Patchwork_116581v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_suspend@basic-s3@smem:
    - bat-rpls-1:         NOTRUN -> [ABORT][1] ([i915#6687] / [i915#7978])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/bat-rpls-1/igt@gem_exec_suspend@basic-s3@smem.html

  * igt@i915_pm_rps@basic-api:
    - bat-dg2-11:         [PASS][2] -> [FAIL][3] ([i915#8308])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/bat-dg2-11/igt@i915_pm_rps@basic-api.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/bat-dg2-11/igt@i915_pm_rps@basic-api.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-2:         [PASS][4] -> [ABORT][5] ([i915#4983] / [i915#7913])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/bat-rpls-2/igt@i915_selftest@live@reset.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/bat-rpls-2/igt@i915_selftest@live@reset.html

  * igt@i915_selftest@live@slpc:
    - bat-rpls-1:         NOTRUN -> [DMESG-FAIL][6] ([i915#6367] / [i915#7996])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/bat-rpls-1/igt@i915_selftest@live@slpc.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-apl-guc:         [PASS][7] -> [DMESG-WARN][8] ([i915#1982])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/fi-apl-guc/igt@i915_suspend@basic-s3-without-i915.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/fi-apl-guc/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_pipe_crc_basic@read-crc:
    - bat-adlp-9:         NOTRUN -> [SKIP][9] ([i915#3546]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/bat-adlp-9/igt@kms_pipe_crc_basic@read-crc.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [DMESG-FAIL][10] ([i915#5334]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@requests:
    - bat-rpls-1:         [ABORT][12] ([i915#7911]) -> [PASS][13]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/bat-rpls-1/igt@i915_selftest@live@requests.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/bat-rpls-1/igt@i915_selftest@live@requests.html

  
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#3546]: https://gitlab.freedesktop.org/drm/intel/issues/3546
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6687]: https://gitlab.freedesktop.org/drm/intel/issues/6687
  [i915#7911]: https://gitlab.freedesktop.org/drm/intel/issues/7911
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7978]: https://gitlab.freedesktop.org/drm/intel/issues/7978
  [i915#7996]: https://gitlab.freedesktop.org/drm/intel/issues/7996
  [i915#8308]: https://gitlab.freedesktop.org/drm/intel/issues/8308


Build changes
-------------

  * Linux: CI_DRM_13022 -> Patchwork_116581v1

  CI-20190529: 20190529
  CI_DRM_13022: acfc88011228f839fcd32798637e419e429304bd @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7258: ad2eb276eda849b7a7985229009a816c7608186c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116581v1: acfc88011228f839fcd32798637e419e429304bd @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

69b9be235707 drm/i915: Implement fdinfo memory stats printing
f36f579a1ae5 drm/msm: Add basic memory stats
8eabef51ce99 drm: Add simple fdinfo memory helpers
972a11ccf5ad drm: Add fdinfo memory stats
67129cbc4c47 drm/i915: Use the fdinfo helper
3df872e57aca drm: Add common fdinfo helper

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/index.html

[-- Attachment #2: Type: text/html, Size: 5474 bytes --]

[Intel-gfx] ✓ Fi.CI.IGT: success for fdinfo alternative memory stats proposal

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 12749 bytes --]

== Series Details ==

Series: fdinfo alternative memory stats proposal
URL   : https://patchwork.freedesktop.org/series/116581/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13022_full -> Patchwork_116581v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (7 -> 8)
------------------------------

  Additional (1): pig-kbl-iris 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_116581v1_full:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-0-async-flip:
    - {shard-dg1}:        NOTRUN -> [FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-dg1-14/igt@kms_big_fb@y-tiled-max-hw-stride-64bpp-rotate-0-async-flip.html

  
Known issues
------------

  Here are the changes found in Patchwork_116581v1_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-apl:          [PASS][2] -> [FAIL][3] ([i915#2842])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-apl1/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-apl6/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gen9_exec_parse@allowed-single:
    - shard-glk:          [PASS][4] -> [ABORT][5] ([i915#5566])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-glk5/igt@gen9_exec_parse@allowed-single.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-glk3/igt@gen9_exec_parse@allowed-single.html

  * igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_gen12_mc_ccs:
    - shard-glk:          NOTRUN -> [SKIP][6] ([fdo#109271] / [i915#3886])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-glk7/igt@kms_ccs@pipe-a-crc-sprite-planes-basic-y_tiled_gen12_mc_ccs.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
    - shard-glk:          [PASS][7] -> [FAIL][8] ([i915#2346])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-glk7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-glk7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-apl:          [PASS][9] -> [FAIL][10] ([i915#2346]) +1 similar issue
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-apl6/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-apl4/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-spr-indfb-draw-mmap-wc:
    - shard-glk:          NOTRUN -> [SKIP][11] ([fdo#109271]) +29 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-glk7/igt@kms_frontbuffer_tracking@fbcpsr-2p-scndscrn-spr-indfb-draw-mmap-wc.html

  * igt@kms_psr2_sf@overlay-plane-move-continuous-sf:
    - shard-glk:          NOTRUN -> [SKIP][12] ([fdo#109271] / [i915#658])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-glk7/igt@kms_psr2_sf@overlay-plane-move-continuous-sf.html

  
#### Possible fixes ####

  * igt@drm_fdinfo@most-busy-idle-check-all@rcs0:
    - {shard-rkl}:        [FAIL][13] ([i915#7742]) -> [PASS][14] +1 similar issue
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-rkl-7/igt@drm_fdinfo@most-busy-idle-check-all@rcs0.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-rkl-1/igt@drm_fdinfo@most-busy-idle-check-all@rcs0.html

  * igt@gem_exec_fair@basic-pace@rcs0:
    - {shard-rkl}:        [FAIL][15] ([i915#2842]) -> [PASS][16] +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-rkl-7/igt@gem_exec_fair@basic-pace@rcs0.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-rkl-7/igt@gem_exec_fair@basic-pace@rcs0.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-glk:          [ABORT][17] ([i915#5566]) -> [PASS][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-glk7/igt@gen9_exec_parse@allowed-all.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-glk7/igt@gen9_exec_parse@allowed-all.html

  * igt@i915_pm_rpm@modeset-non-lpsp:
    - {shard-rkl}:        [SKIP][19] ([i915#1397]) -> [PASS][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-rkl-7/igt@i915_pm_rpm@modeset-non-lpsp.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-rkl-1/igt@i915_pm_rpm@modeset-non-lpsp.html

  * igt@i915_selftest@live@gt_heartbeat:
    - shard-apl:          [DMESG-FAIL][21] ([i915#5334]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-apl3/igt@i915_selftest@live@gt_heartbeat.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-apl2/igt@i915_selftest@live@gt_heartbeat.html

  * igt@kms_cursor_legacy@single-bo@pipe-b:
    - {shard-dg1}:        [INCOMPLETE][23] ([i915#8011] / [i915#8347]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-dg1-14/igt@kms_cursor_legacy@single-bo@pipe-b.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-dg1-15/igt@kms_cursor_legacy@single-bo@pipe-b.html

  * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2:
    - shard-glk:          [FAIL][25] ([i915#79]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13022/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/shard-glk6/igt@kms_flip@2x-flip-vs-expired-vblank-interruptible@ac-hdmi-a1-hdmi-a2.html

  

### Piglit changes ###

#### Issues hit ####

  * igt@i915_suspend@basic-s3-without-i915:
    - pig-kbl-iris:       NOTRUN -> [INCOMPLETE][27] ([i915#5603])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/pig-kbl-iris/igt@i915_suspend@basic-s3-without-i915.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109285]: https://bugs.freedesktop.org/show_bug.cgi?id=109285
  [fdo#109289]: https://bugs.freedesktop.org/show_bug.cgi?id=109289
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111825]: https://bugs.freedesktop.org/show_bug.cgi?id=111825
  [fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#1257]: https://gitlab.freedesktop.org/drm/intel/issues/1257
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#2346]: https://gitlab.freedesktop.org/drm/intel/issues/2346
  [i915#2434]: https://gitlab.freedesktop.org/drm/intel/issues/2434
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#3063]: https://gitlab.freedesktop.org/drm/intel/issues/3063
  [i915#3281]: https://gitlab.freedesktop.org/drm/intel/issues/3281
  [i915#3282]: https://gitlab.freedesktop.org/drm/intel/issues/3282
  [i915#3297]: https://gitlab.freedesktop.org/drm/intel/issues/3297
  [i915#3359]: https://gitlab.freedesktop.org/drm/intel/issues/3359
  [i915#3458]: https://gitlab.freedesktop.org/drm/intel/issues/3458
  [i915#3539]: https://gitlab.freedesktop.org/drm/intel/issues/3539
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3638]: https://gitlab.freedesktop.org/drm/intel/issues/3638
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3708]: https://gitlab.freedesktop.org/drm/intel/issues/3708
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#3938]: https://gitlab.freedesktop.org/drm/intel/issues/3938
  [i915#3955]: https://gitlab.freedesktop.org/drm/intel/issues/3955
  [i915#4077]: https://gitlab.freedesktop.org/drm/intel/issues/4077
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4079]: https://gitlab.freedesktop.org/drm/intel/issues/4079
  [i915#4083]: https://gitlab.freedesktop.org/drm/intel/issues/4083
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4281]: https://gitlab.freedesktop.org/drm/intel/issues/4281
  [i915#4391]: https://gitlab.freedesktop.org/drm/intel/issues/4391
  [i915#4538]: https://gitlab.freedesktop.org/drm/intel/issues/4538
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4812]: https://gitlab.freedesktop.org/drm/intel/issues/4812
  [i915#4833]: https://gitlab.freedesktop.org/drm/intel/issues/4833
  [i915#4852]: https://gitlab.freedesktop.org/drm/intel/issues/4852
  [i915#4854]: https://gitlab.freedesktop.org/drm/intel/issues/4854
  [i915#4859]: https://gitlab.freedesktop.org/drm/intel/issues/4859
  [i915#4860]: https://gitlab.freedesktop.org/drm/intel/issues/4860
  [i915#4879]: https://gitlab.freedesktop.org/drm/intel/issues/4879
  [i915#4880]: https://gitlab.freedesktop.org/drm/intel/issues/4880
  [i915#4881]: https://gitlab.freedesktop.org/drm/intel/issues/4881
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
  [i915#5289]: https://gitlab.freedesktop.org/drm/intel/issues/5289
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5563]: https://gitlab.freedesktop.org/drm/intel/issues/5563
  [i915#5566]: https://gitlab.freedesktop.org/drm/intel/issues/5566
  [i915#5603]: https://gitlab.freedesktop.org/drm/intel/issues/5603
  [i915#5784]: https://gitlab.freedesktop.org/drm/intel/issues/5784
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#6301]: https://gitlab.freedesktop.org/drm/intel/issues/6301
  [i915#6433]: https://gitlab.freedesktop.org/drm/intel/issues/6433
  [i915#6524]: https://gitlab.freedesktop.org/drm/intel/issues/6524
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#6946]: https://gitlab.freedesktop.org/drm/intel/issues/6946
  [i915#6953]: https://gitlab.freedesktop.org/drm/intel/issues/6953
  [i915#7116]: https://gitlab.freedesktop.org/drm/intel/issues/7116
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7697]: https://gitlab.freedesktop.org/drm/intel/issues/7697
  [i915#7711]: https://gitlab.freedesktop.org/drm/intel/issues/7711
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8150]: https://gitlab.freedesktop.org/drm/intel/issues/8150
  [i915#8155]: https://gitlab.freedesktop.org/drm/intel/issues/8155
  [i915#8247]: https://gitlab.freedesktop.org/drm/intel/issues/8247
  [i915#8311]: https://gitlab.freedesktop.org/drm/intel/issues/8311
  [i915#8347]: https://gitlab.freedesktop.org/drm/intel/issues/8347


Build changes
-------------

  * Linux: CI_DRM_13022 -> Patchwork_116581v1

  CI-20190529: 20190529
  CI_DRM_13022: acfc88011228f839fcd32798637e419e429304bd @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7258: ad2eb276eda849b7a7985229009a816c7608186c @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_116581v1: acfc88011228f839fcd32798637e419e429304bd @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_116581v1/index.html

[-- Attachment #2: Type: text/html, Size: 9457 bytes --]