Re: [PATCH v5 4/4] drm/xe/hwmon: Expose individual vram channel temperature

[Raag Jadav <raag.jadav@intel.com>]

On Sat, Jan 10, 2026 at 01:46:44AM +0530, Karthik Poosa wrote:
> Expose individual VRAM temperature attributes.

Please also use 'VRAM' in caps in patch subject.

...

> @@ -257,6 +264,9 @@ static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg
>  				return BMG_PACKAGE_TEMPERATURE;
>  			else if (channel == CHANNEL_VRAM)
>  				return BMG_VRAM_TEMPERATURE;
> +			else if (channel >= CHANNEL_VRAM_N && channel <= CHANNEL_VRAM_N_MAX)
> +				return BMG_VRAM_TEMPERATURE_N(((channel - CHANNEL_VRAM_N) *
> +								sizeof(u32)));

Make (n * sizeof(u32)) as part of BMG_VRAM_TEMPERATURE_N(). With that
perhaps this'll be a single line.

...

> @@ -890,6 +916,21 @@ static void xe_hwmon_get_voltage(struct xe_hwmon *hwmon, int channel, long *valu
>  	*value = DIV_ROUND_CLOSEST(REG_FIELD_GET(VOLTAGE_MASK, reg_val) * 2500, SF_VOLTAGE);
>  }
>  
> +static inline bool is_vram_ch_available(struct xe_hwmon *hwmon, int channel)
> +{
> +	struct xe_reg vram_ch_temp;
> +	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
> +
> +	vram_ch_temp = xe_hwmon_get_reg(hwmon, REG_TEMP, channel);
> +	if (xe_reg_is_valid(vram_ch_temp) && xe_mmio_read32(mmio, vram_ch_temp)) {
> +		/* Create label only for available vram channel */
> +		sprintf(hwmon->temp.vram_label[channel - CHANNEL_VRAM_N], "vram_ch_%d",
> +			(channel - CHANNEL_VRAM_N));
> +		return 1;
> +	}
> +	return 0;
> +}

I'd write this as

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;

        vram_reg = xe_hwmon_get_reg(hwmon, REG_TEMP, channel);
        if (!xe_reg_is_valid(vram_reg) || !xe_mmio_read32(mmio, vram_reg))
                return false;

        /* Create label only for available vram channel */
        sprintf(hwmon->temp.vram_label[vram_id], "vram_ch_%d", vram_id);
        return true;
}

>  static umode_t
>  xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  {
> @@ -903,6 +944,8 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  		case CHANNEL_MCTRL:
>  		case CHANNEL_PCIE:
>  			return hwmon->temp.count ? 0444 : 0;
> +		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
> +			return is_vram_ch_available(hwmon, channel) ? 0444 : 0;

Shouldn't we also check hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN]?

>  		default:
>  			return 0;
>  		}
> @@ -915,6 +958,8 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  		case CHANNEL_MCTRL:
>  		case CHANNEL_PCIE:
>  			return hwmon->temp.count ? 0444 : 0;
> +		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
> +			return is_vram_ch_available(hwmon, channel) ? 0444 : 0;

Ditto, hwmon->temp.limit[TEMP_LIMIT_MEM_CRIT]?

>  		default:
>  			return 0;
>  		}
> @@ -935,6 +980,8 @@ xe_hwmon_temp_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  		case CHANNEL_MCTRL:
>  		case CHANNEL_PCIE:
>  			return hwmon->temp.count ? 0444 : 0;
> +		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
> +			return is_vram_ch_available(hwmon, channel) ? 0444 : 0;
>  		default:
>  			return 0;
>  		}
> @@ -963,6 +1010,16 @@ xe_hwmon_temp_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
>  			return get_mc_temp(hwmon, val);
>  		case CHANNEL_PCIE:
>  			return get_pcie_temp(hwmon, val);
> +		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 bit 31 for sign, bits [30:8] for whole number
> +			 * and bits [7:0] for fraction

Nit: "Temperature format is 24 bits [31:8] signed integer and
8 bits [7:0] fraction."

> +			 */
> +			*val = (s32)(REG_FIELD_GET(TEMP_MASK_VRAM_N, reg_val)) *
> +				(REG_FIELD_GET(TEMP_SIGN_MASK, reg_val) ? -1 : 1) *

Since you're already casting it, I'm wondering if you need to check
for sign?

> +				 MILLIDEGREE_PER_DEGREE;
> +			return 0;
>  		default:
>  			return -EOPNOTSUPP;
>  		}
> @@ -974,6 +1031,7 @@ xe_hwmon_temp_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
>  			*val = hwmon->temp.limit[TEMP_LIMIT_PKG_SHUTDOWN] * MILLIDEGREE_PER_DEGREE;
>  			return 0;
>  		case CHANNEL_VRAM:
> +		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
>  			*val = hwmon->temp.limit[TEMP_LIMIT_MEM_SHUTDOWN] * MILLIDEGREE_PER_DEGREE;
>  			return 0;
>  		default:
> @@ -987,6 +1045,7 @@ xe_hwmon_temp_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
>  			*val = hwmon->temp.limit[TEMP_LIMIT_PKG_CRIT] * MILLIDEGREE_PER_DEGREE;
>  			return 0;
>  		case CHANNEL_VRAM:
> +		case CHANNEL_VRAM_N...CHANNEL_VRAM_N_MAX:
>  			*val = hwmon->temp.limit[TEMP_LIMIT_MEM_CRIT] * MILLIDEGREE_PER_DEGREE;
>  			return 0;
>  		default:
> @@ -1356,16 +1415,20 @@ static int xe_hwmon_read_label(struct device *dev,
>  			       enum hwmon_sensor_types type,
>  			       u32 attr, int channel, const char **str)
>  {
> +	struct xe_hwmon *hwmon = dev_get_drvdata(dev);
> +
>  	switch (type) {
>  	case hwmon_temp:
>  		if (channel == CHANNEL_PKG)
>  			*str = "pkg";
>  		else if (channel == CHANNEL_VRAM)
> -			*str = "vram";
> +			*str = "vram_avg";

If you look at the readings this is actually not average, so it's a bit
misleading.

Raag

>  		else if (channel == CHANNEL_MCTRL)
>  			*str = "mctrl";
>  		else if (channel == CHANNEL_PCIE)
>  			*str = "pcie";
> +		else if (channel >= CHANNEL_VRAM_N && channel <= CHANNEL_VRAM_N_MAX)
> +			*str = hwmon->temp.vram_label[channel - CHANNEL_VRAM_N];
>  		return 0;
>  	case hwmon_power:
>  	case hwmon_energy:
> -- 
> 2.25.1
>