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

Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> · Mon, 25 Sep 2023 09:57:50 +0100

On 22/09/2023 16:23, Steven Price wrote:
On 22/09/2023 14:53, Tvrtko Ursulin wrote:

On 22/09/2023 11:57, Adrián Larumbe wrote:
On 20.09.2023 16:40, Tvrtko Ursulin wrote:
On 20/09/2023 00:34, Adrián Larumbe 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@xxxxxxxxxxxxx>
Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
Reviewed-by: Steven Price <steven.price@xxxxxxx>
---
    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     |  4 ++
    drivers/gpu/drm/panfrost/panfrost_job.c     | 24 +++++++++
    drivers/gpu/drm/panfrost/panfrost_job.h     |  5 ++
    12 files changed, 191 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..28f7046e1b1a 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);
+    spin_lock_init(&pfdev->cycle_counter.lock);
+
        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..3c93a11deab1 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->engine_usage = &file_priv->engine_usage;
        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++) {

FWIW you could future proof this a bit by using "i <
ARRAY_SIZE(engine_names)"
and avoid maybe silent out of bounds reads if someone updates
NUM_JOB_SLOTS
and forgets about this loop. Or stick a warning of some sort.

NUM_JOB_SLOTS is actually the same as the number of engines in the
device. I decided to follow
this loop convention because that's what's being done across the
driver when manipulating
the engine queues, so I thought I'd stick to it for the sake of
consistency. Bear in mind
the loop doesn't pick up the compute-only engine because it's still
not exposed to user space.

So NUM_JOB_SLOTS cannot change, unless a new engine were introduced,
and then someone would
have to update this array accordingly.

Exactly, and until they would, here we'd have a be silent out of bound
memory access. Content of which even gets shared with userspace. ;)

I think using NUM_JOB_SLOTS here seems sensible (as Adrián points out
it's consistent with the rest of the driver). But a BUILD_BUG_ON
checking the array size is could make sense.

In reality I don't see the number of job slots ever changing - panfrost
is now for the 'old' architecture (panthor being the new driver for
later 'CSF' architecture). And even if there was a new design for
pre-CSF - it would be a very big change to the architecture: we've kept
the 3 slots all the way through even though the 3rd is never used on
most GPUs. But equally I've been wrong before ;)

Thanks for this explanation - with that it indeed isn't much need to 
robustify it.

Regards,

Tvrtko


Steve

+        drm_printf(p, "drm-engine-%s:\t%llu ns\n",
+               engine_names[i],
panfrost_priv->engine_usage.elapsed_ns[i]);
+        drm_printf(p, "drm-cycles-%s:\t%llu\n",
+               engine_names[i],
panfrost_priv->engine_usage.cycles[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);

I envisaged a link to driver specific docs at the bottom of
drm-usage-stats.rst so it would be nice if drivers would be adding those
sections and describing their private keys, engine names etc. ;)

Currently there's no panfrost.rst file under Documentation/gpu. I
guess I'll create a new
one and add the engine descriptions and meaning of drm-curfreq key.

Yeah I have to do the same for i915 in my memory stats series. :)

Regards,

Tvrtko







