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

["Souza, Jose" <jose.souza@intel.com>]

On Mon, 2024-01-22 at 15:18 -0500, Rodrigo Vivi wrote:
> 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...

changing to

-	        drm_printf(p, "\tCS timestamp frequency: %u\n", gt->info.reference_clock);
+	        drm_printf(p, "\tCS reference clock: %u\n", gt->info.reference_clock);
 	}

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

thank you for this and all other reviews.

> 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
> >