Re: [PATCH 1/3] drm/xe/hwmon: Detect unavailable temperature sensors

["Poosa, Karthik" <karthik.poosa@intel.com>]

On 26-08-2026 17:20, Nilawar, Badal wrote:
>
> On 25-08-2026 00:11, Karthik Poosa wrote:
>> Add is_temp_valid() to validate sensor presence.
>> A temperature reading of 0xFF on CRI platforms indicates that the
>> corresponding sensor is not present and should be treated as 
>> unavailable.
>>
>> Use this check from xe_hwmon_temp_is_visible() callback so that 
>> attributes
>> for unavailable sensors are not exposed during hwmon device 
>> registration.
>>
>> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_hwmon.c | 79 +++++++++++++++++++++++++++++------
>>   1 file changed, 66 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c 
>> b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 5284cab6703d..2c4eba4b8f8f 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -813,12 +813,21 @@ static int 
>> xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
>>       return ret;
>>   }
>>   +static inline bool is_temp_valid(const struct xe_hwmon *hwmon, u8 
>> value)
>> +{
>> +    /* Value of 0xFF indicates unavailable sensor for platforms from 
>> CRI. */
>> +    if (hwmon->xe->info.platform >= XE_CRESCENTISLAND)
>> +        return value != U8_MAX;
>> +    else
>> +        return value != 0;
>
> How about returning true here?
for BMG value 0 will be there if temperature sensor is not there, which 
is why we are checking this way.
>
>> +}
>> +
>>   static int get_mc_temp(struct xe_hwmon *hwmon, long *val)
>>   {
>>       struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
>>       u32 *dword = (u32 *)hwmon->temp.value;
>> +    int ret, i, count = 0;
>>       s32 average = 0;
>> -    int ret, i;
>>         for (i = 0; i < DIV_ROUND_UP(TEMP_LIMIT_MAX, sizeof(u32)); 
>> i++) {
>>           ret = xe_pcode_read(root_tile, 
>> PCODE_MBOX(PCODE_THERMAL_INFO, READ_THERMAL_DATA, i),
>> @@ -828,11 +837,25 @@ static int get_mc_temp(struct xe_hwmon *hwmon, 
>> long *val)
>>           drm_dbg(&hwmon->xe->drm, "thermal data for group %d val 
>> 0x%x\n", i, dword[i]);
>>       }
>>   -    for (i = TEMP_INDEX_MCTRL; i < hwmon->temp.count - 1; i++)
>> -        average += hwmon->temp.value[i];
>> +    for (i = TEMP_INDEX_MCTRL; i < hwmon->temp.count - 1; i++) {
>> +        if (is_temp_valid(hwmon, hwmon->temp.value[i])) {
>> +            average += hwmon->temp.value[i];
>> +            count++;
>> +        } else {
>> +            drm_dbg(&hwmon->xe->drm, "mc temp sensor %d not 
>> available, val 0x%x\n",
>> +                i, hwmon->temp.value[i]);
> Is this required?
i think we can have this debug log to know which memory controller 
channel is available, of the available count
>> +        }
>> +    }
>> +
>> +    if (!count) {
>> +        drm_warn(&hwmon->xe->drm, "no memory temp sensors 
>> available!\n");
>> +        return -ENXIO;
> Why warning? If sensors are not available then its fine. This will any 
> way avoid exposing attribute.
agree, sashiko also pointed to that, removing this is in next revision
>> +    }
>> +
>> +    average /= count;
>> +    if (val)
>> +        *val = average * MILLIDEGREE_PER_DEGREE;
>>   -    average /= (hwmon->temp.count - TEMP_INDEX_MCTRL - 1);
>> -    *val = average * MILLIDEGREE_PER_DEGREE;
>>       return 0;
>>   }
>>   @@ -852,7 +875,13 @@ static int get_pcie_temp(struct xe_hwmon 
>> *hwmon, long *val)
>>           data = REG_FIELD_GET(PCIE_SENSOR_MASK, data);
>>         data = REG_FIELD_GET(TEMP_MASK, data);
>> -    *val = (s8)data * MILLIDEGREE_PER_DEGREE;
>> +    if (!is_temp_valid(hwmon, data)) {
>> +        drm_warn(&hwmon->xe->drm, "pcie temp sensor not available, 
>> val 0x%x\n", data);
>> +        return -ENXIO;
> Ditto.
I shall remove this.
>> +    }
>> +
>> +    if (val)
>> +        *val = (s8)data * MILLIDEGREE_PER_DEGREE;
>>         return 0;
>>   }
>> @@ -956,11 +985,21 @@ static inline bool is_vram_ch_available(struct 
>> xe_hwmon *hwmon, int channel)
>>       struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>>       int vram_id = channel - CHANNEL_VRAM_N;
>>       struct xe_reg vram_reg;
>> +    u32 reg_val;
>> +    u8 temp;
>>         vram_reg = xe_hwmon_get_reg(hwmon, REG_TEMP, channel);
>> -    if (!xe_reg_is_valid(vram_reg) || !xe_mmio_read32(mmio, vram_reg))
>> +    if (!xe_reg_is_valid(vram_reg))
>>           return false;
>>   +    reg_val = xe_mmio_read32(mmio, vram_reg);
>> +    temp = REG_FIELD_GET(TEMP_MASK, reg_val);
>
> There is sashiko-bot warning on this. Need to fix.
>
> Thanks,
> Badal
agree, next revision will have the fix.
>
>> +    if (!is_temp_valid(hwmon, temp)) {
>> +        drm_dbg(&hwmon->xe->drm, "vram channel %d unavailable, val 
>> 0x%x\n", vram_id,
>> +            reg_val);
>> +        return false;
>> +    }
>> +
>>       /* Create label only for available vram channel */
>>       sprintf(hwmon->temp.vram_label[vram_id], "vram_ch_%d", vram_id);
>>       return true;
>> @@ -977,8 +1016,9 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, 
>> u32 attr, int channel)
>>           case CHANNEL_VRAM:
>>               return hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN] ? 
>> 0444 : 0;
>>           case CHANNEL_MCTRL:
>> +            return !get_mc_temp(hwmon, NULL) && hwmon->temp.count ? 
>> 0444 : 0;
>>           case CHANNEL_PCIE:
>> -            return hwmon->temp.count ? 0444 : 0;
>> +            return !get_pcie_temp(hwmon, NULL) && hwmon->temp.count 
>> ? 0444 : 0;
>>           case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
>>               return (is_vram_ch_available(hwmon, channel) &&
>>                   hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN]) ? 0444 
>> : 0;
>> @@ -992,8 +1032,9 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, 
>> u32 attr, int channel)
>>           case CHANNEL_VRAM:
>>               return hwmon->temp.limit[TEMP_LIMIT_MEM_CRIT] ? 0444 : 0;
>>           case CHANNEL_MCTRL:
>> +            return !get_mc_temp(hwmon, NULL) && hwmon->temp.count ? 
>> 0444 : 0;
>>           case CHANNEL_PCIE:
>> -            return hwmon->temp.count ? 0444 : 0;
>> +            return !get_pcie_temp(hwmon, NULL) && hwmon->temp.count 
>> ? 0444 : 0;
>>           case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
>>               return (is_vram_ch_available(hwmon, channel) &&
>>                   hwmon->temp.limit[TEMP_LIMIT_MEM_CRIT]) ? 0444 : 0;
>> @@ -1011,12 +1052,24 @@ xe_hwmon_temp_is_visible(struct xe_hwmon 
>> *hwmon, u32 attr, int channel)
>>       case hwmon_temp_label:
>>           switch (channel) {
>>           case CHANNEL_PKG:
>> -        case CHANNEL_VRAM:
>> -            return xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_TEMP,
>> -                                channel)) ? 0444 : 0;
>> +        case CHANNEL_VRAM: {
>> +            struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>> +            struct xe_reg reg = xe_hwmon_get_reg(hwmon, REG_TEMP, 
>> channel);
>> +            u32 reg_val;
>> +            u8 temp;
>> +
>> +            if (!xe_reg_is_valid(reg))
>> +                return 0;
>> +
>> +            reg_val = xe_mmio_read32(mmio, reg);
>> +            temp = REG_FIELD_GET(TEMP_MASK, reg_val);
>> +
>> +            return is_temp_valid(hwmon, temp) ? 0444 : 0;
>> +        }
>>           case CHANNEL_MCTRL:
>> +            return !get_mc_temp(hwmon, NULL) && hwmon->temp.count ? 
>> 0444 : 0;
>>           case CHANNEL_PCIE:
>> -            return hwmon->temp.count ? 0444 : 0;
>> +            return !get_pcie_temp(hwmon, NULL) && hwmon->temp.count 
>> ? 0444 : 0;
>>           case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
>>               return is_vram_ch_available(hwmon, channel) ? 0444 : 0;
>>           default: