| Message ID | 20240906093118.3068732-1-raag.jadav@intel.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] drm/i915/hwmon: expose package temperature | expand |
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)
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
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
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
> -----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
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
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
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
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
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
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)