| Message ID | 20220825132118.784407-3-badal.nilawar@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | drm/i915: Add HWMON support | expand |
On Thu, 25 Aug 2022 06:21:13 -0700, Badal Nilawar wrote: > > From: Riana Tauro <riana.tauro@intel.com> > > Use i915 HWMON subsystem to display current input voltage. A couple of suggestions to improve comments in this patch below and after addressing those this patch is: Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 103dd543a214..2192d0fd4c66 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -11,8 +11,10 @@ > #include "i915_hwmon.h" > #include "i915_reg.h" > #include "intel_mchbar_regs.h" > +#include "gt/intel_gt_regs.h" In later patches we have added units for different quantities here. So I think we should add those units for voltage to this patch too. It's in Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon but I think it's better to add to this file too otherwise if anyone looks at it is seems to be missing. So I would add the following to this patch: /* * SF_* - scale factors for particular quantities according to hwmon spec. * - voltage - millivolts */ #define SF_VOLTAGE 1000 > +static int > +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) > +{ > + struct i915_hwmon *hwmon = ddat->hwmon; > + intel_wakeref_t wakeref; > + u32 reg_value; > + > + switch (attr) { > + case hwmon_in_input: > + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > + reg_value = intel_uncore_read(ddat->uncore, hwmon->rg.gt_perf_status); > + /* In units of 2.5 millivolt */ > + *val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * 25, 10); Let's complete this comment to so that it is clear what's happening: /* HW register value is in units of 2.5 millivolt */
> -----Original Message----- > From: Nilawar, Badal <badal.nilawar@intel.com> > Sent: Thursday, August 25, 2022 6:51 PM > To: intel-gfx@lists.freedesktop.org > Cc: Dixit, Ashutosh <ashutosh.dixit@intel.com>; Tauro, Riana > <riana.tauro@intel.com>; Gupta, Anshuman <anshuman.gupta@intel.com>; > Ewins, Jon <jon.ewins@intel.com>; linux-hwmon@vger.kernel.org > Subject: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support > > From: Riana Tauro <riana.tauro@intel.com> > > Use i915 HWMON subsystem to display current input voltage. > > v2: > - Updated date and kernel version in feature description > - Fixed review comments (Ashutosh) > v3: Use macro HWMON_CHANNEL_INFO to define hwmon channel (Guenter) > v4: > - Fixed review comments (Ashutosh) > - Use hwm_ prefix for static functions (Ashutosh) > > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Anshuman Gupta <anshuman.gupta@intel.com> > Signed-off-by: Riana Tauro <riana.tauro@intel.com> > Signed-off-by: Badal Nilawar <badal.nilawar@intel.com> > Acked-by: Guenter Roeck <linux@roeck-us.net> > --- > .../ABI/testing/sysfs-driver-intel-i915-hwmon | 7 +++ > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 3 ++ > drivers/gpu/drm/i915/i915_hwmon.c | 47 +++++++++++++++++++ > 3 files changed, 57 insertions(+) > create mode 100644 Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > new file mode 100644 > index 000000000000..24c4b7477d51 > --- /dev/null > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon > @@ -0,0 +1,7 @@ > +What: /sys/devices/.../hwmon/hwmon<i>/in0_input > +Date: June 2022 > +KernelVersion: 5.19 > +Contact: dri-devel@lists.freedesktop.org > +Description: RO. Current Voltage in millivolt. > + > + Only supported for particular Intel i915 graphics platforms. > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > index 94f9ddcfb3a5..5d4fbda4d326 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > @@ -1508,6 +1508,9 @@ > #define VLV_RENDER_C0_COUNT _MMIO(0x138118) > #define VLV_MEDIA_C0_COUNT _MMIO(0x13811c) > > +#define GEN12_RPSTAT1 _MMIO(0x1381b4) > +#define GEN12_VOLTAGE_MASK REG_GENMASK(10, 0) > + > #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + > ((x) * 4)) > #define GEN11_CSME (31) > #define GEN11_GUNIT (28) > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c > b/drivers/gpu/drm/i915/i915_hwmon.c > index 103dd543a214..2192d0fd4c66 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -11,8 +11,10 @@ > #include "i915_hwmon.h" > #include "i915_reg.h" > #include "intel_mchbar_regs.h" > +#include "gt/intel_gt_regs.h" > > struct hwm_reg { > + i915_reg_t gt_perf_status; > }; > > struct hwm_drvdata { > @@ -29,14 +31,49 @@ struct i915_hwmon { > }; > > static const struct hwmon_channel_info *hwm_info[] = { > + HWMON_CHANNEL_INFO(in, HWMON_I_INPUT), > NULL > }; > > +static umode_t > +hwm_in_is_visible(const struct hwm_drvdata *ddat, u32 attr) { > + switch (attr) { > + case hwmon_in_input: > + return i915_mmio_reg_valid(ddat->hwmon->rg.gt_perf_status) > ? 0444 : 0; > + default: > + return 0; > + } > +} > + > +static int > +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) { > + struct i915_hwmon *hwmon = ddat->hwmon; > + intel_wakeref_t wakeref; > + u32 reg_value; > + > + switch (attr) { > + case hwmon_in_input: Other attributes in this series take hwmon->lock before accessing i915 registers , So do we need lock here as well ? Thanks, Anshuman Gupta. > + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > + reg_value = intel_uncore_read(ddat->uncore, hwmon- > >rg.gt_perf_status); > + /* In units of 2.5 millivolt */ > + *val = > DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * > 25, 10); > + return 0; > + default: > + return -EOPNOTSUPP; > + } > +} > + > static umode_t > hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type, > u32 attr, int channel) > { > + struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata; > + > switch (type) { > + case hwmon_in: > + return hwm_in_is_visible(ddat, attr); > default: > return 0; > } > @@ -46,7 +83,11 @@ static int > hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, > int channel, long *val) > { > + struct hwm_drvdata *ddat = dev_get_drvdata(dev); > + > switch (type) { > + case hwmon_in: > + return hwm_in_read(ddat, attr, val); > default: > return -EOPNOTSUPP; > } > @@ -76,6 +117,12 @@ static const struct hwmon_chip_info hwm_chip_info = { > static void hwm_get_preregistration_info(struct drm_i915_private *i915) { > + struct i915_hwmon *hwmon = i915->hwmon; > + > + if (IS_DG1(i915) || IS_DG2(i915)) > + hwmon->rg.gt_perf_status = GEN12_RPSTAT1; > + else > + hwmon->rg.gt_perf_status = INVALID_MMIO_REG; > } > > void i915_hwmon_register(struct drm_i915_private *i915) > -- > 2.25.1
On Mon, 12 Sep 2022 07:09:28 -0700, Gupta, Anshuman wrote: > > > +static int > > +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) { > > + struct i915_hwmon *hwmon = ddat->hwmon; > > + intel_wakeref_t wakeref; > > + u32 reg_value; > > + > > + switch (attr) { > > + case hwmon_in_input: > > Other attributes in this series take hwmon->lock before accessing i915 > registers , So do we need lock here as well ? The lock is being taken only for RMW and for making sure energy counter updates happen atomically. We are not taking the lock for just reads so IMO no lock is needed here. > > + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > > + reg_value = intel_uncore_read(ddat->uncore, hwmon- > > >rg.gt_perf_status); > > + /* In units of 2.5 millivolt */ > > + *val = > > DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * > > 25, 10); > > + return 0; > > + default: > > + return -EOPNOTSUPP; > > + } > > +} Thanks. -- Ashutosh
> -----Original Message----- > From: Dixit, Ashutosh <ashutosh.dixit@intel.com> > Sent: Monday, September 12, 2022 10:08 PM > To: Gupta, Anshuman <anshuman.gupta@intel.com> > Cc: Nilawar, Badal <badal.nilawar@intel.com>; intel-gfx@lists.freedesktop.org; > Tauro, Riana <riana.tauro@intel.com>; Ewins, Jon <jon.ewins@intel.com>; > linux-hwmon@vger.kernel.org > Subject: Re: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage > support > > On Mon, 12 Sep 2022 07:09:28 -0700, Gupta, Anshuman wrote: > > > > > +static int > > > +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) { > > > + struct i915_hwmon *hwmon = ddat->hwmon; > > > + intel_wakeref_t wakeref; > > > + u32 reg_value; > > > + > > > + switch (attr) { > > > + case hwmon_in_input: > > > > Other attributes in this series take hwmon->lock before accessing i915 > > registers , So do we need lock here as well ? > > The lock is being taken only for RMW and for making sure energy counter > updates happen atomically. We are not taking the lock for just reads so IMO no > lock is needed here. If that is the case, then why it needs to grab a lock for rmw on different Register ? Like in patch 3 of this series grabs hwmon->howmon_lock for rmw on two different register pkg_power_sku_unit and pkg_rapl_limit. One register rmw should be independent of other register here ? Thanks, Anshuman Gupta > > > > + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > > > + reg_value = intel_uncore_read(ddat->uncore, hwmon- > > > >rg.gt_perf_status); > > > + /* In units of 2.5 millivolt */ > > > + *val = > > > DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) > * 25, > > > 10); > > > + return 0; > > > + default: > > > + return -EOPNOTSUPP; > > > + } > > > +} > > Thanks. > -- > Ashutosh
> -----Original Message----- > From: Dixit, Ashutosh <ashutosh.dixit@intel.com> > Sent: Tuesday, September 13, 2022 8:49 PM > To: Gupta, Anshuman <anshuman.gupta@intel.com> > Cc: Nilawar, Badal <badal.nilawar@intel.com>; intel-gfx@lists.freedesktop.org; > Tauro, Riana <riana.tauro@intel.com>; Ewins, Jon <jon.ewins@intel.com>; > linux-hwmon@vger.kernel.org > Subject: Re: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage > support > > On Tue, 13 Sep 2022 01:11:57 -0700, Gupta, Anshuman wrote: > > > > Hi Anshuman, > > > > -----Original Message----- > > > From: Dixit, Ashutosh <ashutosh.dixit@intel.com> > > > Sent: Monday, September 12, 2022 10:08 PM > > > To: Gupta, Anshuman <anshuman.gupta@intel.com> > > > Cc: Nilawar, Badal <badal.nilawar@intel.com>; > > > intel-gfx@lists.freedesktop.org; Tauro, Riana > > > <riana.tauro@intel.com>; Ewins, Jon <jon.ewins@intel.com>; > > > linux-hwmon@vger.kernel.org > > > Subject: Re: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage > > > support > > > > > > On Mon, 12 Sep 2022 07:09:28 -0700, Gupta, Anshuman wrote: > > > > > > > > > +static int > > > > > +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) { > > > > > + struct i915_hwmon *hwmon = ddat->hwmon; > > > > > + intel_wakeref_t wakeref; > > > > > + u32 reg_value; > > > > > + > > > > > + switch (attr) { > > > > > + case hwmon_in_input: > > > > > > > > Other attributes in this series take hwmon->lock before accessing > > > > i915 registers , So do we need lock here as well ? > > > > > > The lock is being taken only for RMW and for making sure energy > > > counter updates happen atomically. We are not taking the lock for > > > just reads so IMO no lock is needed here. > > > > If that is the case, then why it needs to grab a lock for rmw on > > different Register ? Like in patch 3 of this series grabs > > hwmon->howmon_lock for rmw on two different register > > hwmon->pkg_power_sku_unit > > and pkg_rapl_limit. > > I don't see this happening, where do you see it? > > > One register rmw should be independent of other register here ? > > Yes each register RMW is independent. In Patch 3 only hwm_power_write (not > hwm_power_read) is taking the lock for RMW on pkg_rapl_limit (lock is not > taken for pkg_power_sku_unit). So the assumption is that RMW of a single > register are not atomic and must be serialized with a lock. Reads are considered > to not need a lock. Thanks for explanation , and my apologies for the noise. Br, Anshuman Gupta. > > Thanks. > -- > Ashutosh > > > > > > > > > > + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > > > > > + reg_value = intel_uncore_read(ddat->uncore, > hwmon- > > > > > >rg.gt_perf_status); > > > > > + /* In units of 2.5 millivolt */ > > > > > + *val = > > > > > DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, > reg_value) > > > * 25, > > > > > 10); > > > > > + return 0; > > > > > + default: > > > > > + return -EOPNOTSUPP; > > > > > + } > > > > > +} > > > > > > Thanks. > > > -- > > > Ashutosh
On 29-08-2022 23:00, Dixit, Ashutosh wrote: > On Thu, 25 Aug 2022 06:21:13 -0700, Badal Nilawar wrote: >> >> From: Riana Tauro <riana.tauro@intel.com> >> >> Use i915 HWMON subsystem to display current input voltage. > > A couple of suggestions to improve comments in this patch below and after > addressing those this patch is: > > Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > >> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c >> index 103dd543a214..2192d0fd4c66 100644 >> --- a/drivers/gpu/drm/i915/i915_hwmon.c >> +++ b/drivers/gpu/drm/i915/i915_hwmon.c >> @@ -11,8 +11,10 @@ >> #include "i915_hwmon.h" >> #include "i915_reg.h" >> #include "intel_mchbar_regs.h" >> +#include "gt/intel_gt_regs.h" > > In later patches we have added units for different quantities here. So I > think we should add those units for voltage to this patch too. It's in > Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon but I think it's > better to add to this file too otherwise if anyone looks at it is seems to > be missing. > > So I would add the following to this patch: > > /* > * SF_* - scale factors for particular quantities according to hwmon spec. > * - voltage - millivolts > */ Sure I will add above comment. > #define SF_VOLTAGE 1000 we are not using above scale factor for voltage. As our scale factor is 2.5 millivolts shall I add like. #define SF_VOLTAGE_MUL 25 #define SF_VOLTAGE_DIV 10 > >> +static int >> +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) >> +{ >> + struct i915_hwmon *hwmon = ddat->hwmon; >> + intel_wakeref_t wakeref; >> + u32 reg_value; >> + >> + switch (attr) { >> + case hwmon_in_input: >> + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) >> + reg_value = intel_uncore_read(ddat->uncore, hwmon->rg.gt_perf_status); >> + /* In units of 2.5 millivolt */ >> + *val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * 25, 10); And use above scale factors here. *val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * SF_VOLTAGE_MUL, SF_VOLTAGE_DIV); Regards, Badal > > Let's complete this comment to so that it is clear what's happening: > > /* HW register value is in units of 2.5 millivolt */
On Thu, 15 Sep 2022 07:40:37 -0700, Nilawar, Badal wrote: > > On 29-08-2022 23:00, Dixit, Ashutosh wrote: > > On Thu, 25 Aug 2022 06:21:13 -0700, Badal Nilawar wrote: > >> > >> +static int > >> +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) > >> +{ > >> + struct i915_hwmon *hwmon = ddat->hwmon; > >> + intel_wakeref_t wakeref; > >> + u32 reg_value; > >> + > >> + switch (attr) { > >> + case hwmon_in_input: > >> + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) > >> + reg_value = intel_uncore_read(ddat->uncore, hwmon->rg.gt_perf_status); > >> + /* In units of 2.5 millivolt */ > >> + *val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * 25, 10); > > And use above scale factors here. > *val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * > SF_VOLTAGE_MUL, SF_VOLTAGE_DIV); > Regards, > Badal > > > > Let's complete this comment to so that it is clear what's happening: > > > > /* HW register value is in units of 2.5 millivolt */ This was missed in the latest rev so if we could remember to add this that would be great.
diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon new file mode 100644 index 000000000000..24c4b7477d51 --- /dev/null +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon @@ -0,0 +1,7 @@ +What: /sys/devices/.../hwmon/hwmon<i>/in0_input +Date: June 2022 +KernelVersion: 5.19 +Contact: dri-devel@lists.freedesktop.org +Description: RO. Current Voltage in millivolt. + + Only supported for particular Intel i915 graphics platforms. diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index 94f9ddcfb3a5..5d4fbda4d326 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -1508,6 +1508,9 @@ #define VLV_RENDER_C0_COUNT _MMIO(0x138118) #define VLV_MEDIA_C0_COUNT _MMIO(0x13811c) +#define GEN12_RPSTAT1 _MMIO(0x1381b4) +#define GEN12_VOLTAGE_MASK REG_GENMASK(10, 0) + #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4)) #define GEN11_CSME (31) #define GEN11_GUNIT (28) diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 103dd543a214..2192d0fd4c66 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -11,8 +11,10 @@ #include "i915_hwmon.h" #include "i915_reg.h" #include "intel_mchbar_regs.h" +#include "gt/intel_gt_regs.h" struct hwm_reg { + i915_reg_t gt_perf_status; }; struct hwm_drvdata { @@ -29,14 +31,49 @@ struct i915_hwmon { }; static const struct hwmon_channel_info *hwm_info[] = { + HWMON_CHANNEL_INFO(in, HWMON_I_INPUT), NULL }; +static umode_t +hwm_in_is_visible(const struct hwm_drvdata *ddat, u32 attr) +{ + switch (attr) { + case hwmon_in_input: + return i915_mmio_reg_valid(ddat->hwmon->rg.gt_perf_status) ? 0444 : 0; + default: + return 0; + } +} + +static int +hwm_in_read(struct hwm_drvdata *ddat, u32 attr, long *val) +{ + struct i915_hwmon *hwmon = ddat->hwmon; + intel_wakeref_t wakeref; + u32 reg_value; + + switch (attr) { + case hwmon_in_input: + with_intel_runtime_pm(ddat->uncore->rpm, wakeref) + reg_value = intel_uncore_read(ddat->uncore, hwmon->rg.gt_perf_status); + /* In units of 2.5 millivolt */ + *val = DIV_ROUND_CLOSEST(REG_FIELD_GET(GEN12_VOLTAGE_MASK, reg_value) * 25, 10); + return 0; + default: + return -EOPNOTSUPP; + } +} + static umode_t hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type, u32 attr, int channel) { + struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata; + switch (type) { + case hwmon_in: + return hwm_in_is_visible(ddat, attr); default: return 0; } @@ -46,7 +83,11 @@ static int hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr, int channel, long *val) { + struct hwm_drvdata *ddat = dev_get_drvdata(dev); + switch (type) { + case hwmon_in: + return hwm_in_read(ddat, attr, val); default: return -EOPNOTSUPP; } @@ -76,6 +117,12 @@ static const struct hwmon_chip_info hwm_chip_info = { static void hwm_get_preregistration_info(struct drm_i915_private *i915) { + struct i915_hwmon *hwmon = i915->hwmon; + + if (IS_DG1(i915) || IS_DG2(i915)) + hwmon->rg.gt_perf_status = GEN12_RPSTAT1; + else + hwmon->rg.gt_perf_status = INVALID_MMIO_REG; } void i915_hwmon_register(struct drm_i915_private *i915)