> -----Original Message-----
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> Sent: 21 November 2022 17:17
> To: Ghimiray, Himal Prasad <himal.prasad.ghimiray@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 1/1] drm/i915: Export LMEM max memory
> bandwidth via sysfs
>
>
> On 21/11/2022 10:01, Himal Prasad Ghimiray wrote:
> > Export lmem maximum memory bandwidth to the userspace via sysfs
> >
> > (v2)
> > Add TODO comment to have client parts specific condition (Anshuman)
> > Remove prelim prefix from the sysfs node name (Aravind)
>
> Link to userspace consumer?
[Ghimiray, Himal Prasad]
Does above comment means stating name of sysfs node in commit message ?
>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_reg.h | 2 ++
> > drivers/gpu/drm/i915/i915_sysfs.c | 28 ++++++++++++++++++++++++++++
> > 2 files changed, 30 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 8e1892d147741..1d59b84b86ad2
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6606,6 +6606,8 @@
> > #define POWER_SETUP_I1_WATTS REG_BIT(31)
> > #define POWER_SETUP_I1_SHIFT 6 /* 10.6 fixed
> point format */
> > #define POWER_SETUP_I1_DATA_MASK
> REG_GENMASK(15, 0)
> > +#define PCODE_MEMORY_CONFIG 0x70
> > +#define
> MEMORY_CONFIG_SUBCOMMAND_READ_MAX_BANDWIDTH 0x0
> > #define GEN12_PCODE_READ_SAGV_BLOCK_TIME_US 0x23
> > #define XEHP_PCODE_FREQUENCY_CONFIG 0x6e /* xehpsdv,
> pvc */
> > /* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */ diff --git
> > a/drivers/gpu/drm/i915/i915_sysfs.c
> > b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 595e8b5749907..69df2012bd10e 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -37,7 +37,10 @@
> >
> > #include "i915_drv.h"
> > #include "i915_sysfs.h"
> > +#include "i915_reg.h"
> > #include "intel_pm.h"
> > +#include "intel_pcode.h"
> > +
>
> Please don't do whitespace changes if there isn't a good reason.
[Ghimiray, Himal Prasad]
Will address this.
>
> >
> > struct drm_i915_private *kdev_minor_to_i915(struct device *kdev)
> > {
> > @@ -231,11 +234,36 @@ static void i915_setup_error_capture(struct device
> *kdev) {}
> > static void i915_teardown_error_capture(struct device *kdev) {}
> > #endif
> >
> > +static ssize_t
> > +lmem_max_bw_Mbps_show(struct device *dev, struct device_attribute
> > +*attr, char *buff) {
> > + struct drm_i915_private *i915 = kdev_minor_to_i915(dev);
> > + u32 val;
> > + int err;
> > +
> > + err = snb_pcode_read_p(&i915->uncore, PCODE_MEMORY_CONFIG,
> > +
> MEMORY_CONFIG_SUBCOMMAND_READ_MAX_BANDWIDTH,
> > + 0x0, &val);
> > + if (err)
> > + return err;
> > +
> > + return sysfs_emit(buff, "%u\n", val); }
> > +
> > +static DEVICE_ATTR_RO(lmem_max_bw_Mbps);
> > +
> > void i915_setup_sysfs(struct drm_i915_private *dev_priv)
> > {
> > struct device *kdev = dev_priv->drm.primary->kdev;
> > int ret;
> >
> > + /*TODO: Need to add client Parts condition check. */
>
> What does this mean? Are DG1 and DG2 not client parts?
>
[Ghimiray, Himal Prasad]
DG1 and Dg2 are client parts. Rather than adding individual platforms we need an identifier to
differentiate client parts from server part.
> > + if (IS_DG1(dev_priv) || IS_DG2(dev_priv)) {
> > + ret = sysfs_create_file(&kdev->kobj,
> &dev_attr_lmem_max_bw_Mbps.attr);
> > + if (ret)
> > + drm_err(&dev_priv->drm, "Setting up sysfs to read
> max B/W
> > +failed\n");
>
> I suggest at most drm_warn since error is ignored.
[Ghimiray, Himal Prasad]
Will address this.
>
> I also suggest expanding B/W to memory bandwidth. Maybe "Failed to create
> maximum memory bandwidth sysfs file"?
[Ghimiray, Himal Prasad]
Will address this. "Failed to create maximum memory bandwidth sysfs file" looks better.
>
> Regards,
>
> Tvrtko
>
> > + }
> > +
> > if (HAS_L3_DPF(dev_priv)) {
> > ret = device_create_bin_file(kdev, &dpf_attrs);
> > if (ret)
