[PATCH v7 0/4] drm/panthor: Display size of internal kernel BOs through fdinfo

[PATCH v7 0/4] drm/panthor: Display size of internal kernel BOs through fdinfo

[Adrián Larumbe]

This patch series enables display of the size of driver-owned shmem BO's that aren't
exposed to userspace through a DRM handle.

Discussion of previous revision can be found here [1].

Changelog:
v7:
 - Added new commit: mentions the formation rules for driver-specific fdinfo keys
 - Added new commit: adds a helper that lets driver print memory size key:value
   pairs with their driver name as a prefix.
 - Modified later commits to make use of the previous ones.
 - Deleted mentions of now unnecessary memory keys in the old revision.
v6:
 - Replace up_write with up_read, which was left out in the previous version
 - Fixed some minor comment and documentation issues reported by the kernel test robot
v5:
 - Replaced down_write semaphore with the read flavour
 - Fixed typo and added explicit description for drm-shared-internal in
 the fdinfo documentation file for Panthor.
v4:
 - Remove unrelated formating fix
 - Moved calculating overall size of a group's kernel BO's into
 its own static helper.
 - Renamed group kernel BO's size aggregation function to better
 reflect its actual responsibility.

[1] https://lore.kernel.org/dri-devel/20250102203817.956790-1-adrian.larumbe@collabora.com/

Adrián Larumbe (4):
  Documentation/gpu: Clarify format of driver-specific fidnfo keys
  drm/file: Add driver-specific memory region key printing helper
  drm/panthor: Expose size of driver internal BO's over fdinfo
  Documentation/gpu: Add fdinfo meanings of drm-*-internal memory tags

 Documentation/gpu/drm-usage-stats.rst   |  5 ++-
 Documentation/gpu/panthor.rst           | 10 +++++
 drivers/gpu/drm/drm_file.c              | 60 +++++++++++++++++++++----
 drivers/gpu/drm/panthor/panthor_drv.c   | 13 ++++++
 drivers/gpu/drm/panthor/panthor_heap.c  | 26 +++++++++++
 drivers/gpu/drm/panthor/panthor_heap.h  |  2 +
 drivers/gpu/drm/panthor/panthor_mmu.c   | 34 ++++++++++++++
 drivers/gpu/drm/panthor/panthor_mmu.h   |  3 ++
 drivers/gpu/drm/panthor/panthor_sched.c | 51 ++++++++++++++++++++-
 drivers/gpu/drm/panthor/panthor_sched.h |  2 +
 include/drm/drm_file.h                  |  2 +
 11 files changed, 198 insertions(+), 10 deletions(-)


base-commit: c6eabbab359c156669e10d5dec3e71e80ff09bd2
-- 
2.47.1

[PATCH v7 1/4] Documentation/gpu: Clarify format of driver-specific fidnfo keys

[Adrián Larumbe]

This change reflects de facto usage by amdgpu, as exemplified by commit
d6530c33a978 ("drm/amdgpu: expose more memory stats in fdinfo").

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>
---
 Documentation/gpu/drm-usage-stats.rst | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
index 2717cb2a597e..2b5393ed7692 100644
--- a/Documentation/gpu/drm-usage-stats.rst
+++ b/Documentation/gpu/drm-usage-stats.rst
@@ -21,7 +21,10 @@ 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.
-- All keys shall be prefixed with `drm-`.
+- All standardised keys shall be prefixed with `drm-`.
+- Driver-specific keys shall be prefixed with `driver_name-`, where
+  driver_name should ideally be the same as the `name` field in
+  `struct drm_driver`, although this is not mandatory.
 - Whitespace between the delimiter and first non-whitespace character shall be
   ignored when parsing.
 - Keys are not allowed to contain whitespace characters.
-- 
2.47.1

[PATCH v7 2/4] drm/file: Add driver-specific memory region key printing helper

[Adrián Larumbe]

This is motivated by the desire of some dirvers (eg. Panthor) to print the
size of internal memory regions with a prefix that reflects the driver
name, as suggested in the previous documentation commit.

That means a minor refactoring of print_size() was needed so as to make it
more generic in the format of key strings it takes as input.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>
---
 drivers/gpu/drm/drm_file.c | 60 +++++++++++++++++++++++++++++++++-----
 include/drm/drm_file.h     |  2 ++
 2 files changed, 54 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index cb5f22f5bbb6..5deae4cffa79 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -830,8 +830,7 @@ 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,
-		       const char *region, u64 sz)
+static void print_size(struct drm_printer *p, const char *key, u64 sz)
 {
 	const char *units[] = {"", " KiB", " MiB"};
 	unsigned u;
@@ -842,9 +841,54 @@ static void print_size(struct drm_printer *p, const char *stat,
 		sz = div_u64(sz, SZ_1K);
 	}
 
-	drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
+	drm_printf(p, "%s:\t%llu%s\n", key, sz, units[u]);
 }
 
+#define KEY_LEN 1024
+#define DRM_PREFIX "drm"
+
+static void
+print_size_region(struct drm_printer *p, const char *stat,
+		  const char *region, u64 sz)
+{
+	char key[KEY_LEN+1] = {0};
+
+	if (strnlen(stat, KEY_LEN) + strnlen(region, KEY_LEN) +
+	    strlen(DRM_PREFIX) + 2 > KEY_LEN)
+		return;
+
+	snprintf(key, sizeof(key), "%s-%s-%s", DRM_PREFIX, stat, region);
+	print_size(p, key, sz);
+}
+
+/**
+ * drm_driver_print_size - A helper to print driver-specific k:v pairs
+ * @p: The printer to print output to
+ * @file: DRM file private data
+ * @subkey: Name of the key minus the driver name
+ * @sz: Size value expressed in bytes
+ *
+ * Helper to display key:value pairs where the value is a numerical
+ * size expressed in bytes. It's useful for driver-internal memory
+ * whose objects aren't exposed to UM through a DRM handle.
+ */
+void drm_driver_print_size(struct drm_printer *p, struct drm_file *file,
+			   const char *subkey, u64 sz)
+{
+	char key[KEY_LEN+1] = {0};
+	char *drv_name = file->minor->dev->driver->name;
+
+	if (strnlen(subkey, KEY_LEN) + strlen(drv_name) + 1 > KEY_LEN)
+		return;
+
+	snprintf(key, sizeof(key), "%s-%s", drv_name, subkey);
+	print_size(p, key, sz);
+}
+EXPORT_SYMBOL(drm_driver_print_size);
+
+#undef DRM_PREFIX
+#undef KEY_LEN
+
 /**
  * drm_print_memory_stats - A helper to print memory stats
  * @p: The printer to print output to
@@ -858,15 +902,15 @@ void drm_print_memory_stats(struct drm_printer *p,
 			    enum drm_gem_object_status supported_status,
 			    const char *region)
 {
-	print_size(p, "total", region, stats->private + stats->shared);
-	print_size(p, "shared", region, stats->shared);
-	print_size(p, "active", region, stats->active);
+	print_size_region(p, "total", region, stats->private + stats->shared);
+	print_size_region(p, "shared", region, stats->shared);
+	print_size_region(p, "active", region, stats->active);
 
 	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
-		print_size(p, "resident", region, stats->resident);
+		print_size_region(p, "resident", region, stats->resident);
 
 	if (supported_status & DRM_GEM_OBJECT_PURGEABLE)
-		print_size(p, "purgeable", region, stats->purgeable);
+		print_size_region(p, "purgeable", region, stats->purgeable);
 }
 EXPORT_SYMBOL(drm_print_memory_stats);
 
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index f0ef32e9fa5e..07da14859289 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -494,6 +494,8 @@ struct drm_memory_stats {
 
 enum drm_gem_object_status;
 
+void drm_driver_print_size(struct drm_printer *p, struct drm_file *file,
+			   const char *subkey, u64 sz);
 void drm_print_memory_stats(struct drm_printer *p,
 			    const struct drm_memory_stats *stats,
 			    enum drm_gem_object_status supported_status,
-- 
2.47.1

[PATCH v7 3/4] drm/panthor: Expose size of driver internal BO's over fdinfo

[Adrián Larumbe]

This will display the sizes of kenrel BO's bound to an open file, which are
otherwise not exposed to UM through a handle.

The sizes recorded are as follows:
 - Per group: suspend buffer, protm-suspend buffer, syncobjcs
 - Per queue: ringbuffer, profiling slots, firmware interface
 - For all heaps in all heap pools across all VM's bound to an open file,
 record size of all heap chuks, and for each pool the gpu_context BO too.

This does not record the size of FW regions, as these aren't bound to a
specific open file and remain active through the whole life of the driver.

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_drv.c   | 13 +++++++
 drivers/gpu/drm/panthor/panthor_heap.c  | 26 +++++++++++++
 drivers/gpu/drm/panthor/panthor_heap.h  |  2 +
 drivers/gpu/drm/panthor/panthor_mmu.c   | 34 +++++++++++++++++
 drivers/gpu/drm/panthor/panthor_mmu.h   |  3 ++
 drivers/gpu/drm/panthor/panthor_sched.c | 51 ++++++++++++++++++++++++-
 drivers/gpu/drm/panthor/panthor_sched.h |  2 +
 7 files changed, 130 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index ac7e53f6e3f0..c41136af27d2 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -1457,12 +1457,25 @@ static void panthor_gpu_show_fdinfo(struct panthor_device *ptdev,
 	drm_printf(p, "drm-curfreq-panthor:\t%lu Hz\n", ptdev->current_frequency);
 }
 
+static void panthor_show_internal_memory_stats(struct drm_printer *p, struct drm_file *file)
+{
+	struct panthor_file *pfile = file->driver_priv;
+	struct drm_memory_stats status = {0};
+
+	panthor_group_kbo_sizes(pfile, &status);
+	panthor_vm_heaps_sizes(pfile, &status);
+
+	drm_driver_print_size(p, file, "resident-memory", status.resident);
+	drm_driver_print_size(p, file, "active-memory", status.active);
+}
+
 static void panthor_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 {
 	struct drm_device *dev = file->minor->dev;
 	struct panthor_device *ptdev = container_of(dev, struct panthor_device, base);
 
 	panthor_gpu_show_fdinfo(ptdev, file->driver_priv, p);
+	panthor_show_internal_memory_stats(p, file);
 
 	drm_show_memory_stats(p, file);
 }
diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 3796a9eb22af..db0285ce5812 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -603,3 +603,29 @@ void panthor_heap_pool_destroy(struct panthor_heap_pool *pool)
 
 	panthor_heap_pool_put(pool);
 }
+
+/**
+ * panthor_heap_pool_size() - Calculate size of all chunks across all heaps in a pool
+ * @pool: Pool whose total chunk size to calculate.
+ *
+ * This function adds the size of all heap chunks across all heaps in the
+ * argument pool. It also adds the size of the gpu contexts kernel bo.
+ * It is meant to be used by fdinfo for displaying the size of internal
+ * driver BO's that aren't exposed to userspace through a GEM handle.
+ *
+ */
+size_t panthor_heap_pool_size(struct panthor_heap_pool *pool)
+{
+	struct panthor_heap *heap;
+	unsigned long i;
+	size_t size = 0;
+
+	down_read(&pool->lock);
+	xa_for_each(&pool->xa, i, heap)
+		size += heap->chunk_size * heap->chunk_count;
+	up_read(&pool->lock);
+
+	size += pool->gpu_contexts->obj->size;
+
+	return size;
+}
diff --git a/drivers/gpu/drm/panthor/panthor_heap.h b/drivers/gpu/drm/panthor/panthor_heap.h
index 25a5f2bba445..e3358d4e8edb 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.h
+++ b/drivers/gpu/drm/panthor/panthor_heap.h
@@ -27,6 +27,8 @@ struct panthor_heap_pool *
 panthor_heap_pool_get(struct panthor_heap_pool *pool);
 void panthor_heap_pool_put(struct panthor_heap_pool *pool);
 
+size_t panthor_heap_pool_size(struct panthor_heap_pool *pool);
+
 int panthor_heap_grow(struct panthor_heap_pool *pool,
 		      u64 heap_gpu_va,
 		      u32 renderpasses_in_flight,
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index c3f0b0225cf9..32cc0f8b8e23 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -1941,6 +1941,40 @@ struct panthor_heap_pool *panthor_vm_get_heap_pool(struct panthor_vm *vm, bool c
 	return pool;
 }
 
+/**
+ * panthor_vm_heaps_sizes() - Calculate size of all heap chunks across all
+ * heaps over all the heap pools in a VM
+ * @pfile: File.
+ * @status: Memory status to be updated.
+ *
+ * Calculate all heap chunk sizes in all heap pools bound to a VM. If the VM
+ * is active, record the size as active as well.
+ */
+void panthor_vm_heaps_sizes(struct panthor_file *pfile, struct drm_memory_stats *status)
+{
+	struct panthor_vm *vm;
+	unsigned long i;
+
+	if (!pfile->vms)
+		return;
+
+	xa_for_each(&pfile->vms->xa, i, vm) {
+		size_t size;
+
+		mutex_lock(&vm->heaps.lock);
+		if (!vm->heaps.pool) {
+			mutex_unlock(&vm->heaps.lock);
+			continue;
+		}
+		size = panthor_heap_pool_size(vm->heaps.pool);
+		mutex_unlock(&vm->heaps.lock);
+
+		status->resident += size;
+		if (vm->as.id >= 0)
+			status->active += size;
+	}
+}
+
 static u64 mair_to_memattr(u64 mair, bool coherent)
 {
 	u64 memattr = 0;
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.h b/drivers/gpu/drm/panthor/panthor_mmu.h
index 8d21e83d8aba..494c36d732b4 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.h
+++ b/drivers/gpu/drm/panthor/panthor_mmu.h
@@ -9,6 +9,7 @@
 
 struct drm_exec;
 struct drm_sched_job;
+struct drm_memory_stats;
 struct panthor_gem_object;
 struct panthor_heap_pool;
 struct panthor_vm;
@@ -37,6 +38,8 @@ int panthor_vm_flush_all(struct panthor_vm *vm);
 struct panthor_heap_pool *
 panthor_vm_get_heap_pool(struct panthor_vm *vm, bool create);
 
+void panthor_vm_heaps_sizes(struct panthor_file *pfile, struct drm_memory_stats *status);
+
 struct panthor_vm *panthor_vm_get(struct panthor_vm *vm);
 void panthor_vm_put(struct panthor_vm *vm);
 struct panthor_vm *panthor_vm_create(struct panthor_device *ptdev, bool for_mcu,
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index ef4bec7ff9c7..e4acf6a0a9b1 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -618,7 +618,7 @@ struct panthor_group {
 	 */
 	struct panthor_kernel_bo *syncobjs;
 
-	/** @fdinfo: Per-file total cycle and timestamp values reference. */
+	/** @fdinfo: Per-group total cycle and timestamp values and kernel BO sizes. */
 	struct {
 		/** @data: Total sampled values for jobs in queues from this group. */
 		struct panthor_gpu_usage data;
@@ -628,6 +628,9 @@ struct panthor_group {
 		 * and job post-completion processing function
 		 */
 		struct mutex lock;
+
+		/** @kbo_sizes: Aggregate size of private kernel BO's held by the group. */
+		size_t kbo_sizes;
 	} fdinfo;
 
 	/** @state: Group state. */
@@ -3365,6 +3368,29 @@ group_create_queue(struct panthor_group *group,
 	return ERR_PTR(ret);
 }
 
+static void add_group_kbo_sizes(struct panthor_device *ptdev,
+				struct panthor_group *group)
+{
+	struct panthor_queue *queue;
+	int i;
+
+	if (drm_WARN_ON(&ptdev->base, IS_ERR_OR_NULL(group)))
+		return;
+	if (drm_WARN_ON(&ptdev->base, ptdev != group->ptdev))
+		return;
+
+	group->fdinfo.kbo_sizes += group->suspend_buf->obj->size;
+	group->fdinfo.kbo_sizes += group->protm_suspend_buf->obj->size;
+	group->fdinfo.kbo_sizes += group->syncobjs->obj->size;
+
+	for (i = 0; i < group->queue_count; i++) {
+		queue =	group->queues[i];
+		group->fdinfo.kbo_sizes += queue->ringbuf->obj->size;
+		group->fdinfo.kbo_sizes += queue->iface.mem->obj->size;
+		group->fdinfo.kbo_sizes += queue->profiling.slots->obj->size;
+	}
+}
+
 #define MAX_GROUPS_PER_POOL		128
 
 int panthor_group_create(struct panthor_file *pfile,
@@ -3489,6 +3515,7 @@ int panthor_group_create(struct panthor_file *pfile,
 	}
 	mutex_unlock(&sched->reset.lock);
 
+	add_group_kbo_sizes(group->ptdev, group);
 	mutex_init(&group->fdinfo.lock);
 
 	return gid;
@@ -3606,6 +3633,28 @@ void panthor_group_pool_destroy(struct panthor_file *pfile)
 	pfile->groups = NULL;
 }
 
+/**
+ * panthor_group_kbo_sizes() - Retrieve aggregate size of all private kernel BO's
+ * belonging to all the groups owned by an open Panthor file
+ * @pfile: File.
+ * @status: Memory status to be updated.
+ *
+ */
+void panthor_group_kbo_sizes(struct panthor_file *pfile, struct drm_memory_stats *status)
+{
+	struct panthor_group_pool *gpool = pfile->groups;
+	struct panthor_group *group;
+	unsigned long i;
+
+	if (IS_ERR_OR_NULL(gpool))
+		return;
+	xa_for_each(&gpool->xa, i, group) {
+		status->resident += group->fdinfo.kbo_sizes;
+		if (group->csg_id >= 0)
+			status->active += group->fdinfo.kbo_sizes;
+	}
+}
+
 static void job_release(struct kref *ref)
 {
 	struct panthor_job *job = container_of(ref, struct panthor_job, refcount);
diff --git a/drivers/gpu/drm/panthor/panthor_sched.h b/drivers/gpu/drm/panthor/panthor_sched.h
index 5ae6b4bde7c5..2327d441ceb1 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.h
+++ b/drivers/gpu/drm/panthor/panthor_sched.h
@@ -9,6 +9,7 @@ struct dma_fence;
 struct drm_file;
 struct drm_gem_object;
 struct drm_sched_job;
+struct drm_memory_stats;
 struct drm_panthor_group_create;
 struct drm_panthor_queue_create;
 struct drm_panthor_group_get_state;
@@ -36,6 +37,7 @@ void panthor_job_update_resvs(struct drm_exec *exec, struct drm_sched_job *job);
 
 int panthor_group_pool_create(struct panthor_file *pfile);
 void panthor_group_pool_destroy(struct panthor_file *pfile);
+void panthor_group_kbo_sizes(struct panthor_file *pfile, struct drm_memory_stats *status);
 
 int panthor_sched_init(struct panthor_device *ptdev);
 void panthor_sched_unplug(struct panthor_device *ptdev);
-- 
2.47.1

[PATCH v7 4/4] Documentation/gpu: Add fdinfo meanings of drm-*-internal memory tags

[Adrián Larumbe]

A previous commit enabled display of driver-internal kernel BO sizes
through the device file's fdinfo interface.

Expand the description of the relevant driver-specific key:value pairs
with the definitions of the new panthor-*-memory ones.

Reviewed-by: Mihail Atanassov <mihail.atanassov@arm.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: Steven Price <steven.price@arm.com>
Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 Documentation/gpu/panthor.rst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/gpu/panthor.rst b/Documentation/gpu/panthor.rst
index 3f8979fa2b86..76e03304fe7a 100644
--- a/Documentation/gpu/panthor.rst
+++ b/Documentation/gpu/panthor.rst
@@ -26,6 +26,8 @@ the currently possible format options:
      drm-cycles-panthor:     94439687187
      drm-maxfreq-panthor:    1000000000 Hz
      drm-curfreq-panthor:    1000000000 Hz
+     panthor-resident-memory:        10396 KiB
+     panthor-active-memory:  10396 KiB
      drm-total-memory:       16480 KiB
      drm-shared-memory:      0
      drm-active-memory:      16200 KiB
@@ -44,3 +46,11 @@ driver by writing into the appropriate sysfs node::
 
 Where `N` is a bit mask where cycle and timestamp sampling are respectively
 enabled by the first and second bits.
+
+Possible `panthor-*-memory` keys are: `active` and `resident`.
+These values convey the sizes of the internal driver-owned shmem BO's that
+aren't exposed to user-space through a DRM handle, like queue ring buffers,
+sync object arrays and heap chunks. Because they are all allocated and pinned
+at creation time, only `panthor-resident-memory` is necessary to tell us their
+size. `panthor-resident-active` shows the size of kernel BO's associated with
+VM's and groups currently being scheduled for execution by the GPU.
-- 
2.47.1

Re: [PATCH v7 1/4] Documentation/gpu: Clarify format of driver-specific fidnfo keys

[Tvrtko Ursulin]

On 08/01/2025 21:02, Adrián Larumbe wrote:
> This change reflects de facto usage by amdgpu, as exemplified by commit
> d6530c33a978 ("drm/amdgpu: expose more memory stats in fdinfo").
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> ---
>   Documentation/gpu/drm-usage-stats.rst | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/drm-usage-stats.rst b/Documentation/gpu/drm-usage-stats.rst
> index 2717cb2a597e..2b5393ed7692 100644
> --- a/Documentation/gpu/drm-usage-stats.rst
> +++ b/Documentation/gpu/drm-usage-stats.rst
> @@ -21,7 +21,10 @@ 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.
> -- All keys shall be prefixed with `drm-`.
> +- All standardised keys shall be prefixed with `drm-`.
> +- Driver-specific keys shall be prefixed with `driver_name-`, where
> +  driver_name should ideally be the same as the `name` field in
> +  `struct drm_driver`, although this is not mandatory.
>   - Whitespace between the delimiter and first non-whitespace character shall be
>     ignored when parsing.
>   - Keys are not allowed to contain whitespace characters.

LGTM, thanks for documenting it!

Acked-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Native english speaker maybe could clarify if s/Driver-specific/Driver 
specific/ would be more correct.

Regards,

Tvrtko

Re: [PATCH v7 2/4] drm/file: Add driver-specific memory region key printing helper

[Tvrtko Ursulin]

On 08/01/2025 21:02, Adrián Larumbe wrote:
> This is motivated by the desire of some dirvers (eg. Panthor) to print the
> size of internal memory regions with a prefix that reflects the driver
> name, as suggested in the previous documentation commit.
> 
> That means a minor refactoring of print_size() was needed so as to make it
> more generic in the format of key strings it takes as input.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> ---
>   drivers/gpu/drm/drm_file.c | 60 +++++++++++++++++++++++++++++++++-----
>   include/drm/drm_file.h     |  2 ++
>   2 files changed, 54 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index cb5f22f5bbb6..5deae4cffa79 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -830,8 +830,7 @@ 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,
> -		       const char *region, u64 sz)
> +static void print_size(struct drm_printer *p, const char *key, u64 sz)
>   {
>   	const char *units[] = {"", " KiB", " MiB"};
>   	unsigned u;
> @@ -842,9 +841,54 @@ static void print_size(struct drm_printer *p, const char *stat,
>   		sz = div_u64(sz, SZ_1K);
>   	}
>   
> -	drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
> +	drm_printf(p, "%s:\t%llu%s\n", key, sz, units[u]);
>   }
>   
> +#define KEY_LEN 1024
> +#define DRM_PREFIX "drm"
> +
> +static void
> +print_size_region(struct drm_printer *p, const char *stat,
> +		  const char *region, u64 sz)
> +{
> +	char key[KEY_LEN+1] = {0};

A kilobyte of stack makes me a bit uneasy.

Would it not work to make print_size take a prefix and also avoid string 
operations? Something like, not compile tested:

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 2289e71e2fa2..efa4593babbc 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -830,8 +830,12 @@ 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,
-		       const char *region, u64 sz)
+static void
+drm_fdinfo_print_size(struct drm_printer *p,
+		      const char *prefix,
+		      const char *stat,
+		      const char *region,
+		      u64 sz)
  {
  	const char *units[] = {"", " KiB", " MiB"};
  	unsigned u;
@@ -842,8 +846,10 @@ static void print_size(struct drm_printer *p, const 
char *stat,
  		sz = div_u64(sz, SZ_1K);
  	}

-	drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
+	drm_printf(p, "%s-%s-%s:\t%llu%s\n",
+		   prefix, stat, region, sz, units[u]);
  }
+EXPORT_SYMBOL(drm_fdinfo_print_size);

  int drm_memory_stats_is_zero(const struct drm_memory_stats *stats)
  {
@@ -868,17 +874,23 @@ void drm_print_memory_stats(struct drm_printer *p,
  			    enum drm_gem_object_status supported_status,
  			    const char *region)
  {
-	print_size(p, "total", region, stats->private + stats->shared);
-	print_size(p, "shared", region, stats->shared);
+	const char *prefix = "drm";
+
+	drm_fdinfo_print_size(p, prefix, "total", region,
+			      stats->private + stats->shared);
+	drm_fdinfo_print_size(p, prefix, "shared", region, stats->shared);

  	if (supported_status & DRM_GEM_OBJECT_ACTIVE)
-		print_size(p, "active", region, stats->active);
+		drm_fdinfo_print_size(p, prefix, "active", region,
+				      stats->active);

  	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
-		print_size(p, "resident", region, stats->resident);
+		drm_fdinfo_print_size(p, prefix, "resident", region,
+				      stats->resident);

  	if (supported_status & DRM_GEM_OBJECT_PURGEABLE)
-		print_size(p, "purgeable", region, stats->purgeable);
+		drm_fdinfo_print_size(p, prefix, "purgeable", region,
+				      stats->purgeable);
  }
  EXPORT_SYMBOL(drm_print_memory_stats);

?

Regards,

Tvrtko

> +
> +	if (strnlen(stat, KEY_LEN) + strnlen(region, KEY_LEN) +
> +	    strlen(DRM_PREFIX) + 2 > KEY_LEN)
> +		return;
> +
> +	snprintf(key, sizeof(key), "%s-%s-%s", DRM_PREFIX, stat, region);
> +	print_size(p, key, sz);
> +}
> +
> +/**
> + * drm_driver_print_size - A helper to print driver-specific k:v pairs
> + * @p: The printer to print output to
> + * @file: DRM file private data
> + * @subkey: Name of the key minus the driver name
> + * @sz: Size value expressed in bytes
> + *
> + * Helper to display key:value pairs where the value is a numerical
> + * size expressed in bytes. It's useful for driver-internal memory
> + * whose objects aren't exposed to UM through a DRM handle.
> + */
> +void drm_driver_print_size(struct drm_printer *p, struct drm_file *file,
> +			   const char *subkey, u64 sz)
> +{
> +	char key[KEY_LEN+1] = {0};
> +	char *drv_name = file->minor->dev->driver->name;
> +
> +	if (strnlen(subkey, KEY_LEN) + strlen(drv_name) + 1 > KEY_LEN)
> +		return;
> +
> +	snprintf(key, sizeof(key), "%s-%s", drv_name, subkey);
> +	print_size(p, key, sz);
> +}
> +EXPORT_SYMBOL(drm_driver_print_size);
> +
> +#undef DRM_PREFIX
> +#undef KEY_LEN
> +
>   /**
>    * drm_print_memory_stats - A helper to print memory stats
>    * @p: The printer to print output to
> @@ -858,15 +902,15 @@ void drm_print_memory_stats(struct drm_printer *p,
>   			    enum drm_gem_object_status supported_status,
>   			    const char *region)
>   {
> -	print_size(p, "total", region, stats->private + stats->shared);
> -	print_size(p, "shared", region, stats->shared);
> -	print_size(p, "active", region, stats->active);
> +	print_size_region(p, "total", region, stats->private + stats->shared);
> +	print_size_region(p, "shared", region, stats->shared);
> +	print_size_region(p, "active", region, stats->active);
>   
>   	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> -		print_size(p, "resident", region, stats->resident);
> +		print_size_region(p, "resident", region, stats->resident);
>   
>   	if (supported_status & DRM_GEM_OBJECT_PURGEABLE)
> -		print_size(p, "purgeable", region, stats->purgeable);
> +		print_size_region(p, "purgeable", region, stats->purgeable);
>   }
>   EXPORT_SYMBOL(drm_print_memory_stats);
>   
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index f0ef32e9fa5e..07da14859289 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -494,6 +494,8 @@ struct drm_memory_stats {
>   
>   enum drm_gem_object_status;
>   
> +void drm_driver_print_size(struct drm_printer *p, struct drm_file *file,
> +			   const char *subkey, u64 sz);
>   void drm_print_memory_stats(struct drm_printer *p,
>   			    const struct drm_memory_stats *stats,
>   			    enum drm_gem_object_status supported_status,

Re: [PATCH v7 2/4] drm/file: Add driver-specific memory region key printing helper

[Adrián Larumbe]

On 09.01.2025 13:05, Tvrtko Ursulin wrote:
> On 08/01/2025 21:02, Adrián Larumbe wrote:
> > This is motivated by the desire of some dirvers (eg. Panthor) to print the
> > size of internal memory regions with a prefix that reflects the driver
> > name, as suggested in the previous documentation commit.
> > 
> > That means a minor refactoring of print_size() was needed so as to make it
> > more generic in the format of key strings it takes as input.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> > ---
> >   drivers/gpu/drm/drm_file.c | 60 +++++++++++++++++++++++++++++++++-----
> >   include/drm/drm_file.h     |  2 ++
> >   2 files changed, 54 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index cb5f22f5bbb6..5deae4cffa79 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -830,8 +830,7 @@ 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,
> > -		       const char *region, u64 sz)
> > +static void print_size(struct drm_printer *p, const char *key, u64 sz)
> >   {
> >   	const char *units[] = {"", " KiB", " MiB"};
> >   	unsigned u;
> > @@ -842,9 +841,54 @@ static void print_size(struct drm_printer *p, const char *stat,
> >   		sz = div_u64(sz, SZ_1K);
> >   	}
> > -	drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
> > +	drm_printf(p, "%s:\t%llu%s\n", key, sz, units[u]);
> >   }
> > +#define KEY_LEN 1024
> > +#define DRM_PREFIX "drm"
> > +
> > +static void
> > +print_size_region(struct drm_printer *p, const char *stat,
> > +		  const char *region, u64 sz)
> > +{
> > +	char key[KEY_LEN+1] = {0};
> 
> A kilobyte of stack makes me a bit uneasy.
> 
> Would it not work to make print_size take a prefix and also avoid string
> operations? Something like, not compile tested:

This looks good to me, I had insisted we mandate a maximum key length, but
like you said on IRC there's truly no need for this.

> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 2289e71e2fa2..efa4593babbc 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -830,8 +830,12 @@ 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,
> -		       const char *region, u64 sz)
> +static void
> +drm_fdinfo_print_size(struct drm_printer *p,
> +		      const char *prefix,
> +		      const char *stat,
> +		      const char *region,
> +		      u64 sz)
>  {
>  	const char *units[] = {"", " KiB", " MiB"};
>  	unsigned u;
> @@ -842,8 +846,10 @@ static void print_size(struct drm_printer *p, const char
> *stat,
>  		sz = div_u64(sz, SZ_1K);
>  	}
> 
> -	drm_printf(p, "drm-%s-%s:\t%llu%s\n", stat, region, sz, units[u]);
> +	drm_printf(p, "%s-%s-%s:\t%llu%s\n",
> +		   prefix, stat, region, sz, units[u]);
>  }
> +EXPORT_SYMBOL(drm_fdinfo_print_size);
> 
>  int drm_memory_stats_is_zero(const struct drm_memory_stats *stats)
>  {
> @@ -868,17 +874,23 @@ void drm_print_memory_stats(struct drm_printer *p,
>  			    enum drm_gem_object_status supported_status,
>  			    const char *region)
>  {
> -	print_size(p, "total", region, stats->private + stats->shared);
> -	print_size(p, "shared", region, stats->shared);
> +	const char *prefix = "drm";
> +
> +	drm_fdinfo_print_size(p, prefix, "total", region,
> +			      stats->private + stats->shared);
> +	drm_fdinfo_print_size(p, prefix, "shared", region, stats->shared);
> 
>  	if (supported_status & DRM_GEM_OBJECT_ACTIVE)
> -		print_size(p, "active", region, stats->active);
> +		drm_fdinfo_print_size(p, prefix, "active", region,
> +				      stats->active);
> 
>  	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> -		print_size(p, "resident", region, stats->resident);
> +		drm_fdinfo_print_size(p, prefix, "resident", region,
> +				      stats->resident);
> 
>  	if (supported_status & DRM_GEM_OBJECT_PURGEABLE)
> -		print_size(p, "purgeable", region, stats->purgeable);
> +		drm_fdinfo_print_size(p, prefix, "purgeable", region,
> +				      stats->purgeable);
>  }
>  EXPORT_SYMBOL(drm_print_memory_stats);
> 
> ?
> 
> Regards,
> 
> Tvrtko
> 
> > +
> > +	if (strnlen(stat, KEY_LEN) + strnlen(region, KEY_LEN) +
> > +	    strlen(DRM_PREFIX) + 2 > KEY_LEN)
> > +		return;
> > +
> > +	snprintf(key, sizeof(key), "%s-%s-%s", DRM_PREFIX, stat, region);
> > +	print_size(p, key, sz);
> > +}
> > +
> > +/**
> > + * drm_driver_print_size - A helper to print driver-specific k:v pairs
> > + * @p: The printer to print output to
> > + * @file: DRM file private data
> > + * @subkey: Name of the key minus the driver name
> > + * @sz: Size value expressed in bytes
> > + *
> > + * Helper to display key:value pairs where the value is a numerical
> > + * size expressed in bytes. It's useful for driver-internal memory
> > + * whose objects aren't exposed to UM through a DRM handle.
> > + */
> > +void drm_driver_print_size(struct drm_printer *p, struct drm_file *file,
> > +			   const char *subkey, u64 sz)
> > +{
> > +	char key[KEY_LEN+1] = {0};
> > +	char *drv_name = file->minor->dev->driver->name;
> > +
> > +	if (strnlen(subkey, KEY_LEN) + strlen(drv_name) + 1 > KEY_LEN)
> > +		return;
> > +
> > +	snprintf(key, sizeof(key), "%s-%s", drv_name, subkey);
> > +	print_size(p, key, sz);
> > +}
> > +EXPORT_SYMBOL(drm_driver_print_size);
> > +
> > +#undef DRM_PREFIX
> > +#undef KEY_LEN
> > +
> >   /**
> >    * drm_print_memory_stats - A helper to print memory stats
> >    * @p: The printer to print output to
> > @@ -858,15 +902,15 @@ void drm_print_memory_stats(struct drm_printer *p,
> >   			    enum drm_gem_object_status supported_status,
> >   			    const char *region)
> >   {
> > -	print_size(p, "total", region, stats->private + stats->shared);
> > -	print_size(p, "shared", region, stats->shared);
> > -	print_size(p, "active", region, stats->active);
> > +	print_size_region(p, "total", region, stats->private + stats->shared);
> > +	print_size_region(p, "shared", region, stats->shared);
> > +	print_size_region(p, "active", region, stats->active);
> >   	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> > -		print_size(p, "resident", region, stats->resident);
> > +		print_size_region(p, "resident", region, stats->resident);
> >   	if (supported_status & DRM_GEM_OBJECT_PURGEABLE)
> > -		print_size(p, "purgeable", region, stats->purgeable);
> > +		print_size_region(p, "purgeable", region, stats->purgeable);
> >   }
> >   EXPORT_SYMBOL(drm_print_memory_stats);
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index f0ef32e9fa5e..07da14859289 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -494,6 +494,8 @@ struct drm_memory_stats {
> >   enum drm_gem_object_status;
> > +void drm_driver_print_size(struct drm_printer *p, struct drm_file *file,
> > +			   const char *subkey, u64 sz);
> >   void drm_print_memory_stats(struct drm_printer *p,
> >   			    const struct drm_memory_stats *stats,
> >   			    enum drm_gem_object_status supported_status,

Adrian Larumbe