[RFC]Math: Trignometry: Added cordic cos function by ShriramShastry · Pull Request #4053 · thesofproject/sof

ShriramShastry · 2021-04-19T13:15:52Z

…ange is [-2pi to 2pi] and output range is [-1 to +1] Mean and maximum value for the difference between floating to fixed point output is 3.2136e-09 and 0.0000000596

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

cujomalainey

please address checkpatch comments, also im not sure if Johny has mentioned but we are seeing a performance regression with the new sign function that i think we need to figure out before we land this

ShriramShastry · 2021-04-20T02:27:04Z

please address checkpatch comments, also im not sure if Johny has mentioned but we are seeing a performance regression with the new sign function that i think we need to figure out before we land this

It seems exp_fixed function is consuming most of the computation time in 3 band DRC. CORDIC SINE when compared with old sine performs better #3990 (comment)

paulstelian97 · 2021-04-20T08:51:13Z

Isn't it literally possible to do cos(x) = sin(pi/2 - x)?

ShriramShastry · 2021-04-20T09:07:19Z

Isn't it literally possible to do cos(x) = sin(pi/2 - x)?

Yes, this is done by the during coordinate check pi/2. Variable xn and b_yn share

Isn't it literally possible to do cos(x) = sin(pi/2 - x)?

Yes , this is done during co-ordinate check. th_rad_fxp = sign * b_yn; for sine and th_rad_fxp = sign * xn for cosine.

paulstelian97 · 2021-04-20T09:09:11Z

Isn't it literally possible to do cos(x) = sin(pi/2 - x)?
Yes, this is done by the during coordinate check pi/2. Variable xn and b_yn share
Isn't it literally possible to do cos(x) = sin(pi/2 - x)?
Yes , this is done during co-ordinate check. th_rad_fxp = sign * b_yn; for sine and th_rad_fxp = sign * xn for cosine.

Guess that's a lot of common code then, we can factor out the common part into a single function to avoid code duplication.

ShriramShastry · 2021-04-20T09:16:17Z

Isn't it literally possible to do cos(x) = sin(pi/2 - x)?
Yes, this is done by the during coordinate check pi/2. Variable xn and b_yn share
Isn't it literally possible to do cos(x) = sin(pi/2 - x)?
Yes , this is done during co-ordinate check. th_rad_fxp = sign * b_yn; for sine and th_rad_fxp = sign * xn for cosine.
Guess that's a lot of common code then, we can factor out the common part into a single function to avoid code duplication.

Yes. That's correct, will make common function with a separator for sine and cosine.

johnylin76

After testing on Eldrid (TGL board) I still found the time cost is more than the legacy one (non-cordic sine). I still think the burden is the for loop even if reduces from 32 to 16 fort 16bit.

singalsu

Looks pretty good now, please address the int32_t conversion in drc sine function!

johnylin76 · 2021-05-06T09:16:07Z

I used perf_tic/toc to simply measure the cost of one call of different sine functions (run on TGL). Here are the logger results:

Legacy LUT sine:
[120042953.072917] ( 22.187500) c0 multiband_drc ......../multiband_drc.c:426 perf plat last 62 peak 63 cpu last 576, peak 596
Cordic-16bit sine:
[429034994.739583] ( 24.427084) c0 multiband_drc ......../multiband_drc.c:428 perf plat last 97 peak 101 cpu last 948, peak 970
Cordic-32bit sine:
[102051575.885417] ( 25.052084) c0 multiband_drc ......../multiband_drc.c:426 perf plat last 117 peak 121 cpu last 1160, peak 1184

It does reveal that the cost of Cordic functions are larger than the legacy LUT one.

johnylin76 · 2021-05-06T10:13:37Z

I used perf_tic/toc to simply measure the cost of one call of different sine functions (run on TGL). Here are the logger results:
Legacy LUT sine:
[120042953.072917] ( 22.187500) c0 multiband_drc ......../multiband_drc.c:426 perf plat last 62 peak 63 cpu last 576, peak 596
Cordic-16bit sine:
[429034994.739583] ( 24.427084) c0 multiband_drc ......../multiband_drc.c:428 perf plat last 97 peak 101 cpu last 948, peak 970
Cordic-32bit sine:
[102051575.885417] ( 25.052084) c0 multiband_drc ......../multiband_drc.c:426 perf plat last 117 peak 121 cpu last 1160, peak 1184
It does reveal that the cost of Cordic functions are larger than the legacy LUT one.

Here are the measured values:
sine func | time (us) | platform ticks | cpu ticks
Legacy LUT | 22.187500 | 62 | 576
Cordic-16bit | 24.427084 | 97 | 948
Cordic-32bit | 25.052084 | 117 | 1160

I consider that the absolute value of time might be inaccurate. It may contain the time spent on tracing or something else.
cpu ticks should be the more valuable reference for the computation cost.

singalsu · 2021-05-07T07:06:01Z

@johnylin76 Thanks for doing measurements! It might be possible to get the cordic cycles to same level as LUT based with intrinsics. There's not really parallelization opportunities but the multiple, shift, saturate operations should get a speed up. Since the drop of iterations count didn't half the cycles the computation load is equally before/after it. Optimization is something e.g. I could do as patch later. What kind of schedule is preferred?

singalsu

I think the flags should be changed, e.g. to enum.

paulstelian97

I assume by now the mathematical kinks of the function itself are solved (and thus we now know it's correct); the code itself looks good.

Cordic sin cos input value range is [-2*pi to 2*pi] and output range is [-1 to +1] This is common function to calculate trignometric sine and cosine using separate lookup table size for speeds and accuracy calculation. For 32bit sine and cosine Error (max = 0.000000011175871), THD+N = -170.152933 For 16bit sine and cosine Error (max = 0.000061), THD+N = -91.518584 Signed-off-by: ShriramShastry <malladi.sastry@intel.com>

singalsu

Looks good, thanks Sriram!

singalsu

Re-approve, previous got cleared.

lgirdwood · 2021-05-19T09:57:47Z

 inline int32_t drc_sin_fixed(int32_t x)
 {
-	const int32_t PI_OVER_TWO = Q_CONVERT_FLOAT(1.57079632679489661923f, 30);
+	const int32_t PI_OVER_TWO = Q_CONVERT_FLOAT(1.57079632679489661923, 30);


These will only be inline if they are used in the same C file. @ShriramShastry can you do an incremental update that takes all these small inline maths function and moves them to the maths headers (and changes them to static inline).
This will save us a function call on all our small maths ops.

Thanks Liam, Seppo. I have a new PR #4218 to include your inputs.

lgirdwood · 2021-05-19T09:59:39Z

+ */
+inline int32_t sin_fixed_32b(int32_t th_rad_fxp)
+{
+	int32_t sign;


e.g. here, as this will generate 2 function calls when used outside of trig.c i.e.
sin_fixed_32b()
cordic_sin_cos()
We want to eliminate call 1.
Same for all the other small maths functions.

ShriramShastry force-pushed the shastry_cordiccos_dev branch 5 times, most recently from 458b5f3 to 81a0484Compare April 19, 2021 14:07

ShriramShastry changed the title ~~Math: Trignometry: Added cordic cos function Cordic cos input value r…~~[RFC]Math: Trignometry: Added cordic cos function Apr 19, 2021

lgirdwood requested review from cujomalainey, johnylin76 and singalsu April 19, 2021 15:38

cujomalainey suggested changes Apr 19, 2021
View reviewed changes

Comment threadtest/cmocka/src/math/trig/sin_fixed.c Outdated
Comment threadtest/cmocka/src/math/trig/cos_fixed.c Outdated

singalsu requested changes Apr 20, 2021
View reviewed changes

Comment threadsrc/math/trig.c Outdated
Comment threadsrc/math/trig.c Outdated
Comment threadsrc/math/trig.c Outdated

ShriramShastry requested a review from slawblauciak April 20, 2021 09:30

ShriramShastry force-pushed the shastry_cordiccos_dev branch 11 times, most recently from 6042ff3 to d5d1363Compare April 21, 2021 02:00

cujomalainey reviewed Apr 21, 2021
View reviewed changes

Comment threadsrc/math/trig.c Outdated

ShriramShastry force-pushed the shastry_cordiccos_dev branch from d5d1363 to 8e5ff14Compare April 21, 2021 07:15

ShriramShastry requested review from lgirdwood and singalsu May 5, 2021 13:36

ShriramShastry marked this pull request as ready for review May 5, 2021 13:36

ShriramShastry requested review from dbaluta, dgreid, lbetlej, mmaka1 and plbossart as code owners May 5, 2021 13:36

johnylin76 requested changes May 6, 2021
View reviewed changes

Comment threadsrc/audio/drc/drc_math_generic.c Outdated

singalsu reviewed May 6, 2021
View reviewed changes

Comment threadsrc/audio/drc/drc_math_generic.c Outdated

ShriramShastry force-pushed the shastry_cordiccos_dev branch 2 times, most recently from b0c33c0 to 320b58fCompare May 6, 2021 10:38

ShriramShastry requested review from johnylin76 and singalsu May 6, 2021 11:35

singalsu requested changes May 7, 2021
View reviewed changes

Comment threadsrc/math/trig.c Outdated

ShriramShastry force-pushed the shastry_cordiccos_dev branch from 320b58f to 49ad0d2Compare May 12, 2021 10:49

paulstelian97 reviewed May 12, 2021
View reviewed changes

Comment threadsrc/audio/Kconfig Outdated

paulstelian97 approved these changes May 12, 2021
View reviewed changes

singalsu requested changes May 14, 2021
View reviewed changes

singalsu reviewed May 14, 2021
View reviewed changes

Comment threadsrc/include/sof/math/trig.h Outdated

singalsu reviewed May 17, 2021
View reviewed changes

Comment threadsrc/include/sof/math/trig.h Outdated

singalsu approved these changes May 17, 2021
View reviewed changes

slawblauciak approved these changes May 17, 2021
View reviewed changes

lgirdwood reviewed May 19, 2021
View reviewed changes

singalsu mentioned this pull request May 19, 2021
[BUG] Reduce footprint by putting large items under kconfig options #3480
Closed
4 tasks

Uh oh!

Conversation

ShriramShastry commented Apr 19, 2021

Uh oh!

cujomalainey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ShriramShastry commented Apr 20, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paulstelian97 commented Apr 20, 2021

Uh oh!

ShriramShastry commented Apr 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulstelian97 commented Apr 20, 2021

Uh oh!

ShriramShastry commented Apr 20, 2021

Uh oh!

Uh oh!

johnylin76 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnylin76 commented May 6, 2021

Uh oh!

johnylin76 commented May 6, 2021

Uh oh!

singalsu commented May 7, 2021

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

paulstelian97 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

singalsu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwoodMay 19, 2021

Choose a reason for hiding this comment

Uh oh!

ShriramShastryMay 20, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwoodMay 19, 2021

Choose a reason for hiding this comment

Uh oh!

ShriramShastryMay 20, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

ShriramShastry commented Apr 20, 2021•
edited
Loading

singalsu left a comment •
edited
Loading

ShriramShastryMay 20, 2021•
edited
Loading