Re: [PATCH v5 2/4] drm/xe/hwmon: Update xe_hwmon_get_reg to return struct xe_reg

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 04 Apr 2024, Karthik Poosa <karthik.poosa@intel.com> wrote:
> Return register address as struct xe_reg from xe_hwmon_get_reg
> instead of u32. (Lucas)
> Use XE_REG_IS_VALID() from caller to check returned xe_reg. (Badal)
> These changes are done to have abstracted usage of struct xe_reg.
>
> Signed-off-by: Karthik Poosa <karthik.poosa@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_hwmon.c | 47 +++++++++++++++++++----------------
>  1 file changed, 26 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c b/drivers/gpu/drm/xe/xe_hwmon.c
> index 7e8caac838e0..2385f05d9504 100644
> --- a/drivers/gpu/drm/xe/xe_hwmon.c
> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
> @@ -79,46 +79,46 @@ struct xe_hwmon {
>  	struct xe_hwmon_energy_info ei[CHANNEL_MAX];
>  };
>  
> -static u32 xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg, int channel)
> +static struct xe_reg xe_hwmon_get_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
> +				      int channel)
>  {
>  	struct xe_device *xe = gt_to_xe(hwmon->gt);
> -	struct xe_reg reg = XE_REG(0);
>  
>  	switch (hwmon_reg) {
>  	case REG_PKG_RAPL_LIMIT:
>  		if (xe->info.platform == XE_PVC && channel == CHANNEL_PKG)
> -			reg = PVC_GT0_PACKAGE_RAPL_LIMIT;
> +			return PVC_GT0_PACKAGE_RAPL_LIMIT;
>  		else if ((xe->info.platform == XE_DG2) && (channel == CHANNEL_PKG))
> -			reg = PCU_CR_PACKAGE_RAPL_LIMIT;
> +			return PCU_CR_PACKAGE_RAPL_LIMIT;
>  		break;
>  	case REG_PKG_POWER_SKU:
>  		if (xe->info.platform == XE_PVC && channel == CHANNEL_PKG)
> -			reg = PVC_GT0_PACKAGE_POWER_SKU;
> +			return PVC_GT0_PACKAGE_POWER_SKU;
>  		else if ((xe->info.platform == XE_DG2) && (channel == CHANNEL_PKG))
> -			reg = PCU_CR_PACKAGE_POWER_SKU;
> +			return PCU_CR_PACKAGE_POWER_SKU;
>  		break;
>  	case REG_PKG_POWER_SKU_UNIT:
>  		if (xe->info.platform == XE_PVC)
> -			reg = PVC_GT0_PACKAGE_POWER_SKU_UNIT;
> +			return PVC_GT0_PACKAGE_POWER_SKU_UNIT;
>  		else if (xe->info.platform == XE_DG2)
> -			reg = PCU_CR_PACKAGE_POWER_SKU_UNIT;
> +			return PCU_CR_PACKAGE_POWER_SKU_UNIT;
>  		break;
>  	case REG_GT_PERF_STATUS:
>  		if (xe->info.platform == XE_DG2 && channel == CHANNEL_PKG)
> -			reg = GT_PERF_STATUS;
> +			return GT_PERF_STATUS;
>  		break;
>  	case REG_PKG_ENERGY_STATUS:
>  		if (xe->info.platform == XE_PVC && channel == CHANNEL_PKG)
> -			reg = PVC_GT0_PLATFORM_ENERGY_STATUS;
> +			return PVC_GT0_PLATFORM_ENERGY_STATUS;
>  		else if ((xe->info.platform == XE_DG2) && (channel == CHANNEL_PKG))
> -			reg = PCU_CR_PACKAGE_ENERGY_STATUS;
> +			return PCU_CR_PACKAGE_ENERGY_STATUS;
>  		break;
>  	default:
>  		drm_warn(&xe->drm, "Unknown xe hwmon reg id: %d\n", hwmon_reg);
>  		break;
>  	}
>  
> -	return reg.raw;
> +	return XE_REG(0);

You might want to look at i915_reg_defs.h, which abstracts away the
magic 0 as the invalid address, and provides type safety for the reg
validity checks via _Generic:

#define INVALID_MMIO_REG _MMIO(0)

#define i915_mmio_reg_offset(r) \
	_Generic((r), i915_reg_t: (r).reg, i915_mcr_reg_t: (r).reg)
#define i915_mmio_reg_equal(a, b) (i915_mmio_reg_offset(a) == i915_mmio_reg_offset(b))
#define i915_mmio_reg_valid(r) (!i915_mmio_reg_equal(r, INVALID_MMIO_REG))

BR,
Jani.


>  }
>  
>  static void xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
> @@ -127,9 +127,9 @@ static void xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon
>  {
>  	struct xe_reg reg;
>  
> -	reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg, channel);
> +	reg = xe_hwmon_get_reg(hwmon, hwmon_reg, channel);
>  
> -	if (!reg.raw)
> +	if (!XE_REG_IS_VALID(reg))
>  		return;
>  
>  	switch (operation) {
> @@ -400,7 +400,7 @@ static umode_t xe_hwmon_attributes_visible(struct kobject *kobj,
>  
>  	xe_pm_runtime_get(gt_to_xe(hwmon->gt));
>  
> -	ret = xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, index) ? attr->mode : 0;
> +	ret = XE_REG_IS_VALID(xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, index)) ? attr->mode : 0;
>  
>  	xe_pm_runtime_put(gt_to_xe(hwmon->gt));
>  
> @@ -496,16 +496,19 @@ xe_hwmon_power_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  
>  	switch (attr) {
>  	case hwmon_power_max:
> -		return xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT, channel) ? 0664 : 0;
> +		return XE_REG_IS_VALID(xe_hwmon_get_reg(hwmon, REG_PKG_RAPL_LIMIT,
> +				       channel)) ? 0664 : 0;
>  	case hwmon_power_rated_max:
> -		return xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU, channel) ? 0444 : 0;
> +		return XE_REG_IS_VALID(xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU,
> +				       channel)) ? 0444 : 0;
>  	case hwmon_power_crit:
>  		if (channel == CHANNEL_PKG)
>  			return (xe_hwmon_pcode_read_i1(hwmon->gt, &uval) ||
>  				!(uval & POWER_SETUP_I1_WATTS)) ? 0 : 0644;
>  		break;
>  	case hwmon_power_label:
> -		return xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU_UNIT, channel) ? 0444 : 0;
> +		return XE_REG_IS_VALID(xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU_UNIT,
> +				       channel)) ? 0444 : 0;
>  	default:
>  		return 0;
>  	}
> @@ -588,7 +591,8 @@ xe_hwmon_in_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  	switch (attr) {
>  	case hwmon_in_input:
>  	case hwmon_in_label:
> -		return xe_hwmon_get_reg(hwmon, REG_GT_PERF_STATUS, channel) ? 0444 : 0;
> +		return XE_REG_IS_VALID(xe_hwmon_get_reg(hwmon, REG_GT_PERF_STATUS,
> +				       channel)) ? 0444 : 0;
>  	default:
>  		return 0;
>  	}
> @@ -612,7 +616,8 @@ xe_hwmon_energy_is_visible(struct xe_hwmon *hwmon, u32 attr, int channel)
>  	switch (attr) {
>  	case hwmon_energy_input:
>  	case hwmon_energy_label:
> -		return xe_hwmon_get_reg(hwmon, REG_PKG_ENERGY_STATUS, channel) ? 0444 : 0;
> +		return XE_REG_IS_VALID(xe_hwmon_get_reg(hwmon, REG_PKG_ENERGY_STATUS,
> +				       channel)) ? 0444 : 0;
>  	default:
>  		return 0;
>  	}
> @@ -763,7 +768,7 @@ xe_hwmon_get_preregistration_info(struct xe_device *xe)
>  	 * The contents of register PKG_POWER_SKU_UNIT do not change,
>  	 * so read it once and store the shift values.
>  	 */
> -	if (xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU_UNIT, 0)) {
> +	if (XE_REG_IS_VALID(xe_hwmon_get_reg(hwmon, REG_PKG_POWER_SKU_UNIT, 0))) {
>  		xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU_UNIT,
>  				     REG_READ32, &val_sku_unit, 0, 0, 0);
>  		hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT, val_sku_unit);

-- 
Jani Nikula, Intel