Re: [PATCH v3 1/9] drm/xe/hwmon: initialize fan-control backend and table cache

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

On 29-10-2026 00:16, Purkait, Soham wrote:
>
> Hi Karthik,
>
> On 16-10-2026 00:37, Karthik Poosa wrote:
>> Initialize Xe hwmon fan-control support by detecting fan count,
>> reading stock fan control points and min PWM.
>>
>> v2:
>>   - Avoid user table initialization during probe. (Badal)
>>   - Move unused code to appropriate patches.
>>   - Use xe helpers for dmesg logs.
>>
>> v3:
>>   - Rebase.
>>   - Refine a few debug log messages and comments.
>>   - Update xe_hwmon_read_fan_control_info() to return on any stock table
>>     read failures.
>>   - Avoid returning when num_fans is 0 in
>>     xe_hwmon_get_preregistration_info(), ensuring thermal mailbox
>>     initialization is not skipped.
>>   - Remove XE_PCODE_FAN_CONTROL_TIMEOUT_MS which is unncessary as default
>>     timeout itself is now 10ms.
>>   - Use xe_pcode_read() instead of xe_pcode_read_timeout().
>>   - Rename fan_control_point_count to point_count in struct fan_table.
>>   - Resolve kgdoc warnings.
>>   - Add valid check for stock table fan point count.
>>   - Use the fan-enable bitmap reported by FSC_READ_NUM_FANS when enumerating
>>     fans. This avoids assuming contiguous fan indices and allows masks with
>>     gaps to be represented correctly in the hwmon interface.
>>
>> Signed-off-by: Karthik Poosa<karthik.poosa@intel.com>
>> Assisted-by: LLM
>> ---
>>   drivers/gpu/drm/xe/xe_hwmon.c     | 167 +++++++++++++++++++++++++++---
>>   drivers/gpu/drm/xe/xe_pcode_api.h |   5 +
>>   2 files changed, 157 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 42acfeb8818c..f2bd20f8b0a2 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -21,6 +21,7 @@
>>   #include "xe_pcode_api.h"
>>   #include "xe_sriov.h"
>>   #include "xe_pm.h"
>> +#include "xe_printk.h"
>>   #include "xe_vsec.h"
>>   #include "regs/xe_pmt.h"
>>   
>> @@ -137,14 +138,51 @@ struct xe_hwmon_energy_info {
>>   	long accum_energy;
>>   };
>>   
>> +enum fan_table_type {
>> +	/** @STOCK_FAN_TABLE: firmware-provided fan table */
>> +	STOCK_FAN_TABLE,
>> +	/** @USER_FAN_TABLE: user fan table written through sysfs */
>> +	USER_FAN_TABLE,
>> +	/** @FAN_TABLE_MAX: number of fan tables tracked per fan */
>> +	FAN_TABLE_MAX,
>> +};
>> +
>> +/* Check if a fan is enabled based on the fan_enable_mask got from FSC_READ_NUM_FANS. */
>> +#define IS_FAN_ENABLED(fan) (hwmon->fan_enable_mask & BIT(fan))
> If this macro is called from any function having no local variable 
> named hwmon it will break. Should it take hwmon as an argument ?
yes will change this
>> +
>> +/* Maximum number of fan control points supported by each fan. */
>> +#define MAX_FAN_CONTROL_POINTS	(10)
>> +
>> +/* Fan control point index: bit31 selects fan table type; bits30:0 control point index. */
>> +#define FCP_INDEX(FAN_TABLE_TYPE, POINT_NUM) (((FAN_TABLE_TYPE) << 31) | \
> Cast to u32 ? shifting the signed enum into bit 31 could lead to UB.
okay
>> +						((POINT_NUM) & REG_GENMASK(30, 0)))
>> +
>>   /**
>> - * struct xe_hwmon_fan_info - to cache previous fan reading
>> + * struct xe_hwmon_fan_info - cached fan telemetry and control state
>> + *
>> + * Each fan keeps the latest tachometer sampling state along with two fan
>> + * tables: the stock table discovered from firmware and the user table managed
>> + * by hwmon sysfs writes.
>>    */
>>   struct xe_hwmon_fan_info {
>>   	/** @reg_val_prev: previous fan reg val */
>>   	u32 reg_val_prev;
>>   	/** @time_prev: previous timestamp */
>>   	u64 time_prev;
>> +	/** @fan_table: fan control tables */
>> +	struct fan_table {
>> +		/** @point_count: number of fan control points */
>> +		u8 point_count;
>> +		/** @fcp: array of fan control points */
>> +		struct fan_control_point {
>> +			/** @temp: temperature in degree celsius */
>> +			u8 temp;
>> +			/** @speed: fan speed in percentage */
>> +			u8 speed;
>> +		} fcp[MAX_FAN_CONTROL_POINTS];
>> +	} fan_table[FAN_TABLE_MAX];
>> +	/** @min_pwm: minimum fan PWM */
>> +	u32 min_pwm;
>>   };
>>   
>>   /**
>> @@ -193,6 +231,8 @@ struct xe_hwmon {
>>   	int scl_shift_time;
>>   	/** @ei: Energy info for energyN_input */
>>   	struct xe_hwmon_energy_info ei[CHANNEL_MAX];
>> +	/** @fan_enable_mask: bitmask of available fans */
>> +	u32 fan_enable_mask;
>>   	/** @fi: Fan info for fanN_input */
>>   	struct xe_hwmon_fan_info fi[FAN_MAX];
>>   	/** @read_power_limit_from_pcode: pcode accepted power limits write for the channel */
>> @@ -1075,17 +1115,112 @@ static int xe_hwmon_pcode_write_i1(const struct xe_hwmon *hwmon, u32 uval)
>>   			      (uval & POWER_SETUP_I1_DATA_MASK));
>>   }
>>   
>> -static int xe_hwmon_pcode_read_fan_control(const struct xe_hwmon *hwmon, u32 subcmd, u32 *uval)
>> +static int xe_hwmon_pcode_read_fan_control(const struct xe_hwmon *hwmon, u32 subcmd, u8 fan_num,
>> +					   u32 *uval)
>>   {
>>   	struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
> Nit: blank line after variable declaration
ok
>> +	return xe_pcode_read(root_tile, PCODE_MBOX(FAN_SPEED_CONTROL, subcmd, fan_num),
>> +				     uval, NULL);
>> +}
>> +
>> +static int xe_hwmon_get_fan_enable_mask(struct xe_hwmon *hwmon)
>> +{
>> +	int ret;
>>   
>>   	/* Platforms that don't return correct value */
>> -	if (hwmon->xe->info.platform == XE_DG2 && subcmd == FSC_READ_NUM_FANS) {
>> -		*uval = 2;
>> +	if (hwmon->xe->info.platform == XE_DG2) {
>> +		hwmon->fan_enable_mask = GENMASK(1, 0);
>>   		return 0;
>>   	}
>>   
>> -	return xe_pcode_read(root_tile, PCODE_MBOX(FAN_SPEED_CONTROL, subcmd, 0), uval, NULL);
>> +	ret = xe_hwmon_pcode_read_fan_control(hwmon, FSC_READ_NUM_FANS, 0,
>> +					      &hwmon->fan_enable_mask);
>> +	if (ret) {
>> +		xe_warn(hwmon->xe, "failed to read number of fans, ret=%d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* Check if fan mask exceeds FAN_MAX and log a warning if it does */
>> +	if (hwmon->fan_enable_mask & ~GENMASK(FAN_MAX - 1, 0)) {
>> +		xe_warn(hwmon->xe, "fan enable mask exceeds maximum supported fans\n");
> Reset the mask to 0 so IS_FAN_ENABLED() stays false in case fan mask 
> exceeds FAN_MAX.
how about clamping to FAN_MAX ?
>> +		return -EINVAL;
>> +	}
>> +
>> +	xe_info(hwmon->xe, "Number of fans detected: %u\n",
>> +		hweight32(hwmon->fan_enable_mask));
>> +	return 0;
>> +}
>> +
>> +static int xe_hwmon_read_fan_control_info(struct xe_hwmon *hwmon)
>> +{
>> +	struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
>> +	int point;
>> +	int fan;
>> +	int ret;
>> +
>> +	for (fan = 0; fan < FAN_MAX; fan++) {
>> +		u32 stock_fcp_count = 0;
>> +
>> +		struct xe_hwmon_fan_info *fi = &hwmon->fi[fan];
>> +
>> +		if (!IS_FAN_ENABLED(fan))
>> +			continue;
>> +
>> +		ret = xe_hwmon_pcode_read_fan_control(hwmon,
>> +						      FSC_READ_STOCK_FAN_CONTROL_POINTS,
>> +						      fan, &stock_fcp_count);
>> +		if (ret) {
>> +			xe_err(hwmon->xe,
>> +			       "failed to read fan %d stock table point count, ret=%d\n",
>> +			       fan, ret);
> Why not continue for the rest of the fans marking the current one ?
yes we can continue, you want to mark current fan invalid ?
>> +			return ret;
>> +		}
>> +
>> +		if (!stock_fcp_count || stock_fcp_count > MAX_FAN_CONTROL_POINTS) {
>> +			xe_warn(hwmon->xe,
>> +				"fan %d stock table point count %u is invalid\n",
>> +				fan, stock_fcp_count);
> Ditto.
okay
>> +			return -EINVAL;
>> +		}
>> +
>> +		fi->fan_table[STOCK_FAN_TABLE].point_count = stock_fcp_count;
>> +		xe_dbg(hwmon->xe, "fan %d stock table point count is %u\n", fan,
>> +		       fi->fan_table[STOCK_FAN_TABLE].point_count);
>> +
>> +		/* Cache the stock fan table for later use. */
>> +		for (point = 0; point < fi->fan_table[STOCK_FAN_TABLE].point_count;
>> +		     point++) {
>> +			u32 fcp = 0;
>> +
>> +			fcp = FCP_INDEX(STOCK_FAN_TABLE, point);
>
> nit: u32 fcp = FCP_INDEX(STOCK_FAN_TABLE, point);
>
> Regards,
> Soham
ok
>> +			ret = xe_pcode_read(root_tile, PCODE_MBOX(FAN_SPEED_CONTROL,
>> +								  FSC_READ_FAN_TABLE, fan),
>> +					    &fcp, NULL);
>> +			if (ret) {
>> +				xe_err(hwmon->xe, "failed to read fan %d stock point %d, ret=%d\n",
>> +				       fan, point, ret);
>> +				fi->fan_table[STOCK_FAN_TABLE].point_count = 0;
>> +				return ret;
>> +			}
>> +
>> +			fi->fan_table[STOCK_FAN_TABLE].fcp[point].temp =
>> +						REG_FIELD_GET(FAN_CONTROL_POINT_TEMP_MASK, fcp);
>> +			fi->fan_table[STOCK_FAN_TABLE].fcp[point].speed =
>> +						REG_FIELD_GET(FAN_CONTROL_POINT_SPEED_MASK, fcp);
>> +			xe_dbg(hwmon->xe, "fan %d stock point %d: temp %u C, speed %u\n",
>> +			       fan, point, fi->fan_table[STOCK_FAN_TABLE].fcp[point].temp,
>> +			       fi->fan_table[STOCK_FAN_TABLE].fcp[point].speed);
>> +		}
>> +
>> +		/* Read minimum fan PWM */
>> +		ret = xe_hwmon_pcode_read_fan_control(hwmon, FSC_READ_FAN_MIN_PWM, fan,
>> +						      &fi->min_pwm);
>> +		if (ret)
>> +			xe_warn(hwmon->xe, "fan %d min pwm unavailable, ret=%d\n", fan, ret);
>> +		else
>> +			xe_dbg(hwmon->xe, "fan %d min PWM %u\n", fan, fi->min_pwm);
>> +	}
>> +	return 0;
>>   }
>>   
>>   static int xe_hwmon_power_curr_crit_read(struct xe_hwmon *hwmon, int channel,
>> @@ -1615,17 +1750,12 @@ xe_hwmon_energy_read(struct xe_hwmon *hwmon, u32 attr, int channel, long *val)
>>   static umode_t
>>   xe_hwmon_fan_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>>   {
>> -	u32 uval = 0;
>> -
>>   	if (!hwmon->xe->info.has_fan_control)
>>   		return 0;
>>   
>>   	switch (attr) {
>>   	case hwmon_fan_input:
>> -		if (xe_hwmon_pcode_read_fan_control(hwmon, FSC_READ_NUM_FANS, &uval))
>> -			return 0;
>> -
>> -		return channel < uval ? 0444 : 0;
>> +		return IS_FAN_ENABLED(channel) ? 0444 : 0;
>>   	default:
>>   		return 0;
>>   	}
>> @@ -1858,11 +1988,18 @@ xe_hwmon_get_preregistration_info(struct xe_hwmon *hwmon)
>>   		if (xe_hwmon_is_visible(hwmon, hwmon_energy, hwmon_energy_input, channel))
>>   			xe_hwmon_energy_get(hwmon, channel, &energy);
>>   
>> -	/* Initialize 'struct xe_hwmon_fan_info' with initial fan register reading. */
>> -	for (channel = 0; channel < FAN_MAX; channel++)
>> -		if (xe_hwmon_is_visible(hwmon, hwmon_fan, hwmon_fan_input, channel))
>> -			xe_hwmon_fan_input_read(hwmon, channel, &fan_speed);
>> +	if (hwmon->xe->info.has_fan_control) {
>> +		xe_hwmon_get_fan_enable_mask(hwmon);
>> +
>> +		/* Initialize 'struct xe_hwmon_fan_info' with initial fan register reading. */
>> +		for (channel = 0; channel < FAN_MAX; channel++)
>> +			if (IS_FAN_ENABLED(channel))
>> +				xe_hwmon_fan_input_read(hwmon, channel, &fan_speed);
>>   
>> +		/* Fan control tables initialization */
>> +		if (xe_hwmon_read_fan_control_info(hwmon))
>> +			xe_warn(hwmon->xe, "Fan control tables are not available\n");
>> +	}
>>   	return xe_hwmon_init_temp_info(hwmon);
>>   }
>>   
>> diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
>> index 00c4fc7d0124..0349b24a8259 100644
>> --- a/drivers/gpu/drm/xe/xe_pcode_api.h
>> +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
>> @@ -83,6 +83,11 @@
>>   
>>   #define   FAN_SPEED_CONTROL			0x7D
>>   #define     FSC_READ_NUM_FANS			0x4
>> +#define     FSC_READ_STOCK_FAN_CONTROL_POINTS	0x5
>> +#define     FSC_READ_FAN_TABLE			0x7
>> +#define     FAN_CONTROL_POINT_TEMP_MASK		REG_GENMASK(7, 0)
>> +#define     FAN_CONTROL_POINT_SPEED_MASK	REG_GENMASK(15, 8)
>> +#define     FSC_READ_FAN_MIN_PWM		0x8
>>   
>>   #define PCODE_SCRATCH(x)		XE_REG(0x138320 + ((x) * 4))
>>   /* PCODE_SCRATCH0 */