[RFC PATCH 0/2] Allow fdinfo to display size of internal BO's

[RFC PATCH 0/2] Allow fdinfo to display size of internal BO's

[Adrián Larumbe]

This patch series lets DRM fdinfo stats functions display an additional tag
that will reflect the amount of memory internal BOs of a DRM driver take
up.

The rationale for this is that some drivers, like Panthor, need quite a bit
of memory for things like queue ring buffers or tiler heap chunks, and
these will vary dynamically as new scheduler groups, queues or heap chunks
are created by one or more render contexts.

Because these are tied to an open DRM file that represents an ongoing
render context, then it makes sense to provide users with this information
in the DRM file fdinfo descriptor, even though they would never be exposed
to UM through a handle.

These two patches were originally part of a wider series, but broke it down
into two different submissions for ease of discussion.

The previous debate can be found at [1], in its latest two patches.

[1] https://lore.kernel.org/dri-devel/dqhnxhgho6spfh7xhw6yvs2iiqeqzeg63e6jqqpw2g7gkrfphn@dojsixyl4esv/

Adrián Larumbe (2):
  drm/drm_file: Add display of driver's internal memory size
  drm/panthor: register size of internal objects through fdinfo

 drivers/gpu/drm/drm_file.c               |  6 ++-
 drivers/gpu/drm/msm/msm_drv.c            |  2 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c  |  2 +-
 drivers/gpu/drm/panthor/panthor_device.c |  2 +
 drivers/gpu/drm/panthor/panthor_device.h |  6 +++
 drivers/gpu/drm/panthor/panthor_drv.c    | 16 +++++--
 drivers/gpu/drm/panthor/panthor_fw.c     | 14 ++++--
 drivers/gpu/drm/panthor/panthor_fw.h     |  6 ++-
 drivers/gpu/drm/panthor/panthor_gem.c    | 55 ++++++++++++++++++++++--
 drivers/gpu/drm/panthor/panthor_gem.h    | 15 ++++++-
 drivers/gpu/drm/panthor/panthor_heap.c   | 20 ++++++---
 drivers/gpu/drm/panthor/panthor_heap.h   |  6 ++-
 drivers/gpu/drm/panthor/panthor_mmu.c    |  7 ++-
 drivers/gpu/drm/panthor/panthor_mmu.h    |  3 +-
 drivers/gpu/drm/panthor/panthor_sched.c  | 19 ++++----
 drivers/gpu/drm/v3d/v3d_drv.c            |  2 +-
 include/drm/drm_file.h                   |  7 ++-
 17 files changed, 150 insertions(+), 38 deletions(-)

-- 
2.46.2

[RFC PATCH 1/2] drm/drm_file: Add display of driver's internal memory size

[Adrián Larumbe]

Some drivers must allocate a considerable amount of memory for bookkeeping
structures and GPU's MCU-kernel shared communication regions. These are
often created as a result of the invocation of the driver's ioctl()
interface functions, so it is sensible to consider them as being owned by
the render context associated with an open drm file.

However, at the moment drm_show_memory_stats only traverses the UM-exposed
drm objects for which a handle exists. Private driver objects and memory
regions, though connected to a render context, are unaccounted for in their
fdinfo numbers.

Add a new drm_memory_stats 'internal' memory category.

Because deciding what constitutes an 'internal' object and where to find
these are driver-dependent, calculation of this size must be done through a
driver-provided function pointer, which becomes the third argument of
drm_show_memory_stats. Drivers which have no interest in exposing the size
of internal memory objects can keep passing NULL for unaltered behaviour.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/drm_file.c              | 6 +++++-
 drivers/gpu/drm/msm/msm_drv.c           | 2 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
 drivers/gpu/drm/v3d/v3d_drv.c           | 2 +-
 include/drm/drm_file.h                  | 7 ++++++-
 5 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index ad1dc638c83b..937471339c9a 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -856,6 +856,7 @@ void drm_print_memory_stats(struct drm_printer *p,
 	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(p, "internal", region, stats->internal);
 
 	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
 		print_size(p, "resident", region, stats->resident);
@@ -873,7 +874,7 @@ EXPORT_SYMBOL(drm_print_memory_stats);
  * Helper to iterate over GEM objects with a handle allocated in the specified
  * file.
  */
-void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
+void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func)
 {
 	struct drm_gem_object *obj;
 	struct drm_memory_stats status = {};
@@ -919,6 +920,9 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
 	}
 	spin_unlock(&file->table_lock);
 
+	if (func)
+		func(&status, file);
+
 	drm_print_memory_stats(p, &status, supported_status, "memory");
 }
 EXPORT_SYMBOL(drm_show_memory_stats);
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index edbc1ab0fbc8..2b3feb79afc4 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -880,7 +880,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 
 	msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
 
-	drm_show_memory_stats(p, file);
+	drm_show_memory_stats(p, file, NULL);
 }
 
 static const struct file_operations fops = {
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 04d615df5259..aaa8602bf00d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -609,7 +609,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 
 	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
 
-	drm_show_memory_stats(p, file);
+	drm_show_memory_stats(p, file, NULL);
 }
 
 static const struct file_operations panfrost_drm_driver_fops = {
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
index fb35c5c3f1a7..314e77c67972 100644
--- a/drivers/gpu/drm/v3d/v3d_drv.c
+++ b/drivers/gpu/drm/v3d/v3d_drv.c
@@ -195,7 +195,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 			   v3d_queue_to_string(queue), jobs_completed);
 	}
 
-	drm_show_memory_stats(p, file);
+	drm_show_memory_stats(p, file, NULL);
 }
 
 static const struct file_operations v3d_drm_fops = {
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index 8c0030c77308..661d00d5350e 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -469,6 +469,7 @@ void drm_send_event_timestamp_locked(struct drm_device *dev,
  * @resident: Total size of GEM objects backing pages
  * @purgeable: Total size of GEM objects that can be purged (resident and not active)
  * @active: Total size of GEM objects active on one or more engines
+ * @internal: Total size of GEM objects that aren't exposed to user space
  *
  * Used by drm_print_memory_stats()
  */
@@ -478,16 +479,20 @@ struct drm_memory_stats {
 	u64 resident;
 	u64 purgeable;
 	u64 active;
+	u64 internal;
 };
 
 enum drm_gem_object_status;
 
+typedef void (*internal_bos)(struct drm_memory_stats *status,
+			     struct drm_file *file);
+
 void drm_print_memory_stats(struct drm_printer *p,
 			    const struct drm_memory_stats *stats,
 			    enum drm_gem_object_status supported_status,
 			    const char *region);
 
-void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
+void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func);
 void drm_show_fdinfo(struct seq_file *m, struct file *f);
 
 struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);
-- 
2.46.2

[RFC PATCH 2/2] drm/panthor: register size of internal objects through fdinfo

[Adrián Larumbe]

This includes both DRM objects created to support queues, groups and heaps,
and also objects whose pages are shared between the GPU and the MCU.

However, this doesn't include objects that hold the firmware's binary
regions, since these aren't owned by a render context and are allocated
only once at driver's initialisation time.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
---
 drivers/gpu/drm/panthor/panthor_device.c |  2 +
 drivers/gpu/drm/panthor/panthor_device.h |  6 +++
 drivers/gpu/drm/panthor/panthor_drv.c    | 16 +++++--
 drivers/gpu/drm/panthor/panthor_fw.c     | 14 ++++--
 drivers/gpu/drm/panthor/panthor_fw.h     |  6 ++-
 drivers/gpu/drm/panthor/panthor_gem.c    | 55 ++++++++++++++++++++++--
 drivers/gpu/drm/panthor/panthor_gem.h    | 15 ++++++-
 drivers/gpu/drm/panthor/panthor_heap.c   | 20 ++++++---
 drivers/gpu/drm/panthor/panthor_heap.h   |  6 ++-
 drivers/gpu/drm/panthor/panthor_mmu.c    |  7 ++-
 drivers/gpu/drm/panthor/panthor_mmu.h    |  3 +-
 drivers/gpu/drm/panthor/panthor_sched.c  | 19 ++++----
 12 files changed, 136 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_device.c b/drivers/gpu/drm/panthor/panthor_device.c
index 4082c8f2951d..868fa9aba570 100644
--- a/drivers/gpu/drm/panthor/panthor_device.c
+++ b/drivers/gpu/drm/panthor/panthor_device.c
@@ -179,6 +179,8 @@ int panthor_device_init(struct panthor_device *ptdev)
 	if (ret)
 		return ret;
 
+	drmm_mutex_init(&ptdev->base, &ptdev->private_obj_list_lock);
+
 	/*
 	 * Set the dummy page holding the latest flush to 1. This will cause the
 	 * flush to avoided as we know it isn't necessary if the submission
diff --git a/drivers/gpu/drm/panthor/panthor_device.h b/drivers/gpu/drm/panthor/panthor_device.h
index 0e68f5a70d20..414822b35a23 100644
--- a/drivers/gpu/drm/panthor/panthor_device.h
+++ b/drivers/gpu/drm/panthor/panthor_device.h
@@ -190,6 +190,9 @@ struct panthor_device {
 
 	/** @fast_rate: Maximum device clock frequency. Set by DVFS */
 	unsigned long fast_rate;
+
+	/** @private_obj_list_lock: Lock around per-file lists of internal GEM objects */
+	struct mutex private_obj_list_lock;
 };
 
 struct panthor_gpu_usage {
@@ -212,6 +215,9 @@ struct panthor_file {
 
 	/** @stats: cycle and timestamp measures for job execution. */
 	struct panthor_gpu_usage stats;
+
+	/** @private_file_list: File's list of private GEM objects. */
+	struct list_head private_file_list;
 };
 
 int panthor_device_init(struct panthor_device *ptdev);
diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
index f9b93f84d611..b1cfd58d259b 100644
--- a/drivers/gpu/drm/panthor/panthor_drv.c
+++ b/drivers/gpu/drm/panthor/panthor_drv.c
@@ -1128,13 +1128,13 @@ static int panthor_ioctl_tiler_heap_create(struct drm_device *ddev, void *data,
 	if (!vm)
 		return -EINVAL;
 
-	pool = panthor_vm_get_heap_pool(vm, true);
+	pool = panthor_vm_get_heap_pool(vm, true, pfile);
 	if (IS_ERR(pool)) {
 		ret = PTR_ERR(pool);
 		goto out_put_vm;
 	}
 
-	ret = panthor_heap_create(pool,
+	ret = panthor_heap_create(pool, pfile,
 				  args->initial_chunk_count,
 				  args->chunk_size,
 				  args->max_chunks,
@@ -1174,7 +1174,7 @@ static int panthor_ioctl_tiler_heap_destroy(struct drm_device *ddev, void *data,
 	if (!vm)
 		return -EINVAL;
 
-	pool = panthor_vm_get_heap_pool(vm, false);
+	pool = panthor_vm_get_heap_pool(vm, false, NULL);
 	if (IS_ERR(pool)) {
 		ret = PTR_ERR(pool);
 		goto out_put_vm;
@@ -1348,6 +1348,8 @@ panthor_open(struct drm_device *ddev, struct drm_file *file)
 
 	pfile->ptdev = ptdev;
 
+	INIT_LIST_HEAD(&pfile->private_file_list);
+
 	ret = panthor_vm_pool_create(pfile);
 	if (ret)
 		goto err_free_file;
@@ -1375,6 +1377,12 @@ panthor_postclose(struct drm_device *ddev, struct drm_file *file)
 {
 	struct panthor_file *pfile = file->driver_priv;
 
+	/*
+	 * Group's internal BO's are destroyed asynchronously in a separate worker thread,
+	 * so there's a chance by the time BO release happens, the file is already gone.
+	 */
+	panthor_gem_dettach_internal_bos(pfile);
+
 	panthor_group_pool_destroy(pfile);
 	panthor_vm_pool_destroy(pfile);
 
@@ -1464,7 +1472,7 @@ static void panthor_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 
 	panthor_gpu_show_fdinfo(ptdev, file->driver_priv, p);
 
-	drm_show_memory_stats(p, file);
+	drm_show_memory_stats(p, file, panthor_internal_bos);
 }
 
 static const struct file_operations panthor_drm_driver_fops = {
diff --git a/drivers/gpu/drm/panthor/panthor_fw.c b/drivers/gpu/drm/panthor/panthor_fw.c
index 631f639b8b86..98f076e64b3c 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.c
+++ b/drivers/gpu/drm/panthor/panthor_fw.c
@@ -449,6 +449,7 @@ static void panthor_fw_init_section_mem(struct panthor_device *ptdev,
  */
 struct panthor_kernel_bo *
 panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
+				 struct panthor_file *pfile,
 				 struct panthor_fw_ringbuf_input_iface **input,
 				 const struct panthor_fw_ringbuf_output_iface **output,
 				 u32 *input_fw_va, u32 *output_fw_va)
@@ -456,11 +457,12 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
 	struct panthor_kernel_bo *mem;
 	int ret;
 
-	mem = panthor_kernel_bo_create(ptdev, ptdev->fw->vm, SZ_8K,
+	mem = panthor_kernel_bo_create(ptdev, pfile, ptdev->fw->vm, SZ_8K,
 				       DRM_PANTHOR_BO_NO_MMAP,
 				       DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC |
 				       DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED,
 				       PANTHOR_VM_KERNEL_AUTO_VA);
+
 	if (IS_ERR(mem))
 		return mem;
 
@@ -487,9 +489,12 @@ panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
  * Return: A valid pointer in case of success, an ERR_PTR() otherwise.
  */
 struct panthor_kernel_bo *
-panthor_fw_alloc_suspend_buf_mem(struct panthor_device *ptdev, size_t size)
+panthor_fw_alloc_suspend_buf_mem(struct panthor_file *pfile,
+				 struct panthor_device *ptdev,
+				 size_t size)
 {
-	return panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev), size,
+	return panthor_kernel_bo_create(ptdev, pfile,
+					panthor_fw_vm(ptdev), size,
 					DRM_PANTHOR_BO_NO_MMAP,
 					DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC,
 					PANTHOR_VM_KERNEL_AUTO_VA);
@@ -609,7 +614,8 @@ static int panthor_fw_load_section_entry(struct panthor_device *ptdev,
 		if (cache_mode != CSF_FW_BINARY_IFACE_ENTRY_RD_CACHE_MODE_CACHED)
 			vm_map_flags |= DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED;
 
-		section->mem = panthor_kernel_bo_create(ptdev, panthor_fw_vm(ptdev),
+		section->mem = panthor_kernel_bo_create(ptdev, NULL,
+							panthor_fw_vm(ptdev),
 							section_size,
 							DRM_PANTHOR_BO_NO_MMAP,
 							vm_map_flags, va);
diff --git a/drivers/gpu/drm/panthor/panthor_fw.h b/drivers/gpu/drm/panthor/panthor_fw.h
index 22448abde992..552db0e03e7b 100644
--- a/drivers/gpu/drm/panthor/panthor_fw.h
+++ b/drivers/gpu/drm/panthor/panthor_fw.h
@@ -7,6 +7,7 @@
 #include <linux/types.h>
 
 struct panthor_device;
+struct panthor_file;
 struct panthor_kernel_bo;
 
 #define MAX_CSGS				31
@@ -476,11 +477,14 @@ void panthor_fw_ring_csg_doorbells(struct panthor_device *ptdev, u32 csg_slot);
 
 struct panthor_kernel_bo *
 panthor_fw_alloc_queue_iface_mem(struct panthor_device *ptdev,
+				 struct panthor_file *pfile,
 				 struct panthor_fw_ringbuf_input_iface **input,
 				 const struct panthor_fw_ringbuf_output_iface **output,
 				 u32 *input_fw_va, u32 *output_fw_va);
 struct panthor_kernel_bo *
-panthor_fw_alloc_suspend_buf_mem(struct panthor_device *ptdev, size_t size);
+panthor_fw_alloc_suspend_buf_mem(struct panthor_file *pfile,
+				 struct panthor_device *ptdev,
+				 size_t size);
 
 struct panthor_vm *panthor_fw_vm(struct panthor_device *ptdev);
 
diff --git a/drivers/gpu/drm/panthor/panthor_gem.c b/drivers/gpu/drm/panthor/panthor_gem.c
index c60b599665d8..0c4eccbe47a6 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.c
+++ b/drivers/gpu/drm/panthor/panthor_gem.c
@@ -5,6 +5,7 @@
 #include <linux/dma-buf.h>
 #include <linux/dma-mapping.h>
 #include <linux/err.h>
+#include <linux/mutex.h>
 #include <linux/slab.h>
 
 #include <drm/panthor_drm.h>
@@ -24,6 +25,20 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
 	drm_gem_object_put(vm_root_gem);
 }
 
+void panthor_gem_dettach_internal_bos(struct panthor_file *pfile)
+{
+	struct panthor_kernel_bo *kbo, *tmp;
+
+	mutex_lock(&pfile->ptdev->private_obj_list_lock);
+	list_for_each_entry_safe(kbo, tmp,
+				 &pfile->private_file_list,
+				 private_obj) {
+		list_del(&kbo->private_obj);
+		INIT_LIST_HEAD(&kbo->private_obj);
+	}
+	mutex_unlock(&pfile->ptdev->private_obj_list_lock);
+}
+
 /**
  * panthor_kernel_bo_destroy() - Destroy a kernel buffer object
  * @bo: Kernel buffer object to destroy. If NULL or an ERR_PTR(), the destruction
@@ -31,12 +46,22 @@ static void panthor_gem_free_object(struct drm_gem_object *obj)
  */
 void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
 {
+	struct panthor_device *ptdev;
 	struct panthor_vm *vm;
 	int ret;
 
 	if (IS_ERR_OR_NULL(bo))
 		return;
 
+	ptdev = container_of(bo->obj->dev, struct panthor_device, base);
+
+	mutex_lock(&ptdev->private_obj_list_lock);
+	if (!list_empty(&bo->private_obj)) {
+		list_del(&bo->private_obj);
+		INIT_LIST_HEAD(&bo->private_obj);
+	}
+	mutex_unlock(&ptdev->private_obj_list_lock);
+
 	vm = bo->vm;
 	panthor_kernel_bo_vunmap(bo);
 
@@ -57,6 +82,20 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
 	kfree(bo);
 }
 
+void panthor_internal_bos(struct drm_memory_stats *status,
+			  struct drm_file *file)
+{
+	struct panthor_file *pfile = file->driver_priv;
+	struct panthor_kernel_bo *kbo;
+
+	mutex_lock(&pfile->ptdev->private_obj_list_lock);
+	list_for_each_entry(kbo, &pfile->private_file_list, private_obj) {
+		status->resident += kbo->obj->size;
+		status->internal += kbo->obj->size;
+	}
+	mutex_unlock(&pfile->ptdev->private_obj_list_lock);
+}
+
 /**
  * panthor_kernel_bo_create() - Create and map a GEM object to a VM
  * @ptdev: Device.
@@ -72,9 +111,9 @@ void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo)
  * Return: A valid pointer in case of success, an ERR_PTR() otherwise.
  */
 struct panthor_kernel_bo *
-panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
-			 size_t size, u32 bo_flags, u32 vm_map_flags,
-			 u64 gpu_va)
+panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_file *pfile,
+			 struct panthor_vm *vm, size_t size, u32 bo_flags,
+			 u32 vm_map_flags, u64 gpu_va)
 {
 	struct drm_gem_shmem_object *obj;
 	struct panthor_kernel_bo *kbo;
@@ -111,6 +150,16 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
 	bo->exclusive_vm_root_gem = panthor_vm_root_gem(vm);
 	drm_gem_object_get(bo->exclusive_vm_root_gem);
 	bo->base.base.resv = bo->exclusive_vm_root_gem->resv;
+
+	INIT_LIST_HEAD(&kbo->private_obj);
+
+	/* Only FW regions are not bound to an open file */
+	if (pfile) {
+		mutex_lock(&ptdev->private_obj_list_lock);
+		list_add(&kbo->private_obj, &pfile->private_file_list);
+		mutex_unlock(&ptdev->private_obj_list_lock);
+	}
+
 	return kbo;
 
 err_free_va:
diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
index e43021cf6d45..1685b30b1d86 100644
--- a/drivers/gpu/drm/panthor/panthor_gem.h
+++ b/drivers/gpu/drm/panthor/panthor_gem.h
@@ -12,6 +12,8 @@
 #include <linux/rwsem.h>
 
 struct panthor_vm;
+struct panthor_file;
+struct panthor_device;
 
 /**
  * struct panthor_gem_object - Driver specific GEM object.
@@ -75,8 +77,14 @@ struct panthor_kernel_bo {
 	 * @kmap: Kernel CPU mapping of @gem.
 	 */
 	void *kmap;
+
+	/** @private_node: Link to driver's list of private GEM objects. */
+	struct list_head private_obj;
 };
 
+void panthor_internal_bos(struct drm_memory_stats *status,
+			  struct drm_file *file);
+
 static inline
 struct panthor_gem_object *to_panthor_bo(struct drm_gem_object *obj)
 {
@@ -137,10 +145,13 @@ panthor_kernel_bo_vunmap(struct panthor_kernel_bo *bo)
 }
 
 struct panthor_kernel_bo *
-panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
-			 size_t size, u32 bo_flags, u32 vm_map_flags,
+panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_file *pfile,
+			 struct panthor_vm *vm, size_t size,
+			 u32 bo_flags, u32 vm_map_flags,
 			 u64 gpu_va);
 
 void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
 
+void panthor_gem_dettach_internal_bos(struct panthor_file *pfile);
+
 #endif /* __PANTHOR_GEM_H__ */
diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 3796a9eb22af..fd68257061ae 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -86,6 +86,9 @@ struct panthor_heap_pool {
 	/** @ptdev: Device. */
 	struct panthor_device *ptdev;
 
+	/** @pfile: Pointer to Panfrost file struct */
+	struct panthor_file *pfile;
+
 	/** @vm: VM this pool is bound to. */
 	struct panthor_vm *vm;
 
@@ -132,6 +135,7 @@ static void panthor_free_heap_chunk(struct panthor_vm *vm,
 }
 
 static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
+				    struct panthor_file *pfile,
 				    struct panthor_vm *vm,
 				    struct panthor_heap *heap,
 				    bool initial_chunk)
@@ -144,7 +148,7 @@ static int panthor_alloc_heap_chunk(struct panthor_device *ptdev,
 	if (!chunk)
 		return -ENOMEM;
 
-	chunk->bo = panthor_kernel_bo_create(ptdev, vm, heap->chunk_size,
+	chunk->bo = panthor_kernel_bo_create(ptdev, pfile, vm, heap->chunk_size,
 					     DRM_PANTHOR_BO_NO_MMAP,
 					     DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC,
 					     PANTHOR_VM_KERNEL_AUTO_VA);
@@ -201,6 +205,7 @@ static void panthor_free_heap_chunks(struct panthor_vm *vm,
 }
 
 static int panthor_alloc_heap_chunks(struct panthor_device *ptdev,
+				     struct panthor_file *pfile,
 				     struct panthor_vm *vm,
 				     struct panthor_heap *heap,
 				     u32 chunk_count)
@@ -209,7 +214,7 @@ static int panthor_alloc_heap_chunks(struct panthor_device *ptdev,
 	u32 i;
 
 	for (i = 0; i < chunk_count; i++) {
-		ret = panthor_alloc_heap_chunk(ptdev, vm, heap, true);
+		ret = panthor_alloc_heap_chunk(ptdev, pfile, vm, heap, true);
 		if (ret)
 			return ret;
 	}
@@ -265,6 +270,7 @@ int panthor_heap_destroy(struct panthor_heap_pool *pool, u32 handle)
  * Return: a positive handle on success, a negative error otherwise.
  */
 int panthor_heap_create(struct panthor_heap_pool *pool,
+			struct panthor_file *pfile,
 			u32 initial_chunk_count,
 			u32 chunk_size,
 			u32 max_chunks,
@@ -308,7 +314,7 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
 	heap->max_chunks = max_chunks;
 	heap->target_in_flight = target_in_flight;
 
-	ret = panthor_alloc_heap_chunks(pool->ptdev, vm, heap,
+	ret = panthor_alloc_heap_chunks(pool->ptdev, pfile, vm, heap,
 					initial_chunk_count);
 	if (ret)
 		goto err_free_heap;
@@ -466,7 +472,7 @@ int panthor_heap_grow(struct panthor_heap_pool *pool,
 	 * further jobs in this queue fail immediately instead of having to
 	 * wait for the job timeout.
 	 */
-	ret = panthor_alloc_heap_chunk(pool->ptdev, pool->vm, heap, false);
+	ret = panthor_alloc_heap_chunk(pool->ptdev, pool->pfile, pool->vm, heap, false);
 	if (ret)
 		goto out_unlock;
 
@@ -526,7 +532,9 @@ panthor_heap_pool_get(struct panthor_heap_pool *pool)
  * Return: A valid pointer on success, a negative error code otherwise.
  */
 struct panthor_heap_pool *
-panthor_heap_pool_create(struct panthor_device *ptdev, struct panthor_vm *vm)
+panthor_heap_pool_create(struct panthor_device *ptdev,
+			 struct panthor_vm *vm,
+			 struct panthor_file *pfile)
 {
 	size_t bosize = ALIGN(MAX_HEAPS_PER_POOL *
 			      panthor_heap_ctx_stride(ptdev),
@@ -547,7 +555,7 @@ panthor_heap_pool_create(struct panthor_device *ptdev, struct panthor_vm *vm)
 	xa_init_flags(&pool->xa, XA_FLAGS_ALLOC);
 	kref_init(&pool->refcount);
 
-	pool->gpu_contexts = panthor_kernel_bo_create(ptdev, vm, bosize,
+	pool->gpu_contexts = panthor_kernel_bo_create(ptdev, pfile, vm, bosize,
 						      DRM_PANTHOR_BO_NO_MMAP,
 						      DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC,
 						      PANTHOR_VM_KERNEL_AUTO_VA);
diff --git a/drivers/gpu/drm/panthor/panthor_heap.h b/drivers/gpu/drm/panthor/panthor_heap.h
index 25a5f2bba445..1d1b409064e3 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.h
+++ b/drivers/gpu/drm/panthor/panthor_heap.h
@@ -9,8 +9,10 @@
 struct panthor_device;
 struct panthor_heap_pool;
 struct panthor_vm;
+struct panthor_file;
 
 int panthor_heap_create(struct panthor_heap_pool *pool,
+			struct panthor_file *pfile,
 			u32 initial_chunk_count,
 			u32 chunk_size,
 			u32 max_chunks,
@@ -20,7 +22,9 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
 int panthor_heap_destroy(struct panthor_heap_pool *pool, u32 handle);
 
 struct panthor_heap_pool *
-panthor_heap_pool_create(struct panthor_device *ptdev, struct panthor_vm *vm);
+panthor_heap_pool_create(struct panthor_device *ptdev,
+			 struct panthor_vm *vm,
+			 struct panthor_file *pfile);
 void panthor_heap_pool_destroy(struct panthor_heap_pool *pool);
 
 struct panthor_heap_pool *
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 37f1885c54c7..1d49cb405cf1 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -1897,16 +1897,19 @@ struct panthor_vm *panthor_vm_get(struct panthor_vm *vm)
  *
  * Return: A valid pointer on success, an ERR_PTR() otherwise.
  */
-struct panthor_heap_pool *panthor_vm_get_heap_pool(struct panthor_vm *vm, bool create)
+struct panthor_heap_pool *panthor_vm_get_heap_pool(struct panthor_vm *vm, bool create,
+						   struct panthor_file *pfile)
 {
 	struct panthor_heap_pool *pool;
 
+	drm_WARN_ON(&vm->ptdev->base, (!create && pfile));
+
 	mutex_lock(&vm->heaps.lock);
 	if (!vm->heaps.pool && create) {
 		if (vm->destroyed)
 			pool = ERR_PTR(-EINVAL);
 		else
-			pool = panthor_heap_pool_create(vm->ptdev, vm);
+			pool = panthor_heap_pool_create(vm->ptdev, vm, pfile);
 
 		if (!IS_ERR(pool))
 			vm->heaps.pool = panthor_heap_pool_get(pool);
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.h b/drivers/gpu/drm/panthor/panthor_mmu.h
index 6788771071e3..70cd054e9124 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.h
+++ b/drivers/gpu/drm/panthor/panthor_mmu.h
@@ -14,6 +14,7 @@ struct panthor_heap_pool;
 struct panthor_vm;
 struct panthor_vma;
 struct panthor_mmu;
+struct panthor_file;
 
 int panthor_mmu_init(struct panthor_device *ptdev);
 void panthor_mmu_unplug(struct panthor_device *ptdev);
@@ -34,7 +35,7 @@ int panthor_vm_as(struct panthor_vm *vm);
 int panthor_vm_flush_all(struct panthor_vm *vm);
 
 struct panthor_heap_pool *
-panthor_vm_get_heap_pool(struct panthor_vm *vm, bool create);
+panthor_vm_get_heap_pool(struct panthor_vm *vm, bool create, struct panthor_file *pfile);
 
 struct panthor_vm *panthor_vm_get(struct panthor_vm *vm);
 void panthor_vm_put(struct panthor_vm *vm);
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index c7b350fc3eba..c7828643036d 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1428,7 +1428,7 @@ static int group_process_tiler_oom(struct panthor_group *group, u32 cs_id)
 		struct panthor_fw_cs_iface *cs_iface;
 
 		cs_iface = panthor_fw_get_cs_iface(ptdev, csg_id, cs_id);
-		heaps = panthor_vm_get_heap_pool(group->vm, false);
+		heaps = panthor_vm_get_heap_pool(group->vm, false, NULL);
 		heap_address = cs_iface->output->heap_address;
 		vt_start = cs_iface->output->heap_vt_start;
 		vt_end = cs_iface->output->heap_vt_end;
@@ -3254,7 +3254,8 @@ static u32 calc_profiling_ringbuf_num_slots(struct panthor_device *ptdev,
 }
 
 static struct panthor_queue *
-group_create_queue(struct panthor_group *group,
+group_create_queue(struct panthor_file *pfile,
+		   struct panthor_group *group,
 		   const struct drm_panthor_queue_create *args)
 {
 	struct drm_gpu_scheduler *drm_sched;
@@ -3281,7 +3282,7 @@ group_create_queue(struct panthor_group *group,
 
 	queue->priority = args->priority;
 
-	queue->ringbuf = panthor_kernel_bo_create(group->ptdev, group->vm,
+	queue->ringbuf = panthor_kernel_bo_create(group->ptdev, pfile, group->vm,
 						  args->ringbuf_size,
 						  DRM_PANTHOR_BO_NO_MMAP,
 						  DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC |
@@ -3296,7 +3297,7 @@ group_create_queue(struct panthor_group *group,
 	if (ret)
 		goto err_free_queue;
 
-	queue->iface.mem = panthor_fw_alloc_queue_iface_mem(group->ptdev,
+	queue->iface.mem = panthor_fw_alloc_queue_iface_mem(group->ptdev, pfile,
 							    &queue->iface.input,
 							    &queue->iface.output,
 							    &queue->iface.input_fw_va,
@@ -3310,7 +3311,7 @@ group_create_queue(struct panthor_group *group,
 		calc_profiling_ringbuf_num_slots(group->ptdev, args->ringbuf_size);
 
 	queue->profiling.slots =
-		panthor_kernel_bo_create(group->ptdev, group->vm,
+		panthor_kernel_bo_create(group->ptdev, pfile, group->vm,
 					 queue->profiling.slot_count *
 					 sizeof(struct panthor_job_profiling_data),
 					 DRM_PANTHOR_BO_NO_MMAP,
@@ -3413,7 +3414,7 @@ int panthor_group_create(struct panthor_file *pfile,
 	}
 
 	suspend_size = csg_iface->control->suspend_size;
-	group->suspend_buf = panthor_fw_alloc_suspend_buf_mem(ptdev, suspend_size);
+	group->suspend_buf = panthor_fw_alloc_suspend_buf_mem(pfile, ptdev, suspend_size);
 	if (IS_ERR(group->suspend_buf)) {
 		ret = PTR_ERR(group->suspend_buf);
 		group->suspend_buf = NULL;
@@ -3421,14 +3422,14 @@ int panthor_group_create(struct panthor_file *pfile,
 	}
 
 	suspend_size = csg_iface->control->protm_suspend_size;
-	group->protm_suspend_buf = panthor_fw_alloc_suspend_buf_mem(ptdev, suspend_size);
+	group->protm_suspend_buf = panthor_fw_alloc_suspend_buf_mem(pfile, ptdev, suspend_size);
 	if (IS_ERR(group->protm_suspend_buf)) {
 		ret = PTR_ERR(group->protm_suspend_buf);
 		group->protm_suspend_buf = NULL;
 		goto err_put_group;
 	}
 
-	group->syncobjs = panthor_kernel_bo_create(ptdev, group->vm,
+	group->syncobjs = panthor_kernel_bo_create(ptdev, pfile, group->vm,
 						   group_args->queues.count *
 						   sizeof(struct panthor_syncobj_64b),
 						   DRM_PANTHOR_BO_NO_MMAP,
@@ -3448,7 +3449,7 @@ int panthor_group_create(struct panthor_file *pfile,
 	       group_args->queues.count * sizeof(struct panthor_syncobj_64b));
 
 	for (i = 0; i < group_args->queues.count; i++) {
-		group->queues[i] = group_create_queue(group, &queue_args[i]);
+		group->queues[i] = group_create_queue(pfile, group, &queue_args[i]);
 		if (IS_ERR(group->queues[i])) {
 			ret = PTR_ERR(group->queues[i]);
 			group->queues[i] = NULL;
-- 
2.46.2

Re: [RFC PATCH 1/2] drm/drm_file: Add display of driver's internal memory size

[Tvrtko Ursulin]

Hi Adrian,

On 03/10/2024 00:45, Adrián Larumbe wrote:
> Some drivers must allocate a considerable amount of memory for bookkeeping
> structures and GPU's MCU-kernel shared communication regions. These are
> often created as a result of the invocation of the driver's ioctl()
> interface functions, so it is sensible to consider them as being owned by
> the render context associated with an open drm file.
> 
> However, at the moment drm_show_memory_stats only traverses the UM-exposed
> drm objects for which a handle exists. Private driver objects and memory
> regions, though connected to a render context, are unaccounted for in their
> fdinfo numbers.
> 
> Add a new drm_memory_stats 'internal' memory category.
> 
> Because deciding what constitutes an 'internal' object and where to find
> these are driver-dependent, calculation of this size must be done through a
> driver-provided function pointer, which becomes the third argument of
> drm_show_memory_stats. Drivers which have no interest in exposing the size
> of internal memory objects can keep passing NULL for unaltered behaviour.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>   drivers/gpu/drm/drm_file.c              | 6 +++++-
>   drivers/gpu/drm/msm/msm_drv.c           | 2 +-
>   drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
>   drivers/gpu/drm/v3d/v3d_drv.c           | 2 +-
>   include/drm/drm_file.h                  | 7 ++++++-
>   5 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index ad1dc638c83b..937471339c9a 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -856,6 +856,7 @@ void drm_print_memory_stats(struct drm_printer *p,
>   	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(p, "internal", region, stats->internal);
>   
>   	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
>   		print_size(p, "resident", region, stats->resident);
> @@ -873,7 +874,7 @@ EXPORT_SYMBOL(drm_print_memory_stats);
>    * Helper to iterate over GEM objects with a handle allocated in the specified
>    * file.
>    */
> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func)
>   {
>   	struct drm_gem_object *obj;
>   	struct drm_memory_stats status = {};
> @@ -919,6 +920,9 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>   	}
>   	spin_unlock(&file->table_lock);
>   
> +	if (func)
> +		func(&status, file);
> +
>   	drm_print_memory_stats(p, &status, supported_status, "memory");
>   }
>   EXPORT_SYMBOL(drm_show_memory_stats);
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index edbc1ab0fbc8..2b3feb79afc4 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -880,7 +880,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>   
>   	msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
>   
> -	drm_show_memory_stats(p, file);
> +	drm_show_memory_stats(p, file, NULL);
>   }
>   
>   static const struct file_operations fops = {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 04d615df5259..aaa8602bf00d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -609,7 +609,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>   
>   	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
>   
> -	drm_show_memory_stats(p, file);
> +	drm_show_memory_stats(p, file, NULL);
>   }
>   
>   static const struct file_operations panfrost_drm_driver_fops = {
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> index fb35c5c3f1a7..314e77c67972 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.c
> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> @@ -195,7 +195,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>   			   v3d_queue_to_string(queue), jobs_completed);
>   	}
>   
> -	drm_show_memory_stats(p, file);
> +	drm_show_memory_stats(p, file, NULL);
>   }
>   
>   static const struct file_operations v3d_drm_fops = {
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 8c0030c77308..661d00d5350e 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -469,6 +469,7 @@ void drm_send_event_timestamp_locked(struct drm_device *dev,
>    * @resident: Total size of GEM objects backing pages
>    * @purgeable: Total size of GEM objects that can be purged (resident and not active)
>    * @active: Total size of GEM objects active on one or more engines
> + * @internal: Total size of GEM objects that aren't exposed to user space
>    *
>    * Used by drm_print_memory_stats()
>    */
> @@ -478,16 +479,20 @@ struct drm_memory_stats {
>   	u64 resident;
>   	u64 purgeable;
>   	u64 active;
> +	u64 internal;

So equally as in the last round of discussion back in June, internal in 
my mind still does not fit alongside the categories.

Reason is that in some drivers, at least such as i915, "internal" can be:

a) Backed by either system memory or device memory - so this does not 
provice that visibility;

b) They can also be resident or not, active or not, etc - so from that 
angle it also does not fit.

Do you lose anything if you add the internal objects into their 
respective regions and under the existing categories? Like do you have 
an use case in mind which needs to be able to distinguish between 
userspace and internal, or the problem simply is internal are 
unaccounted for?

Regards,

Tvrtko

>   };
>   
>   enum drm_gem_object_status;
>   
> +typedef void (*internal_bos)(struct drm_memory_stats *status,
> +			     struct drm_file *file);
> +
>   void drm_print_memory_stats(struct drm_printer *p,
>   			    const struct drm_memory_stats *stats,
>   			    enum drm_gem_object_status supported_status,
>   			    const char *region);
>   
> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func);
>   void drm_show_fdinfo(struct seq_file *m, struct file *f);
>   
>   struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);

Re: [RFC PATCH 1/2] drm/drm_file: Add display of driver's internal memory size

[Adrián Larumbe]

Hi Tvrtko,

On 04.10.2024 14:41, Tvrtko Ursulin wrote:
> 
> Hi Adrian,
> 
> On 03/10/2024 00:45, Adrián Larumbe wrote:
> > Some drivers must allocate a considerable amount of memory for bookkeeping
> > structures and GPU's MCU-kernel shared communication regions. These are
> > often created as a result of the invocation of the driver's ioctl()
> > interface functions, so it is sensible to consider them as being owned by
> > the render context associated with an open drm file.
> > 
> > However, at the moment drm_show_memory_stats only traverses the UM-exposed
> > drm objects for which a handle exists. Private driver objects and memory
> > regions, though connected to a render context, are unaccounted for in their
> > fdinfo numbers.
> > 
> > Add a new drm_memory_stats 'internal' memory category.
> > 
> > Because deciding what constitutes an 'internal' object and where to find
> > these are driver-dependent, calculation of this size must be done through a
> > driver-provided function pointer, which becomes the third argument of
> > drm_show_memory_stats. Drivers which have no interest in exposing the size
> > of internal memory objects can keep passing NULL for unaltered behaviour.
> > 
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > ---
> >   drivers/gpu/drm/drm_file.c              | 6 +++++-
> >   drivers/gpu/drm/msm/msm_drv.c           | 2 +-
> >   drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> >   drivers/gpu/drm/v3d/v3d_drv.c           | 2 +-
> >   include/drm/drm_file.h                  | 7 ++++++-
> >   5 files changed, 14 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index ad1dc638c83b..937471339c9a 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -856,6 +856,7 @@ void drm_print_memory_stats(struct drm_printer *p,
> >   	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(p, "internal", region, stats->internal);
> >   	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> >   		print_size(p, "resident", region, stats->resident);
> > @@ -873,7 +874,7 @@ EXPORT_SYMBOL(drm_print_memory_stats);
> >    * Helper to iterate over GEM objects with a handle allocated in the specified
> >    * file.
> >    */
> > -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> > +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func)
> >   {
> >   	struct drm_gem_object *obj;
> >   	struct drm_memory_stats status = {};
> > @@ -919,6 +920,9 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> >   	}
> >   	spin_unlock(&file->table_lock);
> > +	if (func)
> > +		func(&status, file);
> > +
> >   	drm_print_memory_stats(p, &status, supported_status, "memory");
> >   }
> >   EXPORT_SYMBOL(drm_show_memory_stats);
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index edbc1ab0fbc8..2b3feb79afc4 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -880,7 +880,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >   	msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
> > -	drm_show_memory_stats(p, file);
> > +	drm_show_memory_stats(p, file, NULL);
> >   }
> >   static const struct file_operations fops = {
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 04d615df5259..aaa8602bf00d 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -609,7 +609,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >   	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
> > -	drm_show_memory_stats(p, file);
> > +	drm_show_memory_stats(p, file, NULL);
> >   }
> >   static const struct file_operations panfrost_drm_driver_fops = {
> > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> > index fb35c5c3f1a7..314e77c67972 100644
> > --- a/drivers/gpu/drm/v3d/v3d_drv.c
> > +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> > @@ -195,7 +195,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> >   			   v3d_queue_to_string(queue), jobs_completed);
> >   	}
> > -	drm_show_memory_stats(p, file);
> > +	drm_show_memory_stats(p, file, NULL);
> >   }
> >   static const struct file_operations v3d_drm_fops = {
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index 8c0030c77308..661d00d5350e 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -469,6 +469,7 @@ void drm_send_event_timestamp_locked(struct drm_device *dev,
> >    * @resident: Total size of GEM objects backing pages
> >    * @purgeable: Total size of GEM objects that can be purged (resident and not active)
> >    * @active: Total size of GEM objects active on one or more engines
> > + * @internal: Total size of GEM objects that aren't exposed to user space
> >    *
> >    * Used by drm_print_memory_stats()
> >    */
> > @@ -478,16 +479,20 @@ struct drm_memory_stats {
> >   	u64 resident;
> >   	u64 purgeable;
> >   	u64 active;
> > +	u64 internal;
> 
> So equally as in the last round of discussion back in June, internal in my
> mind still does not fit alongside the categories.
> 
> Reason is that in some drivers, at least such as i915, "internal" can be:
> 
> a) Backed by either system memory or device memory - so this does not provice
> that visibility;
> 
> b) They can also be resident or not, active or not, etc - so from that angle
> it also does not fit.
> 
> Do you lose anything if you add the internal objects into their respective
> regions and under the existing categories? Like do you have an use case in
> mind which needs to be able to distinguish between userspace and internal, or
> the problem simply is internal are unaccounted for?

The main use case we have in mind is exposing the size of driver buffer
allocations that are triggered in respone to an ioctl(), and so linked to an
open file. I gave a summary of what these could be in the patch description, but
in Panthor's case all these allocations are done with drm shmem functions
because it makes it easier to retrieve the sgtable that gives us their system
memory layout so that we can more easily map them onto the MMU's address space
for a Pantor VM. These BO's, though managed by the drm shmem API, are never
added to the open file list of user-exposed drm objects but we would still like
to tell UM how much memory they take up.

In the case of Panthor, they all add into the resident tally because all these
internal BO's are immediately pinned so that they can also be accessed by the
HW, but it doesn't have to be so for other drivers which might also keep track
of similar allocations.

I think maybe naming that tag as 'internal' is a bit of a misnomer and I could
pick one that more accurately represents its meaning? Something like 'file-internal'
or else 'file-private'.

Regarding a), I don't think where the allocations happen (system or device memory)
is relevant in this case, just that the allocations are tied to an open file, but
not exposed to UM through a DRM buffer object handle.

Regards,
Adrian

> Regards,
> 
> Tvrtko
> 
> >   };
> >   enum drm_gem_object_status;
> > +typedef void (*internal_bos)(struct drm_memory_stats *status,
> > +			     struct drm_file *file);
> > +
> >   void drm_print_memory_stats(struct drm_printer *p,
> >   			    const struct drm_memory_stats *stats,
> >   			    enum drm_gem_object_status supported_status,
> >   			    const char *region);
> > -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
> > +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func);
> >   void drm_show_fdinfo(struct seq_file *m, struct file *f);
> >   struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);

Re: [RFC PATCH 1/2] drm/drm_file: Add display of driver's internal memory size

[Tvrtko Ursulin]

On 09/10/2024 23:55, Adrián Larumbe wrote:
> Hi Tvrtko,
> 
> On 04.10.2024 14:41, Tvrtko Ursulin wrote:
>>
>> Hi Adrian,
>>
>> On 03/10/2024 00:45, Adrián Larumbe wrote:
>>> Some drivers must allocate a considerable amount of memory for bookkeeping
>>> structures and GPU's MCU-kernel shared communication regions. These are
>>> often created as a result of the invocation of the driver's ioctl()
>>> interface functions, so it is sensible to consider them as being owned by
>>> the render context associated with an open drm file.
>>>
>>> However, at the moment drm_show_memory_stats only traverses the UM-exposed
>>> drm objects for which a handle exists. Private driver objects and memory
>>> regions, though connected to a render context, are unaccounted for in their
>>> fdinfo numbers.
>>>
>>> Add a new drm_memory_stats 'internal' memory category.
>>>
>>> Because deciding what constitutes an 'internal' object and where to find
>>> these are driver-dependent, calculation of this size must be done through a
>>> driver-provided function pointer, which becomes the third argument of
>>> drm_show_memory_stats. Drivers which have no interest in exposing the size
>>> of internal memory objects can keep passing NULL for unaltered behaviour.
>>>
>>> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
>>> Cc: Rob Clark <robdclark@gmail.com>
>>> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>> ---
>>>    drivers/gpu/drm/drm_file.c              | 6 +++++-
>>>    drivers/gpu/drm/msm/msm_drv.c           | 2 +-
>>>    drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
>>>    drivers/gpu/drm/v3d/v3d_drv.c           | 2 +-
>>>    include/drm/drm_file.h                  | 7 ++++++-
>>>    5 files changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index ad1dc638c83b..937471339c9a 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -856,6 +856,7 @@ void drm_print_memory_stats(struct drm_printer *p,
>>>    	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(p, "internal", region, stats->internal);
>>>    	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
>>>    		print_size(p, "resident", region, stats->resident);
>>> @@ -873,7 +874,7 @@ EXPORT_SYMBOL(drm_print_memory_stats);
>>>     * Helper to iterate over GEM objects with a handle allocated in the specified
>>>     * file.
>>>     */
>>> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>>> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func)
>>>    {
>>>    	struct drm_gem_object *obj;
>>>    	struct drm_memory_stats status = {};
>>> @@ -919,6 +920,9 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>>>    	}
>>>    	spin_unlock(&file->table_lock);
>>> +	if (func)
>>> +		func(&status, file);
>>> +
>>>    	drm_print_memory_stats(p, &status, supported_status, "memory");
>>>    }
>>>    EXPORT_SYMBOL(drm_show_memory_stats);
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>> index edbc1ab0fbc8..2b3feb79afc4 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>> @@ -880,7 +880,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>    	msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
>>> -	drm_show_memory_stats(p, file);
>>> +	drm_show_memory_stats(p, file, NULL);
>>>    }
>>>    static const struct file_operations fops = {
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> index 04d615df5259..aaa8602bf00d 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> @@ -609,7 +609,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>    	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
>>> -	drm_show_memory_stats(p, file);
>>> +	drm_show_memory_stats(p, file, NULL);
>>>    }
>>>    static const struct file_operations panfrost_drm_driver_fops = {
>>> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
>>> index fb35c5c3f1a7..314e77c67972 100644
>>> --- a/drivers/gpu/drm/v3d/v3d_drv.c
>>> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
>>> @@ -195,7 +195,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>    			   v3d_queue_to_string(queue), jobs_completed);
>>>    	}
>>> -	drm_show_memory_stats(p, file);
>>> +	drm_show_memory_stats(p, file, NULL);
>>>    }
>>>    static const struct file_operations v3d_drm_fops = {
>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>> index 8c0030c77308..661d00d5350e 100644
>>> --- a/include/drm/drm_file.h
>>> +++ b/include/drm/drm_file.h
>>> @@ -469,6 +469,7 @@ void drm_send_event_timestamp_locked(struct drm_device *dev,
>>>     * @resident: Total size of GEM objects backing pages
>>>     * @purgeable: Total size of GEM objects that can be purged (resident and not active)
>>>     * @active: Total size of GEM objects active on one or more engines
>>> + * @internal: Total size of GEM objects that aren't exposed to user space
>>>     *
>>>     * Used by drm_print_memory_stats()
>>>     */
>>> @@ -478,16 +479,20 @@ struct drm_memory_stats {
>>>    	u64 resident;
>>>    	u64 purgeable;
>>>    	u64 active;
>>> +	u64 internal;
>>
>> So equally as in the last round of discussion back in June, internal in my
>> mind still does not fit alongside the categories.
>>
>> Reason is that in some drivers, at least such as i915, "internal" can be:
>>
>> a) Backed by either system memory or device memory - so this does not provice
>> that visibility;
>>
>> b) They can also be resident or not, active or not, etc - so from that angle
>> it also does not fit.
>>
>> Do you lose anything if you add the internal objects into their respective
>> regions and under the existing categories? Like do you have an use case in
>> mind which needs to be able to distinguish between userspace and internal, or
>> the problem simply is internal are unaccounted for?
> 
> The main use case we have in mind is exposing the size of driver buffer
> allocations that are triggered in respone to an ioctl(), and so linked to an

Most of this and below is old and clear - but to this specific point - 
so you do have an use case which specifically wants to know about the 
internal allocations separately from the rest? Could you describe what 
it is?

> open file. I gave a summary of what these could be in the patch description, but
> in Panthor's case all these allocations are done with drm shmem functions
> because it makes it easier to retrieve the sgtable that gives us their system
> memory layout so that we can more easily map them onto the MMU's address space
> for a Pantor VM. These BO's, though managed by the drm shmem API, are never
> added to the open file list of user-exposed drm objects but we would still like
> to tell UM how much memory they take up.
> 
> In the case of Panthor, they all add into the resident tally because all these
> internal BO's are immediately pinned so that they can also be accessed by the
> HW, but it doesn't have to be so for other drivers which might also keep track
> of similar allocations.
> 
> I think maybe naming that tag as 'internal' is a bit of a misnomer and I could
> pick one that more accurately represents its meaning? Something like 'file-internal'
> or else 'file-private'.
> 
> Regarding a), I don't think where the allocations happen (system or device memory)
> is relevant in this case, just that the allocations are tied to an open file, but
> not exposed to UM through a DRM buffer object handle.

On this last paragraph - right.. I possibly got confused on a). Which is 
why I always say it is good to include example output at least in the 
cover letter, if not the commit message.

How would it look on this driver?

drm-total-$what: ..
drm-resident-$what: ..
drm-internal-$what: ...

b) still stands though in that internal can be resident or not, 
purgeable or not.. Which is why I would like to know about the use case.

Also if you add drm-internal for any driver calling 
drm_print_memory_stats I think you "break" at least i915. There internal 
objects are already accounted in the existing categories. And printing 
out internal with zero would be very misleading.

Regards,

Tvrtko

> 
> Regards,
> Adrian
> 
>> Regards,
>>
>> Tvrtko
>>
>>>    };
>>>    enum drm_gem_object_status;
>>> +typedef void (*internal_bos)(struct drm_memory_stats *status,
>>> +			     struct drm_file *file);
>>> +
>>>    void drm_print_memory_stats(struct drm_printer *p,
>>>    			    const struct drm_memory_stats *stats,
>>>    			    enum drm_gem_object_status supported_status,
>>>    			    const char *region);
>>> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
>>> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func);
>>>    void drm_show_fdinfo(struct seq_file *m, struct file *f);
>>>    struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);

Re: [RFC PATCH 1/2] drm/drm_file: Add display of driver's internal memory size

[Adrián Larumbe]

Hi Tvrtko,

On 10.10.2024 10:50, Tvrtko Ursulin wrote:
> 
> On 09/10/2024 23:55, Adrián Larumbe wrote:
> > Hi Tvrtko,
> > 
> > On 04.10.2024 14:41, Tvrtko Ursulin wrote:
> > > 
> > > Hi Adrian,
> > > 
> > > On 03/10/2024 00:45, Adrián Larumbe wrote:
> > > > Some drivers must allocate a considerable amount of memory for bookkeeping
> > > > structures and GPU's MCU-kernel shared communication regions. These are
> > > > often created as a result of the invocation of the driver's ioctl()
> > > > interface functions, so it is sensible to consider them as being owned by
> > > > the render context associated with an open drm file.
> > > > 
> > > > However, at the moment drm_show_memory_stats only traverses the UM-exposed
> > > > drm objects for which a handle exists. Private driver objects and memory
> > > > regions, though connected to a render context, are unaccounted for in their
> > > > fdinfo numbers.
> > > > 
> > > > Add a new drm_memory_stats 'internal' memory category.
> > > > 
> > > > Because deciding what constitutes an 'internal' object and where to find
> > > > these are driver-dependent, calculation of this size must be done through a
> > > > driver-provided function pointer, which becomes the third argument of
> > > > drm_show_memory_stats. Drivers which have no interest in exposing the size
> > > > of internal memory objects can keep passing NULL for unaltered behaviour.
> > > > 
> > > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > ---
> > > >    drivers/gpu/drm/drm_file.c              | 6 +++++-
> > > >    drivers/gpu/drm/msm/msm_drv.c           | 2 +-
> > > >    drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> > > >    drivers/gpu/drm/v3d/v3d_drv.c           | 2 +-
> > > >    include/drm/drm_file.h                  | 7 ++++++-
> > > >    5 files changed, 14 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > > index ad1dc638c83b..937471339c9a 100644
> > > > --- a/drivers/gpu/drm/drm_file.c
> > > > +++ b/drivers/gpu/drm/drm_file.c
> > > > @@ -856,6 +856,7 @@ void drm_print_memory_stats(struct drm_printer *p,
> > > >    	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(p, "internal", region, stats->internal);
> > > >    	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> > > >    		print_size(p, "resident", region, stats->resident);
> > > > @@ -873,7 +874,7 @@ EXPORT_SYMBOL(drm_print_memory_stats);
> > > >     * Helper to iterate over GEM objects with a handle allocated in the specified
> > > >     * file.
> > > >     */
> > > > -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> > > > +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func)
> > > >    {
> > > >    	struct drm_gem_object *obj;
> > > >    	struct drm_memory_stats status = {};
> > > > @@ -919,6 +920,9 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> > > >    	}
> > > >    	spin_unlock(&file->table_lock);
> > > > +	if (func)
> > > > +		func(&status, file);
> > > > +
> > > >    	drm_print_memory_stats(p, &status, supported_status, "memory");
> > > >    }
> > > >    EXPORT_SYMBOL(drm_show_memory_stats);
> > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > > index edbc1ab0fbc8..2b3feb79afc4 100644
> > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > @@ -880,7 +880,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> > > >    	msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
> > > > -	drm_show_memory_stats(p, file);
> > > > +	drm_show_memory_stats(p, file, NULL);
> > > >    }
> > > >    static const struct file_operations fops = {
> > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > index 04d615df5259..aaa8602bf00d 100644
> > > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > @@ -609,7 +609,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> > > >    	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
> > > > -	drm_show_memory_stats(p, file);
> > > > +	drm_show_memory_stats(p, file, NULL);
> > > >    }
> > > >    static const struct file_operations panfrost_drm_driver_fops = {
> > > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> > > > index fb35c5c3f1a7..314e77c67972 100644
> > > > --- a/drivers/gpu/drm/v3d/v3d_drv.c
> > > > +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> > > > @@ -195,7 +195,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> > > >    			   v3d_queue_to_string(queue), jobs_completed);
> > > >    	}
> > > > -	drm_show_memory_stats(p, file);
> > > > +	drm_show_memory_stats(p, file, NULL);
> > > >    }
> > > >    static const struct file_operations v3d_drm_fops = {
> > > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > > > index 8c0030c77308..661d00d5350e 100644
> > > > --- a/include/drm/drm_file.h
> > > > +++ b/include/drm/drm_file.h
> > > > @@ -469,6 +469,7 @@ void drm_send_event_timestamp_locked(struct drm_device *dev,
> > > >     * @resident: Total size of GEM objects backing pages
> > > >     * @purgeable: Total size of GEM objects that can be purged (resident and not active)
> > > >     * @active: Total size of GEM objects active on one or more engines
> > > > + * @internal: Total size of GEM objects that aren't exposed to user space
> > > >     *
> > > >     * Used by drm_print_memory_stats()
> > > >     */
> > > > @@ -478,16 +479,20 @@ struct drm_memory_stats {
> > > >    	u64 resident;
> > > >    	u64 purgeable;
> > > >    	u64 active;
> > > > +	u64 internal;
> > > 
> > > So equally as in the last round of discussion back in June, internal in my
> > > mind still does not fit alongside the categories.
> > > 
> > > Reason is that in some drivers, at least such as i915, "internal" can be:
> > > 
> > > a) Backed by either system memory or device memory - so this does not provice
> > > that visibility;
> > > 
> > > b) They can also be resident or not, active or not, etc - so from that angle
> > > it also does not fit.
> > > 
> > > Do you lose anything if you add the internal objects into their respective
> > > regions and under the existing categories? Like do you have an use case in
> > > mind which needs to be able to distinguish between userspace and internal, or
> > > the problem simply is internal are unaccounted for?
> > 
> > The main use case we have in mind is exposing the size of driver buffer
> > allocations that are triggered in respone to an ioctl(), and so linked to an
> 
> Most of this and below is old and clear - but to this specific point - so you
> do have an use case which specifically wants to know about the internal
> allocations separately from the rest? Could you describe what it is?
> 
> > open file. I gave a summary of what these could be in the patch description, but
> > in Panthor's case all these allocations are done with drm shmem functions
> > because it makes it easier to retrieve the sgtable that gives us their system
> > memory layout so that we can more easily map them onto the MMU's address space
> > for a Pantor VM. These BO's, though managed by the drm shmem API, are never
> > added to the open file list of user-exposed drm objects but we would still like
> > to tell UM how much memory they take up.
> > 
> > In the case of Panthor, they all add into the resident tally because all these
> > internal BO's are immediately pinned so that they can also be accessed by the
> > HW, but it doesn't have to be so for other drivers which might also keep track
> > of similar allocations.
> > 
> > I think maybe naming that tag as 'internal' is a bit of a misnomer and I could
> > pick one that more accurately represents its meaning? Something like 'file-internal'
> > or else 'file-private'.
> > 
> > Regarding a), I don't think where the allocations happen (system or device memory)
> > is relevant in this case, just that the allocations are tied to an open file, but
> > not exposed to UM through a DRM buffer object handle.
> 
> On this last paragraph - right.. I possibly got confused on a). Which is why I
> always say it is good to include example output at least in the cover letter,
> if not the commit message.
> 
> How would it look on this driver?
> 
> drm-total-$what: ..
> drm-resident-$what: ..
> drm-internal-$what: ...

In the case of Panthor, it would look like this:

drm-driver:     panthor
drm-client-id:  3
drm-engine-panthor:     611046570346 ns
drm-cycles-panthor:     1172733302061
drm-maxfreq-panthor:    1000000000 Hz
drm-curfreq-panthor:    1000000000 Hz
drm-total-memory:       16480 KiB
drm-shared-memory:      0
drm-active-memory:      16200 KiB
drm-internal-memory:    10396 KiB
drm-resident-memory:    26876 KiB
drm-purgeable-memory:   0

Then in Panfrost:

drm-driver:     panfrost
drm-client-id:  6
drm-engine-fragment:    481941638 ns
drm-cycles-fragment:    60243117
drm-maxfreq-fragment:   799999987 Hz
drm-curfreq-fragment:   124999998 Hz
drm-engine-vertex-tiler:        55546675 ns
drm-cycles-vertex-tiler:        6943796
drm-maxfreq-vertex-tiler:       799999987 Hz
drm-curfreq-vertex-tiler:       124999998 Hz
drm-total-memory:       138420 KiB
drm-shared-memory:      7200 KiB
drm-active-memory:      0
drm-internal-memory:    0
drm-resident-memory:    2196 KiB
drm-purgeable-memory:   128 KiB


> b) still stands though in that internal can be resident or not, purgeable or
> not.. Which is why I would like to know about the use case.

This is true, DRM file-internal objects or memory allocations could fall
into any of these categories, and adding their sizes to the right one would
be the responsibility of the function pointer passed to drm_show_memory_stats(),
because that decision would have to be made on a per-driver basis.

> Also if you add drm-internal for any driver calling drm_print_memory_stats I
> think you "break" at least i915. There internal objects are already accounted
> in the existing categories. And printing out internal with zero would be very
> misleading.

I wasn't aware of this. So i915 is already doing this kind of accounting for internal
memory allocations. In that case, maybe printing of the 'drm-internal-memory' could
be done conditionally when it's greater than 0 to avoid 'breaking' existing drivers,
or else renaming it to 'drm-file-memory' would be seen as less invasive?

I'm asking this because if, at the end of the day, making this change part of the 
drm fdinfo core is going to clash with existing accounting in other DRM drivers, perhaps
it'd be easier to keep it Panthor-specific and add that tag together with its meaning
to Documentation/gpu/panfrost.rst.

I thought about this at first, but it also struck me as something other drivers might
want to do in the future in a sort of unified way, since internal allocations happening
in response to an ioctl() is a common thing.

Cheers,
Adrian

> Regards,
> 
> Tvrtko
> 
> > 
> > Regards,
> > Adrian
> > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > >    };
> > > >    enum drm_gem_object_status;
> > > > +typedef void (*internal_bos)(struct drm_memory_stats *status,
> > > > +			     struct drm_file *file);
> > > > +
> > > >    void drm_print_memory_stats(struct drm_printer *p,
> > > >    			    const struct drm_memory_stats *stats,
> > > >    			    enum drm_gem_object_status supported_status,
> > > >    			    const char *region);
> > > > -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
> > > > +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func);
> > > >    void drm_show_fdinfo(struct seq_file *m, struct file *f);
> > > >    struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);

Re: [RFC PATCH 1/2] drm/drm_file: Add display of driver's internal memory size

[Tvrtko Ursulin]

On 15/10/2024 20:05, Adrián Larumbe wrote:
> Hi Tvrtko,
> 
> On 10.10.2024 10:50, Tvrtko Ursulin wrote:
>>
>> On 09/10/2024 23:55, Adrián Larumbe wrote:
>>> Hi Tvrtko,
>>>
>>> On 04.10.2024 14:41, Tvrtko Ursulin wrote:
>>>>
>>>> Hi Adrian,
>>>>
>>>> On 03/10/2024 00:45, Adrián Larumbe wrote:
>>>>> Some drivers must allocate a considerable amount of memory for bookkeeping
>>>>> structures and GPU's MCU-kernel shared communication regions. These are
>>>>> often created as a result of the invocation of the driver's ioctl()
>>>>> interface functions, so it is sensible to consider them as being owned by
>>>>> the render context associated with an open drm file.
>>>>>
>>>>> However, at the moment drm_show_memory_stats only traverses the UM-exposed
>>>>> drm objects for which a handle exists. Private driver objects and memory
>>>>> regions, though connected to a render context, are unaccounted for in their
>>>>> fdinfo numbers.
>>>>>
>>>>> Add a new drm_memory_stats 'internal' memory category.
>>>>>
>>>>> Because deciding what constitutes an 'internal' object and where to find
>>>>> these are driver-dependent, calculation of this size must be done through a
>>>>> driver-provided function pointer, which becomes the third argument of
>>>>> drm_show_memory_stats. Drivers which have no interest in exposing the size
>>>>> of internal memory objects can keep passing NULL for unaltered behaviour.
>>>>>
>>>>> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
>>>>> Cc: Rob Clark <robdclark@gmail.com>
>>>>> Cc: Tvrtko Ursulin <tursulin@ursulin.net>
>>>>> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
>>>>> ---
>>>>>     drivers/gpu/drm/drm_file.c              | 6 +++++-
>>>>>     drivers/gpu/drm/msm/msm_drv.c           | 2 +-
>>>>>     drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
>>>>>     drivers/gpu/drm/v3d/v3d_drv.c           | 2 +-
>>>>>     include/drm/drm_file.h                  | 7 ++++++-
>>>>>     5 files changed, 14 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>>> index ad1dc638c83b..937471339c9a 100644
>>>>> --- a/drivers/gpu/drm/drm_file.c
>>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>>> @@ -856,6 +856,7 @@ void drm_print_memory_stats(struct drm_printer *p,
>>>>>     	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(p, "internal", region, stats->internal);
>>>>>     	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
>>>>>     		print_size(p, "resident", region, stats->resident);
>>>>> @@ -873,7 +874,7 @@ EXPORT_SYMBOL(drm_print_memory_stats);
>>>>>      * Helper to iterate over GEM objects with a handle allocated in the specified
>>>>>      * file.
>>>>>      */
>>>>> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>>>>> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func)
>>>>>     {
>>>>>     	struct drm_gem_object *obj;
>>>>>     	struct drm_memory_stats status = {};
>>>>> @@ -919,6 +920,9 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>>>>>     	}
>>>>>     	spin_unlock(&file->table_lock);
>>>>> +	if (func)
>>>>> +		func(&status, file);
>>>>> +
>>>>>     	drm_print_memory_stats(p, &status, supported_status, "memory");
>>>>>     }
>>>>>     EXPORT_SYMBOL(drm_show_memory_stats);
>>>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>>>> index edbc1ab0fbc8..2b3feb79afc4 100644
>>>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>>>> @@ -880,7 +880,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>>>     	msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
>>>>> -	drm_show_memory_stats(p, file);
>>>>> +	drm_show_memory_stats(p, file, NULL);
>>>>>     }
>>>>>     static const struct file_operations fops = {
>>>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>>>> index 04d615df5259..aaa8602bf00d 100644
>>>>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>>>>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>>>> @@ -609,7 +609,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>>>     	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
>>>>> -	drm_show_memory_stats(p, file);
>>>>> +	drm_show_memory_stats(p, file, NULL);
>>>>>     }
>>>>>     static const struct file_operations panfrost_drm_driver_fops = {
>>>>> diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
>>>>> index fb35c5c3f1a7..314e77c67972 100644
>>>>> --- a/drivers/gpu/drm/v3d/v3d_drv.c
>>>>> +++ b/drivers/gpu/drm/v3d/v3d_drv.c
>>>>> @@ -195,7 +195,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>>>>>     			   v3d_queue_to_string(queue), jobs_completed);
>>>>>     	}
>>>>> -	drm_show_memory_stats(p, file);
>>>>> +	drm_show_memory_stats(p, file, NULL);
>>>>>     }
>>>>>     static const struct file_operations v3d_drm_fops = {
>>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>>> index 8c0030c77308..661d00d5350e 100644
>>>>> --- a/include/drm/drm_file.h
>>>>> +++ b/include/drm/drm_file.h
>>>>> @@ -469,6 +469,7 @@ void drm_send_event_timestamp_locked(struct drm_device *dev,
>>>>>      * @resident: Total size of GEM objects backing pages
>>>>>      * @purgeable: Total size of GEM objects that can be purged (resident and not active)
>>>>>      * @active: Total size of GEM objects active on one or more engines
>>>>> + * @internal: Total size of GEM objects that aren't exposed to user space
>>>>>      *
>>>>>      * Used by drm_print_memory_stats()
>>>>>      */
>>>>> @@ -478,16 +479,20 @@ struct drm_memory_stats {
>>>>>     	u64 resident;
>>>>>     	u64 purgeable;
>>>>>     	u64 active;
>>>>> +	u64 internal;
>>>>
>>>> So equally as in the last round of discussion back in June, internal in my
>>>> mind still does not fit alongside the categories.
>>>>
>>>> Reason is that in some drivers, at least such as i915, "internal" can be:
>>>>
>>>> a) Backed by either system memory or device memory - so this does not provice
>>>> that visibility;
>>>>
>>>> b) They can also be resident or not, active or not, etc - so from that angle
>>>> it also does not fit.
>>>>
>>>> Do you lose anything if you add the internal objects into their respective
>>>> regions and under the existing categories? Like do you have an use case in
>>>> mind which needs to be able to distinguish between userspace and internal, or
>>>> the problem simply is internal are unaccounted for?
>>>
>>> The main use case we have in mind is exposing the size of driver buffer
>>> allocations that are triggered in respone to an ioctl(), and so linked to an
>>
>> Most of this and below is old and clear - but to this specific point - so you
>> do have an use case which specifically wants to know about the internal
>> allocations separately from the rest? Could you describe what it is?

What about this?

>>> open file. I gave a summary of what these could be in the patch description, but
>>> in Panthor's case all these allocations are done with drm shmem functions
>>> because it makes it easier to retrieve the sgtable that gives us their system
>>> memory layout so that we can more easily map them onto the MMU's address space
>>> for a Pantor VM. These BO's, though managed by the drm shmem API, are never
>>> added to the open file list of user-exposed drm objects but we would still like
>>> to tell UM how much memory they take up.
>>>
>>> In the case of Panthor, they all add into the resident tally because all these
>>> internal BO's are immediately pinned so that they can also be accessed by the
>>> HW, but it doesn't have to be so for other drivers which might also keep track
>>> of similar allocations.
>>>
>>> I think maybe naming that tag as 'internal' is a bit of a misnomer and I could
>>> pick one that more accurately represents its meaning? Something like 'file-internal'
>>> or else 'file-private'.
>>>
>>> Regarding a), I don't think where the allocations happen (system or device memory)
>>> is relevant in this case, just that the allocations are tied to an open file, but
>>> not exposed to UM through a DRM buffer object handle.
>>
>> On this last paragraph - right.. I possibly got confused on a). Which is why I
>> always say it is good to include example output at least in the cover letter,
>> if not the commit message.
>>
>> How would it look on this driver?
>>
>> drm-total-$what: ..
>> drm-resident-$what: ..
>> drm-internal-$what: ...
> 
> In the case of Panthor, it would look like this:
> 
> drm-driver:     panthor
> drm-client-id:  3
> drm-engine-panthor:     611046570346 ns
> drm-cycles-panthor:     1172733302061
> drm-maxfreq-panthor:    1000000000 Hz
> drm-curfreq-panthor:    1000000000 Hz
> drm-total-memory:       16480 KiB
> drm-shared-memory:      0
> drm-active-memory:      16200 KiB
> drm-internal-memory:    10396 KiB
> drm-resident-memory:    26876 KiB
> drm-purgeable-memory:   0
> 
> Then in Panfrost:
> 
> drm-driver:     panfrost
> drm-client-id:  6
> drm-engine-fragment:    481941638 ns
> drm-cycles-fragment:    60243117
> drm-maxfreq-fragment:   799999987 Hz
> drm-curfreq-fragment:   124999998 Hz
> drm-engine-vertex-tiler:        55546675 ns
> drm-cycles-vertex-tiler:        6943796
> drm-maxfreq-vertex-tiler:       799999987 Hz
> drm-curfreq-vertex-tiler:       124999998 Hz
> drm-total-memory:       138420 KiB
> drm-shared-memory:      7200 KiB
> drm-active-memory:      0
> drm-internal-memory:    0
> drm-resident-memory:    2196 KiB
> drm-purgeable-memory:   128 KiB
> 
> 
>> b) still stands though in that internal can be resident or not, purgeable or
>> not.. Which is why I would like to know about the use case.
> 
> This is true, DRM file-internal objects or memory allocations could fall
> into any of these categories, and adding their sizes to the right one would
> be the responsibility of the function pointer passed to drm_show_memory_stats(),
> because that decision would have to be made on a per-driver basis.

It could work like that yes.

I only wonder if it would creating too much of the usual DRM mess of 
common helpers and vfuncs, and vfuncs implemented via common helpers, 
which call vfuncs, implemented by common helpers.. mental stack overflow.

I also need to go back to one of the early threads on fdinfo to remind 
myself what was my counter proposal to the current design. In case it 
would be more clearly more elegant at this point.

But anyway, first the core question of whether we really need the 
internal separated out.

>> Also if you add drm-internal for any driver calling drm_print_memory_stats I
>> think you "break" at least i915. There internal objects are already accounted
>> in the existing categories. And printing out internal with zero would be very
>> misleading.
> 
> I wasn't aware of this. So i915 is already doing this kind of accounting for internal

Xe and amddgpu too, at least. So I guess the general principle when 
proposing extensions is to have a browse around all current users.

> memory allocations. In that case, maybe printing of the 'drm-internal-memory' could
> be done conditionally when it's greater than 0 to avoid 'breaking' existing drivers,
> or else renaming it to 'drm-file-memory' would be seen as less invasive?

Skipping the print maybe yeah. Or even better a supported status flag as 
is for resident and purgeable. But first the discussion on justifying 
internal as a separate thing.

> I'm asking this because if, at the end of the day, making this change part of the
> drm fdinfo core is going to clash with existing accounting in other DRM drivers, perhaps
> it'd be easier to keep it Panthor-specific and add that tag together with its meaning
> to Documentation/gpu/panfrost.rst.
> 
> I thought about this at first, but it also struck me as something other drivers might
> want to do in the future in a sort of unified way, since internal allocations happening
> in response to an ioctl() is a common thing.

I agree internal allocations should generally be accounted. Question is 
does it need to be separately.

Regards,

Tvrtko

> 
> Cheers,
> Adrian
> 
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> Regards,
>>> Adrian
>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>>
>>>>>     };
>>>>>     enum drm_gem_object_status;
>>>>> +typedef void (*internal_bos)(struct drm_memory_stats *status,
>>>>> +			     struct drm_file *file);
>>>>> +
>>>>>     void drm_print_memory_stats(struct drm_printer *p,
>>>>>     			    const struct drm_memory_stats *stats,
>>>>>     			    enum drm_gem_object_status supported_status,
>>>>>     			    const char *region);
>>>>> -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
>>>>> +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func);
>>>>>     void drm_show_fdinfo(struct seq_file *m, struct file *f);
>>>>>     struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);
> 

Re: [RFC PATCH 1/2] drm/drm_file: Add display of driver's internal memory size

[Adrián Larumbe]

On 16.10.2024 09:07, Tvrtko Ursulin wrote:
> 
> On 15/10/2024 20:05, Adrián Larumbe wrote:
> > Hi Tvrtko,
> > 
> > On 10.10.2024 10:50, Tvrtko Ursulin wrote:
> > > 
> > > On 09/10/2024 23:55, Adrián Larumbe wrote:
> > > > Hi Tvrtko,
> > > > 
> > > > On 04.10.2024 14:41, Tvrtko Ursulin wrote:
> > > > > 
> > > > > Hi Adrian,
> > > > > 
> > > > > On 03/10/2024 00:45, Adrián Larumbe wrote:
> > > > > > Some drivers must allocate a considerable amount of memory for bookkeeping
> > > > > > structures and GPU's MCU-kernel shared communication regions. These are
> > > > > > often created as a result of the invocation of the driver's ioctl()
> > > > > > interface functions, so it is sensible to consider them as being owned by
> > > > > > the render context associated with an open drm file.
> > > > > > 
> > > > > > However, at the moment drm_show_memory_stats only traverses the UM-exposed
> > > > > > drm objects for which a handle exists. Private driver objects and memory
> > > > > > regions, though connected to a render context, are unaccounted for in their
> > > > > > fdinfo numbers.
> > > > > > 
> > > > > > Add a new drm_memory_stats 'internal' memory category.
> > > > > > 
> > > > > > Because deciding what constitutes an 'internal' object and where to find
> > > > > > these are driver-dependent, calculation of this size must be done through a
> > > > > > driver-provided function pointer, which becomes the third argument of
> > > > > > drm_show_memory_stats. Drivers which have no interest in exposing the size
> > > > > > of internal memory objects can keep passing NULL for unaltered behaviour.
> > > > > > 
> > > > > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > > > > Cc: Rob Clark <robdclark@gmail.com>
> > > > > > Cc: Tvrtko Ursulin <tursulin@ursulin.net>
> > > > > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > > ---
> > > > > >     drivers/gpu/drm/drm_file.c              | 6 +++++-
> > > > > >     drivers/gpu/drm/msm/msm_drv.c           | 2 +-
> > > > > >     drivers/gpu/drm/panfrost/panfrost_drv.c | 2 +-
> > > > > >     drivers/gpu/drm/v3d/v3d_drv.c           | 2 +-
> > > > > >     include/drm/drm_file.h                  | 7 ++++++-
> > > > > >     5 files changed, 14 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > > > > index ad1dc638c83b..937471339c9a 100644
> > > > > > --- a/drivers/gpu/drm/drm_file.c
> > > > > > +++ b/drivers/gpu/drm/drm_file.c
> > > > > > @@ -856,6 +856,7 @@ void drm_print_memory_stats(struct drm_printer *p,
> > > > > >     	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(p, "internal", region, stats->internal);
> > > > > >     	if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> > > > > >     		print_size(p, "resident", region, stats->resident);
> > > > > > @@ -873,7 +874,7 @@ EXPORT_SYMBOL(drm_print_memory_stats);
> > > > > >      * Helper to iterate over GEM objects with a handle allocated in the specified
> > > > > >      * file.
> > > > > >      */
> > > > > > -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> > > > > > +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func)
> > > > > >     {
> > > > > >     	struct drm_gem_object *obj;
> > > > > >     	struct drm_memory_stats status = {};
> > > > > > @@ -919,6 +920,9 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> > > > > >     	}
> > > > > >     	spin_unlock(&file->table_lock);
> > > > > > +	if (func)
> > > > > > +		func(&status, file);
> > > > > > +
> > > > > >     	drm_print_memory_stats(p, &status, supported_status, "memory");
> > > > > >     }
> > > > > >     EXPORT_SYMBOL(drm_show_memory_stats);
> > > > > > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > index edbc1ab0fbc8..2b3feb79afc4 100644
> > > > > > --- a/drivers/gpu/drm/msm/msm_drv.c
> > > > > > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > > > > > @@ -880,7 +880,7 @@ static void msm_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> > > > > >     	msm_gpu_show_fdinfo(priv->gpu, file->driver_priv, p);
> > > > > > -	drm_show_memory_stats(p, file);
> > > > > > +	drm_show_memory_stats(p, file, NULL);
> > > > > >     }
> > > > > >     static const struct file_operations fops = {
> > > > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > > > index 04d615df5259..aaa8602bf00d 100644
> > > > > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > > > @@ -609,7 +609,7 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> > > > > >     	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
> > > > > > -	drm_show_memory_stats(p, file);
> > > > > > +	drm_show_memory_stats(p, file, NULL);
> > > > > >     }
> > > > > >     static const struct file_operations panfrost_drm_driver_fops = {
> > > > > > diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c
> > > > > > index fb35c5c3f1a7..314e77c67972 100644
> > > > > > --- a/drivers/gpu/drm/v3d/v3d_drv.c
> > > > > > +++ b/drivers/gpu/drm/v3d/v3d_drv.c
> > > > > > @@ -195,7 +195,7 @@ static void v3d_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> > > > > >     			   v3d_queue_to_string(queue), jobs_completed);
> > > > > >     	}
> > > > > > -	drm_show_memory_stats(p, file);
> > > > > > +	drm_show_memory_stats(p, file, NULL);
> > > > > >     }
> > > > > >     static const struct file_operations v3d_drm_fops = {
> > > > > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > > > > > index 8c0030c77308..661d00d5350e 100644
> > > > > > --- a/include/drm/drm_file.h
> > > > > > +++ b/include/drm/drm_file.h
> > > > > > @@ -469,6 +469,7 @@ void drm_send_event_timestamp_locked(struct drm_device *dev,
> > > > > >      * @resident: Total size of GEM objects backing pages
> > > > > >      * @purgeable: Total size of GEM objects that can be purged (resident and not active)
> > > > > >      * @active: Total size of GEM objects active on one or more engines
> > > > > > + * @internal: Total size of GEM objects that aren't exposed to user space
> > > > > >      *
> > > > > >      * Used by drm_print_memory_stats()
> > > > > >      */
> > > > > > @@ -478,16 +479,20 @@ struct drm_memory_stats {
> > > > > >     	u64 resident;
> > > > > >     	u64 purgeable;
> > > > > >     	u64 active;
> > > > > > +	u64 internal;
> > > > > 
> > > > > So equally as in the last round of discussion back in June, internal in my
> > > > > mind still does not fit alongside the categories.
> > > > > 
> > > > > Reason is that in some drivers, at least such as i915, "internal" can be:
> > > > > 
> > > > > a) Backed by either system memory or device memory - so this does not provice
> > > > > that visibility;
> > > > > 
> > > > > b) They can also be resident or not, active or not, etc - so from that angle
> > > > > it also does not fit.
> > > > > 
> > > > > Do you lose anything if you add the internal objects into their respective
> > > > > regions and under the existing categories? Like do you have an use case in
> > > > > mind which needs to be able to distinguish between userspace and internal, or
> > > > > the problem simply is internal are unaccounted for?
> > > > 
> > > > The main use case we have in mind is exposing the size of driver buffer
> > > > allocations that are triggered in respone to an ioctl(), and so linked to an
> > > 
> > > Most of this and below is old and clear - but to this specific point - so you
> > > do have an use case which specifically wants to know about the internal
> > > allocations separately from the rest? Could you describe what it is?
> 
> What about this?

I think the most obvious case would be accounting the size of tiler heap chunks. These
are created as a response to OOM events when the device's tiling engine runs out of
scratch space. Even though tiler heap growth events are kicked by the scheduler, they
are bound to the DRM context for which the heap had first been created. We would like
to keep a separate count of the size of these objects, together with the ones I described
in the commit messages and before on this thread.

> > > > open file. I gave a summary of what these could be in the patch description, but
> > > > in Panthor's case all these allocations are done with drm shmem functions
> > > > because it makes it easier to retrieve the sgtable that gives us their system
> > > > memory layout so that we can more easily map them onto the MMU's address space
> > > > for a Pantor VM. These BO's, though managed by the drm shmem API, are never
> > > > added to the open file list of user-exposed drm objects but we would still like
> > > > to tell UM how much memory they take up.
> > > > 
> > > > In the case of Panthor, they all add into the resident tally because all these
> > > > internal BO's are immediately pinned so that they can also be accessed by the
> > > > HW, but it doesn't have to be so for other drivers which might also keep track
> > > > of similar allocations.
> > > > 
> > > > I think maybe naming that tag as 'internal' is a bit of a misnomer and I could
> > > > pick one that more accurately represents its meaning? Something like 'file-internal'
> > > > or else 'file-private'.
> > > > 
> > > > Regarding a), I don't think where the allocations happen (system or device memory)
> > > > is relevant in this case, just that the allocations are tied to an open file, but
> > > > not exposed to UM through a DRM buffer object handle.
> > > 
> > > On this last paragraph - right.. I possibly got confused on a). Which is why I
> > > always say it is good to include example output at least in the cover letter,
> > > if not the commit message.
> > > 
> > > How would it look on this driver?
> > > 
> > > drm-total-$what: ..
> > > drm-resident-$what: ..
> > > drm-internal-$what: ...
> > 
> > In the case of Panthor, it would look like this:
> > 
> > drm-driver:     panthor
> > drm-client-id:  3
> > drm-engine-panthor:     611046570346 ns
> > drm-cycles-panthor:     1172733302061
> > drm-maxfreq-panthor:    1000000000 Hz
> > drm-curfreq-panthor:    1000000000 Hz
> > drm-total-memory:       16480 KiB
> > drm-shared-memory:      0
> > drm-active-memory:      16200 KiB
> > drm-internal-memory:    10396 KiB
> > drm-resident-memory:    26876 KiB
> > drm-purgeable-memory:   0
> > 
> > Then in Panfrost:
> > 
> > drm-driver:     panfrost
> > drm-client-id:  6
> > drm-engine-fragment:    481941638 ns
> > drm-cycles-fragment:    60243117
> > drm-maxfreq-fragment:   799999987 Hz
> > drm-curfreq-fragment:   124999998 Hz
> > drm-engine-vertex-tiler:        55546675 ns
> > drm-cycles-vertex-tiler:        6943796
> > drm-maxfreq-vertex-tiler:       799999987 Hz
> > drm-curfreq-vertex-tiler:       124999998 Hz
> > drm-total-memory:       138420 KiB
> > drm-shared-memory:      7200 KiB
> > drm-active-memory:      0
> > drm-internal-memory:    0
> > drm-resident-memory:    2196 KiB
> > drm-purgeable-memory:   128 KiB
> > 
> > 
> > > b) still stands though in that internal can be resident or not, purgeable or
> > > not.. Which is why I would like to know about the use case.
> > 
> > This is true, DRM file-internal objects or memory allocations could fall
> > into any of these categories, and adding their sizes to the right one would
> > be the responsibility of the function pointer passed to drm_show_memory_stats(),
> > because that decision would have to be made on a per-driver basis.
> 
> It could work like that yes.
> 
> I only wonder if it would creating too much of the usual DRM mess of common
> helpers and vfuncs, and vfuncs implemented via common helpers, which call
> vfuncs, implemented by common helpers.. mental stack overflow.
> 
> I also need to go back to one of the early threads on fdinfo to remind myself
> what was my counter proposal to the current design. In case it would be more
> clearly more elegant at this point.
> 
> But anyway, first the core question of whether we really need the internal
> separated out.

In the case of Panthor, we definitely do, or at least that's the consensus we had
struck at Collabora. However, if there's no interest in having this generalised onto
other drivers, then perhaps the best way forward would be keeping this drm fdinfo
key private for Panthor, and maybe discuss whether it would make sense to expand it
to the DRM fdinfo core in the future.    

> > > Also if you add drm-internal for any driver calling drm_print_memory_stats I
> > > think you "break" at least i915. There internal objects are already accounted
> > > in the existing categories. And printing out internal with zero would be very
> > > misleading.
> > 
> > I wasn't aware of this. So i915 is already doing this kind of accounting for internal
> 
> Xe and amddgpu too, at least. So I guess the general principle when proposing
> extensions is to have a browse around all current users.
> 
> > memory allocations. In that case, maybe printing of the 'drm-internal-memory' could
> > be done conditionally when it's greater than 0 to avoid 'breaking' existing drivers,
> > or else renaming it to 'drm-file-memory' would be seen as less invasive?
> 
> Skipping the print maybe yeah. Or even better a supported status flag as is
> for resident and purgeable. But first the discussion on justifying internal as
> a separate thing.
> 
> > I'm asking this because if, at the end of the day, making this change part of the
> > drm fdinfo core is going to clash with existing accounting in other DRM drivers, perhaps
> > it'd be easier to keep it Panthor-specific and add that tag together with its meaning
> > to Documentation/gpu/panfrost.rst.
> > 
> > I thought about this at first, but it also struck me as something other drivers might
> > want to do in the future in a sort of unified way, since internal allocations happening
> > in response to an ioctl() is a common thing.
> 
> I agree internal allocations should generally be accounted. Question is does
> it need to be separately.
> 
> Regards,
> 
> Tvrtko
> 
> > 
> > Cheers,
> > Adrian
> > 
> > > Regards,
> > > 
> > > Tvrtko
> > > 
> > > > 
> > > > Regards,
> > > > Adrian
> > > > 
> > > > > Regards,
> > > > > 
> > > > > Tvrtko
> > > > > 
> > > > > >     };
> > > > > >     enum drm_gem_object_status;
> > > > > > +typedef void (*internal_bos)(struct drm_memory_stats *status,
> > > > > > +			     struct drm_file *file);
> > > > > > +
> > > > > >     void drm_print_memory_stats(struct drm_printer *p,
> > > > > >     			    const struct drm_memory_stats *stats,
> > > > > >     			    enum drm_gem_object_status supported_status,
> > > > > >     			    const char *region);
> > > > > > -void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file);
> > > > > > +void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file, internal_bos func);
> > > > > >     void drm_show_fdinfo(struct seq_file *m, struct file *f);
> > > > > >     struct file *mock_drm_getfile(struct drm_minor *minor, unsigned int flags);