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

[PATCH v2] 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

v2: Use switch case (Anshuman)

Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
Signed-off-by: Raag Jadav <raag.jadav@intel.com>
Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
---
 .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
 drivers/gpu/drm/i915/i915_hwmon.c             | 40 +++++++++++++++++++
 drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
 3 files changed, 52 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..0a9f483b4105 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,37 @@ 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;
+
+	switch (attr) {
+	case 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;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
 static umode_t
 hwm_in_is_visible(const struct hwm_drvdata *ddat, u32 attr)
 {
@@ -692,6 +726,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 +750,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 +848,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 +856,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 v2] drm/i915/hwmon: expose package temperature

[Riana Tauro]

Hi Raag

On 9/6/2024 3:01 PM, 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
> 
> v2: Use switch case (Anshuman)
> 
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
> Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
>   drivers/gpu/drm/i915/i915_hwmon.c             | 40 +++++++++++++++++++
>   drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
>   3 files changed, 52 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..0a9f483b4105 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;
place it alphabetically after rapl_limit
>   	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,37 @@ 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;
> +
> +	switch (attr) {
> +	case 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. */
use millidegree Celsius here

Thanks,
Riana
> +		*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
>   static umode_t
>   hwm_in_is_visible(const struct hwm_drvdata *ddat, u32 attr)
>   {
> @@ -692,6 +726,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 +750,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 +848,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 +856,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)

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

[Patchwork]

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

== Series Details ==

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

== Summary ==

CI Bug Log - changes from CI_DRM_15371 -> Patchwork_137874v3
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_137874v3 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_137874v3, 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_137874v3/index.html

Participating hosts (40 -> 40)
------------------------------

  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_137874v3:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_heartbeat:
    - bat-arls-5:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-arls-5/igt@i915_selftest@live@gt_heartbeat.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-arls-5/igt@i915_selftest@live@gt_heartbeat.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@fbdev@nullptr:
    - bat-arls-1:         [PASS][3] -> [DMESG-WARN][4] ([i915#12102])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-arls-1/igt@fbdev@nullptr.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-arls-1/igt@fbdev@nullptr.html

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

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

  * igt@i915_selftest@live:
    - bat-mtlp-8:         [PASS][7] -> [ABORT][8] ([i915#12061]) +1 other test abort
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-mtlp-8/igt@i915_selftest@live.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-mtlp-8/igt@i915_selftest@live.html
    - bat-arls-1:         [PASS][9] -> [DMESG-WARN][10] ([i915#10341])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-arls-1/igt@i915_selftest@live.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-arls-1/igt@i915_selftest@live.html
    - bat-arls-5:         [PASS][11] -> [INCOMPLETE][12] ([i915#10341])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-arls-5/igt@i915_selftest@live.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-arls-5/igt@i915_selftest@live.html

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

  * igt@i915_selftest@live@workarounds:
    - bat-mtlp-6:         [PASS][15] -> [ABORT][16] ([i915#12061]) +1 other test abort
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-mtlp-6/igt@i915_selftest@live@workarounds.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-mtlp-6/igt@i915_selftest@live@workarounds.html

  * igt@kms_dsc@dsc-basic:
    - fi-kbl-8809g:       NOTRUN -> [SKIP][17] +30 other tests skip
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/fi-kbl-8809g/igt@kms_dsc@dsc-basic.html

  
#### Possible fixes ####

  * igt@fbdev@eof:
    - bat-arls-1:         [DMESG-WARN][18] ([i915#12102]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-arls-1/igt@fbdev@eof.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-arls-1/igt@fbdev@eof.html

  * igt@i915_selftest@live:
    - bat-arls-2:         [DMESG-WARN][20] ([i915#10341]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-arls-2/igt@i915_selftest@live.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-arls-2/igt@i915_selftest@live.html

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

  * igt@kms_pipe_crc_basic@nonblocking-crc:
    - bat-arls-5:         [INCOMPLETE][24] ([i915#11320]) -> [PASS][25]
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_15371/bat-arls-5/igt@kms_pipe_crc_basic@nonblocking-crc.html
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_137874v3/bat-arls-5/igt@kms_pipe_crc_basic@nonblocking-crc.html

  
  [i915#10341]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10341
  [i915#11320]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11320
  [i915#11349]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11349
  [i915#12061]: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/12061
  [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_15371 -> Patchwork_137874v3

  CI-20190529: 20190529
  CI_DRM_15371: d4b15a4609f1b768b0301fd238022ab8d07d15bc @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_8007: 8f9900c288f4cf1244d66baa71bc6d9355747cbd @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_137874v3: d4b15a4609f1b768b0301fd238022ab8d07d15bc @ git://anongit.freedesktop.org/gfx-ci/linux

== Logs ==

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

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

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

[Raag Jadav]

On Fri, Sep 06, 2024 at 03:15:01PM +0530, Riana Tauro wrote:
> Hi Raag
> 
> On 9/6/2024 3:01 PM, 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
> > 
> > v2: Use switch case (Anshuman)
> > 
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11276
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > Reviewed-by: Anshuman Gupta <anshuman.gupta@intel.com>
> > ---
> >   .../ABI/testing/sysfs-driver-intel-i915-hwmon |  8 ++++
> >   drivers/gpu/drm/i915/i915_hwmon.c             | 40 +++++++++++++++++++
> >   drivers/gpu/drm/i915/intel_mchbar_regs.h      |  4 ++
> >   3 files changed, 52 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..0a9f483b4105 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;
> place it alphabetically after rapl_limit

This follows the ordering of enum hwmon_sensor_types (as the rest of the patch).

> >   	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,37 @@ 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;
> > +
> > +	switch (attr) {
> > +	case 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. */
> use millidegree Celsius here

The intent here is to signify the conversion rather than the unit.
But okay, will add if we have another version.

Raag

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

[Andi Shyti]

Hi Raag,

...

> +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;
> +
> +	switch (attr) {
> +	case 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;
> +	default:
> +		return -EOPNOTSUPP;
> +	}

I don't understand this love for single case switches.

Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,
Andi

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

[Andi Shyti]

Hi Raag,

> > > +	case 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. */
> > use millidegree Celsius here
> 
> The intent here is to signify the conversion rather than the unit.
> But okay, will add if we have another version.

is Riana asking to improve the comment here? Then please do, if
someone asks to make better comments it means that he is asking
to answer to an open question that someone might have in the
future.

Sending a v3 is not much of a work but improving the comment
later is not trivial.

Besides you need to retrigger tests anyway because you got a
BAT test failure :-)

Thanks,
Andi

RE: [PATCH v2] drm/i915/hwmon: expose package temperature

[Gupta, Anshuman]

> -----Original Message-----
> From: Andi Shyti <andi.shyti@kernel.org>
> Sent: Tuesday, September 10, 2024 3:54 AM
> To: Jadav, Raag <raag.jadav@intel.com>
> Cc: jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com; Vivi,
> Rodrigo <rodrigo.vivi@intel.com>; tursulin@ursulin.net; linux@roeck-us.net;
> andi.shyti@linux.intel.com; andriy.shevchenko@linux.intel.com; intel-
> gfx@lists.freedesktop.org; linux-hwmon@vger.kernel.org; Gupta, Anshuman
> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
> Tauro, Riana <riana.tauro@intel.com>; Dixit, Ashutosh
> <ashutosh.dixit@intel.com>; Poosa, Karthik <karthik.poosa@intel.com>
> Subject: Re: [PATCH v2] drm/i915/hwmon: expose package temperature
> 
> Hi Raag,
> 
> ...
> 
> > +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;
> > +
> > +	switch (attr) {
> > +	case 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;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> 
> I don't understand this love for single case switches.
IMHO this is kept to keep symmetry in this file to make it more readable.
Also it readable to return error using default case, which is followed in this entire file.
Thanks,
Anshuman. 
> 
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
> 
> Thanks,
> Andi

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

[Raag Jadav]

On Tue, Sep 10, 2024 at 12:27:16AM +0200, Andi Shyti wrote:
> Hi Raag,
> 
> > > > +	case 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. */
> > > use millidegree Celsius here
> > 
> > The intent here is to signify the conversion rather than the unit.
> > But okay, will add if we have another version.
> 
> is Riana asking to improve the comment here? Then please do, if
> someone asks to make better comments it means that he is asking
> to answer to an open question that someone might have in the
> future.

To me this looks quite self documenting, but agree.

> Sending a v3 is not much of a work but improving the comment
> later is not trivial.

Already have it locally, was going to send out after you review :)

Raag

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

[Nilawar, Badal]

On 10-09-2024 10:07, Gupta, Anshuman wrote:
> 
> 
>> -----Original Message-----
>> From: Andi Shyti <andi.shyti@kernel.org>
>> Sent: Tuesday, September 10, 2024 3:54 AM
>> To: Jadav, Raag <raag.jadav@intel.com>
>> Cc: jani.nikula@linux.intel.com; joonas.lahtinen@linux.intel.com; Vivi,
>> Rodrigo <rodrigo.vivi@intel.com>; tursulin@ursulin.net; linux@roeck-us.net;
>> andi.shyti@linux.intel.com; andriy.shevchenko@linux.intel.com; intel-
>> gfx@lists.freedesktop.org; linux-hwmon@vger.kernel.org; Gupta, Anshuman
>> <anshuman.gupta@intel.com>; Nilawar, Badal <badal.nilawar@intel.com>;
>> Tauro, Riana <riana.tauro@intel.com>; Dixit, Ashutosh
>> <ashutosh.dixit@intel.com>; Poosa, Karthik <karthik.poosa@intel.com>
>> Subject: Re: [PATCH v2] drm/i915/hwmon: expose package temperature
>>
>> Hi Raag,
>>
>> ...
>>
>>> +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;
>>> +
>>> +	switch (attr) {
>>> +	case 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;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>
>> I don't understand this love for single case switches.
> IMHO this is kept to keep symmetry in this file to make it more readable.
> Also it readable to return error using default case, which is followed in this entire file.
I agree on this. Let’s stick to file-wide approach and ensure it is 
applied to the fan_input attribute as well.

Regards,
Badal

> Thanks,
> Anshuman.
>>
>> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
>>
>> Thanks,
>> Andi

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

[Andi Shyti]

Hi,

> > > > +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;
> > > > +
> > > > +	switch (attr) {
> > > > +	case 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;
> > > > +	default:
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > 
> > > I don't understand this love for single case switches.
> > IMHO this is kept to keep symmetry in this file to make it more readable.
> > Also it readable to return error using default case, which is followed in this entire file.
> I agree on this. Let’s stick to file-wide approach and ensure it is applied
> to the fan_input attribute as well.

Yes, that's why I'm giving the r-b. I don't like it, but that's
how you guys have decided to do it.

Thanks,
Andi

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

[Raag Jadav]

On Tue, Sep 10, 2024 at 11:57:20AM +0530, Nilawar, Badal wrote:
> On 10-09-2024 10:07, Gupta, Anshuman wrote:
> > > 
> > > ...
> > > 
> > > > +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;
> > > > +
> > > > +	switch (attr) {
> > > > +	case 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;
> > > > +	default:
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > 
> > > I don't understand this love for single case switches.
> > IMHO this is kept to keep symmetry in this file to make it more readable.
> > Also it readable to return error using default case, which is followed in this entire file.
> I agree on this. Let’s stick to file-wide approach and ensure it is applied
> to the fan_input attribute as well.

Since fan patch is already on its way to drm-next, you can submit a fix if you wish.
Although I don't agree with it, I have no objections.

Raag

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

[Andi Shyti]

On Tue, Sep 10, 2024 at 01:58:29PM GMT, Raag Jadav wrote:
> On Tue, Sep 10, 2024 at 11:57:20AM +0530, Nilawar, Badal wrote:
> > On 10-09-2024 10:07, Gupta, Anshuman wrote:
> > > > 
> > > > ...
> > > > 
> > > > > +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;
> > > > > +
> > > > > +	switch (attr) {
> > > > > +	case 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;
> > > > > +	default:
> > > > > +		return -EOPNOTSUPP;
> > > > > +	}
> > > > 
> > > > I don't understand this love for single case switches.
> > > IMHO this is kept to keep symmetry in this file to make it more readable.
> > > Also it readable to return error using default case, which is followed in this entire file.
> > I agree on this. Let’s stick to file-wide approach and ensure it is applied
> > to the fan_input attribute as well.
> 
> Since fan patch is already on its way to drm-next, you can submit a fix if you wish.
> Although I don't agree with it, I have no objections.

nack! :-)

It doesn't make much sense to send a controvertial patch that
refactors good working code to other good (some would say worse)
working code without any functional change.

Thanks,
Andi