component: move comp_copy from header file to source file #6344

btian1 · 2022-09-28T02:41:53Z

On sof zephyr ipc4 build, component performance profiling logs can't
be enabled due to current xcc compiler does not support inline
logging in header file, logs as below:

log_level undeclared (first use in this function)
log_current_const_data undeclared (first use in this function)

Move comp_copy to component.c can resolve this limitation.
However, this brings another cmocka issue for mixer unit test,
this patch also fixed cmocka mixer unit test issue.

BugLink: zephyrproject-rtos/zephyr#43786
Signed-off-by: Baofeng Tian [email protected]

marc-hb · 2022-09-28T05:01:52Z

current xcc is based on gcc 4.x version, does not support static inline log print with ipc4 and zephyr

This problem statement and rationale are too short for me to make sense of them. Can you expand a bit and/or add URL(s)?

lyakh · 2022-09-28T10:57:27Z

src/audio/mixer/mixer.c

 static SHARED_DATA struct comp_driver_info comp_mixer_info = {
 	.drv = &comp_mixer,
 };
+#endif


not much prettier but maybe

#ifndef UNIT_TEST static #endif SHARED_DATA struct comp_driver_info comp_mixer_info = { .drv = &comp_mixer, };

would be better

done, less change always looks better, I also checked, it is working, :)

btian1 · 2022-09-28T13:13:49Z

and rationale are too short for me to make sense of them. Can you

Yes, I added more comments, please check again, if still have problems, you can refer to below link:
zephyrproject-rtos/zephyr#43786

lgirdwood

@aiChaoSONG @btian1 looks like zephyrproject-rtos/zephyr#43786 is closed. Good to remove the comment ?

btian1 · 2022-09-28T13:40:40Z

@aiChaoSONG @btian1 looks like zephyrproject-rtos/zephyr#43786 is closed. Good to remove the comment ?

Hi, Liam

Althrough it was closed, however, this issue is WA by ifndef zephyr due to urgent schedule, now this PR fixed it, after this patch merge, I will reconsolidate another PR #6322 to remove those code with other performance code and test it on IPC4 fw build.
for this PR now, let's keep it.

Thanks
Tim

ranj063 · 2022-09-28T15:40:59Z

src/audio/component.c

@@ -234,3 +234,33 @@ void audio_stream_copy_to_linear(const struct audio_stream __sparse_cache *sourc
 		snk += bytes_copied;
 	}
 }
+


@btian1 why will cmocka tests require comp_copy() On the contrary the comp_drv interface will eventually be deprecated for all modules.

because each cmocka module unit test need comp_copy, once move it from component_ext.h to component.c, cmocka need component.c to make comp_copy real.

regarding remove comp_drv, that's another topic for me, let's keep current solution, I can't wait until comp_drv disappearred.

Yes, Marc, this is correct, regarding the #ifndef ZEPHYR, I will change it in #6322(since perf_cnt.h need change as well), after this merged, #6322 should be based on ipc4+zephyr, ipc4 need this change as must, so I have to wait for this change merge first, then modify #6322.

marc-hb

As far as I understand zephyrproject-rtos/zephyr#43786 affects only static inline, which I believe is why you're moving this to a .c file. Correct?

If correct then why are you keeping the #ifndef __ZEPHYR__? Are you not testing Zephyr?

Very confused sorry.

btian1 · 2022-09-29T02:02:01Z

Hi,

This change is low risk, hope this can be review and merge soon, then I can continue submit another PR for performance build.

Thanks
Tim

marc-hb · 2022-09-29T07:29:18Z

Yes, Marc, this is correct, regarding the #ifndef ZEPHYR, I will change it in #6322 perf_cnt.h need change as well),

"I will finish this commit in the next commit" is not good. Each commit must have one logical change and anyone must be able to git checkout any point in the git history and use it.
https://wiki.openstack.org/wiki/GitCommitMessages#Structural_split_of_changes
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#separate-your-changes
I you have a circular dependency between two commits then it means they should be only one.

At the very least you should fix the comment or the commit message. Right now you're keeping unchanged the zephyrproject-rtos/zephyr#43786 reference to a static inline problem in a commit that moves away from a static inline?! Very confusing. Or maybe the commit message should say "this is the first step but there's still another problem that will be fixed next"

btian1 · 2022-09-29T13:34:45Z

changed, confirmed the bug has been fully fixed, please check again, and if no other issues, please +1

Thanks
Tim

lgirdwood

@btian1 can you say the inline was blocked by old xcc. Was the code not producing correct results or not running at all ? Need to be clear in the commit message as a change like this is hard to understand unless you can paste the old data n the commit vs the new data.

btian1 · 2022-10-08T01:56:45Z

previously, comp_copy is located in header file, type is static inline. however, current xcc is based on gcc 4.x version, does not support static inline log print, this block measure component performance. move it to source file will benefit code size, and have very small impact on performance, since this is component level function and only have one parameter.

@lgirdwood , I explained in commit message with above, inline just block add log and profiling each module, there is no functionality impact for comp_copy.
please check above commit message, if still not clear, I will add more.

btian1 · 2022-10-08T03:10:08Z

Hello, Reviewers,

could you check again and approve this? this patch is only for profiling purpose: to add profiling log to comp_copy with zephyr + ipc4 build, without this change, logs can't added into comp_copy, then can't profiling each component.

Thanks
Tim

aiChaoSONG · 2022-10-10T08:07:38Z

src/include/sof/audio/component_ext.h

-#endif
-
-		ret = dev->drv->ops.copy(dev);
+int comp_copy(struct comp_dev *dev);


I think this should be in component.h

there is no component_ext.c for source file, so component.c can cover component_ext.h and component.h both.
and if move to component.h will cause much more changes, a lot of component_ext.h include will change to component.h.
Agree?

component_ext.h contains all inline functions, no function prototype in it. while component.c and component.h are .c/.h pair.

void sys_comp_init(struct sof *sof);
it have, also defined in component.c

It seems quite confusing indeed. Some prototypes like comp_get_copy_limits() are in component.h while others like comp_new() are in component_ext.h. Given other copy relate functions have prototypes in component.h, I'd put comp_copy() there as well. @ranj063 @juimonen do you happen to recollect the history?

I searched #include <sof/audio/component_ext.h> under SOF directory, it have nearly 50 place to include, if move it to component.h, it will cause much more source file to change.

I don't know history about component.h and compont_ext.h, but currently, they have same component.c source file.
even check component.h, it is mixed with macro, inline, prototype.

If we really want to change, I would suggest discuss first, then raise an CR to merge(or re-arrange) two header files

currently, suggest to keep, since component_ext also have prototype declaration, Agree?

it have nearly 50 place to include, if move it to component.h, it will cause much more source file to change.

This would not be pleasant but it's not the end of world either: it's only a one-line change in each file so this would not break git much. If this is really the best choice then don't be afraid to do it.

Most projects have some messy #includes that no one really cares about... until someone tries to move something exactly like as you're doing here; it's typical. Sometimes this can trigger some #include cleanup and that's usually a good thing.

Note: I do NOT know whether it's the best choice! Discuss and make sure first. Considering how longer and longer this thread is getting, a call should help.

@btian1 I think keeping the comp_copy() declaration is component_ext.h is fine. All the other ops' declaration or definition are in this file. So I see no objection here

I don't know history about component.h and compont_ext.h

Github's browsing features are very user-friendly. Sometimes I drop tig and switch to the web browser instead!

... (top right corner) -> View File -> Blame -> line 1 ->

comp: api: advanced and internal functions separated The current content of component.h is a mix of basic api, common basic helpers for every component developers as well as advanced functions and macros used by infrastructure and very specialized components like host, dai, kpb etc. This patch moves the advanced part to component_ext.h and keeps only basic part in component.h to avoid overloading effects component developers with declarations and code they do not use.

I think this should be in component.h

No, it should not. It is a little helper function that hides comp driver/device details, called by the infrastructure. While the component.h was intended to declare things relevant for component implementors, see the commit message @marc-hb mentioned in the comment above. component.h had been overloaded and unreadable before component_ext.h was created to separate internal non-api things. Not sure if merging back is a good idea.

aiChaoSONG · 2022-10-10T08:36:03Z

I think below commit message is much clearer.

The xcc compiler is not able to detects and applies correct zephyr log context for inline functions, a previous workaround removes the perf logging code by #ifndef guard under zephyr.

Because the perf logging is very useful, this patch re-workarounds the issue by uninlining the comp_copy function and guard the perf logging code with kconfig option (CONFIG_PERFORMANCE_COUNTERS) as it is not always used.

As the comp_copy is also used by cmocka tests, the uninlining brings issue to cmocka tests, this patch also fixes the cmocka tests issue brought by comp_copy uninlining.

BugLink: https://github.com/zephyrproject-rtos/zephyr/issues/43786

src/audio/component.c

singalsu · 2022-10-10T08:51:42Z

src/audio/mixer/mixer.c

@@ -470,7 +470,10 @@ static const struct comp_driver comp_mixer = {
 	},
 };

-static SHARED_DATA struct comp_driver_info comp_mixer_info = {
+#ifndef UNIT_TEST


Why only mixer component needs these additions? Is there something in other components or their unit tests that helps to avoid to do these.

this is because mixer cmocka test need this API for test purpose.

btian1 · 2022-10-10T08:53:53Z

I changed commit message today, and
I am fine for commit message changing, @lgirdwood, do you think Chao's message is more clear to you and other developers? if you are ok, I can change it and make sure review can be passed smoothly.

Thanks
Tim

lgirdwood · 2022-10-10T10:43:59Z

I changed commit message today, and I am fine for commit message changing, @lgirdwood, do you think Chao's message is more clear to you and other developers? if you are ok, I can change it and make sure review can be passed smoothly.

@btian1 in these cases its always best in include the output before and after your change in your commit message. i.e. paste 2 lines of trace messages without your change and then 2 lines of trace with your change. This makes it really easy to see why it's needed as its difficult to understand why its been working so far (whilst it's "not supported" by XCC)

kv2019i

Looks almost ready to merge. Please see a few notes @btian1 inline.

src/audio/component.c

kv2019i · 2022-10-10T11:46:55Z

src/include/sof/audio/component_ext.h

-#endif
-
-		ret = dev->drv->ops.copy(dev);
+int comp_copy(struct comp_dev *dev);


It seems quite confusing indeed. Some prototypes like comp_get_copy_limits() are in component.h while others like comp_new() are in component_ext.h. Given other copy relate functions have prototypes in component.h, I'd put comp_copy() there as well. @ranj063 @juimonen do you happen to recollect the history?

btian1 · 2022-10-11T07:03:15Z

@kv2019i , I talked with Ranjani this afternoon, she have no concern put it to component_ext.h, you can double confirm with her later.

kv2019i

Thanks @btian1 , good now!

kv2019i · 2022-10-12T15:56:03Z

Ready to go but waiting for CI results.

RanderWang

LGTM

mmaka1 · 2022-10-13T09:55:53Z

src/audio/component.c

+
+	/* copy only if we are the owner of the component */
+	if (cpu_is_me(dev->ipc_config.core)) {
+#if CONFIG_PERFORMANCE_COUNTERS


There are empty definitions of perf_...() provided in the header for a configuration with CONFIG_PERFORMANCE_COUNTERS undefined, so it not necessary to use #if-s inside the caller's code. Also the code would be easier to maintain when number of #if-s is limited.

kv2019i · 2022-10-13T10:36:04Z

Similar CI error as with other PRs, and does not seem to be related. A force repush may be needed.

btian1 · 2022-10-14T11:54:10Z

done, force pushed again for this PR

On sof zephyr ipc4 build, component performance profiling logs can't be enabled due to current xcc compiler does not support inline logging in header file, logs as below: log_level undeclared (first use in this function) log_current_const_data undeclared (first use in this function) Move comp_copy to component.c can resolve this limitation. However, this brings another cmocka issue for mixer unit test, this patch also fixed cmocka mixer unit test issue. BugLink: zephyrproject-rtos/zephyr#43786 Signed-off-by: Baofeng Tian <[email protected]>

kv2019i · 2022-10-18T10:42:48Z

Now all good (except for known Intel IPC4 failure), going ahead with merge.

btian1 requested review from aiChaoSONG, singalsu, kv2019i and marc-hb September 28, 2022 02:55

btian1 force-pushed the compcopy branch from d426e4b to 606acf7 Compare September 28, 2022 03:17

btian1 force-pushed the compcopy branch from 606acf7 to 474a3e4 Compare September 28, 2022 05:54

btian1 marked this pull request as ready for review September 28, 2022 07:12

btian1 requested review from aborisovich, lgirdwood, plbossart, mmaka1, lbetlej and dbaluta as code owners September 28, 2022 07:12

btian1 force-pushed the compcopy branch from 474a3e4 to 58ce424 Compare September 28, 2022 08:52

btian1 mentioned this pull request Sep 28, 2022

sof performance build enabling #6322

Closed

lyakh reviewed Sep 28, 2022

View reviewed changes

btian1 force-pushed the compcopy branch 2 times, most recently from 742e5cf to 2f85de0 Compare September 28, 2022 13:11

lgirdwood reviewed Sep 28, 2022

View reviewed changes

ranj063 reviewed Sep 28, 2022

View reviewed changes

marc-hb reviewed Sep 28, 2022

View reviewed changes

btian1 force-pushed the compcopy branch from 2f85de0 to e85c7e1 Compare September 29, 2022 13:52

lgirdwood reviewed Oct 3, 2022

View reviewed changes

andrula-song approved these changes Oct 8, 2022

View reviewed changes

btian1 force-pushed the compcopy branch from e85c7e1 to 54de621 Compare October 10, 2022 01:50

aiChaoSONG reviewed Oct 10, 2022

View reviewed changes

singalsu reviewed Oct 10, 2022

View reviewed changes

kv2019i requested changes Oct 10, 2022

View reviewed changes

btian1 force-pushed the compcopy branch 3 times, most recently from 84fc720 to d48cfec Compare October 11, 2022 02:38

kv2019i approved these changes Oct 11, 2022

View reviewed changes

btian1 force-pushed the compcopy branch from d48cfec to ba862d1 Compare October 12, 2022 13:40

RanderWang approved these changes Oct 13, 2022

View reviewed changes

mmaka1 reviewed Oct 13, 2022

View reviewed changes

btian1 force-pushed the compcopy branch from ba862d1 to d62b074 Compare October 14, 2022 11:53

kv2019i merged commit 47c52cc into thesofproject:main Oct 18, 2022

component: move comp_copy from header file to source file #6344

component: move comp_copy from header file to source file #6344

Conversation

btian1 commented Sep 28, 2022 • edited Loading

marc-hb commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 commented Sep 28, 2022

lgirdwood left a comment

Choose a reason for hiding this comment

btian1 commented Sep 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marc-hb left a comment

Choose a reason for hiding this comment

btian1 commented Sep 29, 2022

marc-hb commented Sep 29, 2022 • edited Loading

btian1 commented Sep 29, 2022 • edited Loading

lgirdwood left a comment

Choose a reason for hiding this comment

btian1 commented Oct 8, 2022 • edited Loading

btian1 commented Oct 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aiChaoSONG commented Oct 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 commented Oct 10, 2022

lgirdwood commented Oct 10, 2022

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btian1 commented Oct 11, 2022

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i commented Oct 12, 2022

RanderWang left a comment

Choose a reason for hiding this comment

mmaka1 Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

kv2019i commented Oct 13, 2022

btian1 commented Oct 14, 2022

kv2019i commented Oct 18, 2022

btian1 commented Sep 28, 2022 •

edited

Loading

marc-hb commented Sep 29, 2022 •

edited

Loading

btian1 commented Sep 29, 2022 •

edited

Loading

btian1 commented Oct 8, 2022 •

edited

Loading

btian1 Oct 11, 2022 •

edited

Loading

mmaka1 Oct 13, 2022 •

edited

Loading