Fix performance counter regression #5728
Conversation
|
Can one of the admins verify this patch?
|
|
test this please |
|
@serhiy-katsyuba-intel pls check your email - invite sent. |
serhiy-katsyuba-intel
force-pushed
the
fix_perf_count
branch
from
fa70537 to
7035652
Compare
lgirdwood
left a comment
lgirdwood
left a comment
There was a problem hiding this comment.
@serhiy-katsyuba-intel can you rebase - we have a conflict. Thanks.
serhiy-katsyuba-intel
force-pushed
the
fix_perf_count
branch
from
7035652 to
aec497c
Compare
|
@lgirdwood, rebase done. |
lgirdwood
requested review from
dbaluta,
lbetlej,
libinyang,
mmaka1,
mrajwa and
plbossart
as code owners
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
I think the fix is good, but don't want to again add more ifdefs for Zephyr. And especially we don't want the trace output to look different. See comments inline.
src/include/sof/audio/component.h
Outdated
There was a problem hiding this comment.
This is not good. We need to have same output on both OS'es.
There is a practical problem in that cavs_timer.c does not expose CCOUNT, so some extra work is needed to expose this in a portable manner. A temporary workaround would be to wrap a call to "asm volatile ("rsr.CCOUNT %0" : "=r"(val));" in Zephyr builds, if platform is XTENSA. On XTOS this would call timer_get_system(cpu_timer_get().
There was a problem hiding this comment.
@serhiy-katsyuba-intel Checked with Zephyr folks and there is no standard interface to get CPU counter, so I suggest we add a SOF side interface to get the CPU cycle count and use that on both XTOS and Zephyr.
There was a problem hiding this comment.
@kv2019i , now both SOF timers, "system" and "platform", have been implemented to use Zephyr's API and both return wall clock (when compiled with Zephyr). What should we do: revert "system" timer API to return CCOUNT? So "platform" timer will be implemented using Zephyr API (wall clock) and "system" timer will be implemented in SOF using CCOUNT on XTENSA. That seems eliminate all ifdefs I've introduced in this PR.
Is my above understanding correct or you meant to add a third timer to SOF that will provide CCOUNT?
There was a problem hiding this comment.
I think we should add a separate interface to get CCOUNT. Zephyr provides a single clock interface and looking at SOF app code in src/audio, this seems sufficient for SOF application usage. I.e. SOF application code can use the native Zephyr interfaces to get the job done (and for XTOS builds, we can map these on top of SOF clk.h like was done already).
So that leaves basicly the perf counters as a sole user that needs cycle count. I'd say it's easier we have a separate small interface for this.
There was a problem hiding this comment.
@serhiy-katsyuba-intel @kv2019i we should use native Zephyr APIs where we can to read the ticks/timers, but IIUC we are missing a Zephyr API to read CCOUNT today ? @andyross ?
There was a problem hiding this comment.
@andyross we cant uses any xtensa specific APIs, it should be possible to have an RTOS API like k_cpu_get_cycles()
There was a problem hiding this comment.
It clearly possible, it just requires that either we (1) document and accept the per-core behavior of the resulting API or (2) write code to provide a calibrated cross-core cycle output. What we can't have (or rather, can't have portably) is an API that just returns CCOUNT, because that's not generically usable as a measure of "time".
There was a problem hiding this comment.
@andyross I'm good with either 1) or 2). As long as we have a portable Zephyr API where we can read the CPU cycles count (and it's understood these units are dynamic and changeable at runtime and not be be used for time keeping but can be used for counting).
There was a problem hiding this comment.
@lgirdwood @andyross It would be good to have an API to access a local cpu counter with no promise on the frequency stability or being globally synced, to be used by users only with understanding of the underlying hw implementation. Note it is not just local (per-core) but also may vary in time depending on the SoC specific integration of the cpu clock sources.
There was a problem hiding this comment.
@andyross did not get a chance to chat on this today, but are you good for a counter API as described above ?
|
@serhiy-katsyuba-intel move to SOF v2.3 since we are pending a Zephyr counter API |
|
Check out zephyrproject-rtos/zephyr#46310 for a first cut of a proposed Zephyr front end. This uses a pre-existing arch_timing API (which... to be perfectly honest I don't love) rather than trying to invent something new. Longer term work will need to integrate it with the platform clock API so the reported rate doesn't become a lie, but it gives direct access to the cycle count with minimal overhead (it returns it as a 64 bit quantity, which is suboptimal for CCOUNT). |
Thanks @andyross, fwiw I would have preferred a counting API rather than a timing API but I understand if this is not possible in the short term. |
serhiy-katsyuba-intel
force-pushed
the
fix_perf_count
branch
4 times, most recently
from
aee56df to
0ee1278
Compare
A regression bug was introduced by thesofprojectgh-5393 pull request. As a source of high precision CPU timestamp (aka "system" timer) this fix uses arch_timing_counter_get() for builds with Zephyr and timer_get_system(cpu_timer_get()) for SOF only builds. Note: arch_timing_counter_get() might not be implemented for your Zephyr platform. In this case k_cycle_get_64() is used instead. This will result in both "platform" and "cpu" timestamps to be equal. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
|
The CI failure (TIMEOUT on check-suspend-resume-with-playback-5.sh TGL test) is most probably caused by some problem with CI. It is highly unlikely that this patch can cause such CI failure.
|
6 participants
A regression bug was introduced by gh-5393 pull request.
SOF can support two timestamp sources: platform timer and
system timer. Zephyr only supports one timer. This fix
restores usage of two timers (platform and system) on
SOF without Zephyr and uses the only one available timer
on SOF with Zephyr.
Signed-off-by: Serhiy Katsyuba serhiy.katsyuba@intel.com