[Intel-xe] [PATCH V3 2/3] drm/xe/ats-m: Expose uid for ATS-M via sysfs

[Upadhyay, Tejas]

> -----Original Message-----
> From: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>
> Sent: Tuesday, November 28, 2023 10:31 PM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>; intel-
> xe at lists.freedesktop.org; Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Subject: Re: [Intel-xe] [PATCH V3 2/3] drm/xe/ats-m: Expose uid for ATS-M via
> sysfs
> 
> 
> 
> On 28.11.2023 16:13, Tejas Upadhyay wrote:
> > On ATS-M CSC firmware calculated unique device id(uid) can be read
> > from defined registers. Only ATS-M has this way of reading uid. Also
> > ATS-M customer require this solution to locate and name their devices
> > effectively.
> >
> > Consumer of csc uid is level0 sysman and end users using sysman.
> 
> is this generic UID or UID only specific to CSC firmware ?
> if former then maybe just name it as "uid" or "uuid" ?
> 
> >
> > This patch exports uid to the userspace via sysfs.
> 
> as this defines uabi, shouldn't this series go over dri-devel ?
> 
> + @Rodrigo
> 
> > For example, uid can be read by:
> > cat /sys/class/drm/cardX/device/csc_uid
> 
> as this is more Xe driver extension then maybe
> 
> /sys/bus/pci/drivers/xe/<BDF>/uuid
> 
> >
> > V2(Michal):
> >   - Make separate patch for csc uid capability
> >   - Use %#llx for 0x
> >   - Dump error value when sysfs creation fails
> >   - Have consistency in csc uid name
> >
> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> > ---
> >  drivers/gpu/drm/xe/regs/xe_regs.h    |  3 +++
> >  drivers/gpu/drm/xe/xe_device.c       | 11 +++++++++++
> >  drivers/gpu/drm/xe/xe_device_sysfs.c | 21 +++++++++++++++++++++
> > drivers/gpu/drm/xe/xe_device_types.h |  2 ++
> >  4 files changed, 37 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/xe/regs/xe_regs.h
> > b/drivers/gpu/drm/xe/regs/xe_regs.h
> > index ec9372aa739f..3599c7b38b61 100644
> > --- a/drivers/gpu/drm/xe/regs/xe_regs.h
> > +++ b/drivers/gpu/drm/xe/regs/xe_regs.h
> > @@ -71,6 +71,9 @@
> >  #define XEHP_CLOCK_GATE_DIS			XE_REG(0x101014)
> >  #define   SGSI_SIDECLK_DIS			REG_BIT(17)
> >
> > +#define DEVUID_LWORD				XE_REG(0x102008)
> > +#define DEVUID_HWORD				XE_REG(0x10200C)
> 
> Bspec uses different names .. and LWORD/HWORD look strange but since it's
> for SW use then maybe at least (assuming ATSM is XEHP):
> 
> #define XEHP_DEVUID_LDW				XE_REG(0x102008)
> #define XEHP_DEVUID_UDW				XE_REG(0x10200C)
> 
> > +
> >  #define GGC					XE_REG(0x108040)
> >  #define   GMS_MASK				REG_GENMASK(15, 8)
> >  #define   GGMS_MASK				REG_GENMASK(7, 6)
> > diff --git a/drivers/gpu/drm/xe/xe_device.c
> > b/drivers/gpu/drm/xe/xe_device.c index d60379d844d2..b54635d512d6
> > 100644
> > --- a/drivers/gpu/drm/xe/xe_device.c
> > +++ b/drivers/gpu/drm/xe/xe_device.c
> > @@ -355,6 +355,15 @@ static void xe_device_sanitize(struct drm_device
> *drm, void *arg)
> >  		xe_gt_sanitize(gt);
> >  }
> >
> > +static void xe_read_csc_uid(struct xe_device *xe) {
> > +	if (xe->info.has_csc_uid) {
> > +		struct xe_gt *mmio = xe_root_mmio_gt(xe);
> > +
> > +		xe->info.csc_uid  = xe_mmio_read64_2x32(mmio,
> DEVUID_LWORD);
> > +	}
> > +}
> > +
> >  int xe_device_probe(struct xe_device *xe)  {
> >  	struct xe_tile *tile;
> > @@ -379,6 +388,8 @@ int xe_device_probe(struct xe_device *xe)
> >  	if (err)
> >  		return err;
> >
> > +	xe_read_csc_uid(xe);
> > +
> >  	err = drmm_add_action_or_reset(&xe->drm, xe_driver_flr_fini, xe);
> >  	if (err)
> >  		return err;
> > diff --git a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > index 99113a5a2b84..4fbf0275d3bf 100644
> > --- a/drivers/gpu/drm/xe/xe_device_sysfs.c
> > +++ b/drivers/gpu/drm/xe/xe_device_sysfs.c
> > @@ -65,11 +65,24 @@ vram_d3cold_threshold_store(struct device *dev,
> > struct device_attribute *attr,
> >
> >  static DEVICE_ATTR_RW(vram_d3cold_threshold);
> >
> > +static ssize_t csc_uid_show(struct device *dev,
> > +			    struct device_attribute *attr,
> > +			    char *buf)
> > +{
> > +	struct pci_dev *pdev = to_pci_dev(dev);
> > +	struct xe_device *xe = pdev_to_xe_device(pdev);
> > +
> > +	return sysfs_emit(buf, "%#llx\n", xe->info.csc_uid);
> 
> if there are no plans to use this value in driver then maybe just read it here as
> it was already suggested by Lucas

Why to wake GT on every sysfs read and read those registers? Does not look correct in my POV.

Thanks,
Tejas
> 
> > +}
> > +
> > +static DEVICE_ATTR_RO(csc_uid);
> > +
> >  static void xe_device_sysfs_fini(struct drm_device *drm, void *arg)
> > {
> >  	struct xe_device *xe = arg;
> >
> >  	sysfs_remove_file(&xe->drm.dev->kobj,
> > &dev_attr_vram_d3cold_threshold.attr);
> > +	sysfs_remove_file(&xe->drm.dev->kobj, &dev_attr_csc_uid.attr);
> 
> it's sad that we don't have proper groups defines and need to add each
> attribute separately (but that's not your fault)
> 
> >  }
> >
> >  void xe_device_sysfs_init(struct xe_device *xe) @@ -83,6 +96,14 @@
> > void xe_device_sysfs_init(struct xe_device *xe)
> >  		return;
> >  	}
> >
> > +	if (xe->info.has_csc_uid) {
> > +		ret = sysfs_create_file(&dev->kobj, &dev_attr_csc_uid.attr);
> > +		if (ret) {
> > +			drm_warn(&xe->drm, "UID sysfs setup failed
> err=%pe\n", ERR_PTR(ret));
> > +			return;
> 
> if you exit here, you will leak dev_attr_vram_d3cold_threshold
> 
> > +		}
> > +	}
> > +
> >  	ret = drmm_add_action_or_reset(&xe->drm, xe_device_sysfs_fini,
> xe);
> >  	if (ret)
> >  		drm_warn(&xe->drm, "Failed to add sysfs fini drm action\n");
> diff
> > --git a/drivers/gpu/drm/xe/xe_device_types.h
> > b/drivers/gpu/drm/xe/xe_device_types.h
> > index 268a40639ec5..e88b64cb0eca 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -239,6 +239,8 @@ struct xe_device {
> >  		u8 vm_max_level;
> >  		/** @va_bits: Maximum bits of a virtual address */
> >  		u8 va_bits;
> > +		/** @csc_uid: device uid calculated by CSC FW, used for
> generating uuid */
> > +		u64 csc_uid;
> >
> >  		/** @is_dgfx: is discrete device */
> >  		u8 is_dgfx:1;