[Intel-xe] [PATCH 1/2] drm/xe/ats-m: Expose uid for ATS-M via sysfs
Upadhyay, Tejas
tejas.upadhyay at intel.com
Mon Nov 13 09:25:32 UTC 2023
> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi at intel.com>
> Sent: Wednesday, November 8, 2023 9:26 PM
> To: Wajdeczko, Michal <Michal.Wajdeczko at intel.com>
> Cc: Upadhyay, Tejas <tejas.upadhyay at intel.com>; intel-
> xe at lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH 1/2] drm/xe/ats-m: Expose uid for ATS-M via
> sysfs
>
> On Wed, Nov 08, 2023 at 03:07:45PM +0100, Michal Wajdeczko wrote:
> >
> >
> >On 08.11.2023 14:54, Tejas Upadhyay wrote:
> >> This patch exports uid to the userspace via sysfs.
> >> For example, uid can be read by:
> >> cat /sys/class/drm/cardX/device/csc_unique_id
> >>
> >> 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 | 4 ++++
> >> drivers/gpu/drm/xe/xe_pci.c | 3 +++
> >> 5 files changed, 42 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/regs/xe_regs.h
> >> b/drivers/gpu/drm/xe/regs/xe_regs.h
> >> index a646d13af03a..1b8de9b8daf1 100644
> >> --- a/drivers/gpu/drm/xe/regs/xe_regs.h
> >> +++ b/drivers/gpu/drm/xe/regs/xe_regs.h
> >> @@ -70,6 +70,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)
> >> +
> >> #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 515cdf599fab..266e0d5d7159
> >> 100644
> >> --- a/drivers/gpu/drm/xe/xe_device.c
> >> +++ b/drivers/gpu/drm/xe/xe_device.c
> >> @@ -344,6 +344,15 @@ static void xe_device_sanitize(struct drm_device
> *drm, void *arg)
> >> xe_gt_sanitize(gt);
> >> }
> >>
> >> +static void xe_read_dev_uid(struct xe_device *xe) {
> >> + if (xe->info.has_uid) {
> >> + struct xe_gt *mmio = xe_root_mmio_gt(xe);
> >> +
> >> + xe->info.uid = xe_mmio_read64_2x32(mmio,
> DEVUID_LWORD);
> >> + }
> >> +}
> >> +
> >> int xe_device_probe(struct xe_device *xe) {
> >> struct xe_tile *tile;
> >> @@ -368,6 +377,8 @@ int xe_device_probe(struct xe_device *xe)
> >> if (err)
> >> return err;
> >>
> >> + xe_read_dev_uid(xe);
> >
> >as this function updates xe->info then maybe it should be a part of
> >xe_info_init() ?
>
> or not update info at all
It cant be done, as I think we need MMIO to get initialized before we read those piece of regs. So xe_device_probe below mmio init looks to be right spot.
>
> >
> >> +
> >> 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..66f69eada76f 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_unique_id_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.uid);
> >
> >%#llx
> >
> >otherwise there will be no 0x prefix so could be treated as decimal
> >
> >> +}
> >> +
> >> +static DEVICE_ATTR_RO(csc_unique_id);
> >
> >what's "csc" ?
> >
> >in patch you use different names:
> > - csc_unique_id
> > - UID
> > - devuid
>
> Agreed. Besides that, the documentation and commit message need to
> answer the *why*, not only how.
>
> Why is this being exported, and what's userspace supposed to do with it.
> Btw, what's the accompanying userspace to use it.
>
> Why is this in sysfs rather than a query? Is this something to be used by a
> sysadmin for something rather than the app using the device?
>
> Why is this special to ats-m? Bspec shows nothing for the register you are
> adding.
>
> Lucas De Marchi
Ok, agreed, I can add more information in commit message about this as well as make consistent naming.
Thanks,
Tejas
>
> >
> >
> >> +
> >> 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_unique_id.attr);
> >> }
> >>
> >> 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_uid) {
> >> + ret = sysfs_create_file(&dev->kobj,
> &dev_attr_csc_unique_id.attr);
> >> + if (ret) {
> >> + drm_warn(&xe->drm, "UID sysfs setup failed\n");
> >
> >if you really want to print warn then maybe also include %pe to help
> >diagnose why it failed
> >
> >> + return;
> >> + }
> >> + }
> >> +
> >> 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 4119ef03fb7e..1cc8488f8c86 100644
> >> --- a/drivers/gpu/drm/xe/xe_device_types.h
> >> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> >> @@ -238,6 +238,8 @@ struct xe_device {
> >> u8 vm_max_level;
> >> /** @va_bits: Maximum bits of a virtual address */
> >> u8 va_bits;
> >> + /** @uid: device uid, used for generating uuid */
> >> + u64 uid;
> >>
> >> /** @is_dgfx: is discrete device */
> >> u8 is_dgfx:1;
> >> @@ -261,6 +263,8 @@ struct xe_device {
> >> u8 supports_mmio_ext:1;
> >> /** @has_heci_gscfi: device has heci gscfi */
> >> u8 has_heci_gscfi:1;
> >> + /** @has_uid: device has uid, used for generating uuid */
> >> + u8 has_uid:1;
> >
> >IMO introduction of has_uid capability flag should be done in separate
> >patch than sysfs and uid changes
> >
> >>
> >> #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> >> struct {
> >> diff --git a/drivers/gpu/drm/xe/xe_pci.c
> >> b/drivers/gpu/drm/xe/xe_pci.c index 2fae45b9d88e..4d91ad049e59
> 100644
> >> --- a/drivers/gpu/drm/xe/xe_pci.c
> >> +++ b/drivers/gpu/drm/xe/xe_pci.c
> >> @@ -60,6 +60,7 @@ struct xe_device_desc {
> >> u8 has_heci_gscfi:1;
> >>
> >> u8 has_llc:1;
> >> + u8 has_uid;
> >> u8 bypass_mtcfg:1;
> >> u8 supports_mmio_ext:1;
> >> };
> >> @@ -298,6 +299,7 @@ static const struct xe_device_desc ats_m_desc = {
> >>
> >> DG2_FEATURES,
> >> .has_display = false,
> >> + .has_uid = true,
> >> };
> >>
> >> static const struct xe_device_desc dg2_desc = { @@ -577,6 +579,7 @@
> >> static int xe_info_init(struct xe_device *xe,
> >> xe->info.graphics_name = graphics_desc->name;
> >> xe->info.media_name = media_desc ? media_desc->name : "none";
> >> xe->info.has_llc = desc->has_llc;
> >> + xe->info.has_uid = desc->has_uid;
> >> xe->info.bypass_mtcfg = desc->bypass_mtcfg;
> >> xe->info.supports_mmio_ext = desc->supports_mmio_ext;
> >> xe->info.tile_mmio_ext_size = graphics_desc->tile_mmio_ext_size;
More information about the Intel-xe
mailing list