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

[Steven Price]

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 ;)

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