Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
On Mon, 03 Oct 2022 13:56:05 -0700, Andi Shyti wrote:
Hi Andi,
Badal is out for a bit so I am posting this version of the patches.
>
> Hi Badal,
>
> [...]
>
> > static void
> > hwm_get_preregistration_info(struct drm_i915_private *i915)
> > {
> > + struct i915_hwmon *hwmon = i915->hwmon;
> > +
> > + if (IS_DG1(i915) || IS_DG2(i915))
>
> why not GRAPHICS_VER(i915) >= 12 here?
Thanks for catching this, because GEN12_RPSTAT1 is indeed available for all
Gen12+. It was done this way because the voltage bits of GEN12_RPSTAT1 are
only available for DG1/DG2. Anyway in v9 I have changed this to just:
/* Available for all Gen12+/dGfx */
hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
That is because hwmon is only availbable for dGfx (there's a check in Patch
1). Also, because of this change the 'IS_DG1(i915) || IS_DG2(i915)' check
has been moved to hwm_in_is_visible.
Thanks.
--
Ashutosh
> > + 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
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
Hi Badal,
[...]
> static void
> hwm_get_preregistration_info(struct drm_i915_private *i915)
> {
> + struct i915_hwmon *hwmon = i915->hwmon;
> +
> + if (IS_DG1(i915) || IS_DG2(i915))
why not GRAPHICS_VER(i915) >= 12 here?
Andi
> + 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
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
On 9/16/2022 8:30 PM, Badal Nilawar wrote:
From: Riana Tauro
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)
v5:
- Added unit of voltage as millivolts (Ashutosh)
- Updated date, kernel version in documentation
Cc: Guenter Roeck
Cc: Anshuman Gupta
Signed-off-by: Riana Tauro
Signed-off-by: Badal Nilawar
Acked-by: Guenter Roeck
Reviewed-by: Ashutosh Dixit
Looks good to me.
Reviewed-by: Anshuman Gupta
---
.../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 | 53 +++
3 files changed, 63 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 ..e2974f928e58
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -0,0 +1,7 @@
+What: /sys/devices/.../hwmon/hwmon/in0_input
+Date: September 2022
+KernelVersion: 6
+Contact: dri-de...@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 2275ee47da95..65336514554d 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -1510,6 +1510,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..45745afa5c5b 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -11,8 +11,16 @@
#include "i915_hwmon.h"
#include "i915_reg.h"
#include "intel_mchbar_regs.h"
+#include "gt/intel_gt_regs.h"
+
+/*
+ * SF_* - scale factors for particular quantities according to hwmon spec.
+ * - voltage - millivolts
+ */
+#define SF_VOLTAGE 1000
struct hwm_reg {
+ i915_reg_t gt_perf_status;
};
struct hwm_drvdata {
@@ -29,14 +37,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);
+ /* HW register value 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 +89,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 +123,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 =
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
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.
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
On 29-08-2022 23:00, Dixit, Ashutosh wrote:
On Thu, 25 Aug 2022 06:21:13 -0700, Badal Nilawar wrote:
From: Riana Tauro
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
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 */
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
> -Original Message-
> From: Dixit, Ashutosh
> Sent: Tuesday, September 13, 2022 8:49 PM
> To: Gupta, Anshuman
> Cc: Nilawar, Badal ; intel-gfx@lists.freedesktop.org;
> Tauro, Riana ; Ewins, Jon ;
> linux-hw...@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
> > > Sent: Monday, September 12, 2022 10:08 PM
> > > To: Gupta, Anshuman
> > > Cc: Nilawar, Badal ;
> > > intel-gfx@lists.freedesktop.org; Tauro, Riana
> > > ; Ewins, Jon ;
> > > linux-hw...@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
Re: [Intel-gfx] [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
> > Sent: Monday, September 12, 2022 10:08 PM
> > To: Gupta, Anshuman
> > Cc: Nilawar, Badal ;
> > intel-gfx@lists.freedesktop.org;
> > Tauro, Riana ; Ewins, Jon ;
> > linux-hw...@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.
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.
--
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
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
> -Original Message-
> From: Dixit, Ashutosh
> Sent: Monday, September 12, 2022 10:08 PM
> To: Gupta, Anshuman
> Cc: Nilawar, Badal ; intel-gfx@lists.freedesktop.org;
> Tauro, Riana ; Ewins, Jon ;
> linux-hw...@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
Re: [Intel-gfx] [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.
> > + 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
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
> -Original Message-
> From: Nilawar, Badal
> Sent: Thursday, August 25, 2022 6:51 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Dixit, Ashutosh ; Tauro, Riana
> ; Gupta, Anshuman ;
> Ewins, Jon ; linux-hw...@vger.kernel.org
> Subject: [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
>
> From: Riana Tauro
>
> 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
> Cc: Anshuman Gupta
> Signed-off-by: Riana Tauro
> Signed-off-by: Badal Nilawar
> Acked-by: Guenter Roeck
> ---
> .../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 ..24c4b7477d51
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> @@ -0,0 +1,7 @@
> +What:/sys/devices/.../hwmon/hwmon/in0_input
> +Date:June 2022
> +KernelVersion: 5.19
> +Contact: dri-de...@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
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
On Thu, 25 Aug 2022 06:21:13 -0700, Badal Nilawar wrote:
>
> From: Riana Tauro
>
> 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
> 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 */
Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support
On 8/12/22 10:37, Badal Nilawar wrote:
From: Riana Tauro
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
Cc: Anshuman Gupta
Signed-off-by: Riana Tauro
Signed-off-by: Badal Nilawar
Acked-by: Guenter Roeck
---
.../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 ..24c4b7477d51
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -0,0 +1,7 @@
+What: /sys/devices/.../hwmon/hwmon/in0_input
+Date: June 2022
+KernelVersion: 5.19
+Contact: dri-de...@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 b3b49f6d6d1c..4604f6dbf8b6 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
+++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
@@ -1498,6 +1498,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 5b80a0f024f0..1893efe796a4 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -10,8 +10,10 @@
#include "i915_drv.h"
#include "i915_hwmon.h"
#include "intel_mchbar_regs.h"
+#include "gt/intel_gt_regs.h"
struct hwm_reg {
+ i915_reg_t gt_perf_status;
};
struct hwm_drvdata {
@@ -28,14 +30,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;
}
@@ -45,7 +82,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;
}
@@ -75,6 +116,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)
