Re: [PATCH v1] drm/xe/hwmon: expose package and vram temperature

[Raag Jadav <raag.jadav@intel.com>]

On Tue, Jan 21, 2025 at 01:56:05PM +0530, Riana Tauro wrote:
> Hi Raag
> 
> On 1/8/2025 2:54 PM, Raag Jadav wrote:
> > Add hwmon support for temp1_input and temp2_input attributes, which will
> > expose package and vram temperature in millidegree Celsius. With this in
> > place we can monitor temperature using lm-sensors tool.
> > 
> > Signed-off-by: Raag Jadav <raag.jadav@intel.com>
> > ---
> >   .../ABI/testing/sysfs-driver-intel-xe-hwmon   | 16 +++++
> >   drivers/gpu/drm/xe/regs/xe_mchbar_regs.h      |  3 +
> >   drivers/gpu/drm/xe/regs/xe_pcode_regs.h       |  2 +
> >   drivers/gpu/drm/xe/xe_hwmon.c                 | 63 +++++++++++++++++++
> >   4 files changed, 84 insertions(+)
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> > index d792a56f59ac..998cfb0ee1a6 100644
> > --- a/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> > +++ b/Documentation/ABI/testing/sysfs-driver-intel-xe-hwmon
> > @@ -108,3 +108,19 @@ Contact:	intel-xe@lists.freedesktop.org
> >   Description:	RO. Package current voltage in millivolt.
> >   		Only supported for particular Intel Xe graphics platforms.
> > +
> > +What:		/sys/bus/pci/drivers/xe/.../hwmon/hwmon<i>/temp1_input
> > +Date:		April 2025
> > +KernelVersion:	6.15
> > +Contact:	intel-xe@lists.freedesktop.org
> > +Description:	RO. Package temperature in millidegree Celsius.
> > +
> > +		Only supported for particular Intel Xe graphics platforms.
> > +
> > +What:		/sys/bus/pci/drivers/xe/.../hwmon/hwmon<i>/temp2_input
> > +Date:		April 2025
> > +KernelVersion:	6.15
> > +Contact:	intel-xe@lists.freedesktop.org
> > +Description:	RO. VRAM temperature in millidegree Celsius.
> > +
> > +		Only supported for particular Intel Xe graphics platforms.
> > diff --git a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> > index 519dd1067a19..f5e5234857c1 100644
> > --- a/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> > +++ b/drivers/gpu/drm/xe/regs/xe_mchbar_regs.h
> > @@ -34,6 +34,9 @@
> >   #define PCU_CR_PACKAGE_ENERGY_STATUS		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x593c)
> > +#define PCU_CR_PACKAGE_TEMPERATURE		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x5978)
> > +#define   TEMP_MASK				REG_GENMASK(7, 0)
> > +
> >   #define PCU_CR_PACKAGE_RAPL_LIMIT		XE_REG(MCHBAR_MIRROR_BASE_SNB + 0x59a0)
> >   #define   PKG_PWR_LIM_1				REG_GENMASK(14, 0)
> >   #define   PKG_PWR_LIM_1_EN			REG_BIT(15)
> > diff --git a/drivers/gpu/drm/xe/regs/xe_pcode_regs.h b/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> > index 0b0b49d850ae..8846eb9ce2a4 100644
> > --- a/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> > +++ b/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> > @@ -21,6 +21,8 @@
> >   #define BMG_PACKAGE_POWER_SKU			XE_REG(0x138098)
> >   #define BMG_PACKAGE_POWER_SKU_UNIT		XE_REG(0x1380dc)
> >   #define BMG_PACKAGE_ENERGY_STATUS		XE_REG(0x138120)
> > +#define BMG_VRAM_TEMPERATURE			XE_REG(0x1382c0)
> > +#define BMG_PACKAGE_TEMPERATURE			XE_REG(0x138434)
> indentation.

It's a git quirk, you won't see it in file.

> Also you are using the same for DG2. Should have a common name

Just following the conventions.

> >   #define BMG_PACKAGE_RAPL_LIMIT			XE_REG(0x138440)
> >   #define BMG_PLATFORM_ENERGY_STATUS		XE_REG(0x138458)
> >   #define BMG_PLATFORM_POWER_LIMIT		XE_REG(0x138460)
> > diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> > index fde56dad3ab7..5b5c844adf4a 100644
> > --- a/drivers/gpu/drm/xe/xe_hwmon.c
> > +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> > @@ -6,6 +6,7 @@
> >   #include <linux/hwmon-sysfs.h>
> >   #include <linux/hwmon.h>
> >   #include <linux/types.h>
> > +#include <linux/units.h>
> >   #include <drm/drm_managed.h>
> >   #include "regs/xe_gt_regs.h"
> > @@ -20,6 +21,7 @@
> >   #include "xe_pm.h"
> >   enum xe_hwmon_reg {
> > +	REG_TEMP,
> add to the end
> >   	REG_PKG_RAPL_LIMIT,
> >   	REG_PKG_POWER_SKU,
> >   	REG_PKG_POWER_SKU_UNIT,
> > @@ -39,6 +41,11 @@ enum xe_hwmon_channel {
> >   	CHANNEL_MAX,
> >   };
> > +enum xe_hwmon_temp {
> > +	TEMP_PKG,
> > +	TEMP_VRAM,
> > +};
> Can't the existing channel enum be used here?

Nope, that'd break the indexes.

> > +
> >   /*
> >    * SF_* - scale factors for particular quantities according to hwmon spec.
> >    */
> > @@ -84,6 +91,19 @@ static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg
> >   	struct xe_device *xe = hwmon->xe;
> >   	switch (hwmon_reg) {
> > +	case REG_TEMP:
> > +		if (xe->info.platform == XE_BATTLEMAGE) {
> > +			if (channel == TEMP_PKG)
> > +				return BMG_PACKAGE_TEMPERATURE;
> > +			else if (channel == TEMP_VRAM)
> > +				return BMG_VRAM_TEMPERATURE;
> > +		} else if (xe->info.platform == XE_DG2) {
> > +			if (channel == TEMP_PKG)
> > +				return PCU_CR_PACKAGE_TEMPERATURE;
> > +			else if (channel == TEMP_VRAM)
> > +				return BMG_VRAM_TEMPERATURE;
> > +		}
> Common code for bmg and dg2. Can be moved out

Again, the conventions + easier to add new platforms this way.

Raag