✗ Fi.CI.BUILD: failure for drm/i915/hwmon: expose package temperature

✗ Fi.CI.BUILD: failure for drm/i915/hwmon: expose package temperature

[Patchwork]

== Series Details ==

Series: drm/i915/hwmon: expose package temperature
URL   : https://patchwork.freedesktop.org/series/137874/
State : failure

== Summary ==

Error: patch https://patchwork.freedesktop.org/api/1.0/series/137874/revisions/1/mbox/ not applied
Applying: drm/i915/hwmon: expose package temperature
Using index info to reconstruct a base tree...
M	Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
M	drivers/gpu/drm/i915/i915_hwmon.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_hwmon.c
Auto-merging Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
CONFLICT (content): Merge conflict in Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 drm/i915/hwmon: expose package temperature
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Build failed, no error log produced

[PATCH v1] drm/i915/hwmon: expose package temperature

[Raag Jadav]

Add hwmon support for temp1_input attribute, which will expose package
temperature in millidegree Celsius. With this in place we can monitor
package temperature using lm-sensors tool.

$ sensors
i915-pci-0300
Adapter: PCI adapter
in0:         990.00 mV
fan1:        1260 RPM
temp1:        +45.0°C
power1:           N/A  (max =  35.00 W)
energy1:      12.62 kJ

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
 drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
 3 files changed, 51 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
index be4141a7522f..a885e5316d02 100644
--- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
+++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
@@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
 Description:	RO. Fan speed of device in RPM.
 
 		Only supported for particular Intel i915 graphics platforms.
+
+What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
+Date:		November 2024
+KernelVersion:	6.12
+Contact:	intel-gfx@lists.freedesktop.org
+Description:	RO. GPU package temperature in millidegree Celsius.
+
+		Only supported for particular Intel i915 graphics platforms.
diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
index 17d30f6b84b0..9f1a2300510b 100644
--- a/drivers/gpu/drm/i915/i915_hwmon.c
+++ b/drivers/gpu/drm/i915/i915_hwmon.c
@@ -7,6 +7,7 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/jiffies.h>
 #include <linux/types.h>
+#include <linux/units.h>
 
 #include "i915_drv.h"
 #include "i915_hwmon.h"
@@ -32,6 +33,7 @@
 
 struct hwm_reg {
 	i915_reg_t gt_perf_status;
+	i915_reg_t pkg_temp;
 	i915_reg_t pkg_power_sku_unit;
 	i915_reg_t pkg_power_sku;
 	i915_reg_t pkg_rapl_limit;
@@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
 };
 
 static const struct hwmon_channel_info * const hwm_info[] = {
+	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
 	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
 	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
 	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
@@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
 				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
 }
 
+static umode_t
+hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
+{
+	struct i915_hwmon *hwmon = ddat->hwmon;
+
+	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
+		return 0444;
+
+	return 0;
+}
+
+static int
+hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
+{
+	struct i915_hwmon *hwmon = ddat->hwmon;
+	intel_wakeref_t wakeref;
+	u32 reg_val;
+
+	if (attr == hwmon_temp_input) {
+		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
+			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
+
+		/* HW register value is in degrees, convert to millidegrees. */
+		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static umode_t
 hwm_in_is_visible(const struct hwm_drvdata *ddat, u32 attr)
 {
@@ -692,6 +725,8 @@ hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
 	struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
 
 	switch (type) {
+	case hwmon_temp:
+		return hwm_temp_is_visible(ddat, attr);
 	case hwmon_in:
 		return hwm_in_is_visible(ddat, attr);
 	case hwmon_power:
@@ -714,6 +749,8 @@ hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
 	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
 
 	switch (type) {
+	case hwmon_temp:
+		return hwm_temp_read(ddat, attr, val);
 	case hwmon_in:
 		return hwm_in_read(ddat, attr, val);
 	case hwmon_power:
@@ -810,6 +847,7 @@ hwm_get_preregistration_info(struct drm_i915_private *i915)
 	hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
 
 	if (IS_DG1(i915) || IS_DG2(i915)) {
+		hwmon->rg.pkg_temp = PCU_PACKAGE_TEMPERATURE;
 		hwmon->rg.pkg_power_sku_unit = PCU_PACKAGE_POWER_SKU_UNIT;
 		hwmon->rg.pkg_power_sku = PCU_PACKAGE_POWER_SKU;
 		hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT;
@@ -817,6 +855,7 @@ hwm_get_preregistration_info(struct drm_i915_private *i915)
 		hwmon->rg.energy_status_tile = INVALID_MMIO_REG;
 		hwmon->rg.fan_speed = PCU_PWM_FAN_SPEED;
 	} else {
+		hwmon->rg.pkg_temp = INVALID_MMIO_REG;
 		hwmon->rg.pkg_power_sku_unit = INVALID_MMIO_REG;
 		hwmon->rg.pkg_power_sku = INVALID_MMIO_REG;
 		hwmon->rg.pkg_rapl_limit = INVALID_MMIO_REG;
diff --git a/drivers/gpu/drm/i915/intel_mchbar_regs.h b/drivers/gpu/drm/i915/intel_mchbar_regs.h
index 73900c098d59..dc2477179c3e 100644
--- a/drivers/gpu/drm/i915/intel_mchbar_regs.h
+++ b/drivers/gpu/drm/i915/intel_mchbar_regs.h
@@ -207,6 +207,10 @@
 #define PCU_PACKAGE_ENERGY_STATUS              _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x593c)
 
 #define GEN6_GT_PERF_STATUS			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5948)
+
+#define PCU_PACKAGE_TEMPERATURE			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5978)
+#define   TEMP_MASK				REG_GENMASK(7, 0)
+
 #define GEN6_RP_STATE_LIMITS			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5994)
 #define GEN6_RP_STATE_CAP			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5998)
 #define   RP0_CAP_MASK				REG_GENMASK(7, 0)
-- 
2.34.1

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Andy Shevchenko]

On Wed, Aug 28, 2024 at 10:15:12AM +0530, Raag Jadav wrote:
> Add hwmon support for temp1_input attribute, which will expose package
> temperature in millidegree Celsius. With this in place we can monitor
> package temperature using lm-sensors tool.
> 
> $ sensors
> i915-pci-0300
> Adapter: PCI adapter
> in0:         990.00 mV
> fan1:        1260 RPM
> temp1:        +45.0°C
> power1:           N/A  (max =  35.00 W)
> energy1:      12.62 kJ

...

> +static umode_t
> +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> +{
> +	struct i915_hwmon *hwmon = ddat->hwmon;
> +
> +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
> +		return 0444;
> +
> +	return 0;

Just a question (I'm fine with this implementation): is the style in this file
to check for correct cases first and return an err/etc at the end?

> +}
> +
> +static int
> +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> +{
> +	struct i915_hwmon *hwmon = ddat->hwmon;
> +	intel_wakeref_t wakeref;
> +	u32 reg_val;
> +
> +	if (attr == hwmon_temp_input) {
> +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
> +
> +		/* HW register value is in degrees, convert to millidegrees. */
> +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> +		return 0;
> +	}

...because here we may drop an indentation level by doing it opposite

	if (x != y)
		return -E...;

	...

> +	return -EOPNOTSUPP;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Raag Jadav]

On Wed, Aug 28, 2024 at 04:59:00PM +0300, Andy Shevchenko wrote:
> On Wed, Aug 28, 2024 at 10:15:12AM +0530, Raag Jadav wrote:
> > Add hwmon support for temp1_input attribute, which will expose package
> > temperature in millidegree Celsius. With this in place we can monitor
> > package temperature using lm-sensors tool.
> > 
> > $ sensors
> > i915-pci-0300
> > Adapter: PCI adapter
> > in0:         990.00 mV
> > fan1:        1260 RPM
> > temp1:        +45.0°C
> > power1:           N/A  (max =  35.00 W)
> > energy1:      12.62 kJ
> 
> ...
> 
> > +static umode_t
> > +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> > +{
> > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > +
> > +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
> > +		return 0444;
> > +
> > +	return 0;
> 
> Just a question (I'm fine with this implementation): is the style in this file
> to check for correct cases first and return an err/etc at the end?

The convention is to use switch case with err being the default, so I'd say yes.

> > +}
> > +
> > +static int
> > +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> > +{
> > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > +	intel_wakeref_t wakeref;
> > +	u32 reg_val;
> > +
> > +	if (attr == hwmon_temp_input) {
> > +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
> > +
> > +		/* HW register value is in degrees, convert to millidegrees. */
> > +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> > +		return 0;
> > +	}
> 
> ...because here we may drop an indentation level by doing it opposite
> 
> 	if (x != y)
> 		return -E...;
> 

True, but the idea is to allow more cases in the future with minimal changes.

Raag

✗ Fi.CI.BAT: failure for drm/i915/hwmon: expose package temperature (rev2)

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4529 bytes --]

== Series Details ==

Series: drm/i915/hwmon: expose package temperature (rev2)
URL   : https://patchwork.freedesktop.org/series/137874/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_15354 -> Patchwork_137874v2
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_137874v2 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_137874v2, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/index.html

Participating hosts (37 -> 37)
------------------------------

  Additional (1): fi-kbl-8809g 
  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_137874v2:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_mocs:
    - bat-arls-5:         [PASS][1] -> [DMESG-FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15354/bat-arls-5/igt@i915_selftest@live@gt_mocs.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/bat-arls-5/igt@i915_selftest@live@gt_mocs.html

  
Known issues
------------

  Here are the changes found in Patchwork_137874v2 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_huc_copy@huc-copy:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][3] ([i915#2190])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/fi-kbl-8809g/igt@gem_huc_copy@huc-copy.html

  * igt@gem_lmem_swapping@basic:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][4] ([i915#4613]) +3 other tests skip
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/fi-kbl-8809g/igt@gem_lmem_swapping@basic.html

  * igt@i915_selftest@live@hangcheck:
    - bat-arls-1:         [PASS][5] -> [DMESG-WARN][6] ([i915#11349])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15354/bat-arls-1/igt@i915_selftest@live@hangcheck.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/bat-arls-1/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@workarounds:
    - bat-mtlp-8:         [PASS][7] -> [ABORT][8] ([i915#12062])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15354/bat-mtlp-8/igt@i915_selftest@live@workarounds.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/bat-mtlp-8/igt@i915_selftest@live@workarounds.html

  * igt@kms_force_connector_basic@force-load-detect:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][9] +30 other tests skip
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/fi-kbl-8809g/igt@kms_force_connector_basic@force-load-detect.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - bat-arls-2:         [DMESG-WARN][10] ([i915#11349]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15354/bat-arls-2/igt@i915_selftest@live@hangcheck.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/bat-arls-2/igt@i915_selftest@live@hangcheck.html

  
#### Warnings ####

  * igt@fbdev@read:
    - bat-arls-1:         [DMESG-FAIL][12] ([i915#12102]) -> [DMESG-WARN][13] ([i915#12102])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15354/bat-arls-1/igt@fbdev@read.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/bat-arls-1/igt@fbdev@read.html

  
  [i915#11349]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11349
  [i915#12062]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12062
  [i915#12102]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12102
  [i915#2190]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/2190
  [i915#4613]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/4613


Build changes
-------------

  * Linux: CI_DRM_15354 -> Patchwork_137874v2

  CI-20190529: 20190529
  CI_DRM_15354: 7378077b0be32160022a73b7050b963d52364618 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8001: d3a77fc98e89cc94b03be2b0903d44f83480b8a0 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_137874v2: 7378077b0be32160022a73b7050b963d52364618 @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v2/index.html

[-- Attachment #2: Type: text/html, Size: 5479 bytes --]

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Nilawar, Badal]

On 28-08-2024 10:15, Raag Jadav wrote:
> Add hwmon support for temp1_input attribute, which will expose package
> temperature in millidegree Celsius. With this in place we can monitor
> package temperature using lm-sensors tool.
> 
> $ sensors
> i915-pci-0300
> Adapter: PCI adapter
> in0:         990.00 mV
> fan1:        1260 RPM
> temp1:        +45.0°C
> power1:           N/A  (max =  35.00 W)
> energy1:      12.62 kJ
> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> ---
>   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
>   drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
>   drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
>   3 files changed, 51 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> index be4141a7522f..a885e5316d02 100644
> --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> @@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
>   Description:	RO. Fan speed of device in RPM.
>   
>   		Only supported for particular Intel i915 graphics platforms.
> +
> +What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
> +Date:		November 2024
> +KernelVersion:	6.12
> +Contact:	intel-gfx@lists.freedesktop.org
> +Description:	RO. GPU package temperature in millidegree Celsius.
> +
> +		Only supported for particular Intel i915 graphics platforms.
> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> index 17d30f6b84b0..9f1a2300510b 100644
> --- a/drivers/gpu/drm/i915/i915_hwmon.c
> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> @@ -7,6 +7,7 @@
>   #include <linux/hwmon-sysfs.h>
>   #include <linux/jiffies.h>
>   #include <linux/types.h>
> +#include <linux/units.h>
>   
>   #include "i915_drv.h"
>   #include "i915_hwmon.h"
> @@ -32,6 +33,7 @@
>   
>   struct hwm_reg {
>   	i915_reg_t gt_perf_status;
> +	i915_reg_t pkg_temp;
>   	i915_reg_t pkg_power_sku_unit;
>   	i915_reg_t pkg_power_sku;
>   	i915_reg_t pkg_rapl_limit;
> @@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
>   };
>   
>   static const struct hwmon_channel_info * const hwm_info[] = {
> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
>   	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
>   	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
>   	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
> @@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
>   				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
>   }
>   
> +static umode_t
> +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> +{
> +	struct i915_hwmon *hwmon = ddat->hwmon;
> +
> +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static int
> +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> +{
> +	struct i915_hwmon *hwmon = ddat->hwmon;
> +	intel_wakeref_t wakeref;
> +	u32 reg_val;
> +
> +	if (attr == hwmon_temp_input) {
> +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
> +
> +		/* HW register value is in degrees, convert to millidegrees. */
> +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> +		return 0;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
Let's try to have synergy between previous attribute, such as 
hwm_fan_input, and this one.

Regards,
Badal
> +
>   static umode_t
>   hwm_in_is_visible(const struct hwm_drvdata *ddat, u32 attr)
>   {
> @@ -692,6 +725,8 @@ hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
>   	struct hwm_drvdata *ddat = (struct hwm_drvdata *)drvdata;
>   
>   	switch (type) {
> +	case hwmon_temp:
> +		return hwm_temp_is_visible(ddat, attr);
>   	case hwmon_in:
>   		return hwm_in_is_visible(ddat, attr);
>   	case hwmon_power:
> @@ -714,6 +749,8 @@ hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>   	struct hwm_drvdata *ddat = dev_get_drvdata(dev);
>   
>   	switch (type) {
> +	case hwmon_temp:
> +		return hwm_temp_read(ddat, attr, val);
>   	case hwmon_in:
>   		return hwm_in_read(ddat, attr, val);
>   	case hwmon_power:
> @@ -810,6 +847,7 @@ hwm_get_preregistration_info(struct drm_i915_private *i915)
>   	hwmon->rg.gt_perf_status = GEN12_RPSTAT1;
>   
>   	if (IS_DG1(i915) || IS_DG2(i915)) {
> +		hwmon->rg.pkg_temp = PCU_PACKAGE_TEMPERATURE;
>   		hwmon->rg.pkg_power_sku_unit = PCU_PACKAGE_POWER_SKU_UNIT;
>   		hwmon->rg.pkg_power_sku = PCU_PACKAGE_POWER_SKU;
>   		hwmon->rg.pkg_rapl_limit = PCU_PACKAGE_RAPL_LIMIT;
> @@ -817,6 +855,7 @@ hwm_get_preregistration_info(struct drm_i915_private *i915)
>   		hwmon->rg.energy_status_tile = INVALID_MMIO_REG;
>   		hwmon->rg.fan_speed = PCU_PWM_FAN_SPEED;
>   	} else {
> +		hwmon->rg.pkg_temp = INVALID_MMIO_REG;
>   		hwmon->rg.pkg_power_sku_unit = INVALID_MMIO_REG;
>   		hwmon->rg.pkg_power_sku = INVALID_MMIO_REG;
>   		hwmon->rg.pkg_rapl_limit = INVALID_MMIO_REG;
> diff --git a/drivers/gpu/drm/i915/intel_mchbar_regs.h b/drivers/gpu/drm/i915/intel_mchbar_regs.h
> index 73900c098d59..dc2477179c3e 100644
> --- a/drivers/gpu/drm/i915/intel_mchbar_regs.h
> +++ b/drivers/gpu/drm/i915/intel_mchbar_regs.h
> @@ -207,6 +207,10 @@
>   #define PCU_PACKAGE_ENERGY_STATUS              _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x593c)
>   
>   #define GEN6_GT_PERF_STATUS			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5948)
> +
> +#define PCU_PACKAGE_TEMPERATURE			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5978)
> +#define   TEMP_MASK				REG_GENMASK(7, 0)
> +
>   #define GEN6_RP_STATE_LIMITS			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5994)
>   #define GEN6_RP_STATE_CAP			_MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5998)
>   #define   RP0_CAP_MASK				REG_GENMASK(7, 0)

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Raag Jadav]

On Thu, Sep 05, 2024 at 11:56:15AM +0530, Nilawar, Badal wrote:
> 
> 
> On 28-08-2024 10:15, Raag Jadav wrote:
> > Add hwmon support for temp1_input attribute, which will expose package
> > temperature in millidegree Celsius. With this in place we can monitor
> > package temperature using lm-sensors tool.
> > 
> > $ sensors
> > i915-pci-0300
> > Adapter: PCI adapter
> > in0:         990.00 mV
> > fan1:        1260 RPM
> > temp1:        +45.0°C
> > power1:           N/A  (max =  35.00 W)
> > energy1:      12.62 kJ
> > 
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
> >   drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
> >   drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
> >   3 files changed, 51 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > index be4141a7522f..a885e5316d02 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > @@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
> >   Description:	RO. Fan speed of device in RPM.
> >   		Only supported for particular Intel i915 graphics platforms.
> > +
> > +What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
> > +Date:		November 2024
> > +KernelVersion:	6.12
> > +Contact:	intel-gfx@lists.freedesktop.org
> > +Description:	RO. GPU package temperature in millidegree Celsius.
> > +
> > +		Only supported for particular Intel i915 graphics platforms.
> > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > index 17d30f6b84b0..9f1a2300510b 100644
> > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > @@ -7,6 +7,7 @@
> >   #include <linux/hwmon-sysfs.h>
> >   #include <linux/jiffies.h>
> >   #include <linux/types.h>
> > +#include <linux/units.h>
> >   #include "i915_drv.h"
> >   #include "i915_hwmon.h"
> > @@ -32,6 +33,7 @@
> >   struct hwm_reg {
> >   	i915_reg_t gt_perf_status;
> > +	i915_reg_t pkg_temp;
> >   	i915_reg_t pkg_power_sku_unit;
> >   	i915_reg_t pkg_power_sku;
> >   	i915_reg_t pkg_rapl_limit;
> > @@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
> >   };
> >   static const struct hwmon_channel_info * const hwm_info[] = {
> > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> >   	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> >   	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
> >   	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
> > @@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
> >   				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
> >   }
> > +static umode_t
> > +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> > +{
> > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > +
> > +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
> > +		return 0444;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> > +{
> > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > +	intel_wakeref_t wakeref;
> > +	u32 reg_val;
> > +
> > +	if (attr == hwmon_temp_input) {
> > +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
> > +
> > +		/* HW register value is in degrees, convert to millidegrees. */
> > +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> > +		return 0;
> > +	}
> > +
> > +	return -EOPNOTSUPP;
> > +}
> Let's try to have synergy between previous attribute, such as hwm_fan_input,
> and this one.

This one's simple enough to be inline IMHO.
Besides, it's already in synergy with hwm_in_read() which has similar
implementation.

Raag

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Anshuman Gupta]

On 2024-09-05 at 11:55:23 +0300, Raag Jadav wrote:
> On Thu, Sep 05, 2024 at 11:56:15AM +0530, Nilawar, Badal wrote:
> > 
> > 
> > On 28-08-2024 10:15, Raag Jadav wrote:
> > > Add hwmon support for temp1_input attribute, which will expose package
> > > temperature in millidegree Celsius. With this in place we can monitor
> > > package temperature using lm-sensors tool.
> > > 
> > > $ sensors
> > > i915-pci-0300
> > > Adapter: PCI adapter
> > > in0:         990.00 mV
> > > fan1:        1260 RPM
> > > temp1:        +45.0°C
> > > power1:           N/A  (max =  35.00 W)
> > > energy1:      12.62 kJ
> > > 
> > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
> > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > ---
> > >   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
> > >   drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
> > >   drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
> > >   3 files changed, 51 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > index be4141a7522f..a885e5316d02 100644
> > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > @@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
> > >   Description:	RO. Fan speed of device in RPM.
> > >   		Only supported for particular Intel i915 graphics platforms.
> > > +
> > > +What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
> > > +Date:		November 2024
> > > +KernelVersion:	6.12
> > > +Contact:	intel-gfx@lists.freedesktop.org
> > > +Description:	RO. GPU package temperature in millidegree Celsius.
> > > +
> > > +		Only supported for particular Intel i915 graphics platforms.
> > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > > index 17d30f6b84b0..9f1a2300510b 100644
> > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > @@ -7,6 +7,7 @@
> > >   #include <linux/hwmon-sysfs.h>
> > >   #include <linux/jiffies.h>
> > >   #include <linux/types.h>
> > > +#include <linux/units.h>
> > >   #include "i915_drv.h"
> > >   #include "i915_hwmon.h"
> > > @@ -32,6 +33,7 @@
> > >   struct hwm_reg {
> > >   	i915_reg_t gt_perf_status;
> > > +	i915_reg_t pkg_temp;
> > >   	i915_reg_t pkg_power_sku_unit;
> > >   	i915_reg_t pkg_power_sku;
> > >   	i915_reg_t pkg_rapl_limit;
> > > @@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
> > >   };
> > >   static const struct hwmon_channel_info * const hwm_info[] = {
> > > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> > >   	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> > >   	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
> > >   	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
> > > @@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
> > >   				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
> > >   }
> > > +static umode_t
> > > +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> > > +{
> > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > +
> > > +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
> > > +		return 0444;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int
> > > +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> > > +{
> > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > +	intel_wakeref_t wakeref;
> > > +	u32 reg_val;
> > > +
> > > +	if (attr == hwmon_temp_input) {
> > > +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > > +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
> > > +
> > > +		/* HW register value is in degrees, convert to millidegrees. */
> > > +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> > > +		return 0;
> > > +	}
> > > +
> > > +	return -EOPNOTSUPP;
> > > +}
> > Let's try to have synergy between previous attribute, such as hwm_fan_input,
> > and this one.
> 
> This one's simple enough to be inline IMHO.
> Besides, it's already in synergy with hwm_in_read() which has similar
> implementation.
Agree this is pretty simple to have an any helper but IMO it would have been cleaner to have a switch
like hwm_in_read() to return -EOPNOTSUPP in default case. i think that was reason switch case was 
used in entire file.

Thanks,
Anshuman

> 
> Raag

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Raag Jadav]

On Thu, Sep 05, 2024 at 07:39:31PM +0530, Anshuman Gupta wrote:
> On 2024-09-05 at 11:55:23 +0300, Raag Jadav wrote:
> > On Thu, Sep 05, 2024 at 11:56:15AM +0530, Nilawar, Badal wrote:
> > > 
> > > 
> > > On 28-08-2024 10:15, Raag Jadav wrote:
> > > > Add hwmon support for temp1_input attribute, which will expose package
> > > > temperature in millidegree Celsius. With this in place we can monitor
> > > > package temperature using lm-sensors tool.
> > > > 
> > > > $ sensors
> > > > i915-pci-0300
> > > > Adapter: PCI adapter
> > > > in0:         990.00 mV
> > > > fan1:        1260 RPM
> > > > temp1:        +45.0°C
> > > > power1:           N/A  (max =  35.00 W)
> > > > energy1:      12.62 kJ
> > > > 
> > > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
> > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > > ---
> > > >   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
> > > >   drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
> > > >   drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
> > > >   3 files changed, 51 insertions(+)
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > index be4141a7522f..a885e5316d02 100644
> > > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > @@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
> > > >   Description:	RO. Fan speed of device in RPM.
> > > >   		Only supported for particular Intel i915 graphics platforms.
> > > > +
> > > > +What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
> > > > +Date:		November 2024
> > > > +KernelVersion:	6.12
> > > > +Contact:	intel-gfx@lists.freedesktop.org
> > > > +Description:	RO. GPU package temperature in millidegree Celsius.
> > > > +
> > > > +		Only supported for particular Intel i915 graphics platforms.
> > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > index 17d30f6b84b0..9f1a2300510b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > @@ -7,6 +7,7 @@
> > > >   #include <linux/hwmon-sysfs.h>
> > > >   #include <linux/jiffies.h>
> > > >   #include <linux/types.h>
> > > > +#include <linux/units.h>
> > > >   #include "i915_drv.h"
> > > >   #include "i915_hwmon.h"
> > > > @@ -32,6 +33,7 @@
> > > >   struct hwm_reg {
> > > >   	i915_reg_t gt_perf_status;
> > > > +	i915_reg_t pkg_temp;
> > > >   	i915_reg_t pkg_power_sku_unit;
> > > >   	i915_reg_t pkg_power_sku;
> > > >   	i915_reg_t pkg_rapl_limit;
> > > > @@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
> > > >   };
> > > >   static const struct hwmon_channel_info * const hwm_info[] = {
> > > > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> > > >   	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> > > >   	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
> > > >   	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
> > > > @@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
> > > >   				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
> > > >   }
> > > > +static umode_t
> > > > +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> > > > +{
> > > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > > +
> > > > +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
> > > > +		return 0444;
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int
> > > > +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> > > > +{
> > > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > > +	intel_wakeref_t wakeref;
> > > > +	u32 reg_val;
> > > > +
> > > > +	if (attr == hwmon_temp_input) {
> > > > +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > > > +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
> > > > +
> > > > +		/* HW register value is in degrees, convert to millidegrees. */
> > > > +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> > > > +		return 0;
> > > > +	}
> > > > +
> > > > +	return -EOPNOTSUPP;
> > > > +}
> > > Let's try to have synergy between previous attribute, such as hwm_fan_input,
> > > and this one.
> > 
> > This one's simple enough to be inline IMHO.
> > Besides, it's already in synergy with hwm_in_read() which has similar
> > implementation.
> Agree this is pretty simple to have an any helper but IMO it would have been cleaner to have a switch
> like hwm_in_read() to return -EOPNOTSUPP in default case. i think that was reason switch case was 
> used in entire file.

Extending on the simplicity argument above, if() makes more sense for a single case.

Raag

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Anshuman Gupta]

On 2024-09-05 at 22:18:17 +0300, Raag Jadav wrote:
> On Thu, Sep 05, 2024 at 07:39:31PM +0530, Anshuman Gupta wrote:
> > On 2024-09-05 at 11:55:23 +0300, Raag Jadav wrote:
> > > On Thu, Sep 05, 2024 at 11:56:15AM +0530, Nilawar, Badal wrote:
> > > > 
> > > > 
> > > > On 28-08-2024 10:15, Raag Jadav wrote:
> > > > > Add hwmon support for temp1_input attribute, which will expose package
> > > > > temperature in millidegree Celsius. With this in place we can monitor
> > > > > package temperature using lm-sensors tool.
> > > > > 
> > > > > $ sensors
> > > > > i915-pci-0300
> > > > > Adapter: PCI adapter
> > > > > in0:         990.00 mV
> > > > > fan1:        1260 RPM
> > > > > temp1:        +45.0°C
> > > > > power1:           N/A  (max =  35.00 W)
> > > > > energy1:      12.62 kJ
> > > > > 
> > > > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
> > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > > > ---
> > > > >   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
> > > > >   drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
> > > > >   drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
> > > > >   3 files changed, 51 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > index be4141a7522f..a885e5316d02 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > @@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
> > > > >   Description:	RO. Fan speed of device in RPM.
> > > > >   		Only supported for particular Intel i915 graphics platforms.
> > > > > +
> > > > > +What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
> > > > > +Date:		November 2024
> > > > > +KernelVersion:	6.12
> > > > > +Contact:	intel-gfx@lists.freedesktop.org
> > > > > +Description:	RO. GPU package temperature in millidegree Celsius.
> > > > > +
> > > > > +		Only supported for particular Intel i915 graphics platforms.
> > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > > index 17d30f6b84b0..9f1a2300510b 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > > @@ -7,6 +7,7 @@
> > > > >   #include <linux/hwmon-sysfs.h>
> > > > >   #include <linux/jiffies.h>
> > > > >   #include <linux/types.h>
> > > > > +#include <linux/units.h>
> > > > >   #include "i915_drv.h"
> > > > >   #include "i915_hwmon.h"
> > > > > @@ -32,6 +33,7 @@
> > > > >   struct hwm_reg {
> > > > >   	i915_reg_t gt_perf_status;
> > > > > +	i915_reg_t pkg_temp;
> > > > >   	i915_reg_t pkg_power_sku_unit;
> > > > >   	i915_reg_t pkg_power_sku;
> > > > >   	i915_reg_t pkg_rapl_limit;
> > > > > @@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
> > > > >   };
> > > > >   static const struct hwmon_channel_info * const hwm_info[] = {
> > > > > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> > > > >   	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> > > > >   	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
> > > > >   	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
> > > > > @@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
> > > > >   				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
> > > > >   }
> > > > > +static umode_t
> > > > > +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> > > > > +{
> > > > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > > > +
> > > > > +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
> > > > > +		return 0444;
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > > +static int
> > > > > +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> > > > > +{
> > > > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > > > +	intel_wakeref_t wakeref;
> > > > > +	u32 reg_val;
> > > > > +
> > > > > +	if (attr == hwmon_temp_input) {
> > > > > +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > > > > +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
> > > > > +
> > > > > +		/* HW register value is in degrees, convert to millidegrees. */
> > > > > +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> > > > > +		return 0;
> > > > > +	}
> > > > > +
> > > > > +	return -EOPNOTSUPP;
> > > > > +}
> > > > Let's try to have synergy between previous attribute, such as hwm_fan_input,
> > > > and this one.
> > > 
> > > This one's simple enough to be inline IMHO.
> > > Besides, it's already in synergy with hwm_in_read() which has similar
> > > implementation.
> > Agree this is pretty simple to have an any helper but IMO it would have been cleaner to have a switch
> > like hwm_in_read() to return -EOPNOTSUPP in default case. i think that was reason switch case was 
> > used in entire file.
> 
> Extending on the simplicity argument above, if() makes more sense for a single case.
IMO lets prefer the style which was used in this entire file,
that is more readable along with other attributes.
Idea behind switch was scalable attribute for future. 
It is something related to individual preference therefore
 let's prefer the symmetry with other hwmon attributes.

With that
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> 
> Raag

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Nilawar, Badal]

On 06-09-2024 11:56, Anshuman Gupta wrote:
> On 2024-09-05 at 22:18:17 +0300, Raag Jadav wrote:
>> On Thu, Sep 05, 2024 at 07:39:31PM +0530, Anshuman Gupta wrote:
>>> On 2024-09-05 at 11:55:23 +0300, Raag Jadav wrote:
>>>> On Thu, Sep 05, 2024 at 11:56:15AM +0530, Nilawar, Badal wrote:
>>>>>
>>>>>
>>>>> On 28-08-2024 10:15, Raag Jadav wrote:
>>>>>> Add hwmon support for temp1_input attribute, which will expose package
>>>>>> temperature in millidegree Celsius. With this in place we can monitor
>>>>>> package temperature using lm-sensors tool.
>>>>>>
>>>>>> $ sensors
>>>>>> i915-pci-0300
>>>>>> Adapter: PCI adapter
>>>>>> in0:         990.00 mV
>>>>>> fan1:        1260 RPM
>>>>>> temp1:        +45.0°C
>>>>>> power1:           N/A  (max =  35.00 W)
>>>>>> energy1:      12.62 kJ
>>>>>>
>>>>>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
>>>>>> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
>>>>>> ---
>>>>>>    .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
>>>>>>    drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
>>>>>>    drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
>>>>>>    3 files changed, 51 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>>>> index be4141a7522f..a885e5316d02 100644
>>>>>> --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>>>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>>>> @@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
>>>>>>    Description:	RO. Fan speed of device in RPM.
>>>>>>    		Only supported for particular Intel i915 graphics platforms.
>>>>>> +
>>>>>> +What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
>>>>>> +Date:		November 2024
>>>>>> +KernelVersion:	6.12
>>>>>> +Contact:	intel-gfx@lists.freedesktop.org
>>>>>> +Description:	RO. GPU package temperature in millidegree Celsius.
>>>>>> +
>>>>>> +		Only supported for particular Intel i915 graphics platforms.
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
>>>>>> index 17d30f6b84b0..9f1a2300510b 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>>>>>> @@ -7,6 +7,7 @@
>>>>>>    #include <linux/hwmon-sysfs.h>
>>>>>>    #include <linux/jiffies.h>
>>>>>>    #include <linux/types.h>
>>>>>> +#include <linux/units.h>
>>>>>>    #include "i915_drv.h"
>>>>>>    #include "i915_hwmon.h"
>>>>>> @@ -32,6 +33,7 @@
>>>>>>    struct hwm_reg {
>>>>>>    	i915_reg_t gt_perf_status;
>>>>>> +	i915_reg_t pkg_temp;
>>>>>>    	i915_reg_t pkg_power_sku_unit;
>>>>>>    	i915_reg_t pkg_power_sku;
>>>>>>    	i915_reg_t pkg_rapl_limit;
>>>>>> @@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
>>>>>>    };
>>>>>>    static const struct hwmon_channel_info * const hwm_info[] = {
>>>>>> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
>>>>>>    	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
>>>>>>    	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
>>>>>>    	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
>>>>>> @@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
>>>>>>    				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
>>>>>>    }
>>>>>> +static umode_t
>>>>>> +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
>>>>>> +{
>>>>>> +	struct i915_hwmon *hwmon = ddat->hwmon;
>>>>>> +
>>>>>> +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
>>>>>> +		return 0444;
>>>>>> +
>>>>>> +	return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int
>>>>>> +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
>>>>>> +{
>>>>>> +	struct i915_hwmon *hwmon = ddat->hwmon;
>>>>>> +	intel_wakeref_t wakeref;
>>>>>> +	u32 reg_val;
>>>>>> +
>>>>>> +	if (attr == hwmon_temp_input) {
>>>>>> +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
>>>>>> +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
>>>>>> +
>>>>>> +		/* HW register value is in degrees, convert to millidegrees. */
>>>>>> +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
>>>>>> +		return 0;
>>>>>> +	}
>>>>>> +
>>>>>> +	return -EOPNOTSUPP;
>>>>>> +}
>>>>> Let's try to have synergy between previous attribute, such as hwm_fan_input,
>>>>> and this one.
>>>>
>>>> This one's simple enough to be inline IMHO.
>>>> Besides, it's already in synergy with hwm_in_read() which has similar
>>>> implementation.
>>> Agree this is pretty simple to have an any helper but IMO it would have been cleaner to have a switch
>>> like hwm_in_read() to return -EOPNOTSUPP in default case. i think that was reason switch case was
>>> used in entire file.
>>
>> Extending on the simplicity argument above, if() makes more sense for a single case.
> IMO lets prefer the style which was used in this entire file,
> that is more readable along with other attributes.
> Idea behind switch was scalable attribute for future.
> It is something related to individual preference therefore
>   let's prefer the symmetry with other hwmon attributes.
I agree with this, but even if this approach is used file-wide, there 
were concerns about using a switch case for a single case while 
implementing the fan_input attribute. 
https://patchwork.freedesktop.org/patch/607642/?series=136036&rev=4"
So I suggested to implement temp_input the way fan_input is implemented, 
at least we should follow this approach to maintain symmetry with new 
attributes. But in case if there is agreement to use file wide approach 
then please follow that approach for fan_input as well.

Regards,
Badal
> 
> With that
> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
>>
>> Raag

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Raag Jadav]

On Fri, Sep 06, 2024 at 04:33:12PM +0530, Nilawar, Badal wrote:
> On 06-09-2024 11:56, Anshuman Gupta wrote:
> > On 2024-09-05 at 22:18:17 +0300, Raag Jadav wrote:
> > > On Thu, Sep 05, 2024 at 07:39:31PM +0530, Anshuman Gupta wrote:
> > > > On 2024-09-05 at 11:55:23 +0300, Raag Jadav wrote:
> > > > > On Thu, Sep 05, 2024 at 11:56:15AM +0530, Nilawar, Badal wrote:
> > > > > > 
> > > > > > 
> > > > > > On 28-08-2024 10:15, Raag Jadav wrote:
> > > > > > > Add hwmon support for temp1_input attribute, which will expose package
> > > > > > > temperature in millidegree Celsius. With this in place we can monitor
> > > > > > > package temperature using lm-sensors tool.
> > > > > > > 
> > > > > > > $ sensors
> > > > > > > i915-pci-0300
> > > > > > > Adapter: PCI adapter
> > > > > > > in0:         990.00 mV
> > > > > > > fan1:        1260 RPM
> > > > > > > temp1:        +45.0°C
> > > > > > > power1:           N/A  (max =  35.00 W)
> > > > > > > energy1:      12.62 kJ
> > > > > > > 
> > > > > > > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
> > > > > > > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > > > > > > ---
> > > > > > >    .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
> > > > > > >    drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
> > > > > > >    drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
> > > > > > >    3 files changed, 51 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > > > index be4141a7522f..a885e5316d02 100644
> > > > > > > --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > > > +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
> > > > > > > @@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
> > > > > > >    Description:	RO. Fan speed of device in RPM.
> > > > > > >    		Only supported for particular Intel i915 graphics platforms.
> > > > > > > +
> > > > > > > +What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
> > > > > > > +Date:		November 2024
> > > > > > > +KernelVersion:	6.12
> > > > > > > +Contact:	intel-gfx@lists.freedesktop.org
> > > > > > > +Description:	RO. GPU package temperature in millidegree Celsius.
> > > > > > > +
> > > > > > > +		Only supported for particular Intel i915 graphics platforms.
> > > > > > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > > > > index 17d30f6b84b0..9f1a2300510b 100644
> > > > > > > --- a/drivers/gpu/drm/i915/i915_hwmon.c
> > > > > > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c
> > > > > > > @@ -7,6 +7,7 @@
> > > > > > >    #include <linux/hwmon-sysfs.h>
> > > > > > >    #include <linux/jiffies.h>
> > > > > > >    #include <linux/types.h>
> > > > > > > +#include <linux/units.h>
> > > > > > >    #include "i915_drv.h"
> > > > > > >    #include "i915_hwmon.h"
> > > > > > > @@ -32,6 +33,7 @@
> > > > > > >    struct hwm_reg {
> > > > > > >    	i915_reg_t gt_perf_status;
> > > > > > > +	i915_reg_t pkg_temp;
> > > > > > >    	i915_reg_t pkg_power_sku_unit;
> > > > > > >    	i915_reg_t pkg_power_sku;
> > > > > > >    	i915_reg_t pkg_rapl_limit;
> > > > > > > @@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
> > > > > > >    };
> > > > > > >    static const struct hwmon_channel_info * const hwm_info[] = {
> > > > > > > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> > > > > > >    	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
> > > > > > >    	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
> > > > > > >    	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
> > > > > > > @@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
> > > > > > >    				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
> > > > > > >    }
> > > > > > > +static umode_t
> > > > > > > +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
> > > > > > > +{
> > > > > > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > > > > > +
> > > > > > > +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
> > > > > > > +		return 0444;
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int
> > > > > > > +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
> > > > > > > +{
> > > > > > > +	struct i915_hwmon *hwmon = ddat->hwmon;
> > > > > > > +	intel_wakeref_t wakeref;
> > > > > > > +	u32 reg_val;
> > > > > > > +
> > > > > > > +	if (attr == hwmon_temp_input) {
> > > > > > > +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
> > > > > > > +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
> > > > > > > +
> > > > > > > +		/* HW register value is in degrees, convert to millidegrees. */
> > > > > > > +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> > > > > > > +		return 0;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return -EOPNOTSUPP;
> > > > > > > +}
> > > > > > Let's try to have synergy between previous attribute, such as hwm_fan_input,
> > > > > > and this one.
> > > > > 
> > > > > This one's simple enough to be inline IMHO.
> > > > > Besides, it's already in synergy with hwm_in_read() which has similar
> > > > > implementation.
> > > > Agree this is pretty simple to have an any helper but IMO it would have been cleaner to have a switch
> > > > like hwm_in_read() to return -EOPNOTSUPP in default case. i think that was reason switch case was
> > > > used in entire file.
> > > 
> > > Extending on the simplicity argument above, if() makes more sense for a single case.
> > IMO lets prefer the style which was used in this entire file,
> > that is more readable along with other attributes.
> > Idea behind switch was scalable attribute for future.
> > It is something related to individual preference therefore
> >   let's prefer the symmetry with other hwmon attributes.
> I agree with this, but even if this approach is used file-wide, there were
> concerns about using a switch case for a single case while implementing the
> fan_input attribute.
> https://patchwork.freedesktop.org/patch/607642/?series=136036&rev=4"
> So I suggested to implement temp_input the way fan_input is implemented, at
> least we should follow this approach to maintain symmetry with new
> attributes. But in case if there is agreement to use file wide approach then
> please follow that approach for fan_input as well.

Let's try to keep it simple and use whatever works for the case.
I'm sure the driver is much easier to maintain with a few less lines.

Raag

Re: [PATCH v1] drm/i915/hwmon: expose package temperature

[Nilawar, Badal]

On 07-09-2024 16:48, Raag Jadav wrote:
> On Fri, Sep 06, 2024 at 04:33:12PM +0530, Nilawar, Badal wrote:
>> On 06-09-2024 11:56, Anshuman Gupta wrote:
>>> On 2024-09-05 at 22:18:17 +0300, Raag Jadav wrote:
>>>> On Thu, Sep 05, 2024 at 07:39:31PM +0530, Anshuman Gupta wrote:
>>>>> On 2024-09-05 at 11:55:23 +0300, Raag Jadav wrote:
>>>>>> On Thu, Sep 05, 2024 at 11:56:15AM +0530, Nilawar, Badal wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 28-08-2024 10:15, Raag Jadav wrote:
>>>>>>>> Add hwmon support for temp1_input attribute, which will expose package
>>>>>>>> temperature in millidegree Celsius. With this in place we can monitor
>>>>>>>> package temperature using lm-sensors tool.
>>>>>>>>
>>>>>>>> $ sensors
>>>>>>>> i915-pci-0300
>>>>>>>> Adapter: PCI adapter
>>>>>>>> in0:         990.00 mV
>>>>>>>> fan1:        1260 RPM
>>>>>>>> temp1:        +45.0°C
>>>>>>>> power1:           N/A  (max =  35.00 W)
>>>>>>>> energy1:      12.62 kJ
>>>>>>>>
>>>>>>>> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
>>>>>>>> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
>>>>>>>> ---
>>>>>>>>     .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
>>>>>>>>     drivers/gpu/drm/i915/i915_hwmon.c             | 39 +++++++++++++++++++
>>>>>>>>     drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
>>>>>>>>     3 files changed, 51 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>>>>>> index be4141a7522f..a885e5316d02 100644
>>>>>>>> --- a/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>>>>>> +++ b/Documentation/ABI/testing/sysfs-driver-intel-i915-hwmon
>>>>>>>> @@ -83,3 +83,11 @@ Contact:	intel-gfx@lists.freedesktop.org
>>>>>>>>     Description:	RO. Fan speed of device in RPM.
>>>>>>>>     		Only supported for particular Intel i915 graphics platforms.
>>>>>>>> +
>>>>>>>> +What:		/sys/bus/pci/drivers/i915/.../hwmon/hwmon<i>/temp1_input
>>>>>>>> +Date:		November 2024
>>>>>>>> +KernelVersion:	6.12
>>>>>>>> +Contact:	intel-gfx@lists.freedesktop.org
>>>>>>>> +Description:	RO. GPU package temperature in millidegree Celsius.
>>>>>>>> +
>>>>>>>> +		Only supported for particular Intel i915 graphics platforms.
>>>>>>>> diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c
>>>>>>>> index 17d30f6b84b0..9f1a2300510b 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/i915_hwmon.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/i915_hwmon.c
>>>>>>>> @@ -7,6 +7,7 @@
>>>>>>>>     #include <linux/hwmon-sysfs.h>
>>>>>>>>     #include <linux/jiffies.h>
>>>>>>>>     #include <linux/types.h>
>>>>>>>> +#include <linux/units.h>
>>>>>>>>     #include "i915_drv.h"
>>>>>>>>     #include "i915_hwmon.h"
>>>>>>>> @@ -32,6 +33,7 @@
>>>>>>>>     struct hwm_reg {
>>>>>>>>     	i915_reg_t gt_perf_status;
>>>>>>>> +	i915_reg_t pkg_temp;
>>>>>>>>     	i915_reg_t pkg_power_sku_unit;
>>>>>>>>     	i915_reg_t pkg_power_sku;
>>>>>>>>     	i915_reg_t pkg_rapl_limit;
>>>>>>>> @@ -280,6 +282,7 @@ static const struct attribute_group *hwm_groups[] = {
>>>>>>>>     };
>>>>>>>>     static const struct hwmon_channel_info * const hwm_info[] = {
>>>>>>>> +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
>>>>>>>>     	HWMON_CHANNEL_INFO(in, HWMON_I_INPUT),
>>>>>>>>     	HWMON_CHANNEL_INFO(power, HWMON_P_MAX | HWMON_P_RATED_MAX | HWMON_P_CRIT),
>>>>>>>>     	HWMON_CHANNEL_INFO(energy, HWMON_E_INPUT),
>>>>>>>> @@ -310,6 +313,36 @@ static int hwm_pcode_write_i1(struct drm_i915_private *i915, u32 uval)
>>>>>>>>     				  POWER_SETUP_SUBCOMMAND_WRITE_I1, 0, uval);
>>>>>>>>     }
>>>>>>>> +static umode_t
>>>>>>>> +hwm_temp_is_visible(const struct hwm_drvdata *ddat, u32 attr)
>>>>>>>> +{
>>>>>>>> +	struct i915_hwmon *hwmon = ddat->hwmon;
>>>>>>>> +
>>>>>>>> +	if (attr == hwmon_temp_input && i915_mmio_reg_valid(hwmon->rg.pkg_temp))
>>>>>>>> +		return 0444;
>>>>>>>> +
>>>>>>>> +	return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int
>>>>>>>> +hwm_temp_read(struct hwm_drvdata *ddat, u32 attr, long *val)
>>>>>>>> +{
>>>>>>>> +	struct i915_hwmon *hwmon = ddat->hwmon;
>>>>>>>> +	intel_wakeref_t wakeref;
>>>>>>>> +	u32 reg_val;
>>>>>>>> +
>>>>>>>> +	if (attr == hwmon_temp_input) {
>>>>>>>> +		with_intel_runtime_pm(ddat->uncore->rpm, wakeref)
>>>>>>>> +			reg_val = intel_uncore_read(ddat->uncore, hwmon->rg.pkg_temp);
>>>>>>>> +
>>>>>>>> +		/* HW register value is in degrees, convert to millidegrees. */
>>>>>>>> +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
>>>>>>>> +		return 0;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>> +	return -EOPNOTSUPP;
>>>>>>>> +}
>>>>>>> Let's try to have synergy between previous attribute, such as hwm_fan_input,
>>>>>>> and this one.
>>>>>>
>>>>>> This one's simple enough to be inline IMHO.
>>>>>> Besides, it's already in synergy with hwm_in_read() which has similar
>>>>>> implementation.
>>>>> Agree this is pretty simple to have an any helper but IMO it would have been cleaner to have a switch
>>>>> like hwm_in_read() to return -EOPNOTSUPP in default case. i think that was reason switch case was
>>>>> used in entire file.
>>>>
>>>> Extending on the simplicity argument above, if() makes more sense for a single case.
>>> IMO lets prefer the style which was used in this entire file,
>>> that is more readable along with other attributes.
>>> Idea behind switch was scalable attribute for future.
>>> It is something related to individual preference therefore
>>>    let's prefer the symmetry with other hwmon attributes.
>> I agree with this, but even if this approach is used file-wide, there were
>> concerns about using a switch case for a single case while implementing the
>> fan_input attribute.
>> https://patchwork.freedesktop.org/patch/607642/?series=136036&rev=4"
>> So I suggested to implement temp_input the way fan_input is implemented, at
>> least we should follow this approach to maintain symmetry with new
>> attributes. But in case if there is agreement to use file wide approach then
>> please follow that approach for fan_input as well.
> 
> Let's try to keep it simple and use whatever works for the case.
> I'm sure the driver is much easier to maintain with a few less lines.
Let's not break symmetry, whichever approach is being followed please 
maintain it file wide.

Regards,
Badal
> 
> Raag