Re: [PATCH i-g-t v4 09/13] drm/xe/hwmon: Add platform-aware VRAM thermal channel support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hwmon: sparse msu_mask causes valid VRAM temperature channels to be dropped
- [Medium] hwmon: VRAM temperature register array mapping overlaps with other CRI registers
--

commit 281083adfcbfcf290d339e8d6d0578c01c2d9ec1
Author: Karthik Poosa <karthik.poosa@intel.com>

drm/xe/hwmon: Add platform-aware VRAM thermal channel support

This commit introduces dynamic VRAM temperature channel support for CRI
platforms by reading the MSU enablement mask from hardware. It also
increases the maximum VRAM channel count to 80.

> diff --git a/drivers/gpu/drm/xe/regs/xe_pcode_regs.h b/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> index c63b409d7a821..11916b4f7cfc0 100644
> --- a/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_pcode_regs.h
> @@ -29,5 +29,6 @@
>  
>  #define CRI_PACKAGE_ENERGY_STATUS		XE_REG(0x138120)
>  #define CRI_PLATFORM_ENERGY_STATUS		XE_REG(0x138458)
> +#define CRI_MSU_VRAM_ENABLE             XE_REG(0x138340)

[ ... ]

> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index cf5d51c685360..1b587bd9487e1 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -281,7 +298,8 @@ 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 (in_range(channel, CHANNEL_VRAM_N, MAX_VRAM_CHANNELS))
> +			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);

[Severity: Medium]
Will this return unrelated hardware registers when the channel index reaches 56
and higher?

The patch increases MAX_VRAM_CHANNELS to 80. BMG_VRAM_TEMPERATURE_N(n)
calculates the register address as 0x138260 + (n * 4). If n = 56, this
evaluates to 0x138340, which overlaps with the newly added
CRI_MSU_VRAM_ENABLE register. If n = 57, it evaluates to 0x138344, mapping
to CRI_PACKAGE_TEMPERATURE.

Should the mapping for high index channels be updated to avoid overlapping
with these registers?

[Severity: Medium]
Does this code silently drop valid channels if the mask is sparse?

In xe_hwmon_pcode_read_thermal_info(), vram_count is calculated using
hweight32(msu_mask) * 4, yielding the population count of active channels
rather than the highest valid channel index.

If msu_mask = 0xA (bits 1 and 3 enabled), vram_count is 8. The in_range()
check enforces a strict upper bound of 8 (channel - CHANNEL_VRAM_N < 8).
As a result, the enabled channels for bit 3 (indices 12-15) fail the
in_range() check and are dropped. 

Can this bounds check be revised to use the highest active bit index?

[ ... ]

> @@ -805,15 +887,38 @@ static int xe_hwmon_pcode_read_thermal_info(struct xe_hwmon *hwmon)
>  	struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
> +	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>  	u32 config = 0;
>  	int ret;
>  
> +	/*
> +	 * Only CRI reports dynamic VRAM channel state. Fixed-channel platforms
> +	 * set the count up front so those sensors stay visible even if the thermal
> +	 * mailbox reads below fail.
> +	 */
> +	if (hwmon->xe->info.has_fixed_vram_channels)
> +		hwmon->temp.vram_count = FIXED_VRAM_CHANNELS;
> +
> +	if (hwmon->xe->info.platform == XE_CRESCENTISLAND) {
> +		hwmon->temp.msu_mask = xe_mmio_read32(mmio, CRI_MSU_VRAM_ENABLE);
> +		/* Each enabled MSU bit maps to VRAM_CHANNELS_PER_MSU channels. */
> +		hwmon->temp.vram_count = hweight32(hwmon->temp.msu_mask) * VRAM_CHANNELS_PER_MSU;

[Severity: Medium]
Does this correctly determine the required channel count bound for sparse
masks?

This calculates the upper boundary using a population count (hweight32)
instead of the highest active bit index. As described above, this truncates
the index space and causes valid higher-indexed channels to be ignored when
lower MSUs are disabled.

> +		drm_dbg(&hwmon->xe->drm, "MSU VRAM enable mask 0x%x, VRAM channel count %d\n",
> +			hwmon->temp.msu_mask, hwmon->temp.vram_count);

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