Re: [Intel-gfx] [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support

Gupta, Anshuman
2022-09-12T14:09:43Z

> -----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/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