Skip to content
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

Trace: Remove ctx dependency from current logging system #8822

Closed
wants to merge 13 commits into from

Conversation

btian1
Copy link
Contributor

@btian1 btian1 commented Jan 31, 2024

This is a preparation for SOF logging to zephyr interface, IPC3 logging need context support, it is not must, and verified with #8725 , so remove these dependencies and will use another new PR to make SOF with zephyr logging interface.

Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the last commit in the series is huge, impossible to review (it seems trivial, but still would be good to at least have a chance), and has a high breakage probability - almost any other PR, that gets merged before this one would break it.
What I personally would do instead, I'd add a new interface (maybe trace_*() or log_*()) and gradually move files over to it - one subsystem at a time, one PR for IPC, one PR for schedulers, etc. Then after all are migrated, you just remove the tr_*() API

@lgirdwood
Copy link
Member

the last commit in the series is huge, impossible to review (it seems trivial, but still would be good to at least have a chance), and has a high breakage probability - almost any other PR, that gets merged before this one would break it. What I personally would do instead, I'd add a new interface (maybe trace_*() or log_*()) and gradually move files over to it - one subsystem at a time, one PR for IPC, one PR for schedulers, etc. Then after all are migrated, you just remove the tr_*() API

This would simplify the merge, i.e. not a big bang and reduced change of conflict races with other PRs.
@btian1 one more thing, I would show the log output before/after the change in the PR introduction.

@btian1 btian1 marked this pull request as draft February 1, 2024 06:54
@dbaluta
Copy link
Collaborator

dbaluta commented Feb 1, 2024

Can you please try to address at least some of the checkpatch.pl errors:

for example this:

WARNING: macros should not use a trailing semicolon
#243: FILE: src/include/sof/trace/trace_nonzephyr.h:137:
+#define trace_check_size_uint32(a) \
+	CT_ASSERT(sizeof(a) <= sizeof(uint32_t), "error: trace argument is bigger than a uint32_t");

I think it is relevant.

@btian1 btian1 force-pushed the logging_change branch 4 times, most recently from c8bf2a4 to ac05db0 Compare February 2, 2024 05:33
@btian1
Copy link
Contributor Author

btian1 commented Feb 2, 2024

@kv2019i @lgirdwood @lyakh @dbaluta , codestyle problem is origin issue, if we want to fix, we can fix with another PR, this PR does not change those code actually.

Regarding the fuzzy error, it may related with #8632 patch merge, please notice.

@btian1 btian1 marked this pull request as ready for review February 2, 2024 05:58
@btian1
Copy link
Contributor Author

btian1 commented Feb 6, 2024

@dbaluta , could you try with this PR for IPC3 logging? I don't want make pause here due to CNY holiday.

@dbaluta
Copy link
Collaborator

dbaluta commented Feb 7, 2024

@LaurentiuM1234 can you please try to see if logging still works after this PR?

@btian1 can you please confirm the testing conditions? Is it with XTOS right?

@LaurentiuM1234
Copy link
Contributor

LaurentiuM1234 commented Feb 7, 2024

@LaurentiuM1234 can you please try to see if logging still works after this PR?

@btian1 can you please confirm the testing conditions? Is it with XTOS right?

tested on QM with Zephyr hybrid build (and CS42888 tplg) and all's good. For reference, I'm attaching a snippet of how the logs look like w/ this patch. I'm assuming this behaves the same as XTOS since they use the same infrastructure.

Screenshot from 2024-02-07 14-15-48

@dbaluta
Copy link
Collaborator

dbaluta commented Feb 7, 2024

@LaurentiuM1234 can you post a snippet of the log w/o the patch. It looks like now the component is shown as "Unknown". I wonder if w/o the patch the real component is correctly shown or there is an earlier bug.

@LaurentiuM1234
Copy link
Contributor

@LaurentiuM1234 can you post a snippet of the log w/o the patch. It looks like now the component is shown as "Unknown". I wonder if w/o the patch the real component is correctly shown or there is an earlier bug.

sorry for the long delay. Attaching snippet (8QM, WM8690, SOF main (557e581)):
Screenshot from 2024-02-12 13-49-09

@dbaluta
Copy link
Collaborator

dbaluta commented Feb 12, 2024

@btian1 looks like with your patch we lose ability to get the component for which the log was generated. Is this intended right?

If so then maybe we can remove that column from sof-logger output.

What's your take on this?

@btian1
Copy link
Contributor Author

btian1 commented Feb 18, 2024

then maybe we can remove that column from sof-logger output.

What's your take on this?

Thanks, @dbaluta @LaurentiuM1234 , due to UID was removed, then sof-logger unable to find each component name for logging, as you suggested, I removed this part print from sof-logger, please try with full PR with rebuild sof-logger again, it should be fine now with/without this patch, the log should be same.

Dai class level logging is not used by dai legacy module, remove it.

Signed-off-by: Baofeng Tian <[email protected]>
This is part of job for remove ipc3 logging ctx dependency.

Signed-off-by: Baofeng Tian <[email protected]>
This is part of remove ipc3 logging context dependency.

Signed-off-by: Baofeng Tian <[email protected]>
This interface will be used to replace existing interface in trace
header file, compared between this and existing interace, the difference
is this interace does not have context dependency.

Signed-off-by: Baofeng Tian <[email protected]>
Replace it with new defined logging interface without context
dependency.

Signed-off-by: Baofeng Tian <[email protected]>
Replace it with new defined logging interface without context
dependency.

Signed-off-by: Baofeng Tian <[email protected]>
Replace it with new defined logging interface without context
dependency.

Signed-off-by: Baofeng Tian <[email protected]>
Replace it with new defined logging interface without context
dependency.

Signed-off-by: Baofeng Tian <[email protected]>
Replace it with new defined logging interface without context
dependency.

Signed-off-by: Baofeng Tian <[email protected]>
Remove unused trace definition, since it is already replaced by
non-zephyr trace definition.

Signed-off-by: Baofeng Tian <[email protected]>
Remove context dependency from four atomic trace definition.

Signed-off-by: Baofeng Tian <[email protected]>
@btian1 btian1 force-pushed the logging_change branch 2 times, most recently from 31d2dc9 to e89eafc Compare February 19, 2024 02:31
Remove it and change source file accordingly.

Signed-off-by: Baofeng Tian <[email protected]>
Due to previous change, there is no valid component uuid
for sof logger, so component name loopup will return unknown,
remove this part.

Signed-off-by: Baofeng Tian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants