Re: [PATCH v2 9/9] drm/xe/hwmon: Update fan info after late binding

["Purkait, Soham" <soham.purkait@intel.com>]

Hi Karthik,

On 17-07-2026 09:47, Karthik Poosa wrote:
> Add xe_hwmon_fan_update_post_lb() to update fan info after late binding.
> Add param is_init into xe_hwmon_read_fan_control_info() to avoid
> resetting user pwm mode post-LB.
> Call init path with is_init=true from preregistration.
>
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> Assisted-by: Codex:gpt-5-3
> ---
>   drivers/gpu/drm/xe/xe_hwmon.c        | 50 +++++++++++++++++++++++++---
>   drivers/gpu/drm/xe/xe_hwmon.h        |  2 ++
>   drivers/gpu/drm/xe/xe_late_bind_fw.c |  3 ++
>   3 files changed, 51 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 65f356c741f5..518ba4438de4 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -1848,7 +1848,7 @@ static const struct attribute_group hwmon_curve_attrgroup = {
>   	.is_visible = xe_hwmon_curve_attributes_visible,
>   };
>   
> -static int xe_hwmon_read_fan_control_info(struct xe_hwmon *hwmon)
> +static int xe_hwmon_read_fan_control_info(struct xe_hwmon *hwmon, bool is_init)
>   {
>   	struct xe_tile *root_tile = xe_device_get_root_tile(hwmon->xe);
>   	int point;
> @@ -1901,8 +1901,13 @@ static int xe_hwmon_read_fan_control_info(struct xe_hwmon *hwmon)
>   			       fi->fan_table[STOCK_FAN_TABLE].fcp[point].speed);
>   		}
>   
> -		/* Set PWM enable mode to automatic i.e stock table in use. */
> -		fi->pwm_enable_mode = XE_FAN_PWM_AUTO_STOCK_TABLE;
> +		/*
> +		 * Set PWM enable mode to automatic i.e stock table in use, only during init.
> +		 * For late binding, the mode is already set by the user and should not be
> +		 * overridden.
> +		 */
> +		if (is_init)
> +			fi->pwm_enable_mode = XE_FAN_PWM_AUTO_STOCK_TABLE;
>   
>   		/* Read minimum fan PWM */
>   		ret = xe_hwmon_pcode_read_fan_control(hwmon, FSC_READ_FAN_MIN_PWM, fan,
> @@ -2737,7 +2742,7 @@ xe_hwmon_get_preregistration_info(struct xe_hwmon *hwmon)
>   				xe_hwmon_fan_input_read(hwmon, channel, &fan_speed);
>   
>   		/* Fan control tables initialization */
> -		if (xe_hwmon_read_fan_control_info(hwmon))
> +		if (xe_hwmon_read_fan_control_info(hwmon, true))
>   			xe_warn(hwmon->xe, "Fan control tables are not available\n");
>   	}
>   
> @@ -2918,4 +2923,41 @@ void xe_hwmon_resume(struct xe_device *xe)
>   	mutex_unlock(&hwmon->hwmon_lock);
>   }
>   
> +/**
> + * xe_hwmon_fan_update_post_lb - Update fan info after late binding
> + * @xe: xe device instance
> + *
> + * This function reads the number of fans and their stock table after late binding.
> + */
> +void xe_hwmon_fan_update_post_lb(struct xe_device *xe)
> +{
> +	struct xe_hwmon *hwmon = xe->hwmon;
> +	int fan;
> +	int num_fans;
Should this be  u32 / u8?
> +
> +	if (!hwmon || !xe->info.has_fan_control)
> +		return;
> +
> +	mutex_lock(&hwmon->hwmon_lock);
> +
> +	/* Read number of fans */
> +	if (xe_hwmon_get_num_fans(hwmon, &num_fans)) {
> +		xe_warn(hwmon->xe, "Failed to read number of fans after LB\n");
> +		goto unlock;
> +	}
> +	hwmon->num_fans = num_fans;
Check the types to avoid type mismatch.
> +	xe_dbg(hwmon->xe, "Number of fans detected after LB: %u\n", hwmon->num_fans);
> +
> +	/* Read stock table */
> +	for (fan = 0; fan < hwmon->num_fans; fan++) {
This loop seems redundant.  xe_hwmon_read_fan_control_info() already has 
this.
> +		if (xe_hwmon_read_fan_control_info(hwmon, false)) {
> +			xe_warn(hwmon->xe, "Failed to read fan %d stock table after LB\n", fan);
> +			continue;
> +		}
> +	}
> +
> +unlock:
> +	mutex_unlock(&hwmon->hwmon_lock);
> +}
> +
>   MODULE_IMPORT_NS("INTEL_PMT_TELEMETRY");
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.h b/drivers/gpu/drm/xe/xe_hwmon.h
> index 6c6f30208508..7198d7a4a1be 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.h
> +++ b/drivers/gpu/drm/xe/xe_hwmon.h
> @@ -14,10 +14,12 @@ struct xe_device;
>   int xe_hwmon_register(struct xe_device *xe);
>   void xe_hwmon_suspend(struct xe_device *xe);
>   void xe_hwmon_resume(struct xe_device *xe);
> +void xe_hwmon_fan_update_post_lb(struct xe_device *xe);
>   #else
>   static inline int xe_hwmon_register(struct xe_device *xe) { return 0; };
>   static inline void xe_hwmon_suspend(struct xe_device *xe) {}
>   static inline void xe_hwmon_resume(struct xe_device *xe) {}
> +static inline void xe_hwmon_fan_update_post_lb(struct xe_device *xe) {}
>   #endif
>   
>   #endif /* _XE_HWMON_H_ */
> diff --git a/drivers/gpu/drm/xe/xe_late_bind_fw.c b/drivers/gpu/drm/xe/xe_late_bind_fw.c
> index 768442ca7da6..34295418ef50 100644
> --- a/drivers/gpu/drm/xe/xe_late_bind_fw.c
> +++ b/drivers/gpu/drm/xe/xe_late_bind_fw.c
> @@ -13,6 +13,7 @@
>   #include <drm/drm_print.h>
>   
>   #include "xe_device.h"
> +#include "xe_hwmon.h"
>   #include "xe_late_bind_fw.h"
>   #include "xe_pcode.h"
>   #include "xe_pcode_api.h"
> @@ -252,6 +253,8 @@ static void xe_late_bind_work(struct work_struct *work)
>   	if (!ret) {
>   		drm_dbg(&xe->drm, "Load %s firmware successful\n",
>   			fw_id_to_name[lbfw->id]);
> +		if (fw_id_to_name[lbfw->id] == XE_LB_FW_FAN_CONTROL)
Comparing a 'const char *' to an enum (0). Should this be : if (lbfw->id 
== XE_LB_FW_FAN_CONTROL) ?

Thanks,
soham
> +			xe_hwmon_fan_update_post_lb(late_bind_to_xe(late_bind));
>   		goto out;
>   	}
>