-
Notifications
You must be signed in to change notification settings - Fork 369
Trace - make clear that trace context is unused with Zephyr #11102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,6 +41,7 @@ | |
| LOG_MODULE_REGISTER(basefw, CONFIG_SOF_LOG_LEVEL); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "srop trace context"? |
||
|
|
||
| SOF_DEFINE_REG_UUID(basefw); | ||
| /* unused with Zephyr, generates no output */ | ||
| DECLARE_TR_CTX(basefw_comp_tr, SOF_UUID(basefw_uuid), LOG_LEVEL_INFO); | ||
|
|
||
| static struct ipc4_system_time_info global_system_time_info; | ||
|
|
@@ -850,7 +851,9 @@ __cold static int basefw_set_large_config(struct comp_dev *dev, uint32_t param_i | |
|
|
||
| static const struct comp_driver comp_basefw = { | ||
| .uid = SOF_RT_UUID(basefw_uuid), | ||
| #ifndef __ZEPHYR__ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this be reversed? I mean these are only needed in LIBRARY builds now (or are they even needed there)? Or maybe that's not accurate enough, but maybe "#ifdef CONFIG_TRACE" (i.e. a build really using sof-logger with dictionaries). And for Zephyr, this doesn't cover the case where you build with Zephyr but disable CONFIG_ZEPHYR_LOG. I think these cases should be prevented somehow.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. all I want to achieve with these is that this code / data doesn't appear in "real" firmware builds made to be run on real hardware. What's the best way to achieve that - can be discussed. This one achieves the goal and passes all CI configurations, so I used it. If the PR in principle is fine, I'd take that and if needed I'd optimise it later - unless there are any real configurations that this is breaking
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack, I'm good that. Not a blocking concern. |
||
| .tctx = &basefw_comp_tr, | ||
| #endif | ||
| .ops = { | ||
| .get_large_config = basefw_get_large_config, | ||
| .set_large_config = basefw_set_large_config, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,7 @@ static const uint32_t max_chain_number = DT_NUM_HDA_HOST_OUT + DT_NUM_HDA_HOST_I | |
| LOG_MODULE_REGISTER(chain_dma, CONFIG_SOF_LOG_LEVEL); | ||
|
|
||
| SOF_DEFINE_REG_UUID(chain_dma); | ||
| /* unused with Zephyr, generates no output */ | ||
| DECLARE_TR_CTX(chain_dma_tr, SOF_UUID(chain_dma_uuid), LOG_LEVEL_INFO); | ||
|
|
||
| /* chain dma component private data */ | ||
|
|
@@ -787,7 +788,9 @@ __cold static void chain_task_free(struct comp_dev *dev) | |
|
|
||
| static const struct comp_driver comp_chain_dma = { | ||
| .uid = SOF_RT_UUID(chain_dma_uuid), | ||
| #ifndef __ZEPHYR__ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Zephyr is only RTOS now. We can remove support for legacy after v2.15
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lgirdwood these preprocessor conditionals are still needed for cmocka, testbench, maybe some other builds too.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ack on @lyakh . @lgirdwood We have no legacy targets anymore, but we do still have test targets that are not Zephyr based. Testbench, ALSA SOF plugin, cmocka -- anything where SOF is built as a library to be integrated to some host application. It's unclear whether using a full Zephyr build for these uses is meaningful (but nobody has tried yet). Cmocka is transitioning to ztest, but testbench and ALSA SOF plugin are bigger questions marks. FYI @singalsu for testbench ties and @tmleman for cmocka/ztest. |
||
| .tctx = &chain_dma_tr, | ||
| #endif | ||
| .ops = { | ||
| .create = chain_task_create, | ||
| .trigger = chain_task_trigger, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,7 @@ LOG_MODULE_REGISTER(pipe, CONFIG_SOF_LOG_LEVEL); | |
|
|
||
| SOF_DEFINE_REG_UUID(pipe); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's with the "!CONFIG_SOF_USERSPACE_LL" in the commit message?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kv2019i see the last hunk in this commit |
||
|
|
||
| /* unused with Zephyr, generates no output */ | ||
| DECLARE_TR_CTX(pipe_tr, SOF_UUID(pipe_uuid), LOG_LEVEL_INFO); | ||
|
|
||
| /* number of pipeline stream metadata objects we export in mailbox */ | ||
|
|
@@ -140,12 +141,15 @@ struct pipeline *pipeline_new(struct k_heap *heap, uint32_t pipeline_id, uint32_ | |
| p->pipeline_id = pipeline_id; | ||
| p->status = COMP_STATE_INIT; | ||
| p->trigger.cmd = COMP_TRIGGER_NO_ACTION; | ||
| #ifndef __ZEPHYR__ | ||
| /* Zephyr's tr_*() macros ignore the trace context, no need to copy it */ | ||
| ret = memcpy_s(&p->tctx, sizeof(struct tr_ctx), &pipe_tr, | ||
| sizeof(struct tr_ctx)); | ||
| if (ret < 0) { | ||
| pipe_err(p, "failed to copy trace settings"); | ||
| goto free; | ||
| } | ||
| #endif | ||
|
|
||
| ret = pipeline_posn_offset_get(&p->posn_offset); | ||
| if (ret < 0) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unused if not CONFIG_TRACE, not really related to Zephyr...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does a Zephyr build with
CONFIG_TRACE=ydo? Does this make any sense? Is ever used? Maybe makeCONFIG_TRACEdepend on!ZEPHYR_SOF_MODULE?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh sof-logger with Zephyr. We used that in the past, but no longer used on Intel platforms. If this is no longer needed, we can remove the whole sof-logger from tree. I think this should be a separate PR and get all stakeholders to review the removal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i sof-logger needs
CONFIG_TRACE?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh It's right in the Kconfig description for CONFIG_TRACE:
This is the option to enable sof-logger compatible trace generation from FW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh It doesn't seem to be used by anyone anymore (at least with Zephyr), so maybe we can remove sof-logger (tools and FW-side infra). This should be a separate PR though:
In src/arch/xtensa/configs/ we still have usage (at least with @thesofproject/amd targets, most recent additions in Apr 2026). A separate PR (to remove sof-logger and CONFIG_TRACE) would be good so people get a chance to review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kv2019i ah, I might be confusing names then. In the same Kconfig:
so, trace was before Zephyr and Zephyr never (or only very briefly as I seem to recall) displayed
trace_point()messages likesof/src/platform/intel/ace/platform.c
Line 99 in 3ebd434
CONFIG_TRACEtoo. It's just that at some point we should replace all thetr_dbg(&tr_ctx,...)calls to remove those unused contexts and that would be a large change. I guess we'll have to do it in several steps one directory at a timeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lyakh Ack, the names are confusing. "trace_point()" interface has not been actively used (since SOF v2.2) and has been never supported in any Zephyr build. If you grep the current sources, all implementations are no-ops for all platforms. The code has been left as this has been convenient for developers to plug in custom debugging (i.e. write trace_point to a shared SRAM location to observe early boot fails). With @lgirdwood now adding a breadcrumbs API zephyrproject-rtos/zephyr#113242 to Zephyr, something like that would be a good backend for SOF trace_point() calls as well (e.g. something that can be read from shared memory even when DSP crashes and the kernel is no longer functional).
sof-logger has been used actively and most SOF targets have used it while transitioning to Zephyr. But it's good to emphasize all platforms should eventually move to Zephyr logging as sof-logger can never capture all logs (it only sees logs from SOF side code, it cannot be used to get logging from the RTOS or drivers), so it's a transition solution at best.