[Intel-xe] [PATCH 1/2] drm/xe/ats-m: Expose uid for ATS-M via sysfs
Upadhyay, Tejas
tejas.upadhyay at intel.com
Thu Nov 16 17:59:56 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
We need mmio init before we access those regs, thus it needs to be in xe_device_probe() where mmio init is done.
@De Marchi, Lucas not update info at all? Do you mean read the reg everytime sysfs asks? That would require GT to be awake everytime we need to read which does not look good.
Thanks,
Tejas
>
> >
> >> +
> >> 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
>
> >
> >
> >> +
> >> 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