Re: [PATCH i-g-t v4 11/13] drm/xe/hwmon: Decode CRI temperature registers as IEEE-754

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwmon: Incorrect IEEE-754 decoding applied to CHANNEL_VRAM_N
--

commit 4017413f8095f651034d897f32ec30cb3740f4f1
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Decode CRI temperature registers as IEEE-754

This commit decodes the package and VRAM temperature registers for Crescent
Island platforms as IEEE-754 floating-point values instead of the previous
fixed-point formats. It implements integer-based decoding to be exposed through
hwmon.

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index e1cff5700e7f9..c411cd375aa1b 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
[ ... ]
> @@ -1249,26 +1312,27 @@ xe_hwmon_temp_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
>  	switch (attr) {
>  	case hwmon_temp_input:
>  		switch (channel) {
[ ... ]
> +		case CHANNEL_PKG:
> +		case CHANNEL_VRAM:
>  		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
>  			reg_val = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_TEMP, channel));
> -			/*
> -			 * This temperature format is 24 bit [31:8] signed integer and 8 bit
> -			 * [7:0] fraction.
> -			 */
> -			*val = (s32)(REG_FIELD_GET(TEMP_MASK_VRAM_N, reg_val)) *
> -				(REG_FIELD_GET(TEMP_SIGN_MASK, reg_val) ? -1 : 1) *
> -				 MILLIDEGREE_PER_DEGREE;
> +			if (hwmon->xe->info.platform >= XE_CRESCENTISLAND) {
> +				return xe_hwmon_temperature_decode((u32)reg_val, val);

[Severity: Medium]
Will this incorrectly apply IEEE-754 decoding to CHANNEL_VRAM_N registers on
Crescent Island?

Looking at xe_hwmon_get_reg(), when the platform is XE_CRESCENTISLAND,
CHANNEL_VRAM_N still maps to the legacy BMG_VRAM_TEMPERATURE_N:

drivers/gpu/drm/xe/xe_hwmon.c:xe_hwmon_get_reg() {
	...
	case REG_TEMP:
		if (xe->info.platform == XE_CRESCENTISLAND) {
			if (channel == CHANNEL_PKG)
				return CRI_PACKAGE_TEMPERATURE;
			else if (channel == CHANNEL_VRAM)
				return CRI_VRAM_TEMPERATURE;
			else if (in_range(channel, CHANNEL_VRAM_N, hwmon->temp.vram_count) &&
				 xe_hwmon_vram_channel_enabled(hwmon, channel - CHANNEL_VRAM_N))
				return BMG_VRAM_TEMPERATURE_N(channel - CHANNEL_VRAM_N);
	...
}

Since BMG_VRAM_TEMPERATURE_N uses a 24.8 fixed-point format, decoding it as
IEEE-754 will cause the exponent bits to evaluate to 0 for most normal
temperatures. This could result in the function returning 0 degrees instead of
the correct value.

Should CHANNEL_VRAM_N bypass the IEEE-754 decoding even on Crescent Island?

> +			} else if (channel >= CHANNEL_VRAM_N) {
> +				/*
> +				 * This temperature format is 24 bit [31:8] signed integer and 8 bit
> +				 * [7:0] fraction for platforms before CRI.
> +				 */
> +				*val = (s32)REG_FIELD_GET(TEMP_MASK_VRAM_N, reg_val) *
> +					(REG_FIELD_GET(TEMP_SIGN_MASK, reg_val) ? -1 : 1) *
> +					MILLIDEGREE_PER_DEGREE;
> +			} else {
> +				*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> +			}
>  			return 0;
>  		default:
>  			return -EOPNOTSUPP;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924105000.2627929-1-karthik.poosa@intel.com?part=11