[PATCH v4 0/6] drm: fdinfo memory stats

[PATCH v4 0/6] drm: fdinfo memory stats

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Similar motivation to other similar recent attempt[1].  But with an
attempt to have some shared code for this.  As well as documentation.

It is probably a bit UMA-centric, I guess devices with VRAM might want
some placement stats as well.  But this seems like a reasonable start.

Basic gputop support: https://patchwork.freedesktop.org/series/116236/
And already nvtop support: https://github.com/Syllo/nvtop/pull/204

[1] https://patchwork.freedesktop.org/series/112397/

Rob Clark (6):
  drm: Add common fdinfo helper
  drm/msm: Switch to fdinfo helper
  drm/amdgpu: Switch to fdinfo helper
  drm/i915: Switch to fdinfo helper
  drm: Add fdinfo memory stats
  drm/msm: Add memory stats to fdinfo

 Documentation/gpu/drm-usage-stats.rst      |  31 +++++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |   3 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c |  16 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h |   2 +-
 drivers/gpu/drm/drm_file.c                 | 111 +++++++++++++++++++++
 drivers/gpu/drm/i915/i915_driver.c         |   3 +-
 drivers/gpu/drm/i915/i915_drm_client.c     |  18 +---
 drivers/gpu/drm/i915/i915_drm_client.h     |   2 +-
 drivers/gpu/drm/msm/msm_drv.c              |  11 +-
 drivers/gpu/drm/msm/msm_gem.c              |  15 +++
 drivers/gpu/drm/msm/msm_gpu.c              |   2 -
 include/drm/drm_drv.h                      |   7 ++
 include/drm/drm_file.h                     |   5 +
 include/drm/drm_gem.h                      |  30 ++++++
 14 files changed, 220 insertions(+), 36 deletions(-)

-- 
2.39.2

[PATCH v4 1/6] drm: Add common fdinfo helper

[Rob Clark]

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 a51ff8cee049..6d5bdd684ae2 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_pid(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 5b86bb7603e7..5edf2a13733b 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 0d1f853092ab..6de6d0e9c634 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;
 
@@ -437,6 +440,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.39.2

[PATCH v4 5/6] drm: Add fdinfo memory stats

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Add support to dump GEM stats to fdinfo.

v2: Fix typos, change size units to match docs, use div_u64
v3: Do it in core
v4: more kerneldoc

Signed-off-by: Rob Clark <robdclark@chromium.org>
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 Documentation/gpu/drm-usage-stats.rst | 21 ++++++++
 drivers/gpu/drm/drm_file.c            | 76 +++++++++++++++++++++++++++
 include/drm/drm_file.h                |  1 +
 include/drm/drm_gem.h                 | 30 +++++++++++
 4 files changed, 128 insertions(+)

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index 2ab32c40e93c..80003e27e28e 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -105,6 +105,27 @@ 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-shared-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are shared with another file (ie. have more
+than a single handle).
+
+- drm-private-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are not shared with another file.
+
+- drm-resident-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are resident in system memory.
+
+- drm-purgeable-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are purgeable.
+
+- drm-active-memory: <uint> [KiB|MiB]
+
+The total size of buffers that are active on one or more rings.
+
 - 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 6d5bdd684ae2..04a7ed7eba8e 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,79 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
 }
 EXPORT_SYMBOL(drm_send_event);
 
+static void print_size(struct drm_printer *p, const char *stat, size_t sz)
+{
+	const char *units[] = {"", " KiB", " MiB"};
+	unsigned u;
+
+	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
+		if (sz < SZ_1K)
+			break;
+		sz = div_u64(sz, SZ_1K);
+	}
+
+	drm_printf(p, "%s:\t%zu%s\n", stat, sz, units[u]);
+}
+
+static void print_memory_stats(struct drm_printer *p, struct drm_file *file)
+{
+	struct drm_gem_object *obj;
+	struct {
+		size_t shared;
+		size_t private;
+		size_t resident;
+		size_t purgeable;
+		size_t active;
+	} size = {0};
+	bool has_status = false;
+	int id;
+
+	spin_lock(&file->table_lock);
+	idr_for_each_entry (&file->object_idr, obj, id) {
+		enum drm_gem_object_status s = 0;
+
+		if (obj->funcs && obj->funcs->status) {
+			s = obj->funcs->status(obj);
+			has_status = true;
+		}
+
+		if (obj->handle_count > 1) {
+			size.shared += obj->size;
+		} else {
+			size.private += obj->size;
+		}
+
+		if (s & DRM_GEM_OBJECT_RESIDENT) {
+			size.resident += obj->size;
+		} else {
+			/* If already purged or not yet backed by pages, don't
+			 * count it as purgeable:
+			 */
+			s &= ~DRM_GEM_OBJECT_PURGEABLE;
+		}
+
+		if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
+			size.active += obj->size;
+
+			/* If still active, don't count as purgeable: */
+			s &= ~DRM_GEM_OBJECT_PURGEABLE;
+		}
+
+		if (s & DRM_GEM_OBJECT_PURGEABLE)
+			size.purgeable += obj->size;
+	}
+	spin_unlock(&file->table_lock);
+
+	print_size(p, "drm-shared-memory", size.shared);
+	print_size(p, "drm-private-memory", size.private);
+	print_size(p, "drm-active-memory", size.active);
+
+	if (has_status) {
+		print_size(p, "drm-resident-memory", size.resident);
+		print_size(p, "drm-purgeable-memory", size.purgeable);
+	}
+}
+
 /**
  * drm_show_fdinfo - helper for drm file fops
  * @seq_file: output stream
@@ -900,6 +974,8 @@ void drm_show_fdinfo(struct seq_file *m, struct file *f)
 
 	if (dev->driver->show_fdinfo)
 		dev->driver->show_fdinfo(&p, file);
+
+	print_memory_stats(&p, file);
 }
 EXPORT_SYMBOL(drm_show_fdinfo);
 
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 6de6d0e9c634..919284bb4f1d 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -41,6 +41,7 @@
 struct dma_fence;
 struct drm_file;
 struct drm_device;
+struct drm_printer;
 struct device;
 struct file;
 
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index 189fd618ca65..9ebd2820ad1f 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -42,6 +42,25 @@
 struct iosys_map;
 struct drm_gem_object;
 
+/**
+ * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
+ * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
+ * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
+ *
+ * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
+ * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
+ * it still active or not resident, in which case drm_show_fdinfo() will not
+ * account for it as purgeable.  So drivers do not need to check if the buffer
+ * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
+ * as purgeable even while it is still busy on the GPU.. it does not _actually_
+ * become puregeable until it becomes idle.  The status gem object func does
+ * not need to consider this.)
+ */
+enum drm_gem_object_status {
+	DRM_GEM_OBJECT_RESIDENT  = BIT(0),
+	DRM_GEM_OBJECT_PURGEABLE = BIT(1),
+};
+
 /**
  * struct drm_gem_object_funcs - GEM object functions
  */
@@ -174,6 +193,17 @@ struct drm_gem_object_funcs {
 	 */
 	int (*evict)(struct drm_gem_object *obj);
 
+	/**
+	 * @status:
+	 *
+	 * The optional status callback can return additional object state
+	 * which determines which stats the object is counted against.  The
+	 * callback is called under table_lock.  Racing against object status
+	 * change is "harmless", and the callback can expect to not race
+	 * against object destruction.
+	 */
+	enum drm_gem_object_status (*status)(struct drm_gem_object *obj);
+
 	/**
 	 * @vm_ops:
 	 *
-- 
2.39.2

Re: [PATCH v4 1/6] drm: Add common fdinfo helper

[Christian König]

Am 13.04.23 um 00:42 schrieb Rob Clark:
> 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.

I'm really wondering if it wouldn't be less mid-layering if we let the 
drivers call the drm function to print the common values instead of the 
other way around?

Apart from that question the patch looks good to me.

Christian.

> +
>   Driver specific implementations
> -===============================
> +-------------------------------
>   
>   :ref:`i915-usage-stats`
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index a51ff8cee049..6d5bdd684ae2 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_pid(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 5b86bb7603e7..5edf2a13733b 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 0d1f853092ab..6de6d0e9c634 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;
>   
> @@ -437,6 +440,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);
>   

Re: [PATCH v4 1/6] drm: Add common fdinfo helper

[Daniel Vetter]

On Thu, Apr 13, 2023 at 10:07:11AM +0200, Christian König wrote:
> Am 13.04.23 um 00:42 schrieb Rob Clark:
> > 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.
> 
> I'm really wondering if it wouldn't be less mid-layering if we let the
> drivers call the drm function to print the common values instead of the
> other way around?

The idea is that we plug this into DRM_GEM_FOPS and then everyone gets it
by default. So it's a bit a tradeoff between midlayering and having
inconsistent uapi between drivers. And there's generic tools that parse
this, so consistency across drivers is good.

My gut feeling was that after a bit of experimenting with lots of
different drivers for fdinfo stuff it's time to push for a bit more
standardization and less fragmentation.

We can of course later on course-correct and shuffle things around again,
e.g. by pushing more things into the gem_bo_fops->status hook (ttm and
other memory manager libs could implement a decent one by default), or
moving more into the drm_driver->show_fdinfo callback again.

If you look at kms we also shuffle things back&forth between core (for
more consistency) and drivers (for more flexibility where needed).

The important part here imo is that we start with some scaffolding to be
able to do this. Like another thing that I think we want is some
drm_fdinfo_print functions that make sure the formatting is guaranteed
consistents and we don't trip up parsers (like some drivers use " \t" as
separator instead of just "\t", I guess by accident).

> Apart from thatquestion the patch looks good to me.

Ack? Or want the above recorded in the commit message, I think it'd make
sense to put it there.
-Daniel

> 
> Christian.
> 
> > +
> >   Driver specific implementations
> > -===============================
> > +-------------------------------
> >   :ref:`i915-usage-stats`
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index a51ff8cee049..6d5bdd684ae2 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_pid(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 5b86bb7603e7..5edf2a13733b 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 0d1f853092ab..6de6d0e9c634 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;
> > @@ -437,6 +440,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);
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v4 1/6] drm: Add common fdinfo helper

[Christian König]

Am 13.04.23 um 10:46 schrieb Daniel Vetter:
> On Thu, Apr 13, 2023 at 10:07:11AM +0200, Christian König wrote:
>> Am 13.04.23 um 00:42 schrieb Rob Clark:
>>> 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.
>> I'm really wondering if it wouldn't be less mid-layering if we let the
>> drivers call the drm function to print the common values instead of the
>> other way around?
> The idea is that we plug this into DRM_GEM_FOPS and then everyone gets it
> by default. So it's a bit a tradeoff between midlayering and having
> inconsistent uapi between drivers. And there's generic tools that parse
> this, so consistency across drivers is good.
>
> My gut feeling was that after a bit of experimenting with lots of
> different drivers for fdinfo stuff it's time to push for a bit more
> standardization and less fragmentation.

Yeah, that's indeed a trade of.

>
> We can of course later on course-correct and shuffle things around again,
> e.g. by pushing more things into the gem_bo_fops->status hook (ttm and
> other memory manager libs could implement a decent one by default), or
> moving more into the drm_driver->show_fdinfo callback again.
>
> If you look at kms we also shuffle things back&forth between core (for
> more consistency) and drivers (for more flexibility where needed).
>
> The important part here imo is that we start with some scaffolding to be
> able to do this. Like another thing that I think we want is some
> drm_fdinfo_print functions that make sure the formatting is guaranteed
> consistents and we don't trip up parsers (like some drivers use " \t" as
> separator instead of just "\t", I guess by accident).

That's indeed a bit ugly and should probably be fixed on a higher level 
in the fs code.

Something like fdinfo_print(seq, name, format, value);

>
>> Apart from thatquestion the patch looks good to me.
> Ack? Or want the above recorded in the commit message, I think it'd make
> sense to put it there.

Well if Rob mentions this trade of in the commit message or even better 
code document feel free to add my rb to the patch.

Christian.

> -Daniel
>
>> Christian.
>>
>>> +
>>>    Driver specific implementations
>>> -===============================
>>> +-------------------------------
>>>    :ref:`i915-usage-stats`
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index a51ff8cee049..6d5bdd684ae2 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_pid(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 5b86bb7603e7..5edf2a13733b 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 0d1f853092ab..6de6d0e9c634 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;
>>> @@ -437,6 +440,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);

Re: [PATCH v4 1/6] drm: Add common fdinfo helper

[Tvrtko Ursulin]

On 13/04/2023 09:46, Daniel Vetter wrote:
> On Thu, Apr 13, 2023 at 10:07:11AM +0200, Christian König wrote:
>> Am 13.04.23 um 00:42 schrieb Rob Clark:
>>> 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.
>>
>> I'm really wondering if it wouldn't be less mid-layering if we let the
>> drivers call the drm function to print the common values instead of the
>> other way around?
> 
> The idea is that we plug this into DRM_GEM_FOPS and then everyone gets it
> by default. So it's a bit a tradeoff between midlayering and having
> inconsistent uapi between drivers. And there's generic tools that parse
> this, so consistency across drivers is good.
> 
> My gut feeling was that after a bit of experimenting with lots of
> different drivers for fdinfo stuff it's time to push for a bit more
> standardization and less fragmentation.
> 
> We can of course later on course-correct and shuffle things around again,
> e.g. by pushing more things into the gem_bo_fops->status hook (ttm and
> other memory manager libs could implement a decent one by default), or
> moving more into the drm_driver->show_fdinfo callback again.
> 
> If you look at kms we also shuffle things back&forth between core (for
> more consistency) and drivers (for more flexibility where needed).
> 
> The important part here imo is that we start with some scaffolding to be
> able to do this. Like another thing that I think we want is some
> drm_fdinfo_print functions that make sure the formatting is guaranteed
> consistents and we don't trip up parsers (like some drivers use " \t" as
> separator instead of just "\t", I guess by accident).

On this particular part I'd say it's even better to keep userspace on 
their toes and not let them hardcode assumption about how much and what 
whitespace characters to expect.

Otherwise I agree that mid-layer or not can be changed later, as long as 
my opens from elsewhere in the thread are closed before merging.

Regards,

Tvrtko

>> Apart from thatquestion the patch looks good to me.
> 
> Ack? Or want the above recorded in the commit message, I think it'd make
> sense to put it there.
> -Daniel
> 
>>
>> Christian.
>>
>>> +
>>>    Driver specific implementations
>>> -===============================
>>> +-------------------------------
>>>    :ref:`i915-usage-stats`
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index a51ff8cee049..6d5bdd684ae2 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_pid(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 5b86bb7603e7..5edf2a13733b 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 0d1f853092ab..6de6d0e9c634 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;
>>> @@ -437,6 +440,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);
>>
> 

Re: [PATCH v4 5/6] drm: Add fdinfo memory stats

[Emil Velikov]

On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> wrote:

> +/**
> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> + *
> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
> + * it still active or not resident, in which case drm_show_fdinfo() will not

nit: s/can/can be/;s/if it still/if it is still/

> + * account for it as purgeable.  So drivers do not need to check if the buffer
> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
> + * become puregeable until it becomes idle.  The status gem object func does

nit: s/puregeable/purgeable/


I think we want a similar note in the drm-usage-stats.rst file.

With the above the whole series is:
Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Fwiw: Keeping the i915 patch as part of this series would be great.
Sure i915_drm_client->id becomes dead code, but it's a piece one can
live with for a release or two. Then again it's not my call to make.

HTH
Emil

Re: [PATCH v4 5/6] drm: Add fdinfo memory stats

[Tvrtko Ursulin]

On 21/04/2023 11:26, Emil Velikov wrote:
> On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> wrote:
> 
>> +/**
>> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
>> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
>> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
>> + *
>> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
>> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
>> + * it still active or not resident, in which case drm_show_fdinfo() will not
> 
> nit: s/can/can be/;s/if it still/if it is still/
> 
>> + * account for it as purgeable.  So drivers do not need to check if the buffer
>> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
>> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
>> + * become puregeable until it becomes idle.  The status gem object func does
> 
> nit: s/puregeable/purgeable/
> 
> 
> I think we want a similar note in the drm-usage-stats.rst file.
> 
> With the above the whole series is:
> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>

Have you maybe noticed my slightly alternative proposal? (*) I am not a 
fan of putting this flavour of accounting into the core with no way to 
opt out. I think it leaves no option but to add more keys in the future 
for any driver which will not be happy with the core accounting.

*) https://patchwork.freedesktop.org/series/116581/

> Fwiw: Keeping the i915 patch as part of this series would be great.
> Sure i915_drm_client->id becomes dead code, but it's a piece one can
> live with for a release or two. Then again it's not my call to make.

Rob can take the i915 patch from my RFC too.

Regards,

Tvrtko

Re: [PATCH v4 5/6] drm: Add fdinfo memory stats

[Emil Velikov]

On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:

> On 21/04/2023 11:26, Emil Velikov wrote:
> > On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> wrote:
> >
> >> +/**
> >> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> >> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
> >> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> >> + *
> >> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
> >> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
> >> + * it still active or not resident, in which case drm_show_fdinfo() will not
> >
> > nit: s/can/can be/;s/if it still/if it is still/
> >
> >> + * account for it as purgeable.  So drivers do not need to check if the buffer
> >> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> >> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
> >> + * become puregeable until it becomes idle.  The status gem object func does
> >
> > nit: s/puregeable/purgeable/
> >
> >
> > I think we want a similar note in the drm-usage-stats.rst file.
> >
> > With the above the whole series is:
> > Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>
> Have you maybe noticed my slightly alternative proposal? (*) I am not a
> fan of putting this flavour of accounting into the core with no way to
> opt out. I think it leaves no option but to add more keys in the future
> for any driver which will not be happy with the core accounting.
>
> *) https://patchwork.freedesktop.org/series/116581/
>

Indeed I saw it. Not a fan of it, I'm afraid.

> > Fwiw: Keeping the i915 patch as part of this series would be great.
> > Sure i915_drm_client->id becomes dead code, but it's a piece one can
> > live with for a release or two. Then again it's not my call to make.
>
> Rob can take the i915 patch from my RFC too.
>

Indeed.

-Emil

Re: [PATCH v4 5/6] drm: Add fdinfo memory stats

[Tvrtko Ursulin]

On 21/04/2023 12:45, Emil Velikov wrote:
> On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin
> <tvrtko.ursulin@linux.intel.com> wrote:
> 
>> On 21/04/2023 11:26, Emil Velikov wrote:
>>> On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> wrote:
>>>
>>>> +/**
>>>> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
>>>> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
>>>> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
>>>> + *
>>>> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
>>>> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
>>>> + * it still active or not resident, in which case drm_show_fdinfo() will not
>>>
>>> nit: s/can/can be/;s/if it still/if it is still/
>>>
>>>> + * account for it as purgeable.  So drivers do not need to check if the buffer
>>>> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
>>>> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
>>>> + * become puregeable until it becomes idle.  The status gem object func does
>>>
>>> nit: s/puregeable/purgeable/
>>>
>>>
>>> I think we want a similar note in the drm-usage-stats.rst file.
>>>
>>> With the above the whole series is:
>>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
>>
>> Have you maybe noticed my slightly alternative proposal? (*) I am not a
>> fan of putting this flavour of accounting into the core with no way to
>> opt out. I think it leaves no option but to add more keys in the future
>> for any driver which will not be happy with the core accounting.
>>
>> *) https://patchwork.freedesktop.org/series/116581/
>>
> 
> Indeed I saw it. Not a fan of it, I'm afraid.

Hard to guess the reasons. :)

Anyway, at a minimum I suggest that if the no opt out version has to go 
in, it is clearly documented drm-*-memory-* is *not* about the full 
memory use of the client, but about memory belonging to user visible 
buffer objects *only*. Possibly going as far as naming the keys as 
drm-user-bo-memory-... That way there is a way to implement proper 
drm-*-memory- in the future.

Regards,

Tvrtko

>>> Fwiw: Keeping the i915 patch as part of this series would be great.
>>> Sure i915_drm_client->id becomes dead code, but it's a piece one can
>>> live with for a release or two. Then again it's not my call to make.
>>
>> Rob can take the i915 patch from my RFC too.
>>
> 
> Indeed.
> 
> -Emil

Re: [PATCH v4 5/6] drm: Add fdinfo memory stats

[Rob Clark]

On Fri, Apr 21, 2023 at 4:59 AM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 21/04/2023 12:45, Emil Velikov wrote:
> > On Fri, 21 Apr 2023 at 12:23, Tvrtko Ursulin
> > <tvrtko.ursulin@linux.intel.com> wrote:
> >
> >> On 21/04/2023 11:26, Emil Velikov wrote:
> >>> On Wed, 12 Apr 2023 at 23:43, Rob Clark <robdclark@gmail.com> wrote:
> >>>
> >>>> +/**
> >>>> + * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> >>>> + * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not unpinned)
> >>>> + * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by userspace
> >>>> + *
> >>>> + * Bitmask of status used for fdinfo memory stats, see &drm_gem_object_funcs.status
> >>>> + * and drm_show_fdinfo().  Note that an object can DRM_GEM_OBJECT_PURGEABLE if
> >>>> + * it still active or not resident, in which case drm_show_fdinfo() will not
> >>>
> >>> nit: s/can/can be/;s/if it still/if it is still/
> >>>
> >>>> + * account for it as purgeable.  So drivers do not need to check if the buffer
> >>>> + * is idle and resident to return this bit.  (Ie. userspace can mark a buffer
> >>>> + * as purgeable even while it is still busy on the GPU.. it does not _actually_
> >>>> + * become puregeable until it becomes idle.  The status gem object func does
> >>>
> >>> nit: s/puregeable/purgeable/
> >>>
> >>>
> >>> I think we want a similar note in the drm-usage-stats.rst file.
> >>>
> >>> With the above the whole series is:
> >>> Reviewed-by: Emil Velikov <emil.l.velikov@gmail.com>
> >>
> >> Have you maybe noticed my slightly alternative proposal? (*) I am not a
> >> fan of putting this flavour of accounting into the core with no way to
> >> opt out. I think it leaves no option but to add more keys in the future
> >> for any driver which will not be happy with the core accounting.
> >>
> >> *) https://patchwork.freedesktop.org/series/116581/
> >>
> >
> > Indeed I saw it. Not a fan of it, I'm afraid.
>
> Hard to guess the reasons. :)
>
> Anyway, at a minimum I suggest that if the no opt out version has to go
> in, it is clearly documented drm-*-memory-* is *not* about the full
> memory use of the client, but about memory belonging to user visible
> buffer objects *only*. Possibly going as far as naming the keys as
> drm-user-bo-memory-... That way there is a way to implement proper
> drm-*-memory- in the future.

I'll go back to the helper approach, just been distracted by a few
other balls in the air.. should hopefully get to it in the next couple
days

BR,
-R

> Regards,
>
> Tvrtko
>
> >>> Fwiw: Keeping the i915 patch as part of this series would be great.
> >>> Sure i915_drm_client->id becomes dead code, but it's a piece one can
> >>> live with for a release or two. Then again it's not my call to make.
> >>
> >> Rob can take the i915 patch from my RFC too.
> >>
> >
> > Indeed.
> >
> > -Emil