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

["Nilawar, Badal" <badal.nilawar@intel.com>]

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.
> 
>> +				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.
> 
> 
>> +}
>> +
>> +#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.

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;
> 
> 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.

Regards,
Badal
> 
> Thanks.
> --
> Ashutosh