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

[Riana Tauro <riana.tauro@intel.com>]

Hi Raag

On 1/23/2025 8:21 AM, Raag Jadav wrote:
> 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.
Did not find this convention in the file.
BMG_VRAM_TEMPERATURE is used in both dg2 and bmg and has a bmg prefix. 
Doesn't seem right		
> 
>>>    #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.
@badal/@karthik Are multiple indexes for the same channel okay?

In the current code, for dg2 only channel 1 is exposed for power and 
channel 0 skipped. Something like that needs to be done here too?

Thanks
Riana

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