Re: [PATCH] drm/panthor: Display priorities of panthor groups over debugfs

[Boris Brezillon <boris.brezillon@collabora.com>]

Hello Nicolas,

On Tue, 18 Aug 2026 21:35:24 +0200
Nicolas Frattaroli <nicolas.frattaroli@collabora.com> wrote:

> Analogous to what was added in Commit d41c79838c47 ("drm/panfrost:
> Display list of device JM contexts over debugfs") for panfrost, add
> similar debugfs information for panthor.
> 
> The group priority does not change over the lifetime of the group, so no
> effort to synchronise with the scheduler lock is being made.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
> Some additional notes: I noticed panthor apparently uses
> xa_for_each{_marked} by guarding it with xa_lock/xa_unlock. That appears
> to be unnecessary, judging by the documentation of it and that nobody
> else does this.
> ---
>  drivers/gpu/drm/panthor/panthor_drv.c   |  1 +
>  drivers/gpu/drm/panthor/panthor_sched.c | 89 +++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/panthor/panthor_sched.h |  5 ++
>  3 files changed, 95 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 46a3080b0b20..8cdba0c1a14b 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1769,6 +1769,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>  {
>  	panthor_mmu_debugfs_init(minor);
>  	panthor_gem_debugfs_init(minor);
> +	panthor_sched_debugfs_init(minor);
>  }
>  #endif
>  
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index 5832dccfc093..44b61e946e2d 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0 or MIT
>  /* Copyright 2023 Collabora ltd. */
>  
> +#include <drm/drm_debugfs.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_exec.h>
>  #include <drm/drm_file.h>
> @@ -4198,3 +4199,91 @@ int panthor_sched_init(struct panthor_device *ptdev)
>  	ptdev->scheduler = sched;
>  	return 0;
>  }
> +
> +#ifdef CONFIG_DEBUG_FS
> +
> +static const char *
> +panthor_sched_prio_str(enum panthor_csg_priority prio)
> +{
> +	switch (prio) {
> +	case PANTHOR_CSG_PRIORITY_LOW:
> +		return "LOW";
> +	case PANTHOR_CSG_PRIORITY_MEDIUM:
> +		return "MEDIUM";
> +	case PANTHOR_CSG_PRIORITY_HIGH:
> +		return "HIGH";
> +	case PANTHOR_CSG_PRIORITY_RT:
> +		return "REAL-TIME";
> +	default:
> +		return "UNKNOWN";
> +	}
> +}
> +
> +static int show_file_groups(struct panthor_file *pfile, struct seq_file *m)
> +{
> +	struct panthor_group *group;
> +	unsigned long i;
> +
> +	if (IS_ERR_OR_NULL(pfile->groups))
> +		return -ENOENT;
> +
> +	xa_for_each_marked(&pfile->groups->xa, i, group, GROUP_REGISTERED) {
> +		seq_printf(m, " Group %lu: priority %s\n", i,
> +			   panthor_sched_prio_str(group->priority));
> +	}

Could this race with the GROUP_DESTROY ioctl and open the door for
potential UAFs on the group object? Seems
panthor_fdinfo_gather_group_samples() has this xa_for_each_marked()
inside an xa_lock()-ed section to cover for this.

> +
> +	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 panthor_file *, struct seq_file *) =
> +		node->info_ent->data;
> +	struct drm_file *file;
> +	int ret = 0;
> +
> +	scoped_cond_guard(mutex_intr, return -EINTR, &ddev->filelist_mutex) {
> +		list_for_each_entry(file, &ddev->filelist, lhead) {
> +			struct task_struct *task;
> +			struct panthor_file *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();

Don't we have this piece of information stored in
panthor_group::task_info already? Unless you really want to reflect
clients that have no groups, or have groups from a given client clearly
outlined in the debugfs output, I'd flatten things out and have these
client-related info printed along the group info in show_file_groups()
(pass a drm_file instead of a panthor_file, so you can get the
client_id from there).

Regards,

Boris

> +
> +			ret = show(pfile, m);
> +			if (ret < 0)
> +				break;
> +
> +			seq_puts(m, "\n");
> +		}
> +	}
> +
> +	return ret;