Message-ID: <20241215125459.40e50028@jic23-huawei>
Date: Sun, 15 Dec 2024 12:54:59 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Matti Vaittinen <mazziesaccount@...il.com>
Cc: Matti Vaittinen <matti.vaittinen@...rohmeurope.com>, Lars-Peter Clausen
 <lars@...afoo.de>, Subhajit Ghosh <subhajit.ghosh@...aklogic.com>, Mudit
 Sharma <muditsharma.info@...il.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] iio: gts: Simplify available scale table build

On Mon, 9 Dec 2024 09:58:41 +0200
Matti Vaittinen <mazziesaccount@...il.com> 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@...il.com>
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;
>  

Powered by blists - more mailing lists