Re: [PATCH v3 3/8] drm/panfrost: Enable debugfs toggling of cycle counter register

[Boris Brezillon]

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)