[PATCH v5 3/4] drm/xe/hwmon: Update xe_hwmon_process_reg
Poosa, Karthik
karthik.poosa at intel.com
Thu Apr 4 10:52:22 UTC 2024
On 04-04-2024 16:05, Riana Tauro wrote:
>
> Hi Karthik
>
> On 4/4/2024 9:24 AM, Karthik Poosa wrote:
>> Return u64 from xe_hwmon_process_reg, instead of a void return,
>> input pointer and a bool return. (Lucas)
> what does the second line mean? which bool return was removed?
it was in v4 version of patch.
https://patchwork.freedesktop.org/patch/586457/?series=131765&rev=4
>>
>> With this caller can directly assign return value to the variable
>> without
>> need of explicit initialization and pass by reference.
>>
>> Signed-off-by: Karthik Poosa <karthik.poosa at intel.com>
>> ---
>> drivers/gpu/drm/xe/xe_hwmon.c | 45 +++++++++++++++++------------------
>> 1 file changed, 22 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_hwmon.c
>> b/drivers/gpu/drm/xe/xe_hwmon.c
>> index 2385f05d9504..114aadb54c8e 100644
>> --- a/drivers/gpu/drm/xe/xe_hwmon.c
>> +++ b/drivers/gpu/drm/xe/xe_hwmon.c
>> @@ -121,31 +121,30 @@ static struct xe_reg xe_hwmon_get_reg(struct
>> xe_hwmon *hwmon, enum xe_hwmon_reg
>> return XE_REG(0);
>> }
>> -static void xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum
>> xe_hwmon_reg hwmon_reg,
>> - enum xe_hwmon_reg_operation operation, u64 *value,
>> +static u64 xe_hwmon_process_reg(struct xe_hwmon *hwmon, enum
>> xe_hwmon_reg hwmon_reg,
>> + enum xe_hwmon_reg_operation operation,
>> u32 clr, u32 set, int channel)
>> {
>> struct xe_reg reg;
>> reg = xe_hwmon_get_reg(hwmon, hwmon_reg, channel);
>> -
>> if (!XE_REG_IS_VALID(reg))
>> - return;
>> + return 0;
>> switch (operation) {
>> case REG_READ32:
>> - *value = xe_mmio_read32(hwmon->gt, reg);
>> + return xe_mmio_read32(hwmon->gt, reg);
>> break;
>> case REG_RMW32:
>> - *value = xe_mmio_rmw32(hwmon->gt, reg, clr, set);
>> + return xe_mmio_rmw32(hwmon->gt, reg, clr, set);
>> break;
>> case REG_READ64:
>> - *value = xe_mmio_read64_2x32(hwmon->gt, reg);
>> + return xe_mmio_read64_2x32(hwmon->gt, reg);
>> break;
>> default:
>> drm_warn(>_to_xe(hwmon->gt)->drm, "Invalid xe hwmon reg
>> operation: %d\n",
>> operation);
>> - break;
>> + return 0;
>> }
>> }
>> @@ -163,7 +162,7 @@ static void xe_hwmon_power_max_read(struct
>> xe_hwmon *hwmon, int channel, long *v
>> mutex_lock(&hwmon->hwmon_lock);
>> - xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ32,
>> ®_val, 0, 0, channel);
>> + reg_val = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> REG_READ32, 0, 0, channel);
>> /* Check if PL1 limit is disabled */
>> if (!(reg_val & PKG_PWR_LIM_1_EN)) {
>> *value = PL1_DISABLE;
>> @@ -173,7 +172,7 @@ static void xe_hwmon_power_max_read(struct
>> xe_hwmon *hwmon, int channel, long *v
>> 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(hwmon, REG_PKG_POWER_SKU, REG_READ64,
>> ®_val, 0, 0, channel);
>> + reg_val = xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU,
>> REG_READ64, 0, 0, channel);
>> min = REG_FIELD_GET(PKG_MIN_PWR, reg_val);
>> min = mul_u64_u32_shr(min, SF_POWER, hwmon->scl_shift_power);
>> max = REG_FIELD_GET(PKG_MAX_PWR, reg_val);
>> @@ -194,9 +193,9 @@ static int xe_hwmon_power_max_write(struct
>> xe_hwmon *hwmon, int channel, long va
>> /* 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_RMW32,
>> ®_val,
>> + reg_val = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> REG_RMW32,
>> PKG_PWR_LIM_1_EN, 0, channel);
>> - xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_READ32,
>> ®_val,
>> + reg_val = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> REG_READ32,
>> PKG_PWR_LIM_1_EN, 0, channel);
>> if (reg_val & PKG_PWR_LIM_1_EN) {
>> @@ -209,7 +208,7 @@ static int xe_hwmon_power_max_write(struct
>> xe_hwmon *hwmon, int channel, long va
>> reg_val = DIV_ROUND_CLOSEST_ULL((u64)value <<
>> hwmon->scl_shift_power, SF_POWER);
>> reg_val = PKG_PWR_LIM_1_EN | REG_FIELD_PREP(PKG_PWR_LIM_1,
>> reg_val);
>> - xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW32,
>> ®_val,
>> + reg_val = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> REG_RMW32,
>> PKG_PWR_LIM_1_EN | PKG_PWR_LIM_1, reg_val, channel);
> indentation
Fixed in v6 patch.
>> unlock:
>> mutex_unlock(&hwmon->hwmon_lock);
>> @@ -220,7 +219,7 @@ static void xe_hwmon_power_rated_max_read(struct
>> xe_hwmon *hwmon, int channel, l
>> {
>> u64 reg_val;
>> - xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU, REG_READ32,
>> ®_val, 0, 0, channel);
>> + reg_val = xe_hwmon_process_reg(hwmon, REG_PKG_POWER_SKU,
>> REG_READ32, 0, 0, channel);
>> reg_val = REG_FIELD_GET(PKG_TDP, reg_val);
>> *value = mul_u64_u32_shr(reg_val, SF_POWER,
>> hwmon->scl_shift_power);
>> }
>> @@ -251,8 +250,8 @@ xe_hwmon_energy_get(struct xe_hwmon *hwmon, int
>> channel, long *energy)
>> struct xe_hwmon_energy_info *ei = &hwmon->ei[channel];
>> u64 reg_val;
>> - xe_hwmon_process_reg(hwmon, REG_PKG_ENERGY_STATUS, REG_READ32,
>> - ®_val, 0, 0, channel);
>> + reg_val = xe_hwmon_process_reg(hwmon, REG_PKG_ENERGY_STATUS,
>> REG_READ32,
>> + 0, 0, channel);
> indentation
>> if (reg_val >= ei->reg_val_prev)
>> ei->accum_energy += reg_val - ei->reg_val_prev;
>> @@ -278,8 +277,8 @@ xe_hwmon_power_max_interval_show(struct device
>> *dev, struct device_attribute *at
>> mutex_lock(&hwmon->hwmon_lock);
>> - xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> - REG_READ32, &r, 0, 0, sensor_index);
>> + r = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
>> + REG_READ32, 0, 0, sensor_index);
> same
>> mutex_unlock(&hwmon->hwmon_lock);
>> @@ -367,7 +366,7 @@ xe_hwmon_power_max_interval_store(struct device
>> *dev, struct device_attribute *a
>> mutex_lock(&hwmon->hwmon_lock);
>> - xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW32,
>> (u64 *)&r,
>> + r = xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW32,
>> PKG_PWR_LIM_1_TIME, rxy, sensor_index);
>> mutex_unlock(&hwmon->hwmon_lock);
>> @@ -483,8 +482,8 @@ static void xe_hwmon_get_voltage(struct xe_hwmon
>> *hwmon, int channel, long *valu
>> {
>> u64 reg_val;
>> - xe_hwmon_process_reg(hwmon, REG_GT_PERF_STATUS,
>> - REG_READ32, ®_val, 0, 0, channel);
>> + reg_val = xe_hwmon_process_reg(hwmon, REG_GT_PERF_STATUS,
>> + REG_READ32, 0, 0, channel);
> same
>> /* HW register value in units of 2.5 millivolt */
>> *value = DIV_ROUND_CLOSEST(REG_FIELD_GET(VOLTAGE_MASK, reg_val)
>> * 2500, SF_VOLTAGE);
>> }
>> @@ -769,8 +768,8 @@ xe_hwmon_get_preregistration_info(struct
>> xe_device *xe)
>> * so read it once and store the shift values.
>> */
>> 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);
>> + val_sku_unit = xe_hwmon_process_reg(hwmon,
>> REG_PKG_POWER_SKU_UNIT,
>> + REG_READ32, 0, 0, 0);
> same
>
> Thanks,
> Riana
>> hwmon->scl_shift_power = REG_FIELD_GET(PKG_PWR_UNIT,
>> val_sku_unit);
>> hwmon->scl_shift_energy = REG_FIELD_GET(PKG_ENERGY_UNIT,
>> val_sku_unit);
>> hwmon->scl_shift_time = REG_FIELD_GET(PKG_TIME_UNIT,
>> val_sku_unit);
More information about the Intel-xe
mailing list