math: Add power scalar and base 2 log function

[ShriramShastry]

fix point math power function having negative and positive values
of base and exponent as function argument

fix point math base 2 logarithm function using a short lookup
tables

Signed-off-by: ShriramShastry malladi.sastry@intel.com

[aiChaoSONG]

SOFCI TEST

[ShriramShastry]

SOFCI TEST

[lgirdwood]

lets make this code non fatal.

[johnylin76]

My only question is why do we need a UUID for power? is it for logging with "power_tr"?

[ShriramShastry]

My only question is why do we need a UUID for power? is it for logging with "power_tr"?

yep there does not seem to other way to create a trace than having uuid. Some other library parts also have uuid.

yes it is for power_tr

[johnylin76]

Offline synced with Malladi.
I finally realized that this implementation of power function actually cannot be adopted to DRC usage, because it only supports the integer exponent. (I should have realized it earlier...)

For DRC usage, it requires the support of fractional value of both base and exponent. Both of them are always positive.
In our current implementation of power in DRC, we use Q6.26 format for base (but assuming that only positive values are given), and Q2.30 for exponent.

The actual use cases for "exponent" of DRC is =1/(attack_frames); attack_frames represents the number of frames it expects to spend for 10dB drop in attack mode. It is a configurable parameter range from 1 to (sample_rate), but "typically" the value is an integer by the scale of tens to hundreds (the case of thousands is not often to be seen but it exists).

The power function implementation in this PR is not usable for DRC. However I think it would have the use case for some other processing algorithms.

[marc-hb]

My only question is why do we need a UUID for power? is it for logging with "power_tr"?

Thanks for catching this. Sorry for the late request.

Logging is dictionary-based to save memory, so it uses UUID for component names.

I think we don't want a new trace component for each single .c file (or in this case: a single log statement for now!), so I think math_power_tr is too specific. How about math_tr instead? Declare it in some non-optional file, maybe in math/numbers.c?

As another, maybe extreme example ipc_tr is used in more than 20 files and more than 200 statements.

[marc-hb]

As reported by checkpatch, there are still many // C99-comments. Are we allowing these now? Happy to change the checkpatch configuration if we do. If not then please change them to /* */ (and run ./scripts/checkpatch --strict -g HEAD locally)

There are a few more warnings in https://sof-ci.01.org/sofpr/PR4941/build11280/checkpatch/

[ShriramShastry]

As reported by checkpatch, there are still many // C99-comments. Are we allowing these now? Happy to change the checkpatch configuration if we do. If not then please change them to /* */ (and run ./scripts/checkpatch --strict -g HEAD locally)

There are a few more warnings in https://sof-ci.01.org/sofpr/PR4941/build11280/checkpatch/

I did the test and now I don't see additional errors
checkpatch_4941.txt

[marc-hb]

There's an inconsistency between https://sof-ci.01.org/sofpr/PR4941/build11287/checkpatch/ and https://github.com/thesofproject/sof/runs/4377441309?check_suite_focus=true. One is configured to catch C99 // comments and the other is not. Which is wrong and which is right?

[ShriramShastry]

https://github.com/thesofproject/sof/runs/4377441309?check_suite_focus=true

https://github.com/thesofproject/sof/runs/4377441309?check_suite_focus=true , can be ignored since https://github.com/thesofproject/sof/runs/4377441309?check_suite_focus=true and ./scripts/checkpatch.pl is same in output. . It could be that --strict is not enforced in the later case and executed with --no_strict option.

I ran ./scripts/checkpatch.pl 0001-math-Add-square-root-function-using-lookup-table.patch --strict -g HEAD locally and output is same as https://github.com/thesofproject/sof/runs/4377441309?check_suite_focus=true

This is my understanding !!

[lgirdwood]

There's an inconsistency between https://sof-ci.01.org/sofpr/PR4941/build11287/checkpatch/ and https://github.com/thesofproject/sof/runs/4377441309?check_suite_focus=true. One is configured to catch C99 // comments and the other is not. Which is wrong and which is right?

SPDX // is allowed (and I cant remember the reason), but this also aligns with Linux.

[lgirdwood]

I think we don't want a new trace component for each single .c file (or in this case: a single log statement for now!), so I think math_power_tr is too specific. How about math_tr instead? Declare it in some non-optional file, maybe in math/numbers.c?

As another, maybe extreme example ipc_tr is used in more than 20 files and more than 200 statements.

Good catch (and we should capture this as a feature cleanup for v2.1), but this should not be a blocker given the direction of travel to Zephyr logging APIs

[lgirdwood]

CI shows known system level PM timeout (unrelated to this PR as it's not used in test) and only C99 SPDX comments (which are allowed)

[marc-hb]

SPDX // is allowed (and I cant remember the reason), but this also aligns with Linux.

I meant // in general, not for SPDX specifically. I will submit the change that turns off this warning and we can discuss there.

CI shows ... and only C99 SPDX comments (which are allowed)

Not just in SPDX, see https://github.com/thesofproject/sof/runs/4377441309?check_suite_focus=true or simply look at the source.

Good catch (and we should capture this as a feature cleanup for v2.1), but this should not be a blocker given the direction of travel to Zephyr logging APIs

@ShriramShastry can you please submit a new PR that moves math_power_tr to a higher level math_tr?

Zephyr logging (and any decent logging solution really) have a compoment equivalent: https://docs.zephyrproject.org/latest/reference/logging/index.html#logging-in-a-module-instance

[marc-hb]

I will submit the change that turns off the C99 comments warning and we can discuss there.

#5046