Re: [RFC PATCH] iio: gts: Simplify available scale table build

[Matti Vaittinen <mazziesaccount@gmail.com>]

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@gmail.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@gmail.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.

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