| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <61095f51-3d74-48e9-96b4-75da4645331e@arm.com>
Date: Wed, 6 Mar 2024 14:31:47 +0000
From: Steven Price <steven.price@....com>
To: Adrián Larumbe <adrian.larumbe@...labora.com>,
boris.brezillon@...labora.com, robh@...nel.org, airlied@...il.com,
daniel@...ll.ch, maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
tzimmermann@...e.de, corbet@....net
Cc: kernel@...labora.com, linux-kernel@...r.kernel.org,
dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 1/1] drm/panfrost: Replace fdinfo's profiling debugfs
knob with sysfs
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@...labora.com>
Reviewed-by: Steven Price <steven.price@....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@...labora.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();
Powered by blists - more mailing lists