On Tue, 5 Sep 2023 19:45:19 +0100
Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx> wrote:
> Allow user space to decide whether the cycle counting register should be
> enabled. The main goal is letting tools like nvtop or IGT's gputop access
> this information in debug builds to obtain engine utilisation numbers.
Given you add a debugfs knob, I would simply drop the module param
introduced in the previous patch, it's just redundant, and I'd expect
people who care about profiling to have DEBUGFS enabled anyway.
>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/panfrost/Makefile | 2 +
> drivers/gpu/drm/panfrost/panfrost_debugfs.c | 51 +++++++++++++++++++++
> drivers/gpu/drm/panfrost/panfrost_debugfs.h | 13 ++++++
> drivers/gpu/drm/panfrost/panfrost_drv.c | 5 ++
> 4 files changed, 71 insertions(+)
> 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..48d5ddfcb1c6
> --- /dev/null
> +++ b/drivers/gpu/drm/panfrost/panfrost_debugfs.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright 2023 Collabora ltd. */
> +
> +#include <linux/debugfs.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"
> +
> +static int
> +profile_get(void *data, u64 *val)
> +{
> + struct drm_device *dev = data;
> + struct panfrost_device *pfdev = dev->dev_private;
> +
> + *val = atomic_read(&pfdev->profile_mode);
> +
> + return 0;
> +}
> +
> +static int
> +profile_set(void *data, u64 val)
> +{
> + struct drm_device *dev = data;
> + struct panfrost_device *pfdev = dev->dev_private;
> +
> + if (atomic_read(&pfdev->profile_mode) == val)
> + return 0;
> +
> + if (val == false)
> + panfrost_cycle_counter_stop(pfdev);
I don't think we want to stop the counter immediately. We should
instead let the job logic switch it off when all jobs with profiling
enabled are done.
> + else
> + atomic_set(&pfdev->profile_mode, 1);
> +
> + return 0;
> +}
> +
> +DEFINE_DEBUGFS_ATTRIBUTE(profile_fops,
> + profile_get, profile_set,
> + "%llu\n");
> +
> +void panfrost_debugfs_init(struct drm_minor *minor)
> +{
> + struct drm_device *dev = minor->dev;
> +
> + debugfs_create_file("profile", 0600, minor->debugfs_root,
> + dev, &profile_fops);
With the panfrost_cycle_counter_stop() call in profile_set() dropped,
you should be able to use debugfs_create_atomic_t() here.
> +}
> 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_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index a2ab99698ca8..2dfd9f79a31b 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);
> @@ -546,6 +547,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)
