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.
Jonathan
>
> ---
> 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.
Maybe make it clear that scale_bytes is of the whole scale table (i think!)
scale_table_bytes.
A few comments might also be useful.
> +{
> + 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.
> - 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;
>
