Skip to content

Conversation

@jsarha
Copy link
Contributor

@jsarha jsarha commented Mar 12, 2025

The sof-ipc-timer collects module initialization and configuration timings, and set pipeline state timings from 'journalctl -k -o short-precise' output if SOF IPC debug is enabled.

Different kind of outputs can be generated with different flags. Adding just -s gives a summary like this:

host-copier.0.playback init min 196 us max 456 us average 322 us of 100
gain.1.1 init min 192 us max 552 us average 327 us of 100
gain.1.1 conf min 197 us max 494 us average 325 us of 100
mixin.1.1 init min 193 us max 8219 us average 406 us of 100
mixout.2.1 init min 197 us max 573 us average 327 us of 100
gain.2.1 init min 197 us max 484 us average 324 us of 100
gain.2.1 conf min 193 us max 418 us average 322 us of 100
smart_amp.2.1 init min 194 us max 526 us average 326 us of 100
smart_amp.2.1 conf min 197 us max 1826 us average 393 us of 100
dai-copier.SSP.NoCodec-0.playback init min 202 us max 2135 us average 479 us of 100
pipeline.1: host-copier.0.playback, gain.1.1, mixin.1.1,
pipeline.1 3 min 200 us max 2592 us average 553 us of 200
pipeline.1 4 min 228 us max 3202 us average 673 us of 100
pipeline.1 2 min 208 us max 1867 us average 565 us of 100
pipeline.2: mixout.2.1, gain.2.1, smart_amp.2.1, dai-copier.SSP.NoCodec-0.playback,
pipeline.2 3 min 199 us max 2667 us average 863 us of 200
pipeline.2 4 min 205 us max 8656 us average 794 us of 100

Signed-off-by: Jyri Sarha jyri.sarha@linux.intel.com

sof-ipc-timer.py: Fix summary only functionality

The sof-ipc-timer collects module initialization and configuration
timings, and set pipeline state timings from 'journalctl -k -o
short-precise' output if SOF IPC debug is enabled.

Different kind of outputs can be generated with different flags. Adding
just -s gives a summary like this:

host-copier.0.playback init     min 196 us      max 456 us      average 322 us of 100
gain.1.1 init   min 192 us      max 552 us      average 327 us of 100
gain.1.1 conf   min 197 us      max 494 us      average 325 us of 100
mixin.1.1 init  min 193 us      max 8219 us     average 406 us of 100
mixout.2.1 init min 197 us      max 573 us      average 327 us of 100
gain.2.1 init   min 197 us      max 484 us      average 324 us of 100
gain.2.1 conf   min 193 us      max 418 us      average 322 us of 100
smart_amp.2.1 init      min 194 us      max 526 us      average 326 us of 100
smart_amp.2.1 conf      min 197 us      max 1826 us     average 393 us of 100
dai-copier.SSP.NoCodec-0.playback init  min 202 us      max 2135 us     average 479 us of 100
pipeline.1: host-copier.0.playback, gain.1.1, mixin.1.1,
pipeline.1 3    min 200 us      max 2592 us     average 553 us of 200
pipeline.1 4    min 228 us      max 3202 us     average 673 us of 100
pipeline.1 2    min 208 us      max 1867 us     average 565 us of 100
pipeline.2: mixout.2.1, gain.2.1, smart_amp.2.1, dai-copier.SSP.NoCodec-0.playback,
pipeline.2 3    min 199 us      max 2667 us     average 863 us of 200
pipeline.2 4    min 205 us      max 8656 us     average 794 us of 100

Signed-off-by: Jyri Sarha <jyri.sarha@linux.intel.com>
@jsarha jsarha force-pushed the dmesg_ipc_diff_timer_v2 branch from 54df69f to fc1974f Compare March 12, 2025 14:14
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.

Super quick and totally superficial codestyle review. You will need to find someone else to review the actual logic!

Some of the pylint warnings seem valid. If you want you can turn off the others with inline comment, just git grep pylint for examples.

https://github.com/thesofproject/sof-test/actions/runs/13813544629/job/38640776340?pr=1262

self.state = -1

def parse_line(self, line):
if match_obj := re.search(r" ipc tx ( |reply|done )", line):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Using the relatively new := saves two lines but costs 15 lines of indentation.

Suggested change
if match_obj := re.search(r" ipc tx ( |reply|done )", line):
match_obj = re.search(r" ipc tx ( |reply|done )", line):
if not match_obj:
return False:

https://medium.com/swlh/return-early-pattern-3d18a41bba8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On those lines I am not particularly pressed about indentation, but its relevant to see the whole branch including the re.search() all at once. To my eye the both are more ore less equal, or at least there is more pressing code clean up before this.

Copy link
Collaborator

@marc-hb marc-hb Mar 19, 2025

Choose a reason for hiding this comment

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

The "Return Early" pattern is about a lot more than indentation. I should stop mentioning indentation, it's only the most obvious and visible symptom.

super().__init__(args, comp_data, pipe_data)

def parse_line(self, line):
if match_obj := re.search(r" Create widget ", line):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as line 144

elif msg_type == "done ":
self.parse_glb_set_done(msg_str, usecs)

class SOFLinuxLogParser:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This singleton seems a bit overkill to just hide global variables but why not...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It saves me some writing to have those common data-members in only one place.

log_parser = SOFLinuxLogParser(args)

if args.filename is None:
log_parser.read_log_data(sys.stdin)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! I thought you needed to read the file twice? NVM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, after reading the logs abit, I realized that the "Create" line will always appear before the IPC exchange related to the widgets they create, so I can do everything in one go.

Copy link
Contributor Author

@jsarha jsarha left a comment

Choose a reason for hiding this comment

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

Thanks for your comments. I think I will need push yet completely new PR, since my current git branch has a lot of new features, fixes and cleanups in mixed order, and I don't think sorting them to separate commits makes too much sense.
... and there is still a lot more cleaning up to do, even with once commit approach, and couple of more features to add.

self.state = -1

def parse_line(self, line):
if match_obj := re.search(r" ipc tx ( |reply|done )", line):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On those lines I am not particularly pressed about indentation, but its relevant to see the whole branch including the re.search() all at once. To my eye the both are more ore less equal, or at least there is more pressing code clean up before this.

elif msg_type == "done ":
self.parse_glb_set_done(msg_str, usecs)

class SOFLinuxLogParser:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It saves me some writing to have those common data-members in only one place.

log_parser = SOFLinuxLogParser(args)

if args.filename is None:
log_parser.read_log_data(sys.stdin)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, after reading the logs abit, I realized that the "Create" line will always appear before the IPC exchange related to the widgets they create, so I can do everything in one go.

@jsarha
Copy link
Contributor Author

jsarha commented Mar 24, 2025

There is yet another PR and this old one is abandoned: #1266

@jsarha jsarha closed this Mar 24, 2025
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.

2 participants