Re: [PATCH v11 6/6] drm/xe/hwmon: Expose power sysfs entries based on firmware support

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, May 29, 2025 at 04:04:30PM +0530, Karthik Poosa wrote:
> Enable hwmon sysfs entries (power_xxx) only when GPU firmware
> supports it.
> Previously, these entries were created if the MMIO register
> was present. Now, we enable based on the data in the register.

First of all, please ignore my previous comment. I had missundertood your
changes on last version of the patch 3.

> 
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hwmon.c | 65 +++++++++++++++++++++--------------
>  1 file changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index c57c613471c3..25a89575a629 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -296,18 +296,12 @@ static void xe_hwmon_power_max_read(struct xe_hwmon *hwmon, u32 attr, int channe
>  	if (hwmon->xe->info.has_mbx_power_limits) {
>  		xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, (u32 *)&reg_val);
>  	} else {
> -		rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
> -		pkg_power_sku = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
> -
>  		/*
> -		 * Valid check of REG_PKG_RAPL_LIMIT is already done in xe_hwmon_power_is_visible.
> +		 * Valid check of these registers is already done in xe_hwmon_power_is_visible.
>  		 * So not checking it again here.
>  		 */

With below block gone, we could even remove the entire comment section to keep
code cleaner here. We already know that all the checks are now in the visible.

with that cleaned up:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> -		if (!xe_reg_is_valid(pkg_power_sku)) {
> -			drm_warn(&xe->drm, "pkg_power_sku invalid\n");
> -			*value = 0;
> -			goto unlock;
> -		}
> +		rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
> +		pkg_power_sku = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
>  		reg_val = xe_mmio_read32(mmio, rapl_limit);
>  	}
>  
> @@ -652,17 +646,20 @@ static umode_t xe_hwmon_attributes_visible(struct kobject *kobj,
>  	int ret = 0;
>  	int channel = (index % 2) ? CHANNEL_PKG : CHANNEL_CARD;
>  	u32 power_attr = (index > 1) ? PL2_HWMON_ATTR : PL1_HWMON_ATTR;
> -	u32 uval;
> +	u32 uval = 0;
> +	struct xe_reg rapl_limit;
> +	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>  
>  	xe_pm_runtime_get(hwmon->xe);
>  
>  	if (hwmon->xe->info.has_mbx_power_limits) {
>  		xe_hwmon_pcode_read_power_limit(hwmon, power_attr, channel, &uval);
> -		ret = (uval & PWR_LIM_EN) ? attr->mode : 0;
>  	} else if (power_attr != PL2_HWMON_ATTR) {
> -		ret = xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT,
> -						       channel)) ? attr->mode : 0;
> +		rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
> +		if (xe_reg_is_valid(rapl_limit))
> +			uval = xe_mmio_read32(mmio, rapl_limit);
>  	}
> +	ret = (uval & PWR_LIM_EN) ? attr->mode : 0;
>  
>  	xe_pm_runtime_put(hwmon->xe);
>  
> @@ -806,24 +803,20 @@ static umode_t
>  xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  {
>  	u32 uval = 0;
> -	struct xe_reg rapl_limit;
> +	struct xe_reg reg;
>  	struct xe_mmio *mmio = xe_root_tile_mmio(hwmon->xe);
>  
>  	switch (attr) {
>  	case hwmon_power_max:
>  	case hwmon_power_cap:
> -	case hwmon_power_label:
>  		if (hwmon->xe->info.has_mbx_power_limits) {
>  			xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, &uval);
>  		} else if (attr != PL2_HWMON_ATTR) {
> -			rapl_limit = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
> -			if (xe_reg_is_valid(rapl_limit))
> -				uval = xe_mmio_read32(mmio, rapl_limit);
> +			reg = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
> +			if (xe_reg_is_valid(reg))
> +				uval = xe_mmio_read32(mmio, reg);
>  		}
>  		if (uval & PWR_LIM_EN) {
> -			if (attr == hwmon_power_label)
> -				return 0444;
> -
>  			drm_info(&hwmon->xe->drm, "%s is supported on channel %d\n",
>  				 PWR_ATTR_TO_STR(attr), channel);
>  			return 0664;
> @@ -832,17 +825,39 @@ xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  			PWR_ATTR_TO_STR(attr), channel);
>  		return 0;
>  	case hwmon_power_rated_max:
> -		if (hwmon->xe->info.has_mbx_power_limits)
> +		if (hwmon->xe->info.has_mbx_power_limits) {
>  			return 0;
> -		else
> -			return xe_reg_is_valid(xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU,
> -					       channel)) ? 0444 : 0;
> +		} else {
> +			reg = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
> +			if (xe_reg_is_valid(reg))
> +				uval = xe_mmio_read32(mmio, reg);
> +			return uval ? 0444 : 0;
> +		}
>  	case hwmon_power_crit:
>  		if (channel == CHANNEL_CARD) {
>  			xe_hwmon_pcode_read_i1(hwmon, &uval);
>  			return (uval & POWER_SETUP_I1_WATTS) ? 0644 : 0;
>  		}
>  		break;
> +	case hwmon_power_label:
> +		if (hwmon->xe->info.has_mbx_power_limits) {
> +			xe_hwmon_pcode_read_power_limit(hwmon, attr, channel, &uval);
> +		} else {
> +			reg = xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel);
> +			if (xe_reg_is_valid(reg))
> +				uval = xe_mmio_read32(mmio, reg);
> +
> +			if (!uval) {
> +				reg = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel);
> +				if (xe_reg_is_valid(reg))
> +					uval = xe_mmio_read32(mmio, reg);
> +			}
> +		}
> +		if ((!(uval & PWR_LIM_EN)) && channel == CHANNEL_CARD) {
> +			xe_hwmon_pcode_read_i1(hwmon, &uval);
> +			return (uval & POWER_SETUP_I1_WATTS) ? 0444 : 0;
> +		}
> +		return (uval) ? 0444 : 0;
>  	default:
>  		return 0;
>  	}
> -- 
> 2.25.1
>