Messages in this thread | | | Subject | Re: [PATCH] hwmon (occ): Switch power average to between poll responses | From | Eddie James <> | Date | Thu, 9 May 2019 10:38:38 -0500 |
| |
On 5/8/19 4:03 PM, Guenter Roeck wrote: > On Tue, May 07, 2019 at 02:35:51PM -0500, Eddie James wrote: >> The average power reported in the hwmon OCC driver is not useful >> because the time it represents is too short. Instead, store the >> previous power accumulator reported by the OCC and average it with the >> latest accumulator to obtain an average between poll responses. This >> does operate under the assumption that the user polls regularly. >> > That looks really bad. Effectively it means that the number reported > as average power is more or less useless/random. On top of that, the code > gets so complicated that it is all but impossible to understand. > > Does it really make sense to report an average that has effectively > no useful meaning (and is, for example, influenced just by reading it) ?
Yea... that's a good point. Basically our userspace environment has no good way to do this either, so I tried to shoe-horn this into the driver, but this approach is indeed bad. I'll abandon this patch.
Thanks!
Eddie
> > Guenter > >> Signed-off-by: Eddie James <eajames@linux.ibm.com> >> --- >> drivers/hwmon/occ/common.c | 133 ++++++++++++++++++++++++++++++++------------- >> drivers/hwmon/occ/common.h | 7 +++ >> 2 files changed, 103 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/hwmon/occ/common.c b/drivers/hwmon/occ/common.c >> index e6d3fb5..6ffcee7 100644 >> --- a/drivers/hwmon/occ/common.c >> +++ b/drivers/hwmon/occ/common.c >> @@ -118,6 +118,53 @@ struct extended_sensor { >> u8 data[6]; >> } __packed; >> >> +static void occ_update_prev_power_avgs(struct occ *occ) >> +{ >> + u8 i; >> + struct power_sensor_1 *ps1; >> + struct power_sensor_2 *ps2; >> + struct power_sensor_a0 *psa0; >> + struct occ_sensor *power = &occ->sensors.power; >> + struct occ_power_avg *prevs = occ->prev_power_avgs; >> + >> + switch (power->version) { >> + case 1: >> + for (i = 0; i < power->num_sensors; ++i) { >> + ps1 = ((struct power_sensor_1 *)power->data) + i; >> + >> + prevs[i].accumulator = >> + get_unaligned_be32(&ps1->accumulator); >> + prevs[i].update_tag = >> + get_unaligned_be32(&ps1->update_tag); >> + } >> + break; >> + case 2: >> + for (i = 0; i < power->num_sensors; ++i) { >> + ps2 = ((struct power_sensor_2 *)power->data) + i; >> + >> + prevs[i].accumulator = >> + get_unaligned_be64(&ps2->accumulator); >> + prevs[i].update_tag = >> + get_unaligned_be32(&ps2->update_tag); >> + } >> + break; >> + case 0xA0: >> + for (i = 0; i < power->num_sensors; ++i) { >> + psa0 = ((struct power_sensor_a0 *)power->data) + i; >> + >> + prevs[i].accumulator = psa0->system.accumulator; >> + prevs[i].update_tag = psa0->system.update_tag; >> + prevs[i + 1].accumulator = psa0->proc.accumulator; >> + prevs[i + 1].update_tag = psa0->proc.update_tag; >> + prevs[i + 2].accumulator = psa0->vdd.accumulator; >> + prevs[i + 2].update_tag = psa0->vdd.update_tag; >> + prevs[i + 3].accumulator = psa0->vdn.accumulator; >> + prevs[i + 3].update_tag = psa0->vdn.update_tag; >> + } >> + break; >> + } >> +} >> + >> static int occ_poll(struct occ *occ) >> { >> int rc; >> @@ -135,6 +182,8 @@ static int occ_poll(struct occ *occ) >> cmd[6] = checksum & 0xFF; /* checksum lsb */ >> cmd[7] = 0; >> >> + occ_update_prev_power_avgs(occ); >> + >> /* mutex should already be locked if necessary */ >> rc = occ->send_cmd(occ, cmd); >> if (rc) { >> @@ -208,6 +257,7 @@ int occ_update_response(struct occ *occ) >> /* limit the maximum rate of polling the OCC */ >> if (time_after(jiffies, occ->last_update + OCC_UPDATE_FREQUENCY)) { >> rc = occ_poll(occ); >> + occ->prev_update = occ->last_update; >> occ->last_update = jiffies; >> } else { >> rc = occ->last_error; >> @@ -364,6 +414,14 @@ static ssize_t occ_show_freq_2(struct device *dev, >> return snprintf(buf, PAGE_SIZE - 1, "%u\n", val); >> } >> >> +static u64 occ_power_avg(struct occ *occ, u8 idx, u64 accum, u32 samples) >> +{ >> + struct occ_power_avg *avg = &occ->prev_power_avgs[idx]; >> + >> + return div_u64((accum - avg->accumulator) * 1000000ULL, >> + samples - avg->update_tag); >> +} >> + >> static ssize_t occ_show_power_1(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> @@ -385,13 +443,12 @@ static ssize_t occ_show_power_1(struct device *dev, >> val = get_unaligned_be16(&power->sensor_id); >> break; >> case 1: >> - val = get_unaligned_be32(&power->accumulator) / >> - get_unaligned_be32(&power->update_tag); >> - val *= 1000000ULL; >> + val = occ_power_avg(occ, sattr->index, >> + get_unaligned_be32(&power->accumulator), >> + get_unaligned_be32(&power->update_tag)); >> break; >> case 2: >> - val = (u64)get_unaligned_be32(&power->update_tag) * >> - occ->powr_sample_time_us; >> + val = jiffies_to_usecs(occ->last_update - occ->prev_update); >> break; >> case 3: >> val = get_unaligned_be16(&power->value) * 1000000ULL; >> @@ -403,12 +460,6 @@ static ssize_t occ_show_power_1(struct device *dev, >> return snprintf(buf, PAGE_SIZE - 1, "%llu\n", val); >> } >> >> -static u64 occ_get_powr_avg(u64 *accum, u32 *samples) >> -{ >> - return div64_u64(get_unaligned_be64(accum) * 1000000ULL, >> - get_unaligned_be32(samples)); >> -} >> - >> static ssize_t occ_show_power_2(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> @@ -431,12 +482,12 @@ static ssize_t occ_show_power_2(struct device *dev, >> get_unaligned_be32(&power->sensor_id), >> power->function_id, power->apss_channel); >> case 1: >> - val = occ_get_powr_avg(&power->accumulator, >> - &power->update_tag); >> + val = occ_power_avg(occ, sattr->index, >> + get_unaligned_be64(&power->accumulator), >> + get_unaligned_be32(&power->update_tag)); >> break; >> case 2: >> - val = (u64)get_unaligned_be32(&power->update_tag) * >> - occ->powr_sample_time_us; >> + val = jiffies_to_usecs(occ->last_update - occ->prev_update); >> break; >> case 3: >> val = get_unaligned_be16(&power->value) * 1000000ULL; >> @@ -452,6 +503,8 @@ static ssize_t occ_show_power_a0(struct device *dev, >> struct device_attribute *attr, char *buf) >> { >> int rc; >> + u32 samples; >> + u64 accum; >> u64 val = 0; >> struct power_sensor_a0 *power; >> struct occ *occ = dev_get_drvdata(dev); >> @@ -469,12 +522,15 @@ static ssize_t occ_show_power_a0(struct device *dev, >> return snprintf(buf, PAGE_SIZE - 1, "%u_system\n", >> get_unaligned_be32(&power->sensor_id)); >> case 1: >> - val = occ_get_powr_avg(&power->system.accumulator, >> - &power->system.update_tag); >> + accum = get_unaligned_be64(&power->system.accumulator); >> + samples = get_unaligned_be32(&power->system.update_tag); >> + val = occ_power_avg(occ, sattr->index, accum, samples); >> break; >> case 2: >> - val = (u64)get_unaligned_be32(&power->system.update_tag) * >> - occ->powr_sample_time_us; >> + case 6: >> + case 10: >> + case 14: >> + val = jiffies_to_usecs(occ->last_update - occ->prev_update); >> break; >> case 3: >> val = get_unaligned_be16(&power->system.value) * 1000000ULL; >> @@ -483,12 +539,9 @@ static ssize_t occ_show_power_a0(struct device *dev, >> return snprintf(buf, PAGE_SIZE - 1, "%u_proc\n", >> get_unaligned_be32(&power->sensor_id)); >> case 5: >> - val = occ_get_powr_avg(&power->proc.accumulator, >> - &power->proc.update_tag); >> - break; >> - case 6: >> - val = (u64)get_unaligned_be32(&power->proc.update_tag) * >> - occ->powr_sample_time_us; >> + accum = get_unaligned_be64(&power->proc.accumulator); >> + samples = get_unaligned_be32(&power->proc.update_tag); >> + val = occ_power_avg(occ, sattr->index + 1, accum, samples); >> break; >> case 7: >> val = get_unaligned_be16(&power->proc.value) * 1000000ULL; >> @@ -497,12 +550,9 @@ static ssize_t occ_show_power_a0(struct device *dev, >> return snprintf(buf, PAGE_SIZE - 1, "%u_vdd\n", >> get_unaligned_be32(&power->sensor_id)); >> case 9: >> - val = occ_get_powr_avg(&power->vdd.accumulator, >> - &power->vdd.update_tag); >> - break; >> - case 10: >> - val = (u64)get_unaligned_be32(&power->vdd.update_tag) * >> - occ->powr_sample_time_us; >> + accum = get_unaligned_be64(&power->vdd.accumulator); >> + samples = get_unaligned_be32(&power->vdd.update_tag); >> + val = occ_power_avg(occ, sattr->index + 2, accum, samples); >> break; >> case 11: >> val = get_unaligned_be16(&power->vdd.value) * 1000000ULL; >> @@ -511,12 +561,9 @@ static ssize_t occ_show_power_a0(struct device *dev, >> return snprintf(buf, PAGE_SIZE - 1, "%u_vdn\n", >> get_unaligned_be32(&power->sensor_id)); >> case 13: >> - val = occ_get_powr_avg(&power->vdn.accumulator, >> - &power->vdn.update_tag); >> - break; >> - case 14: >> - val = (u64)get_unaligned_be32(&power->vdn.update_tag) * >> - occ->powr_sample_time_us; >> + accum = get_unaligned_be64(&power->vdn.accumulator); >> + samples = get_unaligned_be32(&power->vdn.update_tag); >> + val = occ_power_avg(occ, sattr->index + 3, accum, samples); >> break; >> case 15: >> val = get_unaligned_be16(&power->vdn.value) * 1000000ULL; >> @@ -719,6 +766,7 @@ static ssize_t occ_show_extended(struct device *dev, >> static int occ_setup_sensor_attrs(struct occ *occ) >> { >> unsigned int i, s, num_attrs = 0; >> + unsigned int power_avgs_size = 0; >> struct device *dev = occ->bus_dev; >> struct occ_sensors *sensors = &occ->sensors; >> struct occ_attribute *attr; >> @@ -761,9 +809,13 @@ static int occ_setup_sensor_attrs(struct occ *occ) >> /* fall through */ >> case 1: >> num_attrs += (sensors->power.num_sensors * 4); >> + power_avgs_size = sizeof(struct occ_power_avg) * >> + sensors->power.num_sensors; >> break; >> case 0xA0: >> num_attrs += (sensors->power.num_sensors * 16); >> + power_avgs_size = sizeof(struct occ_power_avg) * >> + sensors->power.num_sensors * 4; >> show_power = occ_show_power_a0; >> break; >> default: >> @@ -792,6 +844,13 @@ static int occ_setup_sensor_attrs(struct occ *occ) >> sensors->extended.num_sensors = 0; >> } >> >> + if (power_avgs_size) { >> + occ->prev_power_avgs = devm_kzalloc(dev, power_avgs_size, >> + GFP_KERNEL); >> + if (!occ->prev_power_avgs) >> + return -ENOMEM; >> + } >> + >> occ->attrs = devm_kzalloc(dev, sizeof(*occ->attrs) * num_attrs, >> GFP_KERNEL); >> if (!occ->attrs) >> diff --git a/drivers/hwmon/occ/common.h b/drivers/hwmon/occ/common.h >> index c676e48..08970f8 100644 >> --- a/drivers/hwmon/occ/common.h >> +++ b/drivers/hwmon/occ/common.h >> @@ -87,17 +87,24 @@ struct occ_attribute { >> struct sensor_device_attribute_2 sensor; >> }; >> >> +struct occ_power_avg { >> + u64 accumulator; >> + u32 update_tag; >> +}; >> + >> struct occ { >> struct device *bus_dev; >> >> struct occ_response resp; >> struct occ_sensors sensors; >> + struct occ_power_avg *prev_power_avgs; >> >> int powr_sample_time_us; /* average power sample time */ >> u8 poll_cmd_data; /* to perform OCC poll command */ >> int (*send_cmd)(struct occ *occ, u8 *cmd); >> >> unsigned long last_update; >> + unsigned long prev_update; >> struct mutex lock; /* lock OCC access */ >> >> struct device *hwmon; >> -- >> 1.8.3.1 >>
| |