[PATCH v4 0/6] Add fdinfo support to Panfrost

[PATCH v4 0/6] Add fdinfo support to Panfrost

[Adrián Larumbe]

This patch series adds fdinfo support to the Panfrost DRM driver. It will
display a series of key:value pairs under /proc/pid/fdinfo/fd for render
processes that open the Panfrost DRM file.

The pairs contain basic drm gpu engine and memory region information that
can either be cat by a privileged user or accessed with IGT's gputop
utility.

Changelog:

v1: https://lore.kernel.org/lkml/bb52b872-e41b-3894-285e-b52cfc849782@arm.com/T/

v2: https://lore.kernel.org/lkml/20230901084457.5bc1ad69@collabora.com/T/
 - Changed the way gpu cycles and engine time are calculated, using GPU
 registers and taking into account potential resets.
 - Split render engine values into fragment and vertex/tiler ones.
 - Added more fine-grained calculation of RSS size for BO's.
 - Implemente selection of drm-memory region size units
 - Removed locking of shrinker's mutex in GEM obj status function

v3: https://lore.kernel.org/lkml/20230905184533.959171-1-adrian.larumbe@collabora.com/
 - Changed fdinfo engine names to something more descriptive
 - Mentioned GPU cycle counts aren't an exact measure
 - Handled the case when job->priv might be NULL
 - Handled 32 bit overflow of cycle register
 - Kept fdinfo drm memory stats size unit display within 10k times the
   previous multiplier for more accurate BO size numbers
 - Removed special handling of Prime imported BO RSS
 - Use rss_size only for heap objects
 - Use bo->base.madv instead of specific purgeable flag
 - Fixed kernel test robot warnings

v4:
 - Move cycle counter get and put to panfrost_job_hw_submit and
   panfrost_job_handle_{err,done} for more accuracy
 - Make sure cycle counter refs are released in reset path
 - Drop the model param for toggling cycle counting and do
   leave it down to the debugfs file
 - Don't disable cycle counter when togglint debugfs file,
   let refcounting logic handle it instead.
 - Remove fdinfo data nested structure definion and 'names' field
 - When incrementing BO RSS size in GPU MMU page fault IRQ handler, assume
  granuality of 2MiB for every successful mapping.
 - drm-file picks an fdinfo memory object size unit that doesn't lose precision.

Adrián Larumbe (6):
  drm/panfrost: Add cycle count GPU register definitions
  drm/panfrost: Add fdinfo support GPU load metrics
  drm/panfrost: Add fdinfo support for memory stats
  drm/drm_file: Add DRM obj's RSS reporting function for fdinfo
  drm/panfrost: Implement generic DRM object RSS reporting function
  drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats

 drivers/gpu/drm/drm_file.c                  |  7 ++-
 drivers/gpu/drm/panfrost/Makefile           |  2 +
 drivers/gpu/drm/panfrost/panfrost_debugfs.c | 20 +++++++
 drivers/gpu/drm/panfrost/panfrost_debugfs.h | 13 +++++
 drivers/gpu/drm/panfrost/panfrost_devfreq.c |  8 +++
 drivers/gpu/drm/panfrost/panfrost_devfreq.h |  3 ++
 drivers/gpu/drm/panfrost/panfrost_device.c  |  2 +
 drivers/gpu/drm/panfrost/panfrost_device.h  | 13 +++++
 drivers/gpu/drm/panfrost/panfrost_drv.c     | 59 ++++++++++++++++++++-
 drivers/gpu/drm/panfrost/panfrost_gem.c     | 29 ++++++++++
 drivers/gpu/drm/panfrost/panfrost_gem.h     |  5 ++
 drivers/gpu/drm/panfrost/panfrost_gpu.c     | 41 ++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.h     |  6 +++
 drivers/gpu/drm/panfrost/panfrost_job.c     | 39 ++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_job.h     |  5 ++
 drivers/gpu/drm/panfrost/panfrost_mmu.c     |  1 +
 drivers/gpu/drm/panfrost/panfrost_regs.h    |  5 ++
 include/drm/drm_gem.h                       |  9 ++++
 18 files changed, 264 insertions(+), 3 deletions(-)
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h


base-commit: f45acf7acf75921c0409d452f0165f51a19a74fd
-- 
2.42.0

[PATCH v4 1/6] drm/panfrost: Add cycle count GPU register definitions

[Adrián Larumbe]

These GPU registers will be used when programming the cycle counter, which
we need for providing accurate fdinfo drm-cycles values to user space.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panfrost/panfrost_regs.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_regs.h b/drivers/gpu/drm/panfrost/panfrost_regs.h
index 919f44ac853d..55ec807550b3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_regs.h
+++ b/drivers/gpu/drm/panfrost/panfrost_regs.h
@@ -46,6 +46,8 @@
 #define   GPU_CMD_SOFT_RESET		0x01
 #define   GPU_CMD_PERFCNT_CLEAR		0x03
 #define   GPU_CMD_PERFCNT_SAMPLE	0x04
+#define   GPU_CMD_CYCLE_COUNT_START	0x05
+#define   GPU_CMD_CYCLE_COUNT_STOP	0x06
 #define   GPU_CMD_CLEAN_CACHES		0x07
 #define   GPU_CMD_CLEAN_INV_CACHES	0x08
 #define GPU_STATUS			0x34
@@ -73,6 +75,9 @@
 #define GPU_PRFCNT_TILER_EN		0x74
 #define GPU_PRFCNT_MMU_L2_EN		0x7c
 
+#define GPU_CYCLE_COUNT_LO		0x90
+#define GPU_CYCLE_COUNT_HI		0x94
+
 #define GPU_THREAD_MAX_THREADS		0x0A0	/* (RO) Maximum number of threads per core */
 #define GPU_THREAD_MAX_WORKGROUP_SIZE	0x0A4	/* (RO) Maximum workgroup size */
 #define GPU_THREAD_MAX_BARRIER_SIZE	0x0A8	/* (RO) Maximum threads waiting at a barrier */
-- 
2.42.0

[PATCH v4 2/6] drm/panfrost: Add fdinfo support GPU load metrics

[Adrián Larumbe]

The drm-stats fdinfo tags made available to user space are drm-engine,
drm-cycles, drm-max-freq and drm-curfreq, one per job slot.

This deviates from standard practice in other DRM drivers, where a single
set of key:value pairs is provided for the whole render engine. However,
Panfrost has separate queues for fragment and vertex/tiler jobs, so a
decision was made to calculate bus cycles and workload times separately.

Maximum operating frequency is calculated at devfreq initialisation time.
Current frequency is made available to user space because nvtop uses it
when performing engine usage calculations.

It is important to bear in mind that both GPU cycle and kernel time numbers
provided are at best rough estimations, and always reported in excess from
the actual figure because of two reasons:
 - Excess time because of the delay between the end of a job processing,
   the subsequent job IRQ and the actual time of the sample.
 - Time spent in the engine queue waiting for the GPU to pick up the next
   job.

To avoid race conditions during enablement/disabling, a reference counting
mechanism was introduced, and a job flag that tells us whether a given job
increased the refcount. This is necessary, because user space can toggle
cycle counting through a debugfs file, and a given job might have been in
flight by the time cycle counting was disabled.

The main goal of the debugfs cycle counter knob is letting tools like nvtop
or IGT's gputop switch it at any time, to avoid power waste in case no
engine usage measuring is necessary.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/Makefile           |  2 +
 drivers/gpu/drm/panfrost/panfrost_debugfs.c | 20 ++++++++
 drivers/gpu/drm/panfrost/panfrost_debugfs.h | 13 +++++
 drivers/gpu/drm/panfrost/panfrost_devfreq.c |  8 +++
 drivers/gpu/drm/panfrost/panfrost_devfreq.h |  3 ++
 drivers/gpu/drm/panfrost/panfrost_device.c  |  2 +
 drivers/gpu/drm/panfrost/panfrost_device.h  | 13 +++++
 drivers/gpu/drm/panfrost/panfrost_drv.c     | 57 ++++++++++++++++++++-
 drivers/gpu/drm/panfrost/panfrost_gpu.c     | 41 +++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gpu.h     |  6 +++
 drivers/gpu/drm/panfrost/panfrost_job.c     | 39 ++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_job.h     |  5 ++
 12 files changed, 208 insertions(+), 1 deletion(-)
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
 create mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h

diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
index 7da2b3f02ed9..2c01c1e7523e 100644
--- a/drivers/gpu/drm/panfrost/Makefile
+++ b/drivers/gpu/drm/panfrost/Makefile
@@ -12,4 +12,6 @@ panfrost-y := \
 	panfrost_perfcnt.o \
 	panfrost_dump.o
 
+panfrost-$(CONFIG_DEBUG_FS) += panfrost_debugfs.o
+
 obj-$(CONFIG_DRM_PANFROST) += panfrost.o
diff --git a/drivers/gpu/drm/panfrost/panfrost_debugfs.c b/drivers/gpu/drm/panfrost/panfrost_debugfs.c
new file mode 100644
index 000000000000..cc14eccba206
--- /dev/null
+++ b/drivers/gpu/drm/panfrost/panfrost_debugfs.c
@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright 2023 Collabora ltd. */
+
+#include <linux/debugfs.h>
+#include <linux/platform_device.h>
+#include <drm/drm_debugfs.h>
+#include <drm/drm_file.h>
+#include <drm/panfrost_drm.h>
+
+#include "panfrost_device.h"
+#include "panfrost_gpu.h"
+#include "panfrost_debugfs.h"
+
+void panfrost_debugfs_init(struct drm_minor *minor)
+{
+	struct drm_device *dev = minor->dev;
+	struct panfrost_device *pfdev = platform_get_drvdata(to_platform_device(dev->dev));
+
+	debugfs_create_atomic_t("profile", 0600, minor->debugfs_root, &pfdev->profile_mode);
+}
diff --git a/drivers/gpu/drm/panfrost/panfrost_debugfs.h b/drivers/gpu/drm/panfrost/panfrost_debugfs.h
new file mode 100644
index 000000000000..db1c158bcf2f
--- /dev/null
+++ b/drivers/gpu/drm/panfrost/panfrost_debugfs.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2023 Collabora ltd.
+ */
+
+#ifndef PANFROST_DEBUGFS_H
+#define PANFROST_DEBUGFS_H
+
+#ifdef CONFIG_DEBUG_FS
+void panfrost_debugfs_init(struct drm_minor *minor);
+#endif
+
+#endif  /* PANFROST_DEBUGFS_H */
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 58dfb15a8757..28caffc689e2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -58,6 +58,7 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
 	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
 
 	panfrost_devfreq_update_utilization(pfdevfreq);
+	pfdevfreq->current_frequency = status->current_frequency;
 
 	status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
 						   pfdevfreq->idle_time));
@@ -117,6 +118,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	struct devfreq *devfreq;
 	struct thermal_cooling_device *cooling;
 	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
+	unsigned long freq = ULONG_MAX;
 
 	if (pfdev->comp->num_supplies > 1) {
 		/*
@@ -172,6 +174,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 		return ret;
 	}
 
+	/* Find the fastest defined rate  */
+	opp = dev_pm_opp_find_freq_floor(dev, &freq);
+	if (IS_ERR(opp))
+		return PTR_ERR(opp);
+	pfdevfreq->fast_rate = freq;
+
 	dev_pm_opp_put(opp);
 
 	/*
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index 1514c1f9d91c..48dbe185f206 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -19,6 +19,9 @@ struct panfrost_devfreq {
 	struct devfreq_simple_ondemand_data gov_data;
 	bool opp_of_table_added;
 
+	unsigned long current_frequency;
+	unsigned long fast_rate;
+
 	ktime_t busy_time;
 	ktime_t idle_time;
 	ktime_t time_last_update;
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index fa1a086a862b..9a140af97b24 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -207,6 +207,8 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 
 	spin_lock_init(&pfdev->as_lock);
 
+	atomic_set(&pfdev->profile_mode, false);
+
 	err = panfrost_clk_init(pfdev);
 	if (err) {
 		dev_err(pfdev->dev, "clk init failed %d\n", err);
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index b0126b9fbadc..1e85656dc2f7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -107,6 +107,7 @@ struct panfrost_device {
 	struct list_head scheduled_jobs;
 
 	struct panfrost_perfcnt *perfcnt;
+	atomic_t profile_mode;
 
 	struct mutex sched_lock;
 
@@ -121,6 +122,11 @@ struct panfrost_device {
 	struct shrinker shrinker;
 
 	struct panfrost_devfreq pfdevfreq;
+
+	struct {
+		atomic_t use_count;
+		spinlock_t lock;
+	} cycle_counter;
 };
 
 struct panfrost_mmu {
@@ -135,12 +141,19 @@ struct panfrost_mmu {
 	struct list_head list;
 };
 
+struct panfrost_engine_usage {
+	unsigned long long elapsed_ns[NUM_JOB_SLOTS];
+	unsigned long long cycles[NUM_JOB_SLOTS];
+};
+
 struct panfrost_file_priv {
 	struct panfrost_device *pfdev;
 
 	struct drm_sched_entity sched_entity[NUM_JOB_SLOTS];
 
 	struct panfrost_mmu *mmu;
+
+	struct panfrost_engine_usage engine_usage;
 };
 
 static inline struct panfrost_device *to_panfrost_device(struct drm_device *ddev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index a2ab99698ca8..2d9c115821a7 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -20,6 +20,7 @@
 #include "panfrost_job.h"
 #include "panfrost_gpu.h"
 #include "panfrost_perfcnt.h"
+#include "panfrost_debugfs.h"
 
 static bool unstable_ioctls;
 module_param_unsafe(unstable_ioctls, bool, 0600);
@@ -267,6 +268,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
 	job->requirements = args->requirements;
 	job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev);
 	job->mmu = file_priv->mmu;
+	job->priv = file_priv;
 
 	slot = panfrost_job_get_slot(job);
 
@@ -523,7 +525,55 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
 	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
 };
 
-DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
+
+static void panfrost_gpu_show_fdinfo(struct panfrost_device *pfdev,
+				     struct panfrost_file_priv *panfrost_priv,
+				     struct drm_printer *p)
+{
+	int i;
+
+	/*
+	 * IMPORTANT NOTE: drm-cycles and drm-engine measurements are not
+	 * accurate, as they only provide a rough estimation of the number of
+	 * GPU cycles and CPU time spent in a given context. This is due to two
+	 * different factors:
+	 * - Firstly, we must consider the time the CPU and then the kernel
+	 *   takes to process the GPU interrupt, which means additional time and
+	 *   GPU cycles will be added in excess to the real figure.
+	 * - Secondly, the pipelining done by the Job Manager (2 job slots per
+	 *   engine) implies there is no way to know exactly how much time each
+	 *   job spent on the GPU.
+	 */
+
+	static const char * const engine_names[] = {
+		"fragment", "vertex-tiler", "compute-only"
+	};
+
+	for (i = 0; i < NUM_JOB_SLOTS - 1; i++) {
+		drm_printf(p, "drm-engine-%s:\t%llu ns\n",
+			   engine_names[i], panfrost_priv->engine_usage.cycles[i]);
+		drm_printf(p, "drm-cycles-%s:\t%llu\n",
+			   engine_names[i], panfrost_priv->engine_usage.elapsed_ns[i]);
+		drm_printf(p, "drm-maxfreq-%s:\t%lu Hz\n",
+			   engine_names[i], pfdev->pfdevfreq.fast_rate);
+		drm_printf(p, "drm-curfreq-%s:\t%lu Hz\n",
+			   engine_names[i], pfdev->pfdevfreq.current_frequency);
+	}
+}
+
+static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
+{
+	struct drm_device *dev = file->minor->dev;
+	struct panfrost_device *pfdev = dev->dev_private;
+
+	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
+}
+
+static const struct file_operations panfrost_drm_driver_fops = {
+	.owner = THIS_MODULE,
+	DRM_GEM_FOPS,
+	.show_fdinfo = drm_show_fdinfo,
+};
 
 /*
  * Panfrost driver version:
@@ -535,6 +585,7 @@ static const struct drm_driver panfrost_drm_driver = {
 	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
 	.open			= panfrost_open,
 	.postclose		= panfrost_postclose,
+	.show_fdinfo		= panfrost_show_fdinfo,
 	.ioctls			= panfrost_drm_driver_ioctls,
 	.num_ioctls		= ARRAY_SIZE(panfrost_drm_driver_ioctls),
 	.fops			= &panfrost_drm_driver_fops,
@@ -546,6 +597,10 @@ static const struct drm_driver panfrost_drm_driver = {
 
 	.gem_create_object	= panfrost_gem_create_object,
 	.gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table,
+
+#ifdef CONFIG_DEBUG_FS
+	.debugfs_init		= panfrost_debugfs_init,
+#endif
 };
 
 static int panfrost_probe(struct platform_device *pdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
index 2faa344d89ee..f0be7e19b13e 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
@@ -73,6 +73,13 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
 	gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_MASK_ALL);
 	gpu_write(pfdev, GPU_INT_MASK, GPU_IRQ_MASK_ALL);
 
+	/*
+	 * All in-flight jobs should have released their cycle
+	 * counter references upon reset, but let us make sure
+	 */
+	if (drm_WARN_ON(pfdev->ddev, atomic_read(&pfdev->cycle_counter.use_count) != 0))
+		atomic_set(&pfdev->cycle_counter.use_count, 0);
+
 	return 0;
 }
 
@@ -321,6 +328,40 @@ static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
 		 pfdev->features.shader_present, pfdev->features.l2_present);
 }
 
+void panfrost_cycle_counter_get(struct panfrost_device *pfdev)
+{
+	if (atomic_inc_not_zero(&pfdev->cycle_counter.use_count))
+		return;
+
+	spin_lock(&pfdev->cycle_counter.lock);
+	if (atomic_inc_return(&pfdev->cycle_counter.use_count) == 1)
+		gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_START);
+	spin_unlock(&pfdev->cycle_counter.lock);
+}
+
+void panfrost_cycle_counter_put(struct panfrost_device *pfdev)
+{
+	if (atomic_add_unless(&pfdev->cycle_counter.use_count, -1, 1))
+		return;
+
+	spin_lock(&pfdev->cycle_counter.lock);
+	if (atomic_dec_return(&pfdev->cycle_counter.use_count) == 0)
+		gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_STOP);
+	spin_unlock(&pfdev->cycle_counter.lock);
+}
+
+unsigned long long panfrost_cycle_counter_read(struct panfrost_device *pfdev)
+{
+	u32 hi, lo;
+
+	do {
+		hi = gpu_read(pfdev, GPU_CYCLE_COUNT_HI);
+		lo = gpu_read(pfdev, GPU_CYCLE_COUNT_LO);
+	} while (hi != gpu_read(pfdev, GPU_CYCLE_COUNT_HI));
+
+	return ((u64)hi << 32) | lo;
+}
+
 void panfrost_gpu_power_on(struct panfrost_device *pfdev)
 {
 	int ret;
diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.h b/drivers/gpu/drm/panfrost/panfrost_gpu.h
index 468c51e7e46d..4d62e8901c79 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gpu.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gpu.h
@@ -16,6 +16,12 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev);
 void panfrost_gpu_power_on(struct panfrost_device *pfdev);
 void panfrost_gpu_power_off(struct panfrost_device *pfdev);
 
+void panfrost_stop_cycle_counter(struct panfrost_device *pfdev);
+void panfrost_cycle_counter_get(struct panfrost_device *pfdev);
+void panfrost_cycle_counter_stop(struct panfrost_device *pfdev);
+void panfrost_cycle_counter_put(struct panfrost_device *pfdev);
+unsigned long long panfrost_cycle_counter_read(struct panfrost_device *pfdev);
+
 void panfrost_gpu_amlogic_quirk(struct panfrost_device *pfdev);
 
 #endif
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 033f5e684707..b6d3c5309a8c 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -159,6 +159,23 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
 	struct panfrost_job *job = pfdev->jobs[slot][0];
 
 	WARN_ON(!job);
+	if (job->priv) {
+		if (job->is_profiled) {
+			job->priv->engine_usage.elapsed_ns[slot] +=
+				ktime_to_ns(ktime_sub(ktime_get(), job->start_time));
+			job->priv->engine_usage.cycles[slot] +=
+				panfrost_cycle_counter_read(pfdev) - job->start_cycles;
+
+			/* Reset in case the job has to be requeued */
+			job->start_time = 0;
+			/* A GPU reset puts the Cycle Counter register back to 0 */
+			job->start_cycles = atomic_read(&pfdev->reset.pending) ?
+				0 : panfrost_cycle_counter_read(pfdev);
+		}
+	} else {
+		dev_WARN(pfdev->dev, "Panfrost DRM file closed when job was in-flight\n");
+	}
+
 	pfdev->jobs[slot][0] = pfdev->jobs[slot][1];
 	pfdev->jobs[slot][1] = NULL;
 
@@ -233,6 +250,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 	subslot = panfrost_enqueue_job(pfdev, js, job);
 	/* Don't queue the job if a reset is in progress */
 	if (!atomic_read(&pfdev->reset.pending)) {
+		if (atomic_read(&pfdev->profile_mode)) {
+			panfrost_cycle_counter_get(pfdev);
+			job->is_profiled = true;
+			job->start_time = ktime_get();
+			job->start_cycles = panfrost_cycle_counter_read(pfdev);
+		}
+
 		job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
 		dev_dbg(pfdev->dev,
 			"JS: Submitting atom %p to js[%d][%d] with head=0x%llx AS %d",
@@ -404,6 +428,9 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
 	const char *exception_name = panfrost_exception_name(js_status);
 	bool signal_fence = true;
 
+	if (job->is_profiled)
+		panfrost_cycle_counter_put(job->pfdev);
+
 	if (!panfrost_exception_is_fault(js_status)) {
 		dev_dbg(pfdev->dev, "js event, js=%d, status=%s, head=0x%x, tail=0x%x",
 			js, exception_name,
@@ -453,6 +480,9 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
 static void panfrost_job_handle_done(struct panfrost_device *pfdev,
 				     struct panfrost_job *job)
 {
+	if (job->is_profiled)
+		panfrost_cycle_counter_put(job->pfdev);
+
 	/* Set ->jc to 0 to avoid re-submitting an already finished job (can
 	 * happen when we receive the DONE interrupt while doing a GPU reset).
 	 */
@@ -660,10 +690,14 @@ panfrost_reset(struct panfrost_device *pfdev,
 	 * stuck jobs. Let's make sure the PM counters stay balanced by
 	 * manually calling pm_runtime_put_noidle() and
 	 * panfrost_devfreq_record_idle() for each stuck job.
+	 * Let's also make sure the cycle counting register's refcnt is
+	 * kept balanced to prevent it from running forever
 	 */
 	spin_lock(&pfdev->js->job_lock);
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) {
+			if (pfdev->jobs[i][j]->is_profiled)
+				panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev);
 			pm_runtime_put_noidle(pfdev->dev);
 			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
 		}
@@ -842,6 +876,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
 		}
 	}
 
+	spin_lock_init(&pfdev->cycle_counter.lock);
+
 	panfrost_job_enable_interrupts(pfdev);
 
 	return 0;
@@ -926,6 +962,9 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv)
 			}
 
 			job_write(pfdev, JS_COMMAND(i), cmd);
+
+			/* Jobs can outlive their file context */
+			job->priv = NULL;
 		}
 	}
 	spin_unlock(&pfdev->js->job_lock);
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
index 8becc1ba0eb9..63bc830e057d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.h
+++ b/drivers/gpu/drm/panfrost/panfrost_job.h
@@ -32,6 +32,11 @@ struct panfrost_job {
 
 	/* Fence to be signaled by drm-sched once its done with the job */
 	struct dma_fence *render_done_fence;
+
+	struct panfrost_file_priv *priv;
+	bool is_profiled;
+	ktime_t start_time;
+	u64 start_cycles;
 };
 
 int panfrost_job_init(struct panfrost_device *pfdev);
-- 
2.42.0

Re: [PATCH v4 2/6] drm/panfrost: Add fdinfo support GPU load metrics

[Boris Brezillon]

On Tue, 12 Sep 2023 09:36:56 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> The drm-stats fdinfo tags made available to user space are drm-engine,
> drm-cycles, drm-max-freq and drm-curfreq, one per job slot.
> 
> This deviates from standard practice in other DRM drivers, where a single
> set of key:value pairs is provided for the whole render engine. However,
> Panfrost has separate queues for fragment and vertex/tiler jobs, so a
> decision was made to calculate bus cycles and workload times separately.
> 
> Maximum operating frequency is calculated at devfreq initialisation time.
> Current frequency is made available to user space because nvtop uses it
> when performing engine usage calculations.
> 
> It is important to bear in mind that both GPU cycle and kernel time numbers
> provided are at best rough estimations, and always reported in excess from
> the actual figure because of two reasons:
>  - Excess time because of the delay between the end of a job processing,
>    the subsequent job IRQ and the actual time of the sample.
>  - Time spent in the engine queue waiting for the GPU to pick up the next
>    job.
> 
> To avoid race conditions during enablement/disabling, a reference counting
> mechanism was introduced, and a job flag that tells us whether a given job
> increased the refcount. This is necessary, because user space can toggle
> cycle counting through a debugfs file, and a given job might have been in
> flight by the time cycle counting was disabled.
> 
> The main goal of the debugfs cycle counter knob is letting tools like nvtop
> or IGT's gputop switch it at any time, to avoid power waste in case no
> engine usage measuring is necessary.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

Mostly happy with this version, once you've my new comments you can add

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panfrost/Makefile           |  2 +
>  drivers/gpu/drm/panfrost/panfrost_debugfs.c | 20 ++++++++
>  drivers/gpu/drm/panfrost/panfrost_debugfs.h | 13 +++++
>  drivers/gpu/drm/panfrost/panfrost_devfreq.c |  8 +++
>  drivers/gpu/drm/panfrost/panfrost_devfreq.h |  3 ++
>  drivers/gpu/drm/panfrost/panfrost_device.c  |  2 +
>  drivers/gpu/drm/panfrost/panfrost_device.h  | 13 +++++
>  drivers/gpu/drm/panfrost/panfrost_drv.c     | 57 ++++++++++++++++++++-
>  drivers/gpu/drm/panfrost/panfrost_gpu.c     | 41 +++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_gpu.h     |  6 +++
>  drivers/gpu/drm/panfrost/panfrost_job.c     | 39 ++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_job.h     |  5 ++
>  12 files changed, 208 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
>  create mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h
> 
> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> index 7da2b3f02ed9..2c01c1e7523e 100644
> --- a/drivers/gpu/drm/panfrost/Makefile
> +++ b/drivers/gpu/drm/panfrost/Makefile
> @@ -12,4 +12,6 @@ panfrost-y := \
>  	panfrost_perfcnt.o \
>  	panfrost_dump.o
>  
> +panfrost-$(CONFIG_DEBUG_FS) += panfrost_debugfs.o
> +
>  obj-$(CONFIG_DRM_PANFROST) += panfrost.o
> diff --git a/drivers/gpu/drm/panfrost/panfrost_debugfs.c b/drivers/gpu/drm/panfrost/panfrost_debugfs.c
> new file mode 100644
> index 000000000000..cc14eccba206
> --- /dev/null
> +++ b/drivers/gpu/drm/panfrost/panfrost_debugfs.c
> @@ -0,0 +1,20 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2023 Collabora ltd. */
> +
> +#include <linux/debugfs.h>
> +#include <linux/platform_device.h>
> +#include <drm/drm_debugfs.h>
> +#include <drm/drm_file.h>
> +#include <drm/panfrost_drm.h>
> +
> +#include "panfrost_device.h"
> +#include "panfrost_gpu.h"
> +#include "panfrost_debugfs.h"
> +
> +void panfrost_debugfs_init(struct drm_minor *minor)
> +{
> +	struct drm_device *dev = minor->dev;
> +	struct panfrost_device *pfdev = platform_get_drvdata(to_platform_device(dev->dev));
> +
> +	debugfs_create_atomic_t("profile", 0600, minor->debugfs_root, &pfdev->profile_mode);
> +}
> diff --git a/drivers/gpu/drm/panfrost/panfrost_debugfs.h b/drivers/gpu/drm/panfrost/panfrost_debugfs.h
> new file mode 100644
> index 000000000000..db1c158bcf2f
> --- /dev/null
> +++ b/drivers/gpu/drm/panfrost/panfrost_debugfs.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2023 Collabora ltd.
> + */
> +
> +#ifndef PANFROST_DEBUGFS_H
> +#define PANFROST_DEBUGFS_H
> +
> +#ifdef CONFIG_DEBUG_FS
> +void panfrost_debugfs_init(struct drm_minor *minor);
> +#endif
> +
> +#endif  /* PANFROST_DEBUGFS_H */
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 58dfb15a8757..28caffc689e2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -58,6 +58,7 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
>  	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
>  
>  	panfrost_devfreq_update_utilization(pfdevfreq);
> +	pfdevfreq->current_frequency = status->current_frequency;
>  
>  	status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
>  						   pfdevfreq->idle_time));
> @@ -117,6 +118,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  	struct devfreq *devfreq;
>  	struct thermal_cooling_device *cooling;
>  	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
> +	unsigned long freq = ULONG_MAX;
>  
>  	if (pfdev->comp->num_supplies > 1) {
>  		/*
> @@ -172,6 +174,12 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>  		return ret;
>  	}
>  
> +	/* Find the fastest defined rate  */
> +	opp = dev_pm_opp_find_freq_floor(dev, &freq);
> +	if (IS_ERR(opp))
> +		return PTR_ERR(opp);
> +	pfdevfreq->fast_rate = freq;
> +
>  	dev_pm_opp_put(opp);
>  
>  	/*
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index 1514c1f9d91c..48dbe185f206 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -19,6 +19,9 @@ struct panfrost_devfreq {
>  	struct devfreq_simple_ondemand_data gov_data;
>  	bool opp_of_table_added;
>  
> +	unsigned long current_frequency;
> +	unsigned long fast_rate;
> +
>  	ktime_t busy_time;
>  	ktime_t idle_time;
>  	ktime_t time_last_update;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index fa1a086a862b..9a140af97b24 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -207,6 +207,8 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>  
>  	spin_lock_init(&pfdev->as_lock);
>  
> +	atomic_set(&pfdev->profile_mode, false);

Atomics should be initialized to 0 thanks to kzalloc().

> +
>  	err = panfrost_clk_init(pfdev);
>  	if (err) {
>  		dev_err(pfdev->dev, "clk init failed %d\n", err);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index b0126b9fbadc..1e85656dc2f7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -107,6 +107,7 @@ struct panfrost_device {
>  	struct list_head scheduled_jobs;
>  
>  	struct panfrost_perfcnt *perfcnt;
> +	atomic_t profile_mode;
>  
>  	struct mutex sched_lock;
>  
> @@ -121,6 +122,11 @@ struct panfrost_device {
>  	struct shrinker shrinker;
>  
>  	struct panfrost_devfreq pfdevfreq;
> +
> +	struct {
> +		atomic_t use_count;
> +		spinlock_t lock;
> +	} cycle_counter;
>  };
>  
>  struct panfrost_mmu {
> @@ -135,12 +141,19 @@ struct panfrost_mmu {
>  	struct list_head list;
>  };
>  
> +struct panfrost_engine_usage {
> +	unsigned long long elapsed_ns[NUM_JOB_SLOTS];
> +	unsigned long long cycles[NUM_JOB_SLOTS];
> +};
> +
>  struct panfrost_file_priv {
>  	struct panfrost_device *pfdev;
>  
>  	struct drm_sched_entity sched_entity[NUM_JOB_SLOTS];
>  
>  	struct panfrost_mmu *mmu;
> +
> +	struct panfrost_engine_usage engine_usage;
>  };
>  
>  static inline struct panfrost_device *to_panfrost_device(struct drm_device *ddev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index a2ab99698ca8..2d9c115821a7 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -20,6 +20,7 @@
>  #include "panfrost_job.h"
>  #include "panfrost_gpu.h"
>  #include "panfrost_perfcnt.h"
> +#include "panfrost_debugfs.h"
>  
>  static bool unstable_ioctls;
>  module_param_unsafe(unstable_ioctls, bool, 0600);
> @@ -267,6 +268,7 @@ static int panfrost_ioctl_submit(struct drm_device *dev, void *data,
>  	job->requirements = args->requirements;
>  	job->flush_id = panfrost_gpu_get_latest_flush_id(pfdev);
>  	job->mmu = file_priv->mmu;
> +	job->priv = file_priv;

As I said before, passing file_priv to the job is a bad idea because
it's not refcounted, and you can end up with jobs outliving the FD
context. I know you added NULL checks in the code, but if we start
passing panfrost_file_priv around, I'm sure we'll forget about this
constraint and end up with new code using ->priv without checking for
the NULL situation. I think I'd be more comfortable if you were passing
panfrost_engine_usage instead.

>  
>  	slot = panfrost_job_get_slot(job);
>  
> @@ -523,7 +525,55 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
>  	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
>  };
>  
> -DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
> +
> +static void panfrost_gpu_show_fdinfo(struct panfrost_device *pfdev,
> +				     struct panfrost_file_priv *panfrost_priv,
> +				     struct drm_printer *p)
> +{
> +	int i;
> +
> +	/*
> +	 * IMPORTANT NOTE: drm-cycles and drm-engine measurements are not
> +	 * accurate, as they only provide a rough estimation of the number of
> +	 * GPU cycles and CPU time spent in a given context. This is due to two
> +	 * different factors:
> +	 * - Firstly, we must consider the time the CPU and then the kernel
> +	 *   takes to process the GPU interrupt, which means additional time and
> +	 *   GPU cycles will be added in excess to the real figure.
> +	 * - Secondly, the pipelining done by the Job Manager (2 job slots per
> +	 *   engine) implies there is no way to know exactly how much time each
> +	 *   job spent on the GPU.
> +	 */
> +
> +	static const char * const engine_names[] = {
> +		"fragment", "vertex-tiler", "compute-only"
> +	};
> +
> +	for (i = 0; i < NUM_JOB_SLOTS - 1; i++) {
> +		drm_printf(p, "drm-engine-%s:\t%llu ns\n",
> +			   engine_names[i], panfrost_priv->engine_usage.cycles[i]);
> +		drm_printf(p, "drm-cycles-%s:\t%llu\n",
> +			   engine_names[i], panfrost_priv->engine_usage.elapsed_ns[i]);
> +		drm_printf(p, "drm-maxfreq-%s:\t%lu Hz\n",
> +			   engine_names[i], pfdev->pfdevfreq.fast_rate);
> +		drm_printf(p, "drm-curfreq-%s:\t%lu Hz\n",
> +			   engine_names[i], pfdev->pfdevfreq.current_frequency);
> +	}
> +}
> +
> +static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
> +{
> +	struct drm_device *dev = file->minor->dev;
> +	struct panfrost_device *pfdev = dev->dev_private;
> +
> +	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
> +}
> +
> +static const struct file_operations panfrost_drm_driver_fops = {
> +	.owner = THIS_MODULE,
> +	DRM_GEM_FOPS,
> +	.show_fdinfo = drm_show_fdinfo,
> +};
>  
>  /*
>   * Panfrost driver version:
> @@ -535,6 +585,7 @@ static const struct drm_driver panfrost_drm_driver = {
>  	.driver_features	= DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ,
>  	.open			= panfrost_open,
>  	.postclose		= panfrost_postclose,
> +	.show_fdinfo		= panfrost_show_fdinfo,
>  	.ioctls			= panfrost_drm_driver_ioctls,
>  	.num_ioctls		= ARRAY_SIZE(panfrost_drm_driver_ioctls),
>  	.fops			= &panfrost_drm_driver_fops,
> @@ -546,6 +597,10 @@ static const struct drm_driver panfrost_drm_driver = {
>  
>  	.gem_create_object	= panfrost_gem_create_object,
>  	.gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table,
> +
> +#ifdef CONFIG_DEBUG_FS
> +	.debugfs_init		= panfrost_debugfs_init,
> +#endif
>  };
>  
>  static int panfrost_probe(struct platform_device *pdev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> index 2faa344d89ee..f0be7e19b13e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c
> @@ -73,6 +73,13 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev)
>  	gpu_write(pfdev, GPU_INT_CLEAR, GPU_IRQ_MASK_ALL);
>  	gpu_write(pfdev, GPU_INT_MASK, GPU_IRQ_MASK_ALL);
>  
> +	/*
> +	 * All in-flight jobs should have released their cycle
> +	 * counter references upon reset, but let us make sure
> +	 */
> +	if (drm_WARN_ON(pfdev->ddev, atomic_read(&pfdev->cycle_counter.use_count) != 0))
> +		atomic_set(&pfdev->cycle_counter.use_count, 0);
> +
>  	return 0;
>  }
>  
> @@ -321,6 +328,40 @@ static void panfrost_gpu_init_features(struct panfrost_device *pfdev)
>  		 pfdev->features.shader_present, pfdev->features.l2_present);
>  }
>  
> +void panfrost_cycle_counter_get(struct panfrost_device *pfdev)
> +{
> +	if (atomic_inc_not_zero(&pfdev->cycle_counter.use_count))
> +		return;
> +
> +	spin_lock(&pfdev->cycle_counter.lock);
> +	if (atomic_inc_return(&pfdev->cycle_counter.use_count) == 1)
> +		gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_START);
> +	spin_unlock(&pfdev->cycle_counter.lock);
> +}
> +
> +void panfrost_cycle_counter_put(struct panfrost_device *pfdev)
> +{
> +	if (atomic_add_unless(&pfdev->cycle_counter.use_count, -1, 1))
> +		return;
> +
> +	spin_lock(&pfdev->cycle_counter.lock);
> +	if (atomic_dec_return(&pfdev->cycle_counter.use_count) == 0)
> +		gpu_write(pfdev, GPU_CMD, GPU_CMD_CYCLE_COUNT_STOP);
> +	spin_unlock(&pfdev->cycle_counter.lock);
> +}
> +
> +unsigned long long panfrost_cycle_counter_read(struct panfrost_device *pfdev)
> +{
> +	u32 hi, lo;
> +
> +	do {
> +		hi = gpu_read(pfdev, GPU_CYCLE_COUNT_HI);
> +		lo = gpu_read(pfdev, GPU_CYCLE_COUNT_LO);
> +	} while (hi != gpu_read(pfdev, GPU_CYCLE_COUNT_HI));
> +
> +	return ((u64)hi << 32) | lo;
> +}
> +
>  void panfrost_gpu_power_on(struct panfrost_device *pfdev)
>  {
>  	int ret;
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.h b/drivers/gpu/drm/panfrost/panfrost_gpu.h
> index 468c51e7e46d..4d62e8901c79 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gpu.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.h
> @@ -16,6 +16,12 @@ int panfrost_gpu_soft_reset(struct panfrost_device *pfdev);
>  void panfrost_gpu_power_on(struct panfrost_device *pfdev);
>  void panfrost_gpu_power_off(struct panfrost_device *pfdev);
>  
> +void panfrost_stop_cycle_counter(struct panfrost_device *pfdev);
> +void panfrost_cycle_counter_get(struct panfrost_device *pfdev);
> +void panfrost_cycle_counter_stop(struct panfrost_device *pfdev);
> +void panfrost_cycle_counter_put(struct panfrost_device *pfdev);
> +unsigned long long panfrost_cycle_counter_read(struct panfrost_device *pfdev);
> +
>  void panfrost_gpu_amlogic_quirk(struct panfrost_device *pfdev);
>  
>  #endif
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 033f5e684707..b6d3c5309a8c 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -159,6 +159,23 @@ panfrost_dequeue_job(struct panfrost_device *pfdev, int slot)
>  	struct panfrost_job *job = pfdev->jobs[slot][0];
>  
>  	WARN_ON(!job);
> +	if (job->priv) {
> +		if (job->is_profiled) {
> +			job->priv->engine_usage.elapsed_ns[slot] +=
> +				ktime_to_ns(ktime_sub(ktime_get(), job->start_time));
> +			job->priv->engine_usage.cycles[slot] +=
> +				panfrost_cycle_counter_read(pfdev) - job->start_cycles;
> +
> +			/* Reset in case the job has to be requeued */
> +			job->start_time = 0;
> +			/* A GPU reset puts the Cycle Counter register back to 0 */
> +			job->start_cycles = atomic_read(&pfdev->reset.pending) ?
> +				0 : panfrost_cycle_counter_read(pfdev);

Reading start_cycles here is useless, you'll read it again when
resubmitting. Same for the start_time, I'd expect the re-submission to
set this field.

> +		}
> +	} else {
> +		dev_WARN(pfdev->dev, "Panfrost DRM file closed when job was in-flight\n");

I'd drop this warning, it's a valid situation.

> +	}
> +
>  	pfdev->jobs[slot][0] = pfdev->jobs[slot][1];
>  	pfdev->jobs[slot][1] = NULL;
>  
> @@ -233,6 +250,13 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>  	subslot = panfrost_enqueue_job(pfdev, js, job);
>  	/* Don't queue the job if a reset is in progress */
>  	if (!atomic_read(&pfdev->reset.pending)) {
> +		if (atomic_read(&pfdev->profile_mode)) {
> +			panfrost_cycle_counter_get(pfdev);
> +			job->is_profiled = true;
> +			job->start_time = ktime_get();
> +			job->start_cycles = panfrost_cycle_counter_read(pfdev);
> +		}
> +
>  		job_write(pfdev, JS_COMMAND_NEXT(js), JS_COMMAND_START);
>  		dev_dbg(pfdev->dev,
>  			"JS: Submitting atom %p to js[%d][%d] with head=0x%llx AS %d",
> @@ -404,6 +428,9 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
>  	const char *exception_name = panfrost_exception_name(js_status);
>  	bool signal_fence = true;
>  
> +	if (job->is_profiled)
> +		panfrost_cycle_counter_put(job->pfdev);

Can we move that to panfrost_dequeue_job(), so we don't have to do it
in both panfrost_job_handle_err() and panfrost_job_handle_done()?

> +
>  	if (!panfrost_exception_is_fault(js_status)) {
>  		dev_dbg(pfdev->dev, "js event, js=%d, status=%s, head=0x%x, tail=0x%x",
>  			js, exception_name,
> @@ -453,6 +480,9 @@ static void panfrost_job_handle_err(struct panfrost_device *pfdev,
>  static void panfrost_job_handle_done(struct panfrost_device *pfdev,
>  				     struct panfrost_job *job)
>  {
> +	if (job->is_profiled)
> +		panfrost_cycle_counter_put(job->pfdev);
> +
>  	/* Set ->jc to 0 to avoid re-submitting an already finished job (can
>  	 * happen when we receive the DONE interrupt while doing a GPU reset).
>  	 */
> @@ -660,10 +690,14 @@ panfrost_reset(struct panfrost_device *pfdev,
>  	 * stuck jobs. Let's make sure the PM counters stay balanced by
>  	 * manually calling pm_runtime_put_noidle() and
>  	 * panfrost_devfreq_record_idle() for each stuck job.
> +	 * Let's also make sure the cycle counting register's refcnt is
> +	 * kept balanced to prevent it from running forever
>  	 */
>  	spin_lock(&pfdev->js->job_lock);
>  	for (i = 0; i < NUM_JOB_SLOTS; i++) {
>  		for (j = 0; j < ARRAY_SIZE(pfdev->jobs[0]) && pfdev->jobs[i][j]; j++) {
> +			if (pfdev->jobs[i][j]->is_profiled)
> +				panfrost_cycle_counter_put(pfdev->jobs[i][j]->pfdev);
>  			pm_runtime_put_noidle(pfdev->dev);
>  			panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
>  		}
> @@ -842,6 +876,8 @@ int panfrost_job_init(struct panfrost_device *pfdev)
>  		}
>  	}
>  
> +	spin_lock_init(&pfdev->cycle_counter.lock);

Ah, so this is where you're initializing the lock. Was searching for
this in panfrost_device_init().

> +
>  	panfrost_job_enable_interrupts(pfdev);
>  
>  	return 0;
> @@ -926,6 +962,9 @@ void panfrost_job_close(struct panfrost_file_priv *panfrost_priv)
>  			}
>  
>  			job_write(pfdev, JS_COMMAND(i), cmd);
> +
> +			/* Jobs can outlive their file context */
> +			job->priv = NULL;
>  		}
>  	}
>  	spin_unlock(&pfdev->js->job_lock);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> index 8becc1ba0eb9..63bc830e057d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> @@ -32,6 +32,11 @@ struct panfrost_job {
>  
>  	/* Fence to be signaled by drm-sched once its done with the job */
>  	struct dma_fence *render_done_fence;
> +
> +	struct panfrost_file_priv *priv;
> +	bool is_profiled;
> +	ktime_t start_time;
> +	u64 start_cycles;
>  };
>  
>  int panfrost_job_init(struct panfrost_device *pfdev);

[PATCH v4 3/6] drm/panfrost: Add fdinfo support for memory stats

[Adrián Larumbe]

A new DRM GEM object function is added so that drm_show_memory_stats can
provide more accurate memory usage numbers.

Ideally, in panfrost_gem_status, the BO's purgeable flag would be checked
after locking the driver's shrinker mutex, but drm_show_memory_stats takes
over the drm file's object handle database spinlock, so there's potential
for a race condition here.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c |  2 ++
 drivers/gpu/drm/panfrost/panfrost_gem.c | 14 ++++++++++++++
 2 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 2d9c115821a7..e71a89a283cd 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -567,6 +567,8 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
 	struct panfrost_device *pfdev = dev->dev_private;
 
 	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
+
+	drm_show_memory_stats(p, file);
 }
 
 static const struct file_operations panfrost_drm_driver_fops = {
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 3c812fbd126f..7d8f83d20539 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -195,6 +195,19 @@ static int panfrost_gem_pin(struct drm_gem_object *obj)
 	return drm_gem_shmem_pin(&bo->base);
 }
 
+static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj)
+{
+	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
+	enum drm_gem_object_status res = 0;
+
+	res |= (bo->base.madv == PANFROST_MADV_DONTNEED) ?
+		DRM_GEM_OBJECT_PURGEABLE : 0;
+
+	res |= (bo->base.pages) ? DRM_GEM_OBJECT_RESIDENT : 0;
+
+	return res;
+}
+
 static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.free = panfrost_gem_free_object,
 	.open = panfrost_gem_open,
@@ -206,6 +219,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.vmap = drm_gem_shmem_object_vmap,
 	.vunmap = drm_gem_shmem_object_vunmap,
 	.mmap = drm_gem_shmem_object_mmap,
+	.status = panfrost_gem_status,
 	.vm_ops = &drm_gem_shmem_vm_ops,
 };
 
-- 
2.42.0

Re: [PATCH v4 3/6] drm/panfrost: Add fdinfo support for memory stats

[Boris Brezillon]

On Tue, 12 Sep 2023 09:36:57 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> A new DRM GEM object function is added so that drm_show_memory_stats can
> provide more accurate memory usage numbers.
> 
> Ideally, in panfrost_gem_status, the BO's purgeable flag would be checked
> after locking the driver's shrinker mutex, but drm_show_memory_stats takes
> over the drm file's object handle database spinlock, so there's potential
> for a race condition here.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c |  2 ++
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 2d9c115821a7..e71a89a283cd 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -567,6 +567,8 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file)
>  	struct panfrost_device *pfdev = dev->dev_private;
>  
>  	panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p);
> +
> +	drm_show_memory_stats(p, file);
>  }
>  
>  static const struct file_operations panfrost_drm_driver_fops = {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 3c812fbd126f..7d8f83d20539 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -195,6 +195,19 @@ static int panfrost_gem_pin(struct drm_gem_object *obj)
>  	return drm_gem_shmem_pin(&bo->base);
>  }
>  
> +static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj)
> +{
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +	enum drm_gem_object_status res = 0;
> +
> +	res |= (bo->base.madv == PANFROST_MADV_DONTNEED) ?
> +		DRM_GEM_OBJECT_PURGEABLE : 0;
> +
> +	res |= (bo->base.pages) ? DRM_GEM_OBJECT_RESIDENT : 0;
> +
> +	return res;
> +}
> +
>  static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.free = panfrost_gem_free_object,
>  	.open = panfrost_gem_open,
> @@ -206,6 +219,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.vmap = drm_gem_shmem_object_vmap,
>  	.vunmap = drm_gem_shmem_object_vunmap,
>  	.mmap = drm_gem_shmem_object_mmap,
> +	.status = panfrost_gem_status,
>  	.vm_ops = &drm_gem_shmem_vm_ops,
>  };
>  

[PATCH v4 4/6] drm/drm_file: Add DRM obj's RSS reporting function for fdinfo

[Adrián Larumbe]

Some BO's might be mapped onto physical memory chunkwise and on demand,
like Panfrost's tiler heap. In this case, even though the
drm_gem_shmem_object page array might already be allocated, only a very
small fraction of the BO is currently backed by system memory, but
drm_show_memory_stats will then proceed to add its entire virtual size to
the file's total resident size regardless.

This led to very unrealistic RSS sizes being reckoned for Panfrost, where
said tiler heap buffer is initially allocated with a virtual size of 128
MiB, but only a small part of it will eventually be backed by system memory
after successive GPU page faults.

Provide a new DRM object generic function that would allow drivers to
return a more accurate RSS size for their BOs.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/drm_file.c | 5 ++++-
 include/drm/drm_gem.h      | 9 +++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 883d83bc0e3d..762965e3d503 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -944,7 +944,10 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
 		}
 
 		if (s & DRM_GEM_OBJECT_RESIDENT) {
-			status.resident += obj->size;
+			if (obj->funcs && obj->funcs->rss)
+				status.resident += obj->funcs->rss(obj);
+			else
+				status.resident += obj->size;
 		} else {
 			/* If already purged or not yet backed by pages, don't
 			 * count it as purgeable:
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index bc9f6aa2f3fe..16364487fde9 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -208,6 +208,15 @@ struct drm_gem_object_funcs {
 	 */
 	enum drm_gem_object_status (*status)(struct drm_gem_object *obj);
 
+	/**
+	 * @rss:
+	 *
+	 * Return resident size of the object in physical memory.
+	 *
+	 * Called by drm_show_memory_stats().
+	 */
+	size_t (*rss)(struct drm_gem_object *obj);
+
 	/**
 	 * @vm_ops:
 	 *
-- 
2.42.0

[PATCH v4 5/6] drm/panfrost: Implement generic DRM object RSS reporting function

[Adrián Larumbe]

BO's RSS is updated every time new pages are allocated on demand and mapped
for the object at GPU page fault's IRQ handler, but only for heap buffers.
The reason this is unnecessary for non-heap buffers is that they are mapped
onto the GPU's VA space and backed by physical memory in their entirety at
BO creation time.

This calculation is unnecessary for imported PRIME objects, since heap
buffers cannot be exported by our driver, and the actual BO RSS size is the
one reported in its attached dmabuf structure.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_gem.c | 15 +++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_gem.h |  5 +++++
 drivers/gpu/drm/panfrost/panfrost_mmu.c |  1 +
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
index 7d8f83d20539..4365434b48db 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
@@ -208,6 +208,20 @@ static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj
 	return res;
 }
 
+static size_t panfrost_gem_rss(struct drm_gem_object *obj)
+{
+	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
+
+	if (bo->is_heap) {
+		return bo->heap_rss_size;
+	} else if (bo->base.pages) {
+		WARN_ON(bo->heap_rss_size);
+		return bo->base.base.size;
+	} else {
+		return 0;
+	}
+}
+
 static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.free = panfrost_gem_free_object,
 	.open = panfrost_gem_open,
@@ -220,6 +234,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
 	.vunmap = drm_gem_shmem_object_vunmap,
 	.mmap = drm_gem_shmem_object_mmap,
 	.status = panfrost_gem_status,
+	.rss = panfrost_gem_rss,
 	.vm_ops = &drm_gem_shmem_vm_ops,
 };
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index ad2877eeeccd..13c0a8149c3a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -36,6 +36,11 @@ struct panfrost_gem_object {
 	 */
 	atomic_t gpu_usecount;
 
+	/*
+	 * Object chunk size currently mapped onto physical memory
+	 */
+	size_t heap_rss_size;
+
 	bool noexec		:1;
 	bool is_heap		:1;
 };
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index d54d4e7b2195..7b1490cdaa48 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -522,6 +522,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
 
 	bomapping->active = true;
+	bo->heap_rss_size += SZ_2;
 
 	dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", as, addr);
 
-- 
2.42.0

Re: [PATCH v4 5/6] drm/panfrost: Implement generic DRM object RSS reporting function

[Boris Brezillon]

On Tue, 12 Sep 2023 09:36:59 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> BO's RSS is updated every time new pages are allocated on demand and mapped
> for the object at GPU page fault's IRQ handler, but only for heap buffers.
> The reason this is unnecessary for non-heap buffers is that they are mapped
> onto the GPU's VA space and backed by physical memory in their entirety at
> BO creation time.
> 
> This calculation is unnecessary for imported PRIME objects, since heap
> buffers cannot be exported by our driver, and the actual BO RSS size is the
> one reported in its attached dmabuf structure.
> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/gpu/drm/panfrost/panfrost_gem.c | 15 +++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_gem.h |  5 +++++
>  drivers/gpu/drm/panfrost/panfrost_mmu.c |  1 +
>  3 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 7d8f83d20539..4365434b48db 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -208,6 +208,20 @@ static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj
>  	return res;
>  }
>  
> +static size_t panfrost_gem_rss(struct drm_gem_object *obj)
> +{
> +	struct panfrost_gem_object *bo = to_panfrost_bo(obj);
> +
> +	if (bo->is_heap) {
> +		return bo->heap_rss_size;
> +	} else if (bo->base.pages) {
> +		WARN_ON(bo->heap_rss_size);
> +		return bo->base.base.size;
> +	} else {
> +		return 0;
> +	}
> +}
> +
>  static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.free = panfrost_gem_free_object,
>  	.open = panfrost_gem_open,
> @@ -220,6 +234,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = {
>  	.vunmap = drm_gem_shmem_object_vunmap,
>  	.mmap = drm_gem_shmem_object_mmap,
>  	.status = panfrost_gem_status,
> +	.rss = panfrost_gem_rss,
>  	.vm_ops = &drm_gem_shmem_vm_ops,
>  };
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index ad2877eeeccd..13c0a8149c3a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -36,6 +36,11 @@ struct panfrost_gem_object {
>  	 */
>  	atomic_t gpu_usecount;
>  
> +	/*
> +	 * Object chunk size currently mapped onto physical memory
> +	 */
> +	size_t heap_rss_size;
> +
>  	bool noexec		:1;
>  	bool is_heap		:1;
>  };
> diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> index d54d4e7b2195..7b1490cdaa48 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
> @@ -522,6 +522,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
>  		   IOMMU_WRITE | IOMMU_READ | IOMMU_NOEXEC, sgt);
>  
>  	bomapping->active = true;
> +	bo->heap_rss_size += SZ_2;
>  
>  	dev_dbg(pfdev->dev, "mapped page fault @ AS%d %llx", as, addr);
>  

[PATCH v4 6/6] drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats

[Adrián Larumbe]

The current implementation will try to pick the highest available size
display unit as soon as the BO size exceeds that of the previous
multiplier. That can lead to loss of precision in BO's whose size is
not a multiple of a MiB.

Fix it by changing the unit selection criteria.

For much bigger BO's, their size will naturally be aligned on something
bigger than a 4 KiB page, so in practice it is very unlikely their display
unit would default to KiB.

Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
---
 drivers/gpu/drm/drm_file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 762965e3d503..bf7d2fe46bfa 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -879,7 +879,7 @@ static void print_size(struct drm_printer *p, const char *stat,
 	unsigned u;
 
 	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
-		if (sz < SZ_1K)
+		if (sz & (SZ_1K - 1))
 			break;
 		sz = div_u64(sz, SZ_1K);
 	}
-- 
2.42.0

Re: [PATCH v4 6/6] drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats

[Boris Brezillon]

On Tue, 12 Sep 2023 09:37:00 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> The current implementation will try to pick the highest available size
> display unit as soon as the BO size exceeds that of the previous
> multiplier. That can lead to loss of precision in BO's whose size is
> not a multiple of a MiB.
> 
> Fix it by changing the unit selection criteria.
> 
> For much bigger BO's, their size will naturally be aligned on something
> bigger than a 4 KiB page, so in practice it is very unlikely their display
> unit would default to KiB.

Let's wait for Rob's opinion on this.

> 
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
>  drivers/gpu/drm/drm_file.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 762965e3d503..bf7d2fe46bfa 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -879,7 +879,7 @@ static void print_size(struct drm_printer *p, const char *stat,
>  	unsigned u;
>  
>  	for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> -		if (sz < SZ_1K)
> +		if (sz & (SZ_1K - 1))
>  			break;
>  		sz = div_u64(sz, SZ_1K);
>  	}

Re: [PATCH v4 6/6] drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats

[Rob Clark]

On Tue, Sep 12, 2023 at 2:32 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Tue, 12 Sep 2023 09:37:00 +0100
> Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
>
> > The current implementation will try to pick the highest available size
> > display unit as soon as the BO size exceeds that of the previous
> > multiplier. That can lead to loss of precision in BO's whose size is
> > not a multiple of a MiB.
> >
> > Fix it by changing the unit selection criteria.
> >
> > For much bigger BO's, their size will naturally be aligned on something
> > bigger than a 4 KiB page, so in practice it is very unlikely their display
> > unit would default to KiB.
>
> Let's wait for Rob's opinion on this.

This would mean that if you have SZ_1G + SZ_1K worth of buffers, you'd
report the result in KiB.. which seems like overkill to me, esp given
that the result is just a snapshot in time of a figure that
realistically is dynamic.

Maybe if you have SZ_1G+SZ_1K worth of buffers you should report the
result with more precision than GiB, but more than MiB seems a bit
overkill.

BR,
-R

> >
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> >  drivers/gpu/drm/drm_file.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index 762965e3d503..bf7d2fe46bfa 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -879,7 +879,7 @@ static void print_size(struct drm_printer *p, const char *stat,
> >       unsigned u;
> >
> >       for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> > -             if (sz < SZ_1K)
> > +             if (sz & (SZ_1K - 1))
> >                       break;
> >               sz = div_u64(sz, SZ_1K);
> >       }
>

Re: [PATCH v4 6/6] drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats

[Rob Clark]

On Tue, Sep 12, 2023 at 6:46 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Sep 12, 2023 at 2:32 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Tue, 12 Sep 2023 09:37:00 +0100
> > Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> >
> > > The current implementation will try to pick the highest available size
> > > display unit as soon as the BO size exceeds that of the previous
> > > multiplier. That can lead to loss of precision in BO's whose size is
> > > not a multiple of a MiB.
> > >
> > > Fix it by changing the unit selection criteria.
> > >
> > > For much bigger BO's, their size will naturally be aligned on something
> > > bigger than a 4 KiB page, so in practice it is very unlikely their display
> > > unit would default to KiB.
> >
> > Let's wait for Rob's opinion on this.
>
> This would mean that if you have SZ_1G + SZ_1K worth of buffers, you'd
> report the result in KiB.. which seems like overkill to me, esp given
> that the result is just a snapshot in time of a figure that
> realistically is dynamic.
>
> Maybe if you have SZ_1G+SZ_1K worth of buffers you should report the
> result with more precision than GiB, but more than MiB seems a bit
> overkill.
>
> BR,
> -R
>
> > >
> > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > ---
> > >  drivers/gpu/drm/drm_file.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > index 762965e3d503..bf7d2fe46bfa 100644
> > > --- a/drivers/gpu/drm/drm_file.c
> > > +++ b/drivers/gpu/drm/drm_file.c
> > > @@ -879,7 +879,7 @@ static void print_size(struct drm_printer *p, const char *stat,
> > >       unsigned u;
> > >
> > >       for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> > > -             if (sz < SZ_1K)

btw, I was thinking more along the lines of:

   if (sz < 10*SZ_1K)

(or perhaps maybe 100*SZ_1K)

I mean, any visualization tool is going to scale the y axis based on
the order of magnitude.. and if I'm looking at the fdinfo with my
eyeballs I don't want to count the # of digits manually to do the
conversion in my head.  The difference btwn 4 or 5 or maybe 6 digits
is easy enough to eyeball, but more than that is too much for my
eyesight, and I'm not seeing how it is useful ;-)

But if someone really has a valid use case for having precision in 1KB
then I'm willing to be overruled.  But I'm not a fan of the earlier
approach of different drivers reporting results differently, the whole
point of fdinfo was to have some standardized reporting.

BR,
-R

> > > +             if (sz & (SZ_1K - 1))
> > >                       break;
> > >               sz = div_u64(sz, SZ_1K);
> > >       }
> >

Re: [PATCH v4 6/6] drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats

[Boris Brezillon]

On Tue, 12 Sep 2023 19:14:35 -0700
Rob Clark <robdclark@gmail.com> wrote:

> On Tue, Sep 12, 2023 at 6:46 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Tue, Sep 12, 2023 at 2:32 AM Boris Brezillon
> > <boris.brezillon@collabora.com> wrote:  
> > >
> > > On Tue, 12 Sep 2023 09:37:00 +0100
> > > Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> > >  
> > > > The current implementation will try to pick the highest available size
> > > > display unit as soon as the BO size exceeds that of the previous
> > > > multiplier. That can lead to loss of precision in BO's whose size is
> > > > not a multiple of a MiB.
> > > >
> > > > Fix it by changing the unit selection criteria.
> > > >
> > > > For much bigger BO's, their size will naturally be aligned on something
> > > > bigger than a 4 KiB page, so in practice it is very unlikely their display
> > > > unit would default to KiB.  
> > >
> > > Let's wait for Rob's opinion on this.  
> >
> > This would mean that if you have SZ_1G + SZ_1K worth of buffers, you'd
> > report the result in KiB.. which seems like overkill to me, esp given
> > that the result is just a snapshot in time of a figure that
> > realistically is dynamic.

Yeah, my point was that, generally, such big buffers tend to have
a bigger size alignment (like 2MB for anything bigger than 1GB), but
maybe this assumption doesn't stand for all drivers.

> >
> > Maybe if you have SZ_1G+SZ_1K worth of buffers you should report the
> > result with more precision than GiB, but more than MiB seems a bit
> > overkill.
> >
> > BR,
> > -R
> >  
> > > >
> > > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > > ---
> > > >  drivers/gpu/drm/drm_file.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > > index 762965e3d503..bf7d2fe46bfa 100644
> > > > --- a/drivers/gpu/drm/drm_file.c
> > > > +++ b/drivers/gpu/drm/drm_file.c
> > > > @@ -879,7 +879,7 @@ static void print_size(struct drm_printer *p, const char *stat,
> > > >       unsigned u;
> > > >
> > > >       for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> > > > -             if (sz < SZ_1K)  
> 
> btw, I was thinking more along the lines of:
> 
>    if (sz < 10*SZ_1K)
> 
> (or perhaps maybe 100*SZ_1K)

I think I suggested doing that at some point:

		if ((sz & (SZ_1K - 1)) &&
		    sz < UPPER_UNIT_THRESHOLD * SZ_1K)
			break;

so we can keep using the upper unit if the size is a multiple of this
upper unit, even if it's smaller than the selected threshold.

> 
> I mean, any visualization tool is going to scale the y axis based on
> the order of magnitude.. and if I'm looking at the fdinfo with my
> eyeballs I don't want to count the # of digits manually to do the
> conversion in my head.  The difference btwn 4 or 5 or maybe 6 digits
> is easy enough to eyeball, but more than that is too much for my
> eyesight, and I'm not seeing how it is useful ;-)
> 
> But if someone really has a valid use case for having precision in 1KB
> then I'm willing to be overruled.

So, precision loss was one aspect, but my main concern was having
things displayed in KiB when they could have been displayed in MiB,
because the size is a multiple of a MiB but still not big enough to
pass the threshold test (which was set to 10000x in the previous
version).

> But I'm not a fan of the earlier
> approach of different drivers reporting results differently, the whole
> point of fdinfo was to have some standardized reporting.

Totally agree with that.

> 
> BR,
> -R
> 
> > > > +             if (sz & (SZ_1K - 1))
> > > >                       break;
> > > >               sz = div_u64(sz, SZ_1K);
> > > >       }  
> > >  

Re: [PATCH v4 6/6] drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats

[Rob Clark]

On Wed, Sep 13, 2023 at 12:36 AM Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> On Tue, 12 Sep 2023 19:14:35 -0700
> Rob Clark <robdclark@gmail.com> wrote:
>
> > On Tue, Sep 12, 2023 at 6:46 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Tue, Sep 12, 2023 at 2:32 AM Boris Brezillon
> > > <boris.brezillon@collabora.com> wrote:
> > > >
> > > > On Tue, 12 Sep 2023 09:37:00 +0100
> > > > Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> > > >
> > > > > The current implementation will try to pick the highest available size
> > > > > display unit as soon as the BO size exceeds that of the previous
> > > > > multiplier. That can lead to loss of precision in BO's whose size is
> > > > > not a multiple of a MiB.
> > > > >
> > > > > Fix it by changing the unit selection criteria.
> > > > >
> > > > > For much bigger BO's, their size will naturally be aligned on something
> > > > > bigger than a 4 KiB page, so in practice it is very unlikely their display
> > > > > unit would default to KiB.
> > > >
> > > > Let's wait for Rob's opinion on this.
> > >
> > > This would mean that if you have SZ_1G + SZ_1K worth of buffers, you'd
> > > report the result in KiB.. which seems like overkill to me, esp given
> > > that the result is just a snapshot in time of a figure that
> > > realistically is dynamic.
>
> Yeah, my point was that, generally, such big buffers tend to have
> a bigger size alignment (like 2MB for anything bigger than 1GB), but
> maybe this assumption doesn't stand for all drivers.

Maybe for CMA?  Regardless, this # is the sum of buffer sizes, so you
could still get that 1G+1K scenario

> > >
> > > Maybe if you have SZ_1G+SZ_1K worth of buffers you should report the
> > > result with more precision than GiB, but more than MiB seems a bit
> > > overkill.
> > >
> > > BR,
> > > -R
> > >
> > > > >
> > > > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > > > ---
> > > > >  drivers/gpu/drm/drm_file.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > > > > index 762965e3d503..bf7d2fe46bfa 100644
> > > > > --- a/drivers/gpu/drm/drm_file.c
> > > > > +++ b/drivers/gpu/drm/drm_file.c
> > > > > @@ -879,7 +879,7 @@ static void print_size(struct drm_printer *p, const char *stat,
> > > > >       unsigned u;
> > > > >
> > > > >       for (u = 0; u < ARRAY_SIZE(units) - 1; u++) {
> > > > > -             if (sz < SZ_1K)
> >
> > btw, I was thinking more along the lines of:
> >
> >    if (sz < 10*SZ_1K)
> >
> > (or perhaps maybe 100*SZ_1K)
>
> I think I suggested doing that at some point:
>
>                 if ((sz & (SZ_1K - 1)) &&
>                     sz < UPPER_UNIT_THRESHOLD * SZ_1K)
>                         break;
>
> so we can keep using the upper unit if the size is a multiple of this
> upper unit, even if it's smaller than the selected threshold.

yeah, that wfm

BR,
-R

>
> >
> > I mean, any visualization tool is going to scale the y axis based on
> > the order of magnitude.. and if I'm looking at the fdinfo with my
> > eyeballs I don't want to count the # of digits manually to do the
> > conversion in my head.  The difference btwn 4 or 5 or maybe 6 digits
> > is easy enough to eyeball, but more than that is too much for my
> > eyesight, and I'm not seeing how it is useful ;-)
> >
> > But if someone really has a valid use case for having precision in 1KB
> > then I'm willing to be overruled.
>
> So, precision loss was one aspect, but my main concern was having
> things displayed in KiB when they could have been displayed in MiB,
> because the size is a multiple of a MiB but still not big enough to
> pass the threshold test (which was set to 10000x in the previous
> version).
>
> > But I'm not a fan of the earlier
> > approach of different drivers reporting results differently, the whole
> > point of fdinfo was to have some standardized reporting.
>
> Totally agree with that.
>
> >
> > BR,
> > -R
> >
> > > > > +             if (sz & (SZ_1K - 1))
> > > > >                       break;
> > > > >               sz = div_u64(sz, SZ_1K);
> > > > >       }
> > > >
>

Re: [PATCH v4 6/6] drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats

[Boris Brezillon]

On Wed, 13 Sep 2023 09:46:45 -0700
Rob Clark <robdclark@gmail.com> wrote:

> On Wed, Sep 13, 2023 at 12:36 AM Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > On Tue, 12 Sep 2023 19:14:35 -0700
> > Rob Clark <robdclark@gmail.com> wrote:
> >  
> > > On Tue, Sep 12, 2023 at 6:46 PM Rob Clark <robdclark@gmail.com> wrote:  
> > > >
> > > > On Tue, Sep 12, 2023 at 2:32 AM Boris Brezillon
> > > > <boris.brezillon@collabora.com> wrote:  
> > > > >
> > > > > On Tue, 12 Sep 2023 09:37:00 +0100
> > > > > Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> > > > >  
> > > > > > The current implementation will try to pick the highest available size
> > > > > > display unit as soon as the BO size exceeds that of the previous
> > > > > > multiplier. That can lead to loss of precision in BO's whose size is
> > > > > > not a multiple of a MiB.
> > > > > >
> > > > > > Fix it by changing the unit selection criteria.
> > > > > >
> > > > > > For much bigger BO's, their size will naturally be aligned on something
> > > > > > bigger than a 4 KiB page, so in practice it is very unlikely their display
> > > > > > unit would default to KiB.  
> > > > >
> > > > > Let's wait for Rob's opinion on this.  
> > > >
> > > > This would mean that if you have SZ_1G + SZ_1K worth of buffers, you'd
> > > > report the result in KiB.. which seems like overkill to me, esp given
> > > > that the result is just a snapshot in time of a figure that
> > > > realistically is dynamic.  
> >
> > Yeah, my point was that, generally, such big buffers tend to have
> > a bigger size alignment (like 2MB for anything bigger than 1GB), but
> > maybe this assumption doesn't stand for all drivers.  
> 
> Maybe for CMA?  Regardless, this # is the sum of buffer sizes, so you
> could still get that 1G+1K scenario

My bad, for some reason I had per-buffer size printing in mind.