On Mon, 16 Dec 2024 10:56:37 +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>
Looks good to me, but I want to leave it on list a while before applying.
Ideal if it gets some tested-by or other tags before I pick it up.
As always, this is fiddly code, so the more eyes the better!
Jonathan
> ---
> Revision history:
> v2:
> - Add a few comments
> - Use more descriptive variable name
>
> This is still only tested using the kunit tests. All further testing is
> still highly appreciated!
> ---
> drivers/iio/industrialio-gts-helper.c | 272 ++++++++++++++++----------
> 1 file changed, 174 insertions(+), 98 deletions(-)
>
> diff --git a/drivers/iio/industrialio-gts-helper.c b/drivers/iio/industrialio-gts-helper.c
> index 291c0fc332c9..65697be58a10 100644
> --- a/drivers/iio/industrialio-gts-helper.c
> +++ b/drivers/iio/industrialio-gts-helper.c
> @@ -160,16 +160,123 @@ static void iio_gts_purge_avail_scale_table(struct iio_gts *gts)
> gts->num_avail_all_scales = 0;
> }
>
> +static int scale_eq(int *sc1, int *sc2)
> +{
> + return sc1[0] == sc2[0] && sc1[1] == sc2[1];
> +}
> +
> +static int scale_smaller(int *sc1, int *sc2)
> +{
> + if (sc1[0] != sc2[0])
> + return sc1[0] < sc2[0];
> +
> + /* If integer parts are equal, fixp parts */
> + return sc1[1] < sc2[1];
> +}
> +
> +/*
> + * Do a single table listing all the unique scales that any combination of
> + * supported gains and times can provide.
> + */
> +static int do_combined_scaletable(struct iio_gts *gts,
> + size_t all_scales_tbl_bytes)
> +{
> + int t_idx, i, new_idx;
> + int **scales = gts->per_time_avail_scale_tables;
> + int *all_scales = kcalloc(gts->num_itime, all_scales_tbl_bytes,
> + GFP_KERNEL);
> +
> + if (!all_scales)
> + return -ENOMEM;
> + /*
> + * Create table containing all of the supported scales by looping
> + * through all of the per-time scales and copying the unique scales
> + * into one sorted table.
> + *
> + * 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.
> + */
> + t_idx = gts->num_itime - 1;
> + memcpy(all_scales, scales[t_idx], all_scales_tbl_bytes);
> + new_idx = gts->num_hwgain * 2;
> +
> + 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;
> +}
> +
> +static void iio_gts_free_int_table_array(int **arr, int num_tables)
> +{
> + int i;
> +
> + for (i = 0; i < num_tables; i++)
> + kfree(arr[i]);
> +
> + kfree(arr);
> +}
> +
> +static int iio_gts_alloc_int_table_array(int ***arr, int num_tables, int num_table_items)
> +{
> + int i, **tmp;
> +
> + tmp = kcalloc(num_tables, sizeof(**arr), GFP_KERNEL);
> + if (!tmp)
> + return -ENOMEM;
> +
> + for (i = 0; i < num_tables; i++) {
> + tmp[i] = kcalloc(num_table_items, sizeof(int), GFP_KERNEL);
> + if (!tmp[i])
> + goto err_free;
> + }
> +
> + *arr = tmp;
> +
> + return 0;
> +err_free:
> + iio_gts_free_int_table_array(tmp, i);
> +
> + return -ENOMEM;
> +}
> +
> static int iio_gts_gain_cmp(const void *a, const void *b)
> {
> return *(int *)a - *(int *)b;
> }
>
> -static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
> +static int fill_and_sort_scaletables(struct iio_gts *gts, int **gains, int **scales)
> {
> - int i, j, new_idx, time_idx, ret = 0;
> - int *all_gains;
> - size_t gain_bytes;
> + int i, j, ret;
>
> for (i = 0; i < gts->num_itime; i++) {
> /*
> @@ -189,71 +296,69 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
> }
> }
>
> - gain_bytes = array_size(gts->num_hwgain, sizeof(int));
> - all_gains = kcalloc(gts->num_itime, gain_bytes, GFP_KERNEL);
> - if (!all_gains)
> - return -ENOMEM;
> + return 0;
> +}
> +
> +static void compute_per_time_gains(struct iio_gts *gts, int **gains)
> +{
> + int i, j;
> +
> + for (i = 0; i < gts->num_itime; i++) {
> + for (j = 0; j < gts->num_hwgain; j++)
> + gains[i][j] = gts->hwgain_table[j].gain *
> + gts->itime_table[i].mul;
> + }
> +}
> +
> +static int compute_per_time_tables(struct iio_gts *gts, int **scales)
> +{
> + int **per_time_gains;
> + int ret;
>
> /*
> - * 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.
> + * Create a temporary array of the 'total gains' for each integration
> + * time.
> */
> - time_idx = gts->num_itime - 1;
> - memcpy(all_gains, gains[time_idx], gain_bytes);
> - new_idx = gts->num_hwgain;
> + ret = iio_gts_alloc_int_table_array(&per_time_gains, gts->num_itime,
> + gts->num_hwgain);
> + if (ret)
> + return ret;
>
> - while (time_idx-- > 0) {
> - for (j = 0; j < gts->num_hwgain; j++) {
> - int candidate = gains[time_idx][j];
> - int chk;
> + compute_per_time_gains(gts, per_time_gains);
>
> - if (candidate > all_gains[new_idx - 1]) {
> - all_gains[new_idx] = candidate;
> - new_idx++;
> + /* Convert the gains to scales and populate the scale tables */
> + ret = fill_and_sort_scaletables(gts, per_time_gains, scales);
>
> - continue;
> - }
> - for (chk = 0; chk < new_idx; chk++)
> - if (candidate <= all_gains[chk])
> - break;
> + iio_gts_free_int_table_array(per_time_gains, gts->num_itime);
>
> - if (candidate == all_gains[chk])
> - continue;
> + return ret;
> +}
>
> - memmove(&all_gains[chk + 1], &all_gains[chk],
> - (new_idx - chk) * sizeof(int));
> - all_gains[chk] = candidate;
> - new_idx++;
> - }
> - }
> +/*
> + * Create a table of supported scales for each supported integration time.
> + * This can be used as available_scales by drivers which don't allow scale
> + * setting to change the integration time to display correct set of scales
> + * depending on the used integration time.
> + */
> +static int **create_per_time_scales(struct iio_gts *gts)
> +{
> + int **per_time_scales, ret;
>
> - gts->avail_all_scales_table = kcalloc(new_idx, 2 * sizeof(int),
> - GFP_KERNEL);
> - if (!gts->avail_all_scales_table) {
> - ret = -ENOMEM;
> - goto free_out;
> - }
> - gts->num_avail_all_scales = new_idx;
> + ret = iio_gts_alloc_int_table_array(&per_time_scales, gts->num_itime,
> + gts->num_hwgain * 2);
> + if (ret)
> + return ERR_PTR(ret);
>
> - for (i = 0; i < gts->num_avail_all_scales; i++) {
> - ret = iio_gts_total_gain_to_scale(gts, all_gains[i],
> - >s->avail_all_scales_table[i * 2],
> - >s->avail_all_scales_table[i * 2 + 1]);
> + ret = compute_per_time_tables(gts, per_time_scales);
> + if (ret)
> + goto err_out;
>
> - if (ret) {
> - kfree(gts->avail_all_scales_table);
> - gts->num_avail_all_scales = 0;
> - goto free_out;
> - }
> - }
> + return per_time_scales;
>
> -free_out:
> - kfree(all_gains);
> +err_out:
> + iio_gts_free_int_table_array(per_time_scales, gts->num_itime);
>
> - return ret;
> + return ERR_PTR(ret);
> }
>
> /**
> @@ -275,55 +380,26 @@ static int gain_to_scaletables(struct iio_gts *gts, int **gains, int **scales)
> */
> static int iio_gts_build_avail_scale_table(struct iio_gts *gts)
> {
> - int **per_time_gains, **per_time_scales, i, j, ret = -ENOMEM;
> -
> - per_time_gains = kcalloc(gts->num_itime, sizeof(*per_time_gains), GFP_KERNEL);
> - if (!per_time_gains)
> - return ret;
> -
> - per_time_scales = kcalloc(gts->num_itime, sizeof(*per_time_scales), GFP_KERNEL);
> - if (!per_time_scales)
> - goto free_gains;
> + int ret, all_scales_tbl_bytes;
> + int **per_time_scales;
>
> - for (i = 0; i < gts->num_itime; i++) {
> - per_time_scales[i] = kcalloc(gts->num_hwgain, 2 * sizeof(int),
> - GFP_KERNEL);
> - if (!per_time_scales[i])
> - goto err_free_out;
> -
> - per_time_gains[i] = kcalloc(gts->num_hwgain, sizeof(int),
> - GFP_KERNEL);
> - if (!per_time_gains[i]) {
> - kfree(per_time_scales[i]);
> - goto err_free_out;
> - }
> -
> - for (j = 0; j < gts->num_hwgain; j++)
> - per_time_gains[i][j] = gts->hwgain_table[j].gain *
> - gts->itime_table[i].mul;
> - }
> + if (unlikely(check_mul_overflow(gts->num_hwgain, 2 * sizeof(int),
> + &all_scales_tbl_bytes)))
> + return -EOVERFLOW;
>
> - ret = gain_to_scaletables(gts, per_time_gains, per_time_scales);
> - if (ret)
> - goto err_free_out;
> + per_time_scales = create_per_time_scales(gts);
> + if (IS_ERR(per_time_scales))
> + return PTR_ERR(per_time_scales);
>
> - for (i = 0; i < gts->num_itime; i++)
> - kfree(per_time_gains[i]);
> - kfree(per_time_gains);
> gts->per_time_avail_scale_tables = per_time_scales;
>
> - return 0;
> -
> -err_free_out:
> - for (i--; i >= 0; i--) {
> - kfree(per_time_scales[i]);
> - kfree(per_time_gains[i]);
> + ret = do_combined_scaletable(gts, all_scales_tbl_bytes);
> + if (ret) {
> + iio_gts_free_int_table_array(per_time_scales, gts->num_itime);
> + return ret;
> }
> - kfree(per_time_scales);
> -free_gains:
> - kfree(per_time_gains);
>
> - return ret;
> + return 0;
> }
>
> static void iio_gts_us_to_int_micro(int *time_us, int *int_micro_times,
