Hi Jonathan,
Thanks for the comments!
On 15/12/2024 14:54, Jonathan Cameron wrote:
On Mon, 9 Dec 2024 09:58:41 +0200
Matti Vaittinen <mazziesaccount@xxxxxxxxx> wrote:
Make available scale building more clear. This hurts the performance
quite a bit by looping throgh the scales many times instead of doing
everything in one loop. It however simplifies logic by:
- decoupling the gain and scale allocations & computations
- keeping the temporary 'per_time_gains' table inside the
per_time_scales computation function.
- separating building the 'all scales' table in own function and doing
it based on the already computed per-time scales.
Signed-off-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
Hi Matti,
I'm definitely keen to see easier to follow code and agree that the
cost doesn't matter (Within reason).
I think a few more comments and rethinks of function names would
make it clearer still. If each subfunction called has a clear
statement of what it's inputs and outputs are that would help
a lot as sort functions in particular tend to be tricky to figure out
by eyeballing them.
I'll see if I can come up with something more descriptive while keeping
the names reasonably short.
---
In my (not always) humble (enough) opinion:
- Building the available scales tables was confusing.
- The result of this patch looks much clearer and is simpler to follow.
- Handles memory allocations and freeing in somehow easyish to follow
manner while still:
- Avoids introducing mid-function variables
- Avoids mixing and matching 'scoped' allocs with regular ones
I however send this as an RFC because it hurts the performance quite a
bit. (No measurements done, I doubt exact numbers matter. I'd just say
it more than doubles the time, prbably triples or quadruples). Well, it
is not really on a hot path though, tables are computed once at
start-up, and with a sane amount of gains/times this is likely not a
real problem.
This has been tested only by running the kunit tests for the gts-helpers
in a beaglebone black. Further testing and eyeing is appreciated :)
---
drivers/iio/industrialio-gts-helper.c | 250 +++++++++++++++-----------
1 file changed, 149 insertions(+), 101 deletions(-)
diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
index 291c0fc332c9..01206bc3e48e 100644
--- a/drivers/iio/industrialio-gts-helper.c
+++ b/drivers/iio/industrialio-gts-helper.c
@@ -160,16 +160,108 @@ static void iio_gts_purge_avail_scale_table(struct iio_gts *gts)
gts->num_avail_all_scales = 0;
}
+
+static int do_combined_scaletable(struct iio_gts *gts, size_t scale_bytes)
Probably name this to indicate what it is doing to the combined scaletable.
Hmm. I think I understand what you mean. Still, I kind of think the
function name should reflect what the function does (creates the scale
table where all the scales are listed by combining all unique scales
from the per-time scale tables).
Maybe this could be accompanied by a comment which also explains what
how this is done.
Maybe make it clear that scale_bytes is of the whole scale table (i think!)
scale_table_bytes.
I like this idea :)
A few comments might also be useful.
I agree. Especially if we keep the name reflecting the creation of the
"all scales" table :)
+{
+ int t_idx, i, new_idx;
+ int **scales = gts->per_time_avail_scale_tables;
+ int *all_scales = kcalloc(gts->num_itime, scale_bytes, GFP_KERNEL);
+
+ if (!all_scales)
+ return -ENOMEM;
+
+ t_idx = gts->num_itime - 1;
+ memcpy(all_scales, scales[t_idx], scale_bytes);
I'm not 100% sure what that is copying in, so maybe a comment.
Is it just filling the final integration time with the unadjusted
scale table? If so, maybe say why.
+ new_idx = gts->num_hwgain * 2;
Comment on where you are starting the index. One row into a matrix?
+
+ while (t_idx-- > 0) {
+ for (i = 0; i < gts->num_hwgain ; i++) {
+ int *candidate = &scales[t_idx][i * 2];
+ int chk;
+
+ if (scale_smaller(candidate, &all_scales[new_idx - 2])) {
+ all_scales[new_idx] = candidate[0];
+ all_scales[new_idx + 1] = candidate[1];
+ new_idx += 2;
+
+ continue;
+ }
+ for (chk = 0; chk < new_idx; chk += 2)
+ if (!scale_smaller(candidate, &all_scales[chk]))
+ break;
+
+
+ if (scale_eq(candidate, &all_scales[chk]))
+ continue;
+
+ memmove(&all_scales[chk + 2], &all_scales[chk],
+ (new_idx - chk) * sizeof(int));
+ all_scales[chk] = candidate[0];
+ all_scales[chk + 1] = candidate[1];
+ new_idx += 2;
+ }
+ }
+
+ gts->num_avail_all_scales = new_idx / 2;
+ gts->avail_all_scales_table = all_scales;
+
+ return 0;
+}
- /*
- * We assume all the gains for same integration time were unique.
- * It is likely the first time table had greatest time multiplier as
- * the times are in the order of preference and greater times are
- * usually preferred. Hence we start from the last table which is likely
- * to have the smallest total gains.
- */
ah. This is one of the comments I'd like to see up above.
Right! I'll re-add this comment to correct location :)
- time_idx = gts->num_itime - 1;
- memcpy(all_gains, gains[time_idx], gain_bytes);
- new_idx = gts->num_hwgain;
+static void compute_per_time_gains(struct iio_gts *gts, int **gains)
+{
+ int i, j;
Thanks a lot Jonathan! I feel your feedback helps to make this better :)
Yours,
-- Matti
