[Intel-xe] [PATCH V3 2/3] drm/xe/ats-m: Expose uid for ATS-M via sysfs
Upadhyay, Tejas
tejas.upadhyay at intel.com
Fri Dec 1 02:49:02 UTC 2023
> -----Original Message-----
> From: De Marchi, Lucas <lucas.demarchi at intel.com>
> Sent: Friday, December 1, 2023 4:40 AM
> To: Vivi, Rodrigo <rodrigo.vivi at intel.com>
> Cc: Upadhyay, Tejas <tejas.upadhyay at intel.com>; intel-
> xe at lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH V3 2/3] drm/xe/ats-m: Expose uid for ATS-M via
> sysfs
>
> On Thu, Nov 30, 2023 at 04:32:38PM -0500, Rodrigo Vivi wrote:
> >On Wed, Nov 29, 2023 at 12:00:39AM -0500, Upadhyay, Tejas wrote:
> >>
> >>
> >> > -----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
> >
> >The biggest problem that I see here is actually the justification.
> >It doesn't matter which wrapper are using our sysfs or not.
> >
> >A better commit message would be something like:
> >
> >In case of multiple similar dgfx cards plugged to the same node, the
> >user cannot rely on DRM card number nor on PCI address to identify
> >which exact part it is operating at since these numbers can change upon
> >order enumeration. In the case of FW flashing or other cases
>
> kind of sounds like what we have for network with predictable network
> names.
>
> https://access.redhat.com/documentation/en-
> us/red_hat_enterprise_linux/7/html/networking_guide/ch-
> consistent_network_device_naming
>
> This would provide the firmware naming, but I wonder if it can't be solved
> today entirely in userspace.
>
> >where the admin might know specifically which device it is accessing,
> >it needs to have an unique identifier number. To solve this case, CSC
> >FW provides an unique number that can be query. Let's expose this
> >number in the sysfs to make the device clearly identifiable.
>
> if this is the justification (like you said, it should be in the commit message so
> we know what's the use to give a proper review), then it also answers the
> question "why is it ok to wake the device on sysfs access rather than doing on
> device probe?" asked elsewhere: because clients will almost never access it.
> It's not like a userspace client is reading this value 10 times per second
Considering making it ADMIN attribute like MattR suggested, looks like it should be ok to wake GT to read UID. I will update commit message like Lucas and Rodrigo suggested and sent out new version.
Tejas
>
> Lucas De Marchi
>
> >
> >> >
> >> > > 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.
> >
> >Why not to wake? it shouldn't be a query that user is doing every time right?
> >
> >>
> >> 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;
More information about the Intel-xe
mailing list