Re: [PATCH 9/9] drm/panthor: add DRM_PANTHOR_VM_BIND_OP_MAP_DUMPABLE

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

On Sat, 19 Jul 2025 17:01:46 -0700
Chia-I Wu <olvaffe@gmail.com> wrote:

> When the flag is set, bo data is captured for devcoredump.
> 
> Signed-off-by: Chia-I Wu <olvaffe@gmail.com>
> ---
>  drivers/gpu/drm/panthor/panthor_coredump.c | 36 ++++++++++++++++++++++
>  drivers/gpu/drm/panthor/panthor_drv.c      |  3 +-
>  drivers/gpu/drm/panthor/panthor_mmu.c      |  7 +++--
>  include/uapi/drm/panthor_drm.h             |  7 +++++
>  4 files changed, 49 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_coredump.c b/drivers/gpu/drm/panthor/panthor_coredump.c
> index 5502452a5baa..db5695b38c2d 100644
> --- a/drivers/gpu/drm/panthor/panthor_coredump.c
> +++ b/drivers/gpu/drm/panthor/panthor_coredump.c
> @@ -5,6 +5,7 @@
>  #include <drm/drm_print.h>
>  #include <drm/drm_managed.h>
>  #include <generated/utsrelease.h>
> +#include <linux/ascii85.h>
>  #include <linux/devcoredump.h>
>  #include <linux/err.h>
>  #include <linux/pm_runtime.h>
> @@ -99,6 +100,26 @@ static const char *reason_str(enum panthor_coredump_reason reason)
>  	}
>  }
>  
> +static void print_bo(struct drm_printer *p, struct panthor_gem_object *bo,
> +		     u64 offset, u64 size)
> +{
> +	struct iosys_map map;
> +	const u32 *vals;
> +	u64 count;
> +	char buf[ASCII85_BUFSZ];
> +
> +	if (drm_gem_vmap(&bo->base.base, &map))
> +		return;
> +
> +	/* offset and size are aligned to panthor_vm_page_size, which is SZ_4K */
> +	vals = map.vaddr + offset;
> +	count = size / sizeof(u32);
> +	for (u64 i = 0; i < count; i++)
> +		drm_puts(p, ascii85_encode(vals[i], buf));
> +
> +	drm_gem_vunmap(&bo->base.base, &map);
> +}
> +
>  static void print_vma(struct drm_printer *p,
>  		      const struct panthor_coredump_vma_state *vma, u32 vma_id,
>  		      size_t *max_dyn_size)
> @@ -129,6 +150,21 @@ static void print_vma(struct drm_printer *p,
>  			}
>  		}
>  	}
> +
> +	if (vma->flags & DRM_PANTHOR_VM_BIND_OP_MAP_DUMPABLE) {
> +		drm_puts(p, "    data: |\n");
> +		drm_puts(p, "      ");
> +
> +		/* bo data is dynamic */
> +		if (max_dyn_size) {
> +			*max_dyn_size +=
> +				vma->size / sizeof(u32) * (ASCII85_BUFSZ - 1);
> +		} else {
> +			print_bo(p, bo, vma->bo_offset, vma->size);
> +		}

Back when Daniel was working on it, I suggested dumping VAs and BOs
content separately, so we can shrink the dumps when sparse is involved.
Otherwise you'll have these huge VA range filled with repeated dummy
pages. It's then up to the coredump analysis tool to reconstruct the
mapping between VAs and BOs.

> +
> +		drm_puts(p, "\n");
> +	}
>  }
>  
>  static void print_as(struct drm_printer *p,
> diff --git a/drivers/gpu/drm/panthor/panthor_drv.c b/drivers/gpu/drm/panthor/panthor_drv.c
> index 1116f2d2826e..6c4de1e73cd1 100644
> --- a/drivers/gpu/drm/panthor/panthor_drv.c
> +++ b/drivers/gpu/drm/panthor/panthor_drv.c
> @@ -1608,6 +1608,7 @@ static void panthor_debugfs_init(struct drm_minor *minor)
>   * - 1.3 - adds DRM_PANTHOR_GROUP_STATE_INNOCENT flag
>   * - 1.4 - adds DRM_IOCTL_PANTHOR_BO_SET_LABEL ioctl
>   * - 1.5 - adds DRM_PANTHOR_SET_USER_MMIO_OFFSET ioctl
> + * - 1.6 - adds DRM_PANTHOR_VM_BIND_OP_MAP_DUMPABLE flag
>   */
>  static const struct drm_driver panthor_drm_driver = {
>  	.driver_features = DRIVER_RENDER | DRIVER_GEM | DRIVER_SYNCOBJ |
> @@ -1621,7 +1622,7 @@ static const struct drm_driver panthor_drm_driver = {
>  	.name = "panthor",
>  	.desc = "Panthor DRM driver",
>  	.major = 1,
> -	.minor = 5,
> +	.minor = 6,
>  
>  	.gem_create_object = panthor_gem_create_object,
>  	.gem_prime_import_sg_table = drm_gem_shmem_prime_import_sg_table,
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 7862c99984b6..72b1b2799b65 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2045,10 +2045,11 @@ static void panthor_vma_init(struct panthor_vma *vma, u32 flags)
>  	vma->flags = flags;
>  }
>  
> -#define PANTHOR_VM_MAP_FLAGS \
> +#define PANTHOR_VM_MAP_FLAGS                   \
>  	(DRM_PANTHOR_VM_BIND_OP_MAP_READONLY | \
> -	 DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC | \
> -	 DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED)
> +	 DRM_PANTHOR_VM_BIND_OP_MAP_NOEXEC |   \
> +	 DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED | \
> +	 DRM_PANTHOR_VM_BIND_OP_MAP_DUMPABLE)
>  
>  static int panthor_gpuva_sm_step_map(struct drm_gpuva_op *op, void *priv)
>  {
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index e1f43deb7eca..c4c5e38365e9 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -496,6 +496,13 @@ enum drm_panthor_vm_bind_op_flags {
>  	 */
>  	DRM_PANTHOR_VM_BIND_OP_MAP_UNCACHED = 1 << 2,
>  
> +	/**
> +	 * @DRM_PANTHOR_VM_BIND_OP_MAP_DUMPABLE: Dump the VMA for devcoredump.
> +	 *
> +	 * Only valid with DRM_PANTHOR_VM_BIND_OP_TYPE_MAP.
> +	 */
> +	DRM_PANTHOR_VM_BIND_OP_MAP_DUMPABLE = 1 << 3,

It feels weird to have this verbose-dump option exposed as a VM
bind flag. Is there anything in the Vulkan GPU crash extension that
allows flagging individual memory objects are dumpable? I understand
that dumping all the VM data means generating potentially huge dumps,
and that you sometimes could trim that out because all you care about
in your debug session is CS/shader binaries, but other times it proves
useful to have regular buffers dumped too. If the coredump is
partial, it means you'll have to go and ask for users to try and
reproduce the issue with a dump_all flags set.

Given devcoredump is a device interface (meaning we can't really filter
coredumps per context), I'd be tempted to make this 'dont-dump-BOs'
option an opt-out debugfs knob, so that, by default, everything is
dumped.

> +
>  	/**
>  	 * @DRM_PANTHOR_VM_BIND_OP_TYPE_MASK: Mask used to determine the type of operation.
>  	 */