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

["Nilawar, Badal" <badal.nilawar@intel.com>]

On 24-01-2025 11:46, Riana Tauro wrote:
> 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 for looping me in this. Yes, Channel 0 represent card specific 
attributes and Channel 1 represent package specific attributes. That's 
how it should be followed.
With that BMG_PACKAGE_TEMPERATURE should go under CHANNEL_PKG. For 
BMG_VRAM_TEMPERATURE new channel (channel 3) should be added in enum 
xe_hwmon_channel.

Regards,
Badal


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