Re: [PATCH v12 03/15] drm/panfrost: Move debugfs initialisation to relevant subsystems

[Steven Price]

On 29/09/2026 04:44, Adrián Larumbe wrote:
> Because debugfs knobs are a device interface, and also the different knobs
> deal with specific subsystems, it's best to move their initialisations away
> from panfrost_drv.c and into the subsystem they provide information about.
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
> Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/panfrost/panfrost_device.c |   8 ++
>  drivers/gpu/drm/panfrost/panfrost_device.h |   4 +
>  drivers/gpu/drm/panfrost/panfrost_drv.c    | 129 +----------------------------
>  drivers/gpu/drm/panfrost/panfrost_gem.c    |  29 ++++++-
>  drivers/gpu/drm/panfrost/panfrost_gem.h    |   3 +-
>  drivers/gpu/drm/panfrost/panfrost_job.c    |  99 ++++++++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_job.h    |   4 +
>  7 files changed, 144 insertions(+), 132 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index c1a2e93a2b9b..5bf528f41278 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -561,3 +561,11 @@ EXPORT_GPL_DEV_PM_OPS(panfrost_pm_ops) = {
>  	RUNTIME_PM_OPS(panfrost_device_runtime_suspend, panfrost_device_runtime_resume, NULL)
>  	SYSTEM_SLEEP_PM_OPS(panfrost_device_suspend, panfrost_device_resume)
>  };
> +
> +#ifdef CONFIG_DEBUG_FS
> +void panfrost_device_debugfs_init(struct drm_minor *minor)
> +{
> +	panfrost_gem_debugfs_init(minor);
> +	panfrost_sched_debugfs_init(minor);
> +}
> +#endif // CONFIG_DEBUG_FS

NIT: I'd usually try to avoid #ifdefs like this by making the header
files define a no-op dummy inline function when !CONFIG_DEBUG_FS. It
saves polluting the code with a potentially large number of #ifdefs.

But I have to admit this isn't too bad and a definite improvement, so
it's up to you - either way:

Reviewed-by: Steven Price <steven.price@xxxxxxx>

Thanks,
Steve

> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 1fe1554f2652..a0b9a2145fc9 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -341,4 +341,8 @@ panfrost_device_schedule_reset(struct panfrost_device *pfdev)
>  	queue_work(pfdev->reset.wq, &pfdev->reset.work);
>  }
>  
> +#ifdef CONFIG_DEBUG_FS
> +void panfrost_device_debugfs_init(struct drm_minor *minor);
> +#endif // CONFIG_DEBUG_FS
> +
>  #endif
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 59af79a534fa..55fc22e8d4d4 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -792,133 +792,6 @@ static const struct file_operations panfrost_drm_driver_fops = {
>  	.show_fdinfo = drm_show_fdinfo,
>  };
>  
> -#ifdef CONFIG_DEBUG_FS
> -static int panthor_gems_show(struct seq_file *m, void *data)
> -{
> -	struct drm_info_node *node = m->private;
> -	struct panfrost_device *pfdev = to_panfrost_device(node->minor->dev);
> -
> -	panfrost_gem_debugfs_print_bos(pfdev, m);
> -
> -	return 0;
> -}
> -
> -static void show_panfrost_jm_ctx(struct panfrost_jm_ctx *jm_ctx, u32 handle,
> -				 struct seq_file *m)
> -{
> -	struct drm_device *ddev = ((struct drm_info_node *)m->private)->minor->dev;
> -	const char *prio = "UNKNOWN";
> -
> -	static const char * const prios[] = {
> -		[DRM_SCHED_PRIORITY_HIGH] = "HIGH",
> -		[DRM_SCHED_PRIORITY_NORMAL] = "NORMAL",
> -		[DRM_SCHED_PRIORITY_LOW] = "LOW",
> -	};
> -
> -	if (jm_ctx->slot_entity[0].priority !=
> -	    jm_ctx->slot_entity[1].priority)
> -		drm_warn(ddev, "Slot priorities should be the same in a single context");
> -
> -	if (jm_ctx->slot_entity[0].priority < ARRAY_SIZE(prios))
> -		prio = prios[jm_ctx->slot_entity[0].priority];
> -
> -	seq_printf(m, " JM context %u: priority %s\n", handle, prio);
> -}
> -
> -static int show_file_jm_ctxs(struct panfrost_file_priv *pfile,
> -			     struct seq_file *m)
> -{
> -	struct panfrost_jm_ctx *jm_ctx;
> -	unsigned long i;
> -
> -	xa_lock(&pfile->jm_ctxs);
> -	xa_for_each(&pfile->jm_ctxs, i, jm_ctx) {
> -		jm_ctx = panfrost_jm_ctx_get(jm_ctx);
> -		xa_unlock(&pfile->jm_ctxs);
> -		show_panfrost_jm_ctx(jm_ctx, i, m);
> -		panfrost_jm_ctx_put(jm_ctx);
> -		xa_lock(&pfile->jm_ctxs);
> -	}
> -	xa_unlock(&pfile->jm_ctxs);
> -
> -	return 0;
> -}
> -
> -static struct drm_info_list panthor_debugfs_list[] = {
> -	{"gems",
> -	 panthor_gems_show, 0, NULL},
> -};
> -
> -static int panthor_gems_debugfs_init(struct drm_minor *minor)
> -{
> -	drm_debugfs_create_files(panthor_debugfs_list,
> -				 ARRAY_SIZE(panthor_debugfs_list),
> -				 minor->debugfs_root, minor);
> -
> -	return 0;
> -}
> -
> -static int show_each_file(struct seq_file *m, void *arg)
> -{
> -	struct drm_info_node *node = (struct drm_info_node *)m->private;
> -	struct drm_device *ddev = node->minor->dev;
> -	int (*show)(struct panfrost_file_priv *, struct seq_file *) =
> -		node->info_ent->data;
> -	struct drm_file *file;
> -	int ret;
> -
> -	ret = mutex_lock_interruptible(&ddev->filelist_mutex);
> -	if (ret)
> -		return ret;
> -
> -	list_for_each_entry(file, &ddev->filelist, lhead) {
> -		struct task_struct *task;
> -		struct panfrost_file_priv *pfile = file->driver_priv;
> -		struct pid *pid;
> -
> -		/*
> -		 * Although we have a valid reference on file->pid, that does
> -		 * not guarantee that the task_struct who called get_pid() is
> -		 * still alive (e.g. get_pid(current) => fork() => exit()).
> -		 * Therefore, we need to protect this ->comm access using RCU.
> -		 */
> -		rcu_read_lock();
> -		pid = rcu_dereference(file->pid);
> -		task = pid_task(pid, PIDTYPE_TGID);
> -		seq_printf(m, "client_id %8llu pid %8d command %s:\n",
> -			   file->client_id, pid_nr(pid),
> -			   task ? task->comm : "<unknown>");
> -		rcu_read_unlock();
> -
> -		ret = show(pfile, m);
> -		if (ret < 0)
> -			break;
> -
> -		seq_puts(m, "\n");
> -	}
> -
> -	mutex_unlock(&ddev->filelist_mutex);
> -	return ret;
> -}
> -
> -static struct drm_info_list panfrost_sched_debugfs_list[] = {
> -	{ "sched_ctxs", show_each_file, 0, show_file_jm_ctxs },
> -};
> -
> -static void panfrost_sched_debugfs_init(struct drm_minor *minor)
> -{
> -	drm_debugfs_create_files(panfrost_sched_debugfs_list,
> -				 ARRAY_SIZE(panfrost_sched_debugfs_list),
> -				 minor->debugfs_root, minor);
> -}
> -
> -static void panfrost_debugfs_init(struct drm_minor *minor)
> -{
> -	panthor_gems_debugfs_init(minor);
> -	panfrost_sched_debugfs_init(minor);
> -}
> -#endif
> -
>  /*
>   * Panfrost driver version:
>   * - 1.0 - initial interface
> @@ -950,7 +823,7 @@ static const struct drm_driver panfrost_drm_driver = {
>  	.gem_prime_import	= panfrost_gem_prime_import,
>  	.gem_prime_import_sg_table = panfrost_gem_prime_import_sg_table,
>  #ifdef CONFIG_DEBUG_FS
> -	.debugfs_init = panfrost_debugfs_init,
> +	.debugfs_init = panfrost_device_debugfs_init,
>  #endif
>  };
>  
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c
> index 383ac8cd5645..518f24549b9d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c
> @@ -9,6 +9,7 @@
>  #include <linux/dma-mapping.h>
>  
>  #include <drm/panfrost_drm.h>
> +#include <drm/drm_debugfs.h>
>  #include <drm/drm_print.h>
>  #include "panfrost_device.h"
>  #include "panfrost_gem.h"
> @@ -735,8 +736,8 @@ static void panfrost_gem_debugfs_bo_print(struct panfrost_gem_object *bo,
>  		totals->reclaimable += resident_size;
>  }
>  
> -void panfrost_gem_debugfs_print_bos(struct panfrost_device *pfdev,
> -				    struct seq_file *m)
> +static void panfrost_gem_debugfs_print_bos(struct panfrost_device *pfdev,
> +					   struct seq_file *m)
>  {
>  	struct gem_size_totals totals = {0};
>  	struct panfrost_gem_object *bo;
> @@ -756,4 +757,28 @@ void panfrost_gem_debugfs_print_bos(struct panfrost_device *pfdev,
>  	seq_printf(m, "Total size: %zd, Total resident: %zd, Total reclaimable: %zd\n",
>  		   totals.size, totals.resident, totals.reclaimable);
>  }
> +
> +static int panfrost_gems_show(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct panfrost_device *pfdev = to_panfrost_device(node->minor->dev);
> +
> +	panfrost_gem_debugfs_print_bos(pfdev, m);
> +
> +	return 0;
> +}
> +
> +static struct drm_info_list panfrost_debugfs_list[] = {
> +	{"gems",
> +	 panfrost_gems_show, 0, NULL},
> +};
> +
> +int panfrost_gem_debugfs_init(struct drm_minor *minor)
> +{
> +	drm_debugfs_create_files(panfrost_debugfs_list,
> +				 ARRAY_SIZE(panfrost_debugfs_list),
> +				 minor->debugfs_root, minor);
> +
> +	return 0;
> +}
>  #endif
> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
> index 5c823cdbd980..3008afcf4bff 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
> @@ -164,8 +164,7 @@ int panfrost_gem_sync(struct drm_gem_object *obj, u32 type,
>  void panfrost_gem_internal_set_label(struct drm_gem_object *obj, const char *label);
>  
>  #ifdef CONFIG_DEBUG_FS
> -void panfrost_gem_debugfs_print_bos(struct panfrost_device *pfdev,
> -				    struct seq_file *m);
> +int panfrost_gem_debugfs_init(struct drm_minor *minor);
>  #endif
>  
>  #endif /* __PANFROST_GEM_H__ */
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 476c936b54e6..00fb36c577df 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -8,6 +8,8 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/dma-resv.h>
> +#include <drm/drm_debugfs.h>
> +#include <drm/drm_print.h>
>  #include <drm/gpu_scheduler.h>
>  #include <drm/panfrost_drm.h>
>  
> @@ -1138,3 +1140,100 @@ int panfrost_jm_ctx_destroy(struct drm_file *file, u32 handle)
>  	panfrost_jm_ctx_put(jm_ctx);
>  	return 0;
>  }
> +
> +#ifdef CONFIG_DEBUG_FS
> +static void show_panfrost_jm_ctx(struct panfrost_jm_ctx *jm_ctx, u32 handle,
> +				 struct seq_file *m)
> +{
> +	struct drm_device *ddev = ((struct drm_info_node *)m->private)->minor->dev;
> +	const char *prio = "UNKNOWN";
> +
> +	static const char * const prios[] = {
> +		[DRM_SCHED_PRIORITY_HIGH] = "HIGH",
> +		[DRM_SCHED_PRIORITY_NORMAL] = "NORMAL",
> +		[DRM_SCHED_PRIORITY_LOW] = "LOW",
> +	};
> +
> +	if (jm_ctx->slot_entity[0].priority !=
> +	    jm_ctx->slot_entity[1].priority)
> +		drm_warn(ddev, "Slot priorities should be the same in a single context");
> +
> +	if (jm_ctx->slot_entity[0].priority < ARRAY_SIZE(prios))
> +		prio = prios[jm_ctx->slot_entity[0].priority];
> +
> +	seq_printf(m, " JM context %u: priority %s\n", handle, prio);
> +}
> +
> +static int show_file_jm_ctxs(struct panfrost_file_priv *pfile,
> +			     struct seq_file *m)
> +{
> +	struct panfrost_jm_ctx *jm_ctx;
> +	unsigned long i;
> +
> +	xa_lock(&pfile->jm_ctxs);
> +	xa_for_each(&pfile->jm_ctxs, i, jm_ctx) {
> +		jm_ctx = panfrost_jm_ctx_get(jm_ctx);
> +		xa_unlock(&pfile->jm_ctxs);
> +		show_panfrost_jm_ctx(jm_ctx, i, m);
> +		panfrost_jm_ctx_put(jm_ctx);
> +		xa_lock(&pfile->jm_ctxs);
> +	}
> +	xa_unlock(&pfile->jm_ctxs);
> +
> +	return 0;
> +}
> +
> +static int show_each_file(struct seq_file *m, void *arg)
> +{
> +	struct drm_info_node *node = (struct drm_info_node *)m->private;
> +	struct drm_device *ddev = node->minor->dev;
> +	int (*show)(struct panfrost_file_priv *, struct seq_file *) =
> +		node->info_ent->data;
> +	struct drm_file *file;
> +	int ret;
> +
> +	ret = mutex_lock_interruptible(&ddev->filelist_mutex);
> +	if (ret)
> +		return ret;
> +
> +	list_for_each_entry(file, &ddev->filelist, lhead) {
> +		struct task_struct *task;
> +		struct panfrost_file_priv *pfile = file->driver_priv;
> +		struct pid *pid;
> +
> +		/*
> +		 * Although we have a valid reference on file->pid, that does
> +		 * not guarantee that the task_struct who called get_pid() is
> +		 * still alive (e.g. get_pid(current) => fork() => exit()).
> +		 * Therefore, we need to protect this ->comm access using RCU.
> +		 */
> +		rcu_read_lock();
> +		pid = rcu_dereference(file->pid);
> +		task = pid_task(pid, PIDTYPE_TGID);
> +		seq_printf(m, "client_id %8llu pid %8d command %s:\n",
> +			   file->client_id, pid_nr(pid),
> +			   task ? task->comm : "<unknown>");
> +		rcu_read_unlock();
> +
> +		ret = show(pfile, m);
> +		if (ret < 0)
> +			break;
> +
> +		seq_puts(m, "\n");
> +	}
> +
> +	mutex_unlock(&ddev->filelist_mutex);
> +	return ret;
> +}
> +
> +static struct drm_info_list panfrost_sched_debugfs_list[] = {
> +	{ "sched_ctxs", show_each_file, 0, show_file_jm_ctxs },
> +};
> +
> +void panfrost_sched_debugfs_init(struct drm_minor *minor)
> +{
> +	drm_debugfs_create_files(panfrost_sched_debugfs_list,
> +				 ARRAY_SIZE(panfrost_sched_debugfs_list),
> +				 minor->debugfs_root, minor);
> +}
> +#endif
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.h b/drivers/gpu/drm/panfrost/panfrost_job.h
> index c3f57e41a571..3e3b9717838f 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.h
> @@ -74,4 +74,8 @@ int panfrost_job_get_slot(struct panfrost_job *job);
>  int panfrost_job_push(struct panfrost_job *job);
>  void panfrost_job_put(struct panfrost_job *job);
>  
> +#ifdef CONFIG_DEBUG_FS
> +void panfrost_sched_debugfs_init(struct drm_minor *minor);
> +#endif
> +
>  #endif
>