[Intel-xe] [PATCH 1/2] drm/xe/ats-m: Expose uid for ATS-M via sysfs
Matt Roper
matthew.d.roper at intel.com
Thu Nov 16 18:38:47 UTC 2023
On Thu, Nov 16, 2023 at 12:25:37PM -0600, Lucas De Marchi wrote:
> On Thu, Nov 16, 2023 at 05:59:56PM +0000, Upadhyay, Tejas wrote:
> >
> >
> > > -----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.
>
> that goes back to the other point that there wasn't a proper commit
> message explaining the use. How often will userspace read it? Why does
> it need that?
Is this something that we even want random, unprivileged userspace to
have access to? From the name, it sounds like this provides some kind
of unique ID (like a serial number?) that could be used to uniquely
identify a specific GPU. In some settings that could raise privacy
concerns and may not be desirable.
We definitely need a better explanation of exactly what this is and what
the goal of making it available is.
Matt
>
> I was actually meaning that stashing this inside the info and thus
> requiring the specific initialization order may not be the best
> approach.
>
> AFAICS this is only used to cache the value of that register and expose
> on sysfs. From the kernel side we don't really need that value for any
> initialization, so you could just read it very late and stash directly
> on xe.
>
> That said, there's a series from Michal Winiarski splitting the xe_info_init(),
> and it would already have mmio init done.
>
> Lucas De Marchi
>
> >
> > 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;
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
More information about the Intel-xe
mailing list