Re: [PATCH v3 4/5] drm/xe/hwmon: Decode CRI temperature registers as IEEE-754

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Sep 10, 2026 at 11:57:55PM +0530, Karthik Poosa wrote:
> CRI temperature registers encode package and VRAM temperatures as
> IEEE-754 floating-point values. Decode these values before exposing
> them through hwmon.
>  
> Use integer-based IEEE-754 decoding since kernel code does not use
> floating-point operations and hwmon reports integer temperatures.
> 
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> Assisted-by: GitHub Copilot; Model: MAI-Code-1.1-Flash

nowadays 'Assisted-by: LLM' should be enough.
And this one here was not the previous style anyway.

> ---
> 
> v2: Reject temperatures that overflow long on 32-bit (Sashiko).
> 
>  drivers/gpu/drm/xe/xe_hwmon.c | 85 +++++++++++++++++++++++++++++------
>  1 file changed, 71 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index c682c5a33650..e4a210916df2 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -1159,6 +1159,62 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  	}
>  }
>  
> +/**
> + * xe_hwmon_temperature_decode - Decode an IEEE-754 temperature register value
> + * @reg_val: The raw register value to decode
> + *
> + * IEEE-754 single-precision format:
> + *
> + *  31 30                         23 22                         0
> + * +--+-----------------------------+----------------------------+
> + * | S|          exponent           |          fraction          |
> + * +--+-----------------------------+----------------------------+
> + *
> + * value = (-1)^S * 1.fraction * 2^(exponent - 127)
> + *
> + * Return: 0 on success or an error if @reg_val is non-finite or cannot be
> + * represented in millidegrees Celsius.
> + */
> +static int xe_hwmon_temperature_decode(u32 reg_val, long *temperature)
> +{
> +	u32 exponent = (reg_val >> 23) & 0xff;
> +	u32 mantissa = reg_val & 0x7fffff;

Perhaps all of these deserves some define somewhere?!

Perhaps something like LLM suggested:

        #define IEEE754_SIGN            BIT(31)
        #define IEEE754_EXP_MASK        GENMASK(30, 23)
        #define IEEE754_MANTISSA_MASK   GENMASK(22, 0)
        #define IEEE754_EXP_BIAS        127
        #define IEEE754_MANTISSA_BITS   23
        #define IEEE754_EXP_SPECIAL     0xff    /* Inf / NaN */

and use FIELD_GET() for the extraction

> +	s64 value;
> +	int shift;
> +
> +	/* Exponent 0xff encodes infinity or NaN. */
> +	if (exponent == 0xff)
> +		return -EINVAL;
> +
> +	/* Subnormal values are smaller than one millidegree Celsius. */
> +	if (!exponent) {
> +		*temperature = 0;
> +		return 0;
> +	}
> +
> +	value = (s64)(mantissa | BIT(23)) * MILLIDEGREE_PER_DEGREE;
> +	shift = exponent - 150;
> +	if (shift >= 0) {
> +		if (shift >= BITS_PER_LONG || value > (LONG_MAX >> shift))
> +			return -ERANGE;
> +		value <<= shift;
> +	} else if (shift > -64) {
> +		value >>= -shift;
> +	} else {
> +		value = 0;
> +	}
> +
> +	/* Ensure the magnitude fits in long, which is 32-bit on some arches. */
> +	if (value > LONG_MAX)
> +		return -ERANGE;
> +
> +	if (reg_val & BIT(31))
> +		value = -value;
> +
> +	*temperature = value;
> +	return 0;
> +}
> +
>  static int
>  xe_hwmon_temp_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
>  {
> @@ -1168,26 +1224,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:
> -			reg_val = xe_mmio_read32(mmio, xe_hwmon_get_reg(hwmon, REG_TEMP, channel));
> -
> -			/* HW register value is in degrees Celsius, convert to millidegrees. */
> -			*val = REG_FIELD_GET(TEMP_MASK, reg_val) * MILLIDEGREE_PER_DEGREE;
> -			return 0;
>  		case CHANNEL_MCTRL:
>  			return get_mc_temp(hwmon, val);
>  		case CHANNEL_PCIE:
>  			return get_pcie_temp(hwmon, val);
> +		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);
> +			} 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;
> -- 
> 2.25.1
>