Re: [PATCH 2/3] drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI

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

[-- Attachment #1: Type: text/plain, Size: 5482 bytes --]


On 25-08-2026 00:27, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
>
> New issues:
> - [High] VRAM temperature sensors are silently lost on platforms where the thermal mailbox read fails or is unsupported.
> - [High] The dynamic VRAM temperature sensor count feature for CRI is completely non-functional due to missing static attribute definitions and register mappings.
>
> Pre-existing issues:
> - [Medium] The hardware monitoring device is registered directly from the DRM subsystem rather than using an auxiliary device driver.
> --
>
> --- Patch [2]: [PATCH 2/3] drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI ---
> commit ef0f46c26d79ec5ef4b877579a9d69bdbc93ae6c
> Author: Karthik Poosa<karthik.poosa@intel.com>
> Subject: drm/xe/hwmon: Use VRAM temperature sensor count from thermal config on CRI
>
> This patch reads the number of VRAM temperature sensor channels from the
> second byte of READ_THERMAL_CONFIG on CRI platforms. It uses this reported
> count to cap the exposed hwmon attributes for VRAM temperature sensors,
> while retaining the maximum supported channel count for non-CRI platforms.
>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index c15312b0858ca..8c8a519711230 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> [ ... ]
>
>> @@ -786,6 +790,17 @@ static int xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
>>   	drm_dbg(&hwmon->xe->drm, "thermal config count 0x%x\n", config);
>>   	hwmon->temp.count = REG_FIELD_GET(TEMP_MASK, config);
>>   
>> +	if (hwmon->xe->info.platform >= XE_CRESCENTISLAND) {
>> +		hwmon->temp.vram_count = REG_FIELD_GET(VRAM_COUNT_MASK, config);
>> +		if (hwmon->temp.vram_count > MAX_VRAM_CHANNELS && hwmon->temp.vram_count) {
>> +			drm_warn(&hwmon->xe->drm, "VRAM channel count %d exceeds max %d, clamping\n",
>> +				 hwmon->temp.vram_count, MAX_VRAM_CHANNELS);
>> +			hwmon->temp.vram_count = MAX_VRAM_CHANNELS;
>> +		}
> [Severity: High]
> Can the extra sensors on CRI platforms actually be exposed to sysfs?
>
> The HWMON_CHANNEL_INFO(temp, ...) static attribute array doesn't appear
> to be expanded in this patch. Since the static array caps the maximum
> channels visible to sysfs, won't the hwmon core stop querying early
> regardless of the dynamic vram_count value?
Yes, HWMON_CHANNEL_INFO() needs to be increased to accommodate the new 
VRAM channels of CRI.
>
> Also, looking at xe_hwmon_get_reg(), there doesn't seem to be a branch
> for XE_CRESCENTISLAND under the REG_TEMP case. Will is_vram_ch_available()
> receive an invalid register and fail for all VRAM sensors on CRI?
XE_CRESENTISLAND check is there in xe_hwmon_get_reg(), see 
https://gitlab.freedesktop.org/drm/tip/-/blob/drm-tip/drivers/gpu/drm/xe/xe_hwmon.c?ref_type=heads#L269
CRI and BMG share same offsets
>
>> +	} else {
>> +		hwmon->temp.vram_count = 16; /* For older platforms, max is 16 VRAM channels */
>> +	}
>> +
>>   	return ret;
>>   }
> [Severity: High]
> If xe_pcode_read() fails earlier in xe_hwmon_pcode_read_thermal_info(), the
> function returns early. Does this leave hwmon->temp.vram_count uninitialized
> at 0 instead of setting the fallback value of 16?
we shall initialize hwmon->temp.vram_count to 16 before pcode operations 
for this.
>
>> @@ -964,6 +979,9 @@ static inline bool is_vram_ch_available(struct xe_hwmon *hwmon, int channel)
>>   	u32 reg_val;
>>   	u8 temp;
>>   
>> +	if (vram_id >= hwmon->temp.vram_count)
>> +		return false;
>> +
> [Severity: High]
> If vram_count was left at 0 due to an early return during the pcode read,
> this bounds check will unconditionally return false.
>
> Does this mean a transient pcode error permanently disables all VRAM
> temperature monitoring on older platforms, whereas previously the channels
> were still exposed?
>
> [ ... ]
> same as above
>> @@ -1621,6 +1639,13 @@ int xe_hwmon_register(struct xe_device *xe)
>>   
>>   	xe_hwmon_get_preregistration_info(hwmon);
>>   
>> +	hwmon->temp.vram_label = devm_kcalloc(dev, hwmon->temp.vram_count,
>> +					      MAX_LABEL_SIZE, GFP_KERNEL);
>> +	if (!hwmon->temp.vram_label) {
>> +		xe->hwmon = NULL;
>> +		return -ENOMEM;
>> +	}
>> +
>>   	drm_dbg(&xe->drm, "Register xe hwmon interface\n");
>>   
>>   	/*  hwmon_dev points to device hwmon<i> */
> [Severity: Medium]
> This is a pre-existing issue, but should this hardware monitoring device be
> registered using an auxiliary device driver?
>
> The hwmon subsystem documentation dictates that if the main functionality of
> a chip is not hardware monitoring (such as a DRM controller), its hardware
> monitoring functionality should be implemented as an auxiliary device driver
> in drivers/hwmon/, rather than being registered directly from the DRM
> subsystem.
This is a pre-existing design choice in the Xe driver.
The hwmon device is currently registered directly from the DRM driver,
similar to several other GPU drivers exposing thermal and power 
telemetry through hwmon.

While the hwmon documentation recommends using a separate hwmon driver 
when hardware monitoring is not the primary function of the device,
migrating the existing Xe hwmon implementation to an auxiliary device 
would be a larger architectural change and is outside the scope of this 
series.
This patch set only adds CRI-specific thermal sensor handling on top of 
the existing hwmon infrastructure.


[-- Attachment #2: Type: text/html, Size: 8879 bytes --]