Re: [PATCH v4 4/4] drm/panthor: show device-wide list of DRM GEM objects over DebugFS

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

On Wed,  2 Apr 2025 12:54:29 +0100
Adrián Larumbe <adrian.larumbe@collabora.com> wrote:

> +static void panthor_gem_debugfs_bo_print(struct panthor_gem_object *bo,
> +					 struct seq_file *m,
> +					 struct gem_size_totals *totals)
> +{
> +	unsigned int refcount = kref_read(&bo->base.base.refcount);
> +	char creator_info[32] = {};
> +	bool has_flags = false;
> +	size_t resident_size;
> +
> +	/* Skip BOs being destroyed. */
> +	if (!refcount)
> +		return;
> +
> +	resident_size = bo->base.pages != NULL ? bo->base.base.size : 0;
> +
> +	snprintf(creator_info, sizeof(creator_info),
> +		 "%s/%d", bo->debugfs.creator.process_name, bo->debugfs.creator.tgid);
> +	seq_printf(m, "%-32s%-16d%-16d%-16zd%-16zd%-16lx",
> +		   creator_info,
> +		   bo->base.base.name,
> +		   refcount,
> +		   bo->base.base.size,
> +		   resident_size,
> +		   drm_vma_node_start(&bo->base.base.vma_node));
> +
> +	seq_puts(m, "(");
> +	has_flags = panfrost_gem_print_flag("imported", bo->base.base.import_attach != NULL,
> +					    has_flags, m);
> +	has_flags = panfrost_gem_print_flag("exported", bo->base.base.dma_buf != NULL,
> +					    has_flags, m);
> +	if (bo->base.madv < 0)
> +		has_flags = panfrost_gem_print_flag("purged", true, has_flags, m);
> +	else if (bo->base.madv > 0)
> +		has_flags = panfrost_gem_print_flag("purgeable", true, has_flags, m);

I would probably go:

	has_flags = panfrost_gem_print_flag("purged", bo->base.madv < 0, has_flags, m);
	has_flags = panfrost_gem_print_flag("purgeable", bo->base.madv > 0, has_flags, m);

to keep it one line per flag.

BTW, most of those flags are encoding the GEM state, so maybe the column
should be named state, and the helper panfrost_gem_print_state_flag().

> +	if (!has_flags)
> +		seq_puts(m, "none");
> +	seq_puts(m, ")");
> +
> +	seq_printf(m, "%-6s0x%-2x", "", bo->debugfs.bo_mask);

It's probably better if we print the debugfs flags like the GEM flags
(one string per flag, with a ',' separator). We can even make it a
helper function taking a list of flags and their associated strings so
we can use it for both panthor_gem_object::flags and
panthor_gem_object::debugfs::flags.

static void
panthor_gem_debugfs_print_flags(const char *names,
				u32 name_count,
				u32 flags)
{
	bool first = true;

	seq_puts(m, "(");

	if (!flags)
		seq_puts(m, "none");

	while (flags) {
		u32 bit = fls(flags) - 1;

		if (!first)
			seq_puts(m, ",");

		if (bit >= name_count || !names[bit])
			seq_printf(m, "unknown-bit%d", bit);
		else
			seq_puts(m, name);

		first = false;
		flags &= ~BIT(bit);
	}

	seq_puts(m, ")");
}

> +
> +	mutex_lock(&bo->label.lock);
> +	seq_printf(m, "%-6s%-60s", "", bo->label.str ? : NULL);
> +	mutex_unlock(&bo->label.lock);
> +	seq_puts(m, "\n");
> +
> +	totals->size += bo->base.base.size;
> +	totals->resident += resident_size;
> +	if (bo->base.madv > 0)
> +		totals->reclaimable += resident_size;
> +}
> +
> +void panthor_gem_debugfs_print_bos(struct panthor_device *ptdev,
> +				   struct seq_file *m)
> +{
> +	struct gem_size_totals totals = {0};
> +	struct panthor_gem_object *bo;
> +
> +	seq_puts(m, "created-by                      global-name     refcount        size            resident-size   file-offset     flags     kflags     label\n");
> +	seq_puts(m, "------------------------------------------------------------------------------------------------------------------------------------------------\n");
> +
> +	scoped_guard(mutex, &ptdev->gems.lock) {
> +		list_for_each_entry(bo, &ptdev->gems.node, debugfs.node)
> +			panthor_gem_debugfs_bo_print(bo, m, &totals);
> +	}
> +
> +	seq_puts(m, "==========================================================================================================================================================\n");
> +	seq_printf(m, "Total size: %zd, Total resident: %zd, Total reclaimable: %zd\n",
> +		   totals.size, totals.resident, totals.reclaimable);
> +}
> +#endif
> diff --git a/drivers/gpu/drm/panthor/panthor_gem.h b/drivers/gpu/drm/panthor/panthor_gem.h
> index 49daa5088a0d..22ecc0d39d5e 100644
> --- a/drivers/gpu/drm/panthor/panthor_gem.h
> +++ b/drivers/gpu/drm/panthor/panthor_gem.h
> @@ -15,6 +15,32 @@ struct panthor_vm;
>  
>  #define PANTHOR_BO_LABEL_MAXLEN	PAGE_SIZE
>  
> +#define PANTHOR_BO_KERNEL	BIT(0)

s/PANTHOR_BO_KERNEL/PANTHOR_DEBUGFS_BO_FLAG_KERNEL/

> +#define PANTHOR_BO_FW_MAPPED	BIT(1)

s/PANTHOR_BO_FW_MAPPED/PANTHOR_DEBUGFS_BO_FLAG_FW_MAPPED/

And it'd be better if those flags were documented.

I would also add a

#define PANTHOR_DEBUGFS_BO_FLAG_INITIALIZED BIT(0)

and move the other flags one bit left.

> +
> +/**
> + * struct panthor_gem_debugfs - GEM object's DebugFS list information
> + */
> +struct panthor_gem_debugfs {
> +	/**
> +	 * @node: Node used to insert the object in the device-wide list of
> +	 * GEM objects, to display information about it through a DebugFS file.
> +	 */
> +	struct list_head node;
> +
> +	/** @creator: Information about the UM process which created the GEM. */
> +	struct {
> +		/** @creator.process_name: Group leader name in owning thread's process */
> +		char process_name[TASK_COMM_LEN];
> +
> +		/** @creator.tgid: PID of the thread's group leader within its process */
> +		pid_t tgid;
> +	} creator;
> +
> +	/** @bo_mask: Bitmask encoding BO type as {USER, KERNEL} x {GPU, FW} */
> +	u32 bo_mask;

s/bo_mask/flags/ and mention that it's a combination of
PANTHOR_DEBUGFS_BO_FLAG_xxx in the doc.

> +};
> +
>  /**
>   * struct panthor_gem_object - Driver specific GEM object.
>   */
> @@ -62,6 +88,10 @@ struct panthor_gem_object {
>  		/** @lock.str: Protects access to the @label.str field. */
>  		struct mutex lock;
>  	} label;
> +
> +#ifdef CONFIG_DEBUG_FS
> +	struct panthor_gem_debugfs debugfs;
> +#endif
>  };
>  
>  /**
> @@ -157,4 +187,9 @@ panthor_kernel_bo_create(struct panthor_device *ptdev, struct panthor_vm *vm,
>  
>  void panthor_kernel_bo_destroy(struct panthor_kernel_bo *bo);
>  
> +#ifdef CONFIG_DEBUG_FS
> +void panthor_gem_debugfs_print_bos(struct panthor_device *pfdev,
> +				   struct seq_file *m);
> +#endif
> +
>  #endif /* __PANTHOR_GEM_H__ */