[Intel-xe] [PATCH v6 1/5] drm/xe/hwmon: Expose power attributes

[Dixit, Ashutosh]

On Wed, 27 Sep 2023 03:28:51 -0700, Nilawar, Badal wrote:
>
> Hi Ashutosh,
>
> On 27-09-2023 10:15, Dixit, Ashutosh wrote:
> > On Mon, 25 Sep 2023 01:18:38 -0700, Badal Nilawar wrote:
> >>
> >
> > Hi Badal,
> >
> >> +static int xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg,
> >
> > Maybe xe_hwmon_read_write_reg? process_reg sounds bad. Basically we don't
> > process a register, we read or write it.
> I don't think it sound that bad. When we say process register apart from
> read/write/rmw what else we will be doing. I think lets not rename this
> function.

OK, maybe leave as is (though another option is xe_hwmon_operate_reg since
we already have xe_hwmon_reg_op, or xe_hwmon_rw_reg).

> >
> >> +				enum xe_hwmon_reg_operation operation, u32 *value,
> >> +				u32 clr, u32 set)
> >> +{
> >> +	struct xe_reg reg;
> >> +
> >> +	reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
> >> +
> >> +	if (!reg.raw)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	switch (operation) {
> >> +	case REG_READ:
> >> +		*value = xe_mmio_read32(hwmon->gt, reg);
> >> +		return 0;
> >> +	case REG_WRITE:
> >> +		xe_mmio_write32(hwmon->gt, reg, *value);
> >> +		return 0;
> >> +	case REG_RMW:
> >> +		*value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
> >> +		return 0;
> >> +	default:
> >> +		drm_warn(&gt_to_xe(hwmon->gt)->drm, "Invalid xe hwmon reg operation: %d\n",
> >> +			 operation);
> >> +		return -EOPNOTSUPP;
> >> +	}
> >> +}
> >> +
> >> +int xe_hwmon_process_reg_read64(struct xe_hwmon *hwmon, enum xe_hwmon_reg hwmon_reg, u64 *value)
> >> +{
> >> +	struct xe_reg reg;
> >> +
> >> +	reg.raw = xe_hwmon_get_reg(hwmon, hwmon_reg);
> >> +
> >> +	if (!reg.raw)
> >> +		return -EOPNOTSUPP;
> >> +
> >> +	*value = xe_mmio_read64_2x32(hwmon->gt, reg);
> >> +
> >> +	return 0;
> >
> > We can't make read64 part of enum xe_hwmon_reg_operation?
> read64 takes argument "u64 *value" so kept it separate.

OK, makes sense.

> >
> >
> >> +}
> >> +
> >> +#define PL1_DISABLE 0
> >> +
> >> +/*
> >> + * HW allows arbitrary PL1 limits to be set but silently clamps these values to
> >> + * "typical but not guaranteed" min/max values in REG_PKG_POWER_SKU. Follow the
> >> + * same pattern for sysfs, allow arbitrary PL1 limits to be set but display
> >> + * clamped values when read.
> >> + */
> >> +static int xe_hwmon_power_max_read(struct xe_hwmon *hwmon, long *value)
> >> +{
> >> +	u32 reg_val;
> >> +	u64 reg_val64, min, max;
> >> +
> >> +	xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val, 0, 0);
> >> +	/* Check if PL1 limit is disabled */
> >> +	if (!(reg_val & PKG_PWR_LIM_1_EN)) {
> >> +		*value = PL1_DISABLE;
> >> +		return 0;
> >> +	}
> >> +
> >> +	reg_val = REG_FIELD_GET(PKG_PWR_LIM_1, reg_val);
> >> +	*value = mul_u64_u32_shr(reg_val, SF_POWER, hwmon->scl_shift_power);
> >> +
> >> +	xe_hwmon_process_reg_read64(hwmon, REG_PKG_POWER_SKU, &reg_val64);
> >> +	min = REG_FIELD_GET(PKG_MIN_PWR, reg_val64);
> >> +	min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
> >> +	max = REG_FIELD_GET(PKG_MAX_PWR, reg_val64);
> >> +	max = mul_u64_u32_shr(max, SF_POWER, hwmon->scl_shift_power);
> >> +
> >> +	if (min && max)
> >> +		*value = clamp_t(u64, *value, min, max);
> >
> > Not exactly correct. Should be:
> >
> >	if (min)
> >		clamp at min
> >	if (max)
> >		clamp at max
> >
> > I was thinking of changing it for i915 but was lazy.
> Sure, thanks for pointing this.
> >
> >
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> +static int xe_hwmon_power_max_write(struct xe_hwmon *hwmon, long value)
> >> +{
> >> +	u32 reg_val;
> >> +
> >> +	/* Disable PL1 limit and verify, as limit cannot be disabled on all platforms */
> >> +	if (value == PL1_DISABLE) {
> >> +		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, &reg_val,
> >> +				     PKG_PWR_LIM_1_EN, 0);
> >> +		xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ, &reg_val,
> >
> > If we are not checking for return codes from these functions, why are they
> > not void?
> Top level functions expect return. For function xe_hwmon_power_max_write
> returning error if PL1 disable not possible. The functions
> xe_hwmon_power_max_read/xe_hwmon_power_rated_max_read can be made void,
> then it will look like. What difference its going to make? I feel existing
> approach is much readable.

As I have pointed out in the other mail it is not. It raises more questions
about why the return code is not being checked, whether the function can
return an error. So it is better to be crisp as to what can actually happen.

>
> case hwmon_power_max:
>          xe_hwmon_power_max_read(hwmon, val);
>	 return 0;
> case hwmon_power_rated_max:
>          xe_hwmon_power_rated_max_read(hwmon, val);
>	 return 0;

This is fine.

> >
> > Also, how about separate read/write/rmw functions as Andi was suggesting?
> > They would be clearer I think.
> Would not prefer to add further abstraction, lets keep as is. Going further
> while adding new platforms will think about adding it.

OK, no need to add wrappers.

Thanks.
--
Ashutosh