Re: [PATCH 7/9] drm/xe: Print more device information in devcoredump

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Mon, Jan 22, 2024 at 09:04:43AM -0800, José Roberto de Souza wrote:
> To properly decode batch buffer Mesa tools needs to know what
> platform is this one, for now we can do that with PCI id but
> already making it future proof by also printing GTs GMD version.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Maarten Lankhorst <dev@lankhorst.se>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_devcoredump.c |  2 ++
>  drivers/gpu/drm/xe/xe_device.c      | 20 ++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_device.h      |  2 ++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/xe/xe_devcoredump.c b/drivers/gpu/drm/xe/xe_devcoredump.c
> index a0e3732440ab5..43b66ca710f85 100644
> --- a/drivers/gpu/drm/xe/xe_devcoredump.c
> +++ b/drivers/gpu/drm/xe/xe_devcoredump.c
> @@ -63,6 +63,7 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset,
>  				   size_t count, void *data, size_t datalen)
>  {
>  	struct xe_devcoredump *coredump = data;
> +	struct xe_device *xe = devcoredump_to_xe_device(coredump);
>  	struct xe_devcoredump_snapshot *ss;
>  	struct drm_printer p;
>  	struct drm_print_iterator iter;
> @@ -89,6 +90,7 @@ static ssize_t xe_devcoredump_read(char *buffer, loff_t offset,
>  	drm_printf(&p, "Snapshot time: %lld.%09ld\n", ts.tv_sec, ts.tv_nsec);
>  	ts = ktime_to_timespec64(ss->boot_time);
>  	drm_printf(&p, "Uptime: %lld.%09ld\n", ts.tv_sec, ts.tv_nsec);
> +	xe_device_snapshot_print(xe, &p);
>  
>  	drm_printf(&p, "\n**** GuC CT ****\n");
>  	xe_guc_ct_snapshot_print(coredump->snapshot.ct, &p);
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index b4cdcf1b2081a..e0136e7d5ae52 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -742,3 +742,23 @@ u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64 address)
>  {
>  	return address & GENMASK_ULL(highest_address_bit_get(xe), 0);
>  }
> +
> +void xe_device_snapshot_print(struct xe_device *xe, struct drm_printer *p)
> +{
> +	struct xe_gt *gt;
> +	u8 id;
> +
> +	drm_printf(p, "PCI ID: 0x%04x\n", xe->info.devid);
> +	drm_printf(p, "PCI revision: 0x%02x\n", xe->info.revid);
> +
> +	for_each_gt(gt, xe, id) {
> +		drm_printf(p, "GT id: %u\n", id);
> +		drm_printf(p, "\tType: %s\n",
> +			   gt->info.type == XE_GT_TYPE_MAIN ? "main" : "media");
> +		drm_printf(p, "\tIP ver: %u.%u.%u\n",
> +			   REG_FIELD_GET(GMD_ID_ARCH_MASK, gt->info.gmdid),
> +			   REG_FIELD_GET(GMD_ID_RELEASE_MASK, gt->info.gmdid),
> +			   REG_FIELD_GET(GMD_ID_REVID, gt->info.gmdid));
> +		drm_printf(p, "\tCS timestamp frequency: %u\n", gt->info.reference_clock);

I don't like much the word 'frequency' along with this because of the
traditional confusion with the main running GT frequency.
Imho any combination with the words: timestamp reference crystal clock would
be better.

But I will leave the decision to you...

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
regardless

> +	}
> +}
> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> index 781a1aa164ecf..7df7cce218770 100644
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
> @@ -183,4 +183,6 @@ u32 xe_device_ccs_bytes(struct xe_device *xe, u64 size);
>  u64 xe_device_canonicalize_addr(struct xe_device *xe, u64 address);
>  u64 xe_device_uncanonicalize_addr(struct xe_device *xe, u64 address);
>  
> +void xe_device_snapshot_print(struct xe_device *xe, struct drm_printer *p);
> +
>  #endif
> -- 
> 2.43.0
>