On Wed, 2 Apr 2025 12:54:29 +0100
Adrián Larumbe <adrian.laru...@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__ */
