Skip to content

profiling task: modify task profiling with performance counter macro #6407

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
Merged
dbaluta merged 1 commit into from
Oct 14, 2022
Merged

profiling task: modify task profiling with performance counter macro #6407

dbaluta merged 1 commit into from
Oct 14, 2022

Conversation

@btian1
Copy link
Contributor

@btian1 btian1 commented Oct 12, 2022 •
edited
Loading

This patch is used to make sure profiling code only can be run with performance profiling build, and unify all performance profiling with same format and usage.

Signed-off-by: Baofeng Tian baofeng.tian@intel.com

@btian1 btian1 mentioned this pull request Oct 12, 2022
Closed
@btian1 btian1 requested review from kv2019i and lyakh October 12, 2022 05:29
@dbaluta
Copy link
Collaborator

dbaluta commented Oct 12, 2022

Please check compile errors.

mmaka1
mmaka1 reviewed Oct 12, 2022
View reviewed changes
src/schedule/zephyr_ll.c Outdated
Copy link

@mmaka1 mmaka1 Oct 12, 2022

Choose a reason for hiding this comment

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

task_perf_... undefined?

Copy link
Contributor Author

@btian1 btian1 Oct 12, 2022

Choose a reason for hiding this comment

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

defined in #6353
I know, looks not good, the initial purpose want to put all perf_cnt changes in one PR.
does this acceptable? if not, I have to move the definition to this patch.

Copy link
Contributor Author

@btian1 btian1 Oct 12, 2022

Choose a reason for hiding this comment

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

I changed to make each PR independent.

src/schedule/zephyr_ll.c Outdated
Copy link

@mmaka1 mmaka1 Oct 12, 2022

Choose a reason for hiding this comment

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

Should ..._avg be coupled with CONFIGU_PERFORMACE_COUNTERS_RUN_AVERAGE?

Copy link
Contributor Author

@btian1 btian1 Oct 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

yeah, in theory, it should, however, I even want to remove the CONFIG_PERFORMANCE_COUNTERS_RUN_AVERAGE, because since performance counter enabled, no need second switch, what do you think? if you agree, I can raise another patch to remove AVERAGE, I don't think it make too much sense.

I did not do this, because I don't want to break history, due to I am new, if you want a change, I can make it, :)

Copy link

@mmaka1 mmaka1 Oct 12, 2022

Choose a reason for hiding this comment

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

Yes, I'd simplify and merge run_average into a single config option.

Copy link
Contributor Author

@btian1 btian1 Oct 12, 2022

Choose a reason for hiding this comment

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

ok, I will submit another patch to do this after all are merged.

dbaluta
dbaluta reviewed Oct 12, 2022
View reviewed changes
src/include/sof/schedule/task.h Outdated
Copy link
Collaborator

@dbaluta dbaluta Oct 12, 2022

Choose a reason for hiding this comment

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

You have now two new lines. Just keep one.

@btian1
profiling task: modify task profiling with performance counter macro
e07162c 
This patch is used to make sure profiling code only can be run
with performance profiling build, and unify all performance profiling
with same format and usage.

Signed-off-by: Baofeng Tian <baofeng.tian@intel.com>
@dbaluta dbaluta merged commit 33ea9ea into thesofproject:main Oct 14, 2022
@dbaluta
Copy link
Collaborator

dbaluta commented Oct 14, 2022

Merged this, but then noticed that there is a build error:

 Parsing /zep_workspace/zephyr/Kconfig
Loaded configuration '/zep_workspace/zephyr/boards/xtensa/intel_adsp_ace15_mtpm/intel_adsp_ace15_mtpm_defconfig'
Merged configuration '/zep_workspace/sof/app/prj.conf'
Merged configuration '/zep_workspace/sof/app/boards/intel_adsp_ace15_mtpm.conf'
/zep_workspace/sof/app/boards/intel_adsp_ace15_mtpm.conf:9: warning: attempt to assign the value 'y' to the undefined symbol DW_ICTL_ACE_V1X

Anyhow, this doesn't look to be related to this commit. So I think it should be fine.

Someone at Intel needs to look into the build failure.

@kv2019i
Copy link
Collaborator

kv2019i commented Oct 14, 2022

@dbaluta That's ok, that is fixed in mainline SOF already.

marc-hb
marc-hb reviewed Oct 17, 2022
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.

BTW there is still a similar tr_info() in src/schedule/zephyr_domain.c even after this PR. That's all I know.

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

Reviewers

@dbaluta dbaluta dbaluta approved these changes

@marc-hb marc-hb marc-hb left review comments

@singalsu singalsu singalsu approved these changes

@mmaka1 mmaka1 mmaka1 approved these changes

@abonislawski abonislawski abonislawski approved these changes

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

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

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

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

@lyakh lyakh Awaiting requested review from lyakh

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@btian1 @dbaluta @kv2019i @singalsu @mmaka1 @abonislawski @marc-hb