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.

Sign up for GitHub

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

Jump to bottom

Update some debugging information #351

Merged
merged 3 commits into from
Jun 11, 2021
Merged

Update some debugging information #351

merged 3 commits into from
Jun 11, 2021

Conversation

ujfalusi
Copy link
Contributor

Add clarifications and details for debugging SOF issues.

The updates are created based on issue #347 in hope that the information in the documentation will be more user friendly.

@ujfalusi ujfalusi mentioned this pull request May 31, 2021
Open
lyakh
lyakh requested changes May 31, 2021
View reviewed changes
Copy link
Contributor

@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 missing -l was there before, but since you already touch that text, we can fix it as well

getting_started/intel_debug/suggestions.rst Outdated Show resolved Hide resolved
developer_guides/debugability/logger/index.rst Outdated Show resolved Hide resolved
kv2019i
kv2019i approved these changes May 31, 2021
View reviewed changes
getting_started/intel_debug/suggestions.rst Outdated Show resolved Hide resolved
marc-hb
marc-hb reviewed Jun 1, 2021
View reviewed changes
Copy link
Collaborator

@marc-hb marc-hb left a comment

Choose a reason for hiding this comment

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

The term "mailbox" is all over the code and probably in the documentation too so it's useful to mention it here.

developer_guides/debugability/logger/index.rst Outdated
.. note::
debugfs files used by ``sof-logger``

- ``etrace``: direct access (memcpy) to TRACE window of the SOF firmware
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- ``etrace``: direct access (memcpy) to TRACE window of the SOF firmware
- ``etrace``: direct access to TRACE window of the SOF firmware in the shared memory "mailbox"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marc-hb, the trace window is not within the 'mailbox' memory. We have a bigger shared area and within that area we have the 'mailbox' (up and down region), trace, debug, stream, regs and exception windows.
Some of these windows are optional and only used for debugging, like the trace region.

Would it be better if we use the term 'region' instead of 'window' when referring to the TRACE window/region of the shared area?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

However the sof-logger's help documents the t option:
sof-logger: -t DMA 'trace' stream instead of 'etrace' error mailbox

Probably something like this:
etrace: direct access to TRACE error mailbox of the firmware

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have a bigger shared area and within that area we have the 'mailbox' (up and down region), trace, debug, stream, regs and exception windows.

Thanks, can you please share some pointers to this information? It looks like our terminology is inconsistent.

However the sof-logger's help documents the t option:

I recently changed this help message so it does not count :-) However I changed it based on symbols like CONFIG_TRACEM and MAILBOX_TRACE_BASE and other comments git grep -i -e mailbox -e mbox -- src/trace/; most of these are not from me.

So maybe "mailbox" is not the correct term here. On the other hand, "shared memory" is IMHO still better than "(memcpy)" in documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would go with a simple:
etrace: direct access to TRACE window of the SOF firmware
If ever we will support non mem mapped type of DSP with SOF then this will be still valid.
We can use region instead of window, that should be fine as well.

The regions/windows are defined under xsram_window and they are really just sections within a shared memory between the CPU and DSP in currently supported platforms.
Imho to say that something is a mailbox it would take a bit more than a shared memory, a signaling mechanism like doorbells should be there. TRACE does not have associated doorbell afaik.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@lyakh Have your change requests been completed? @ujfalusi Are you ready for the document to be merged? thanks

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would go with a simple: etrace: direct access to TRACE window of the SOF firmware

I feel like this is a confusing way to say "non-DMA trace" or "shared memory". What's wrong with either of these?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lyakh Have your change requests been completed? @ujfalusi Are you ready for the document to be merged? thanks

@deb-intel yes, they have! I've approved the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@marc-hb, can you check the new wording for etrace:
etrace: direct access to the shared TRACE window of the SOF firmware

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good considering the next one mentions "DMA". I'm still frustrated by the lack of good, "non-DMA/mailbox" name in general (not just in these docs) but that should certainly not delay this useful addition.

@ujfalusi
debugability: logger: Add note for etrace and trace debugfs files
6740a97 
Add a note section with brief information about the etrace and trace
debugfs files.

Signed-off-by: Peter Ujfalusi <[email protected]>
@ujfalusi
intel_debug: Add link to dynamic debug documentation of the Linux kernel
664da17 
To avoid confusion, add link to the kernel documentation about the dynamic
debug feature used by the Linux kernel.

Signed-off-by: Peter Ujfalusi <[email protected]>
@ujfalusi
intel_debug: Add instruction to enable the trace debugfs
b44a2e5 
Distribution kernels ship with trace not enabled by default.
Add instruction on how to enable the file in order to provide traces.

Signed-off-by: Peter Ujfalusi <[email protected]>
@lgirdwood lgirdwood merged commit 9d72740 into thesofproject:master Jun 11, 2021
@lgirdwood
Copy link
Member

@deb-intel fyi - ready for grammar check :)

@deb-intel
Copy link
Collaborator

@lrgirdwo Sounds good. I plan to publish the site today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyakh lyakh lyakh approved these changes

@lgirdwood lgirdwood lgirdwood approved these changes

@kv2019i kv2019i kv2019i approved these changes

@marc-hb marc-hb marc-hb approved these changes

@deb-intel deb-intel Awaiting requested review from deb-intel deb-intel is a code owner

@intelkevinputnam intelkevinputnam Awaiting requested review from intelkevinputnam intelkevinputnam is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ujfalusi @lgirdwood @deb-intel @lyakh @kv2019i @marc-hb