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

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

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


On 10-01-2026 21:53, Raag Jadav wrote:
> 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.
>
> ...
ok
>
>> @@ -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.
ok
>
> ...
>
>> @@ -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;
> }
I'll agree with vram_id and boolean values, for readability,
other than that I would like to stick to current implementation.
>>   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]?
that can be secondary check, then this would apply to all channels !
>
>>   		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?
|REG_FIELD_GET() returns unsigned type, which gets stored |the lower 24 
bits of an |s32|, discarding the sign bit; consequently, negative values 
are interpreted as positive, requiring an explicit sign check.
>
>> +				 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

what is your suggestion for that label here ?

>
>>   		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
>>

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