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

[Michal Wajdeczko]

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

> +}
> +
> +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;