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

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

On 12-01-2026 13:41, Raag Jadav wrote:
> On Sun, Jan 11, 2026 at 12:52:39AM +0530, Poosa, Karthik wrote:
>> On 10-01-2026 21:53, Raag Jadav wrote:
>>> On Sat, Jan 10, 2026 at 01:46:44AM +0530, Karthik Poosa wrote:
> ...
>
>>>> +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);i!
>>>           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.
> The usual practice is early return negative cases, but upto you.
okay we can do that in next revision
>
> Also, just curious: Do we need the 'ch' string here? We already know
> it's channel, right?
yes to differentiate between existing channel "vram", I've appended with 
_ch_X
>>>>    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 !
> For the channels that return data from the mailbox, we'd want to make sure
> the data source is also working. Else we'll have dummy attributes exposing
> no useful data.
okay, I shall add this in next revision
>
>>>>    		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.
> Would something like this work?
>
> 	s32 vram_n = (reg_val & TEMP_SIGN_MASK) | REG_FIELD_GET(TEMP_MASK_VRAM_N, reg_val);
this is okay
>
> 	*val = vram_n * MILLIDEGREE_PER_DEGREE;

but this wont work as it will treat value as unsigned and we'll need to 
again check the sign check here.

existing one does these all in a single statement.

> 	return 0;
>
>>>> +				 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.
>> what is your suggestion for that label here ?
> Since this is a stable ABI, let's first make sure that we can actually
> change output string. If we can, then something like vram_high or
> vram_peak would be more appropriate.
>
> Note: This is different from _max attribute which signifies the limit.
>
> Raag

1. Actually, this label change can be a separate patch.

2. We are modifying label here, ABI still remains same, so this should 
be okay

vram_peak seems apt, as it the current max value of all vrams

Rodrigo, can you also share your comments on this ?

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