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

Telemetry: change performance timing method to ccount #9361

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

tobonex
Copy link
Contributor

@tobonex tobonex commented Aug 12, 2024

When it comes to performance, to compare Sof with its reference we should use the same method of timing. Changing timing source to ccount for telemetry and performance monitor.

@tobonex tobonex changed the title Telemetry perf count change Telemetry: change performance timing method to ccount Aug 12, 2024
@tobonex tobonex force-pushed the telemetry_perf_count_change branch from 9f88050 to d0964dc Compare August 12, 2024 16:48
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Mostly ok, but the board config change needs to be marked better. Please see comment.


telemetry_update(begin_stamp, current_stamp);
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

So this is called more often after the patch, I guess this is intended in the patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I intended this to be more aligned with performance overlay... but this does seem to update the values more often which seems unnecessary. Not sure how it should work. I could delete this one commit for now, as this not a main focus of this PR.

@@ -110,3 +110,4 @@ CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING=y
CONFIG_COMP_STUBS=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

The git commit prefix is wrong, this patch applies to one Intel platform only. And what about other Intel SOF targets? Shouldn't we have same timer source for all Intel platforms?

Adds abstract function to change time source for performance measurement
via config.

Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
Change performance counter for telemetry to abstract one.
This enables choice of time source.

Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
Change performance counters to use abstract function to use multiple
time sources via config.

Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
Use ccount for performance measurement for mtl to have a better comparison with
reference firmware.

Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
Use ccount for performance measurement for lnl to have a better comparison with
reference firmware.

Signed-off-by: Tobiasz Dryjanski <tobiaszx.dryjanski@intel.com>
@lgirdwood
Copy link
Member

@kv2019i good for you now ?

Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Ok, let's proceed. The git commit prefixes are still wrong, but this is a problem with other PRs as well, so I won't block because of this. E.g. for the ace15 config file, the history looks like this:

55ecc19ab6e5 app: intel_adsp_ace15_mtpm.conf: set DRC as built-in
cfd27fbde0e1 src: convert to a loadable module
9f480375057e llext: automatically calculate module addresses

Only first one is correct, so it's not just this PR.

But oh well, you did add the board mentions on the summary lines, so this is already better.
For future, git commits should have a module prefix and this should make clear what is changed in a commit:

[area]: [summary of change]

[Commit message body (must be non-empty)]

Signed-off-by: [Your Full Name] <[your.email@address]>

@kv2019i kv2019i merged commit 1872a10 into thesofproject:main Sep 18, 2024
44 of 47 checks passed
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.

4 participants