performance: perf cnt bugfix and add more macros for message print #6353
Conversation
btian1
force-pushed
the
perfcnt
branch
2 times, most recently
from
dcd2dfb to
8c4a1d5
Compare
btian1
requested review from
dbaluta,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
xtos/include/sof/lib/perf_cnt.h
Outdated
There was a problem hiding this comment.
is this 64-bit arithmetics? If it's 32-bit then that will overflow: UINT32_MAX + plat_ts will be calculated first and it doesn't fit in 32 bits. Just change that to UINT32_MAX - (pcd)->plat_ts) + plat_ts. And I'm wondering why one would decide to use parentheses in
x = (a + b);
...
There was a problem hiding this comment.
Thanks, changed based on your comments, please review again, if no other issues, please +1 for merge.
xtos/include/sof/lib/perf_cnt.h
Outdated
There was a problem hiding this comment.
ditto. Please remove all external parentheses
btian1
force-pushed
the
perfcnt
branch
3 times, most recently
from
f1cc90b to
2e058da
Compare
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
possibly for a separate PR: these are getting rather big for macros, wondering if they could be converted to inline functions
xtos/include/sof/lib/perf_cnt.h
Outdated
There was a problem hiding this comment.
lines 122 and 123 aren't needed? Are .plat_ts and .cpu_ts updated elsewhere now?
There was a problem hiding this comment.
line 122 and 123 will be updated with perf_cnt_init before each module beginning, then after each module, will calculate the diff and decide the data.
There was a problem hiding this comment.
this already split from original #6322 , how about keep it, too much prs will make upstream more difficult? it is not complicate actually, regarding change to inline, due to still pass function into the macro, how about keep for now?
I even want to rewrite perf_cnt, but due to I am new, and don't know too much about history, so I change based on current, in future, if there is new request, I may rewrite this module.
There was a problem hiding this comment.
sorry, I don't understand. perf_cnt_init() initialises .plat_ts and .cpu_ts but when calculating the current deltas, you have to compare to the previous call, not to the initialisation values? What am I missing?
There was a problem hiding this comment.
in: perf_cnt_init()
get initial clock for plat_ts and cpu_ts.
then followed by function calling.
....
then get current clock with local plat_ts and cpu_ts
then get the delta and peak for this function calling.
record the delta and accumulated it with 1024 times to get average.
it will loop around per 1 seconds.
There was a problem hiding this comment.
Ok, I see now, so it looks like it is redundant in the current implementation? Would be good to have that change - just removing those two lines as a separate commit, but well, at least I know now why they are removed, thanks
There was a problem hiding this comment.
Hmm, sorry, I don't understand how this works. At every function we calculate "(pcd)->plat_delta_last = plat_ts - (pcd)->plat_ts;" , but if "(pcd)->plat_ts" is not update (as removed in this patch), the delta is not correctly calculated (it's delta to initial value, not delta to previous call). Did I miss where a new place where "(pcd)->plat_ts" is updated?
There was a problem hiding this comment.
Thanks @btian1 , so the old code really was wrong.
xtos/include/sof/lib/perf_cnt.h
Outdated
There was a problem hiding this comment.
Ok, I see now, so it looks like it is redundant in the current implementation? Would be good to have that change - just removing those two lines as a separate commit, but well, at least I know now why they are removed, thanks
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Thanks @btian1 . Splitting PRs into simple separate changes is a good way to get changes incrementally done. But in this case, I believe I'm missing something essential as I don't get how the delta calculation works after this PR. Please see my comment inline.
xtos/include/sof/lib/perf_cnt.h
Outdated
There was a problem hiding this comment.
Hmm, sorry, I don't understand how this works. At every function we calculate "(pcd)->plat_delta_last = plat_ts - (pcd)->plat_ts;" , but if "(pcd)->plat_ts" is not update (as removed in this patch), the delta is not correctly calculated (it's delta to initial value, not delta to previous call). Did I miss where a new place where "(pcd)->plat_ts" is updated?
|
@kv2019i , (pcd)->plat_ts will be updated in perf_cnt_init before each module running. due to required to split to multiple patches, changes may cause some mis-understanding. |
xtos/include/sof/lib/perf_cnt.h
Outdated
There was a problem hiding this comment.
Thanks @btian1 , so the old code really was wrong.
btian1
force-pushed
the
perfcnt
branch
2 times, most recently
from
dadb93f to
00b1e69
Compare
|
And same CI error on this PR as well. FYI @wszypelt |
Current hw cycle is based on 38400000, and perf cnt module did not consider the clock wrap case, i.e the later cycle maybe small than the previous one. so add code to handle this situation. Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>
|
Known fail with Intel IPC4 test set, otherwise looks good, merging. |
6 participants
Current cycle is based on 38400000, and perf cnt module did not consider
the clock wrap case, i.e the later cycle maybe small than the previous one.
so add code to handle this situation.
Signed-off-by: Baofeng Tian baofeng.tian@intel.com