* Re: [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs
2024-03-06 1:56 ` [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs Adrián Larumbe
@ 2024-03-06 8:33 ` Tvrtko Ursulin
2024-03-11 10:02 ` Boris Brezillon
2024-03-06 8:41 ` Boris Brezillon
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Tvrtko Ursulin @ 2024-03-06 8:33 UTC (permalink / raw)
To: Adrián Larumbe, boris.brezillon, robh, steven.price, airlied,
daniel, maarten.lankhorst, mripard, tzimmermann, corbet
Cc: kernel, linux-kernel, dri-devel, linux-doc
On 06/03/2024 01:56, Adrián Larumbe wrote:
> Debugfs isn't always available in production builds that try to squeeze
> every single byte out of the kernel image, but we still need a way to
> toggle the timestamp and cycle counter registers so that jobs can be
> profiled for fdinfo's drm engine and cycle calculations.
>
> Drop the debugfs knob and replace it with a sysfs file that accomplishes
> the same functionality, and document its ABI in a separate file.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> ---
> .../testing/sysfs-driver-panfrost-profiling | 10 +++++
> Documentation/gpu/panfrost.rst | 9 ++++
> drivers/gpu/drm/panfrost/Makefile | 2 -
> drivers/gpu/drm/panfrost/panfrost_debugfs.c | 21 ----------
> drivers/gpu/drm/panfrost/panfrost_debugfs.h | 14 -------
> drivers/gpu/drm/panfrost/panfrost_device.h | 2 +-
> drivers/gpu/drm/panfrost/panfrost_drv.c | 41 ++++++++++++++++---
> drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> 8 files changed, 57 insertions(+), 44 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
> delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-panfrost-profiling b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> new file mode 100644
> index 000000000000..1d8bb0978920
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> @@ -0,0 +1,10 @@
> +What: /sys/bus/platform/drivers/panfrost/.../profiling
> +Date: February 2024
> +KernelVersion: 6.8.0
> +Contact: Adrian Larumbe <adrian.larumbe@collabora.com>
> +Description:
> + Get/set drm fdinfo's engine and cycles profiling status.
> + Valid values are:
> + 0: Don't enable fdinfo job profiling sources.
> + 1: Enable fdinfo job profiling sources, this enables both the GPU's
> + timestamp and cycle counter registers.
> \ No newline at end of file
> diff --git a/Documentation/gpu/panfrost.rst b/Documentation/gpu/panfrost.rst
> index b80e41f4b2c5..51ba375fd80d 100644
> --- a/Documentation/gpu/panfrost.rst
> +++ b/Documentation/gpu/panfrost.rst
> @@ -38,3 +38,12 @@ the currently possible format options:
>
> Possible `drm-engine-` key names are: `fragment`, and `vertex-tiler`.
> `drm-curfreq-` values convey the current operating frequency for that engine.
> +
> +Users must bear in mind that engine and cycle sampling are disabled by default,
> +because of power saving concerns. `fdinfo` users and benchmark applications which
> +query the fdinfo file must make sure to toggle the job profiling status of the
> +driver by writing into the appropriate sysfs node::
> +
> + echo <N> > /sys/bus/platform/drivers/panfrost/[a-f0-9]*.gpu/profiling
A late thought - how it would work to not output the inactive fdinfo
keys when this knob is not enabled?
Generic userspace like gputop already handles that and wouldn't show the
stat. Which may be more user friendly than showing stats permanently at
zero. It may be moot once you add the auto-toggle to gputop (or so) but
perhaps worth considering.
Regards,
Tvrtko
> +
> +Where `N` is either `0` or `1`, depending on the desired enablement status.
> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> index 2c01c1e7523e..7da2b3f02ed9 100644
> --- a/drivers/gpu/drm/panfrost/Makefile
> +++ b/drivers/gpu/drm/panfrost/Makefile
> @@ -12,6 +12,4 @@ 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
> deleted file mode 100644
> index 72d4286a6bf7..000000000000
> --- a/drivers/gpu/drm/panfrost/panfrost_debugfs.c
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/* Copyright 2023 Collabora ltd. */
> -/* Copyright 2023 Amazon.com, Inc. or its affiliates. */
> -
> -#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
> deleted file mode 100644
> index c5af5f35877f..000000000000
> --- a/drivers/gpu/drm/panfrost/panfrost_debugfs.h
> +++ /dev/null
> @@ -1,14 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/*
> - * Copyright 2023 Collabora ltd.
> - * Copyright 2023 Amazon.com, Inc. or its affiliates.
> - */
> -
> -#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_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 62f7e3527385..cffcb0ac7c11 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -130,7 +130,7 @@ struct panfrost_device {
> struct list_head scheduled_jobs;
>
> struct panfrost_perfcnt *perfcnt;
> - atomic_t profile_mode;
> + bool profile_mode;
>
> struct mutex sched_lock;
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index a926d71e8131..9696702800a4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -20,7 +20,6 @@
> #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);
> @@ -600,10 +599,6 @@ 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)
> @@ -692,6 +687,41 @@ static void panfrost_remove(struct platform_device *pdev)
> drm_dev_put(ddev);
> }
>
> +static ssize_t profiling_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", pfdev->profile_mode);
> +}
> +
> +
> +static ssize_t profiling_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> + bool value;
> + int err;
> +
> + err = kstrtobool(buf, &value);
> + if (err)
> + return err;
> +
> + pfdev->profile_mode = value;
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR_RW(profiling);
> +
> +static struct attribute *panfrost_attrs[] = {
> + &dev_attr_profiling.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(panfrost);
> +
> /*
> * The OPP core wants the supply names to be NULL terminated, but we need the
> * correct num_supplies value for regulator core. Hence, we NULL terminate here
> @@ -789,6 +819,7 @@ static struct platform_driver panfrost_driver = {
> .name = "panfrost",
> .pm = pm_ptr(&panfrost_pm_ops),
> .of_match_table = dt_match,
> + .dev_groups = panfrost_groups,
> },
> };
> module_platform_driver(panfrost_driver);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 0c2dbf6ef2a5..a61ef0af9a4e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -243,7 +243,7 @@ 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)) {
> + if (pfdev->profile_mode) {
> panfrost_cycle_counter_get(pfdev);
> job->is_profiled = true;
> job->start_time = ktime_get();
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs
2024-03-06 8:33 ` Tvrtko Ursulin
@ 2024-03-11 10:02 ` Boris Brezillon
2024-03-12 0:47 ` Adrián Larumbe
0 siblings, 1 reply; 8+ messages in thread
From: Boris Brezillon @ 2024-03-11 10:02 UTC (permalink / raw)
To: Tvrtko Ursulin
Cc: Adrián Larumbe, robh, steven.price, airlied, daniel,
maarten.lankhorst, mripard, tzimmermann, corbet, kernel,
linux-kernel, dri-devel, linux-doc
On Wed, 6 Mar 2024 08:33:47 +0000
Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> On 06/03/2024 01:56, Adrián Larumbe wrote:
> > Debugfs isn't always available in production builds that try to squeeze
> > every single byte out of the kernel image, but we still need a way to
> > toggle the timestamp and cycle counter registers so that jobs can be
> > profiled for fdinfo's drm engine and cycle calculations.
> >
> > Drop the debugfs knob and replace it with a sysfs file that accomplishes
> > the same functionality, and document its ABI in a separate file.
> >
> > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > ---
> > .../testing/sysfs-driver-panfrost-profiling | 10 +++++
> > Documentation/gpu/panfrost.rst | 9 ++++
> > drivers/gpu/drm/panfrost/Makefile | 2 -
> > drivers/gpu/drm/panfrost/panfrost_debugfs.c | 21 ----------
> > drivers/gpu/drm/panfrost/panfrost_debugfs.h | 14 -------
> > drivers/gpu/drm/panfrost/panfrost_device.h | 2 +-
> > drivers/gpu/drm/panfrost/panfrost_drv.c | 41 ++++++++++++++++---
> > drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> > 8 files changed, 57 insertions(+), 44 deletions(-)
> > create mode 100644 Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> > delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
> > delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h
> >
> > diff --git a/Documentation/ABI/testing/sysfs-driver-panfrost-profiling b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> > new file mode 100644
> > index 000000000000..1d8bb0978920
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> > @@ -0,0 +1,10 @@
> > +What: /sys/bus/platform/drivers/panfrost/.../profiling
> > +Date: February 2024
> > +KernelVersion: 6.8.0
> > +Contact: Adrian Larumbe <adrian.larumbe@collabora.com>
> > +Description:
> > + Get/set drm fdinfo's engine and cycles profiling status.
> > + Valid values are:
> > + 0: Don't enable fdinfo job profiling sources.
> > + 1: Enable fdinfo job profiling sources, this enables both the GPU's
> > + timestamp and cycle counter registers.
> > \ No newline at end of file
> > diff --git a/Documentation/gpu/panfrost.rst b/Documentation/gpu/panfrost.rst
> > index b80e41f4b2c5..51ba375fd80d 100644
> > --- a/Documentation/gpu/panfrost.rst
> > +++ b/Documentation/gpu/panfrost.rst
> > @@ -38,3 +38,12 @@ the currently possible format options:
> >
> > Possible `drm-engine-` key names are: `fragment`, and `vertex-tiler`.
> > `drm-curfreq-` values convey the current operating frequency for that engine.
> > +
> > +Users must bear in mind that engine and cycle sampling are disabled by default,
> > +because of power saving concerns. `fdinfo` users and benchmark applications which
> > +query the fdinfo file must make sure to toggle the job profiling status of the
> > +driver by writing into the appropriate sysfs node::
> > +
> > + echo <N> > /sys/bus/platform/drivers/panfrost/[a-f0-9]*.gpu/profiling
>
> A late thought - how it would work to not output the inactive fdinfo
> keys when this knob is not enabled?
>
> Generic userspace like gputop already handles that and wouldn't show the
> stat. Which may be more user friendly than showing stats permanently at
> zero. It may be moot once you add the auto-toggle to gputop (or so) but
> perhaps worth considering.
I agree with Tvrtko, if the line being printed in fdinfo relies on some
sysfs knob to be valid, we'd rather not print the information in that
case, instead of printing zero.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs
2024-03-11 10:02 ` Boris Brezillon
@ 2024-03-12 0:47 ` Adrián Larumbe
0 siblings, 0 replies; 8+ messages in thread
From: Adrián Larumbe @ 2024-03-12 0:47 UTC (permalink / raw)
To: Boris Brezillon
Cc: Tvrtko Ursulin, robh, steven.price, airlied, daniel,
maarten.lankhorst, mripard, tzimmermann, corbet, kernel,
linux-kernel, dri-devel, linux-doc
On 11.03.2024 11:02, Boris Brezillon wrote:
> On Wed, 6 Mar 2024 08:33:47 +0000
> Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>
> > On 06/03/2024 01:56, Adrián Larumbe wrote:
> > > Debugfs isn't always available in production builds that try to squeeze
> > > every single byte out of the kernel image, but we still need a way to
> > > toggle the timestamp and cycle counter registers so that jobs can be
> > > profiled for fdinfo's drm engine and cycle calculations.
> > >
> > > Drop the debugfs knob and replace it with a sysfs file that accomplishes
> > > the same functionality, and document its ABI in a separate file.
> > >
> > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
> > > ---
> > > .../testing/sysfs-driver-panfrost-profiling | 10 +++++
> > > Documentation/gpu/panfrost.rst | 9 ++++
> > > drivers/gpu/drm/panfrost/Makefile | 2 -
> > > drivers/gpu/drm/panfrost/panfrost_debugfs.c | 21 ----------
> > > drivers/gpu/drm/panfrost/panfrost_debugfs.h | 14 -------
> > > drivers/gpu/drm/panfrost/panfrost_device.h | 2 +-
> > > drivers/gpu/drm/panfrost/panfrost_drv.c | 41 ++++++++++++++++---
> > > drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> > > 8 files changed, 57 insertions(+), 44 deletions(-)
> > > create mode 100644 Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> > > delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
> > > delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-panfrost-profiling b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> > > new file mode 100644
> > > index 000000000000..1d8bb0978920
> > > --- /dev/null
> > > +++ b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> > > @@ -0,0 +1,10 @@
> > > +What: /sys/bus/platform/drivers/panfrost/.../profiling
> > > +Date: February 2024
> > > +KernelVersion: 6.8.0
> > > +Contact: Adrian Larumbe <adrian.larumbe@collabora.com>
> > > +Description:
> > > + Get/set drm fdinfo's engine and cycles profiling status.
> > > + Valid values are:
> > > + 0: Don't enable fdinfo job profiling sources.
> > > + 1: Enable fdinfo job profiling sources, this enables both the GPU's
> > > + timestamp and cycle counter registers.
> > > \ No newline at end of file
> > > diff --git a/Documentation/gpu/panfrost.rst b/Documentation/gpu/panfrost.rst
> > > index b80e41f4b2c5..51ba375fd80d 100644
> > > --- a/Documentation/gpu/panfrost.rst
> > > +++ b/Documentation/gpu/panfrost.rst
> > > @@ -38,3 +38,12 @@ the currently possible format options:
> > >
> > > Possible `drm-engine-` key names are: `fragment`, and `vertex-tiler`.
> > > `drm-curfreq-` values convey the current operating frequency for that engine.
> > > +
> > > +Users must bear in mind that engine and cycle sampling are disabled by default,
> > > +because of power saving concerns. `fdinfo` users and benchmark applications which
> > > +query the fdinfo file must make sure to toggle the job profiling status of the
> > > +driver by writing into the appropriate sysfs node::
> > > +
> > > + echo <N> > /sys/bus/platform/drivers/panfrost/[a-f0-9]*.gpu/profiling
> >
> > A late thought - how it would work to not output the inactive fdinfo
> > keys when this knob is not enabled?
> >
> > Generic userspace like gputop already handles that and wouldn't show the
> > stat. Which may be more user friendly than showing stats permanently at
> > zero. It may be moot once you add the auto-toggle to gputop (or so) but
> > perhaps worth considering.
>
> I agree with Tvrtko, if the line being printed in fdinfo relies on some
> sysfs knob to be valid, we'd rather not print the information in that
> case, instead of printing zero.
Me too. I'll go first change both gputop and nvtop to make sure they use the new
sysfs knob for Panfrost, and then submit a new patch that handles printing of
the drm-cycles-* and drm-engine-* stats depending on the profiling knob state.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs
2024-03-06 1:56 ` [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs Adrián Larumbe
2024-03-06 8:33 ` Tvrtko Ursulin
@ 2024-03-06 8:41 ` Boris Brezillon
2024-03-06 14:31 ` Steven Price
2024-03-11 10:00 ` Boris Brezillon
3 siblings, 0 replies; 8+ messages in thread
From: Boris Brezillon @ 2024-03-06 8:41 UTC (permalink / raw)
To: Adrián Larumbe
Cc: robh, steven.price, airlied, daniel, maarten.lankhorst, mripard,
tzimmermann, corbet, kernel, linux-kernel, dri-devel, linux-doc
On Wed, 6 Mar 2024 01:56:36 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
> Debugfs isn't always available in production builds that try to squeeze
> every single byte out of the kernel image, but we still need a way to
> toggle the timestamp and cycle counter registers so that jobs can be
> profiled for fdinfo's drm engine and cycle calculations.
>
> Drop the debugfs knob and replace it with a sysfs file that accomplishes
> the same functionality, and document its ABI in a separate file.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> .../testing/sysfs-driver-panfrost-profiling | 10 +++++
> Documentation/gpu/panfrost.rst | 9 ++++
> drivers/gpu/drm/panfrost/Makefile | 2 -
> drivers/gpu/drm/panfrost/panfrost_debugfs.c | 21 ----------
> drivers/gpu/drm/panfrost/panfrost_debugfs.h | 14 -------
> drivers/gpu/drm/panfrost/panfrost_device.h | 2 +-
> drivers/gpu/drm/panfrost/panfrost_drv.c | 41 ++++++++++++++++---
> drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> 8 files changed, 57 insertions(+), 44 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
> delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-panfrost-profiling b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> new file mode 100644
> index 000000000000..1d8bb0978920
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> @@ -0,0 +1,10 @@
> +What: /sys/bus/platform/drivers/panfrost/.../profiling
> +Date: February 2024
> +KernelVersion: 6.8.0
> +Contact: Adrian Larumbe <adrian.larumbe@collabora.com>
> +Description:
> + Get/set drm fdinfo's engine and cycles profiling status.
> + Valid values are:
> + 0: Don't enable fdinfo job profiling sources.
> + 1: Enable fdinfo job profiling sources, this enables both the GPU's
> + timestamp and cycle counter registers.
> \ No newline at end of file
> diff --git a/Documentation/gpu/panfrost.rst b/Documentation/gpu/panfrost.rst
> index b80e41f4b2c5..51ba375fd80d 100644
> --- a/Documentation/gpu/panfrost.rst
> +++ b/Documentation/gpu/panfrost.rst
> @@ -38,3 +38,12 @@ the currently possible format options:
>
> Possible `drm-engine-` key names are: `fragment`, and `vertex-tiler`.
> `drm-curfreq-` values convey the current operating frequency for that engine.
> +
> +Users must bear in mind that engine and cycle sampling are disabled by default,
> +because of power saving concerns. `fdinfo` users and benchmark applications which
> +query the fdinfo file must make sure to toggle the job profiling status of the
> +driver by writing into the appropriate sysfs node::
> +
> + echo <N> > /sys/bus/platform/drivers/panfrost/[a-f0-9]*.gpu/profiling
> +
> +Where `N` is either `0` or `1`, depending on the desired enablement status.
> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> index 2c01c1e7523e..7da2b3f02ed9 100644
> --- a/drivers/gpu/drm/panfrost/Makefile
> +++ b/drivers/gpu/drm/panfrost/Makefile
> @@ -12,6 +12,4 @@ 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
> deleted file mode 100644
> index 72d4286a6bf7..000000000000
> --- a/drivers/gpu/drm/panfrost/panfrost_debugfs.c
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/* Copyright 2023 Collabora ltd. */
> -/* Copyright 2023 Amazon.com, Inc. or its affiliates. */
> -
> -#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
> deleted file mode 100644
> index c5af5f35877f..000000000000
> --- a/drivers/gpu/drm/panfrost/panfrost_debugfs.h
> +++ /dev/null
> @@ -1,14 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/*
> - * Copyright 2023 Collabora ltd.
> - * Copyright 2023 Amazon.com, Inc. or its affiliates.
> - */
> -
> -#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_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 62f7e3527385..cffcb0ac7c11 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -130,7 +130,7 @@ struct panfrost_device {
> struct list_head scheduled_jobs;
>
> struct panfrost_perfcnt *perfcnt;
> - atomic_t profile_mode;
> + bool profile_mode;
>
> struct mutex sched_lock;
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index a926d71e8131..9696702800a4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -20,7 +20,6 @@
> #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);
> @@ -600,10 +599,6 @@ 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)
> @@ -692,6 +687,41 @@ static void panfrost_remove(struct platform_device *pdev)
> drm_dev_put(ddev);
> }
>
> +static ssize_t profiling_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", pfdev->profile_mode);
> +}
> +
> +
> +static ssize_t profiling_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> + bool value;
> + int err;
> +
> + err = kstrtobool(buf, &value);
> + if (err)
> + return err;
> +
> + pfdev->profile_mode = value;
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR_RW(profiling);
> +
> +static struct attribute *panfrost_attrs[] = {
> + &dev_attr_profiling.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(panfrost);
> +
> /*
> * The OPP core wants the supply names to be NULL terminated, but we need the
> * correct num_supplies value for regulator core. Hence, we NULL terminate here
> @@ -789,6 +819,7 @@ static struct platform_driver panfrost_driver = {
> .name = "panfrost",
> .pm = pm_ptr(&panfrost_pm_ops),
> .of_match_table = dt_match,
> + .dev_groups = panfrost_groups,
> },
> };
> module_platform_driver(panfrost_driver);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 0c2dbf6ef2a5..a61ef0af9a4e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -243,7 +243,7 @@ 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)) {
> + if (pfdev->profile_mode) {
> panfrost_cycle_counter_get(pfdev);
> job->is_profiled = true;
> job->start_time = ktime_get();
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs
2024-03-06 1:56 ` [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs Adrián Larumbe
2024-03-06 8:33 ` Tvrtko Ursulin
2024-03-06 8:41 ` Boris Brezillon
@ 2024-03-06 14:31 ` Steven Price
2024-03-11 10:00 ` Boris Brezillon
3 siblings, 0 replies; 8+ messages in thread
From: Steven Price @ 2024-03-06 14:31 UTC (permalink / raw)
To: Adrián Larumbe, boris.brezillon, robh, airlied, daniel,
maarten.lankhorst, mripard, tzimmermann, corbet
Cc: kernel, linux-kernel, dri-devel, linux-doc
On 06/03/2024 01:56, Adrián Larumbe wrote:
> Debugfs isn't always available in production builds that try to squeeze
> every single byte out of the kernel image, but we still need a way to
> toggle the timestamp and cycle counter registers so that jobs can be
> profiled for fdinfo's drm engine and cycle calculations.
>
> Drop the debugfs knob and replace it with a sysfs file that accomplishes
> the same functionality, and document its ABI in a separate file.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com>
Reviewed-by: Steven Price <steven.price@arm.com>
> ---
> .../testing/sysfs-driver-panfrost-profiling | 10 +++++
> Documentation/gpu/panfrost.rst | 9 ++++
> drivers/gpu/drm/panfrost/Makefile | 2 -
> drivers/gpu/drm/panfrost/panfrost_debugfs.c | 21 ----------
> drivers/gpu/drm/panfrost/panfrost_debugfs.h | 14 -------
> drivers/gpu/drm/panfrost/panfrost_device.h | 2 +-
> drivers/gpu/drm/panfrost/panfrost_drv.c | 41 ++++++++++++++++---
> drivers/gpu/drm/panfrost/panfrost_job.c | 2 +-
> 8 files changed, 57 insertions(+), 44 deletions(-)
> create mode 100644 Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
> delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h
>
> diff --git a/Documentation/ABI/testing/sysfs-driver-panfrost-profiling b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> new file mode 100644
> index 000000000000..1d8bb0978920
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> @@ -0,0 +1,10 @@
> +What: /sys/bus/platform/drivers/panfrost/.../profiling
> +Date: February 2024
> +KernelVersion: 6.8.0
> +Contact: Adrian Larumbe <adrian.larumbe@collabora.com>
> +Description:
> + Get/set drm fdinfo's engine and cycles profiling status.
> + Valid values are:
> + 0: Don't enable fdinfo job profiling sources.
> + 1: Enable fdinfo job profiling sources, this enables both the GPU's
> + timestamp and cycle counter registers.
> \ No newline at end of file
> diff --git a/Documentation/gpu/panfrost.rst b/Documentation/gpu/panfrost.rst
> index b80e41f4b2c5..51ba375fd80d 100644
> --- a/Documentation/gpu/panfrost.rst
> +++ b/Documentation/gpu/panfrost.rst
> @@ -38,3 +38,12 @@ the currently possible format options:
>
> Possible `drm-engine-` key names are: `fragment`, and `vertex-tiler`.
> `drm-curfreq-` values convey the current operating frequency for that engine.
> +
> +Users must bear in mind that engine and cycle sampling are disabled by default,
> +because of power saving concerns. `fdinfo` users and benchmark applications which
> +query the fdinfo file must make sure to toggle the job profiling status of the
> +driver by writing into the appropriate sysfs node::
> +
> + echo <N> > /sys/bus/platform/drivers/panfrost/[a-f0-9]*.gpu/profiling
> +
> +Where `N` is either `0` or `1`, depending on the desired enablement status.
> diff --git a/drivers/gpu/drm/panfrost/Makefile b/drivers/gpu/drm/panfrost/Makefile
> index 2c01c1e7523e..7da2b3f02ed9 100644
> --- a/drivers/gpu/drm/panfrost/Makefile
> +++ b/drivers/gpu/drm/panfrost/Makefile
> @@ -12,6 +12,4 @@ 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
> deleted file mode 100644
> index 72d4286a6bf7..000000000000
> --- a/drivers/gpu/drm/panfrost/panfrost_debugfs.c
> +++ /dev/null
> @@ -1,21 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/* Copyright 2023 Collabora ltd. */
> -/* Copyright 2023 Amazon.com, Inc. or its affiliates. */
> -
> -#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
> deleted file mode 100644
> index c5af5f35877f..000000000000
> --- a/drivers/gpu/drm/panfrost/panfrost_debugfs.h
> +++ /dev/null
> @@ -1,14 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0 */
> -/*
> - * Copyright 2023 Collabora ltd.
> - * Copyright 2023 Amazon.com, Inc. or its affiliates.
> - */
> -
> -#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_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 62f7e3527385..cffcb0ac7c11 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -130,7 +130,7 @@ struct panfrost_device {
> struct list_head scheduled_jobs;
>
> struct panfrost_perfcnt *perfcnt;
> - atomic_t profile_mode;
> + bool profile_mode;
>
> struct mutex sched_lock;
>
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index a926d71e8131..9696702800a4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -20,7 +20,6 @@
> #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);
> @@ -600,10 +599,6 @@ 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)
> @@ -692,6 +687,41 @@ static void panfrost_remove(struct platform_device *pdev)
> drm_dev_put(ddev);
> }
>
> +static ssize_t profiling_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", pfdev->profile_mode);
> +}
> +
> +
> +static ssize_t profiling_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> + bool value;
> + int err;
> +
> + err = kstrtobool(buf, &value);
> + if (err)
> + return err;
> +
> + pfdev->profile_mode = value;
> +
> + return len;
> +}
> +
> +static DEVICE_ATTR_RW(profiling);
> +
> +static struct attribute *panfrost_attrs[] = {
> + &dev_attr_profiling.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(panfrost);
> +
> /*
> * The OPP core wants the supply names to be NULL terminated, but we need the
> * correct num_supplies value for regulator core. Hence, we NULL terminate here
> @@ -789,6 +819,7 @@ static struct platform_driver panfrost_driver = {
> .name = "panfrost",
> .pm = pm_ptr(&panfrost_pm_ops),
> .of_match_table = dt_match,
> + .dev_groups = panfrost_groups,
> },
> };
> module_platform_driver(panfrost_driver);
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 0c2dbf6ef2a5..a61ef0af9a4e 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -243,7 +243,7 @@ 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)) {
> + if (pfdev->profile_mode) {
> panfrost_cycle_counter_get(pfdev);
> job->is_profiled = true;
> job->start_time = ktime_get();
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs
2024-03-06 1:56 ` [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs Adrián Larumbe
` (2 preceding siblings ...)
2024-03-06 14:31 ` Steven Price
@ 2024-03-11 10:00 ` Boris Brezillon
3 siblings, 0 replies; 8+ messages in thread
From: Boris Brezillon @ 2024-03-11 10:00 UTC (permalink / raw)
To: Adrián Larumbe
Cc: robh, steven.price, airlied, daniel, maarten.lankhorst, mripard,
tzimmermann, corbet, kernel, linux-kernel, dri-devel, linux-doc
On Wed, 6 Mar 2024 01:56:36 +0000
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:
>
> +static ssize_t profiling_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%d\n", pfdev->profile_mode);
> +}
> +
> +
Dropped the extra blank and queued the patch to drm-misc-next.
> +static ssize_t profiling_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t len)
> +{
> + struct panfrost_device *pfdev = dev_get_drvdata(dev);
> + bool value;
> + int err;
> +
> + err = kstrtobool(buf, &value);
> + if (err)
> + return err;
> +
> + pfdev->profile_mode = value;
> +
> + return len;
> +}
^ permalink raw reply [flat|nested] 8+ messages in thread