zephyr: sof-logger #4503
zephyr: sof-logger #4503
Conversation
src/trace/trace.c
Outdated
| @@ -532,7 +532,7 @@ void trace_init(struct sof *sof) | |||
| dma_trace_init_early(sof); | |||
| } | |||
|
|
|||
| void mtrace_dict_entry(uint32_t dict_entry_address, int n_args, ...) | |||
| void mtrace_dict_entry(bool atomic, uint32_t dict_entry_address, int n_args, ...) | |||
There was a problem hiding this comment.
atomic sounds a bit confusing to me in this case. You want it to say, that it's already called with the lock held / from an atomic context, right? Maybe external_lock? But I see that it's following other similar functions.
There was a problem hiding this comment.
I don't like the name of this parameter and in fact it already caused confusion and one bug in the past that lasted for months! see #4246 . HOWEVER this is the exact same parameter name used everywhere else and I don't want to break consistency in this PR. I will add some doxygen.
There was a problem hiding this comment.
Is this all that's needed to enable SOF DMA trace under Zephyr?
After many bug fixes and attempted alternatives, yes. Except for the untested #4377 scheduler change and fixing build failures but please try it and report any issue if you think this can be useful to you it, should be "good enough" already.
src/trace/trace.c
Outdated
| @@ -532,7 +532,7 @@ void trace_init(struct sof *sof) | |||
| dma_trace_init_early(sof); | |||
| } | |||
|
|
|||
| void mtrace_dict_entry(uint32_t dict_entry_address, int n_args, ...) | |||
| void mtrace_dict_entry(bool atomic, uint32_t dict_entry_address, int n_args, ...) | |||
There was a problem hiding this comment.
I don't like the name of this parameter and in fact it already caused confusion and one bug in the past that lasted for months! see #4246 . HOWEVER this is the exact same parameter name used everywhere else and I don't want to break consistency in this PR. I will add some doxygen.
| DECLARE_SOF_UUID("zll-schedule", zll_sched_uuid, 0x1547fe68, 0xde0c, 0x11eb, | ||
| 0x84, 0x61, 0x31, 0x58, 0xa1, 0x29, 0x48, 0x53); | ||
|
|
||
| DECLARE_TR_CTX(ll_tr, SOF_UUID(zll_sched_uuid), LOG_LEVEL_INFO); |
There was a problem hiding this comment.
Can you, please, rename ll_tr to something else, maybe zll_tr.
In i.MX case, we don't want to use zephyr_ll since this is limited to TIMER_DOMAIN (see here) and we are using DMA_DOMAIN.
We want, for now, to keep using ll_schedule.
Therefore, when compiling this, for i.MX, I get: zephyr_ll.c:22: multiple definition of ll_tr;
There was a problem hiding this comment.
I'd like to help but you're asking me to fix a problem with a configuration that is currently not possible in this branch, probably only on your branch. With this branch there is AFAIK no way to compile ll_schedule.c in Zephyr. After a fair amount of time spent realizing this I tried this naive:
--- a/zephyr/CMakeLists.txt
+++ b/zephyr/CMakeLists.txt
@@ -479,6 +479,7 @@ zephyr_library_sources(
${SOF_SRC_PATH}/schedule/schedule.c
${SOF_SRC_PATH}/schedule/dma_single_chan_domain.c
${SOF_SRC_PATH}/schedule/dma_multi_chan_domain.c
+ ${SOF_SRC_PATH}/schedule/ll_schedule.c
${SOF_SRC_PATH}/schedule/zephyr_ll.c
${SOF_SRC_PATH}/schedule/zephyr.c... but that does not work because:
sof/src/schedule/zephyr_ll.c:418: multiple definition of `zephyr_ll_task_init';
modules/sof/libmodules_sof.a(ll_schedule.c.obj):sof/src/schedule/ll_schedule.c:464: first defined here
sof/src/schedule/zephyr_ll.c:447: multiple definition of `zephyr_ll_scheduler_init';
modules/sof/libmodules_sof.a(ll_schedule.c.obj):sof/src/schedule/ll_schedule.c:624: first defined here
In other words @lyakh in PR #4377 made zephyr_ll.c and ll_schedule.c somehow mutually exclusive even before this PR does with ll_tr.
There was a problem hiding this comment.
Forgot this: a simple rename and declaration of DECLARE_TR_CTX(Zll_tr,... in zephyr_ll.c is not possible because ll_tr is used in at least 5 different C files, some shared across both Zephyr and non-Zephyr.
Maybe I can figure out something else but I need to reproduce your problem first, so if you have a way to compile both zephyr_ll.c and ll_schedule.c together please submit it first.
There was a problem hiding this comment.
@iuliana-prodan @dbaluta so DMA domain is becoming problematic as it does not scale well to multiple low latency streams (it's simple and great when there is a smaller number of streams though) or when the codec is driving the clocks.
I think we need to improve the DMA driven domain as follows.
Today we have
- Zephyr LL gets its LL system tick from the timer. This runs in IRQ context and is non preemtable.
- LL system tick wakes up high priority LL thread.
- LL thread executes pipeline work.
We need to make one change to support DMA domain here and make it synchronous. Step 1 should also be able to be
- Zephyr LL gets its LL system tick from the DMA IRQ. This runs in IRQ context and is non preemtable.
This means we keep the DMA based time domain and we can also scale all LL streams to this time domain.
Fwiw, this is how things worked on OMAP4, the topology would tell FW which DMA channel was to be used as time domain. If the DMA channel stopped then FW had to then use the DMA channel from the next running stream. PR #4471 is allowing the trigger(start|stops) to be synchronised and this would mean that DMA IRQs could also be synchronised.
There was a problem hiding this comment.
@iuliana-prodan can you please:
- File a new issue about the longer term DMA domain discussion and solutions so @lgirdwood 's words of wisdom above do not get lost, buried deep down in this unrelated pull request
- Share a (hopefully simple) draft PR showing how you compile
zephyr_ll.candll_schedule.ctogether so I can reproduce the problem specific to your branch. It does not matter to me if that draft never gets merged, I just need a way to reproduce your problem if you would like me to solve it.
There was a problem hiding this comment.
@lgirdwood @marc-hb I've created a pull request with my fixes for i.MX with Zephyr -see #4524
Now we can run successfully playback and record scenarios with Zephyr.
@lgirdwood until we can extend ll_zephyr with DMA_IRQ (I've created a feature request for this - #4523) we'll be using ll_schedule. Hope it's ok with you.
@marc-hb using my pull request (#4524) and enabling dma-trace you'll run into zephyr_ll.c:22: multiple definition of ll_tr;
That's because in commit 1 I'm adding ll_schedule to be compiled for Zephyr and the zephyr_ll is added mandatory for all platforms - see here.
There was a problem hiding this comment.
@iuliana-prodan I commented in your PR - maybe you shouldn't include both ll_zephyr.c and ll_schedule.c in your builds?
|
Don't try this yet, I screwed up the EDIT: fixed in v2, go for it. v2:
I don't see how the |
marc-hb
force-pushed
the
zephyr-dma-trace
branch
from
7b534d8
to
1334e64
Compare
marc-hb
requested review from
akloniex,
dbaluta,
lbetlej,
lgirdwood,
mmaka1,
mrajwa and
plbossart
as code owners
marc-hb
force-pushed
the
zephyr-dma-trace
branch
from
1334e64
to
47a349e
Compare
| DECLARE_SOF_UUID("zll-schedule", zll_sched_uuid, 0x1547fe68, 0xde0c, 0x11eb, | ||
| 0x84, 0x61, 0x31, 0x58, 0xa1, 0x29, 0x48, 0x53); | ||
|
|
||
| DECLARE_TR_CTX(ll_tr, SOF_UUID(zll_sched_uuid), LOG_LEVEL_INFO); |
|
I don't see how the alsabat failure in On the other hand will need to fix: |
src/trace/trace.c
Outdated
| @@ -544,9 +544,18 @@ void mtrace_dict_entry(uint32_t dict_entry_address, int n_args, ...) | |||
| dict_entry_address, tstamp); | |||
|
|
|||
| va_start(ap, n_args); | |||
| for (i = 0; i < n_args; i++) | |||
| for (i = 0; i < n_args && i < _TRACE_EVENT_MAX_ARGUMENT_COUNT; i++) | |||
There was a problem hiding this comment.
i < MIN(n_args, _TRACE_EVENT_MAX_ARGUMENT_COUNT)
There was a problem hiding this comment.
I considered this but I have bad MIN() memories #4002
Amazingly this saves 12 bytes so I'm doing it. I would have bet the compiler was able to optimize this but apparently not.
There was a problem hiding this comment.
Not sure why the UTs would fail, so rerun just in case a CI issue.
Because I changed one function prototype.
Remembering how to compile unit tests was quite the ordeal so I fixed the docs: thesofproject/sof-docs#363
SOFCI TEST
Only Jenkins reads that.
@lgirdwood we wanted to switch trace do an EDF task
It seems to work as is but @kv2019i has been keeping an eye on performance so he may not be happy with trace_work() waking up every 1ms (instead of 500ms as requested) to do nothing most of the time.
src/trace/trace.c
Outdated
| @@ -544,9 +544,18 @@ void mtrace_dict_entry(uint32_t dict_entry_address, int n_args, ...) | |||
| dict_entry_address, tstamp); | |||
|
|
|||
| va_start(ap, n_args); | |||
| for (i = 0; i < n_args; i++) | |||
| for (i = 0; i < n_args && i < _TRACE_EVENT_MAX_ARGUMENT_COUNT; i++) | |||
There was a problem hiding this comment.
I considered this but I have bad MIN() memories #4002
Amazingly this saves 12 bytes so I'm doing it. I would have bet the compiler was able to optimize this but apparently not.
|
@marc-hb wrote:
No, I'm ok with going ahead and merging this in the ll trace. It's still fairly close to XTOS build behaviour where the DMA traces were flushed from the idle thread. Now we have tools to measure perf, so it's a simpler to also optimize the DMA trace code in incremental PRs. |
Yes, we can go ahead and merge today because its better than the trace we have today and we need this to help developers. Still draft status @marc-hb ? That being said, the trace work needs to be an small preemptable thread (with minimal stack) that wakes up every 500ms or when the buffer > watermark in order to program the DMA. @marc-hb when will this part be ready ? |
After I understand something about schedulers. What sort of regular timer should I use? PS: I realized #4428 is "busy looping" too. |
marc-hb
force-pushed
the
zephyr-dma-trace
branch
from
20f9cce
to
14fc955
Compare
|
The So the only known issue left is
If you agree with this then I think this is ready to be merged. If not then please 'request changes' and please help and offer some concrete https://sof-ci.01.org/sofpr/PR4503/build9755/devicetest/ is all green except one unavailable system PS: this PR should not have any significant functional change for XTOS-SOF; only for Zephyr |
Commit a439ea9 ("zephyr: ll-schedule: switch over to a simplified implementation") replaced ll_schedule.c with zephyr_ll.c when compiling with Zephyr. So the struct tr_ctx named "ll_tr" is now missing. Declare it in ll_schedule.c too. No one noticed that ll_tr was missing because the DMA trace does not work in Zephyr and the main branch yet. Signed-off-by: Marc Herbert <[email protected]>
Because it's mostly working now and avoids macro nesting complexity; there is already #ifdef CONFIG_DW_SPI #elif CONFIG_TRACE Just for the record this is reverting a very tiny part of commit cf8e35f ("zephyr: init: create a zephyr entry point in SOF.") Signed-off-by: Marc Herbert <[email protected]>
Reverts commit 74cacc3 ("zephyr: ipc: dont enable DMA trace transport.") modified by commit d728276 ("ipc3: don't declare unused variables") Signed-off-by: Marc Herbert <[email protected]>
Preparation to use mtrace_dict_entry() also the _log_message() level too and not just for very early mtrace_printf() tracing. Signed-off-by: Marc Herbert <[email protected]>
From deep down trace.c:va_tracelog() up to the _log_message() level. Also rename va_tracelog() to the more specific dma_tracelog() Preparation to support the DMA trace in Zephyr. The only functional change in this commit is that DMA messages copied to the shared memory are not de-duplicated any more (a.k.a "adaptive rate limiting" or CONFIG_TRACE_FILTERING_ADAPTIVE). These are generally supposed to be high level hence rare enough; otherwise there is probably a "bigger problem". Signed-off-by: Marc Herbert <[email protected]>
As of July 2021 we support (too) many tracing options and this duplication is unfortunately the only way I found to support them all while giving the compiler the opportunity to optimize away as many strings as possible. Supported configurations: - Systems with limited memory and zero space for full strings, must use SOF dictionary only. - Systems with enough space for all strings to be in memory. - Anything in between - Support to duplicate only important message to both the DMA and the mailbox (the default) - CONFIG_TRACEM: supports duplicating ALL messages to both the DMA and the mailbox - CONFIG_TRACEV: supports deleting verbose statements at compile time to save space - CONFIG_TRACE: support turning off all traces at compile-time - SOF dict trace de-duplication a.k.a. "adaptive filtering" - Dynamic log levels per component - Redirection to Zephyr's shared memory tracing that requires full strings. Signed-off-by: Marc Herbert <[email protected]>
marc-hb
force-pushed
the
zephyr-dma-trace
branch
from
14fc955
to
5ea6e5b
Compare
marc-hb
dismissed
iuliana-prodan’s stale review
Change of plans, ll_scheduler.c and zephyr_ll.c are mutually exclusive again
marc-hb
requested review from
lgirdwood,
lyakh,
cujomalainey,
iuliana-prodan,
keqiaozhang and
ktrzcinx
|
The CI Zephyr DUT needs a reboot. |
Reviewing commits one by one is highly recommended.
This is a superset of #4452
EDIT: due to CML-specific suspend/resume issue #4558 this was partly reverted by #4578