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

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

Merged
merged 1 commit into from
Oct 18, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/audio/component.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,3 +234,27 @@ void audio_stream_copy_to_linear(const struct audio_stream __sparse_cache *sourc
snk += bytes_copied;
}
kv2019i marked this conversation as resolved.
Show resolved Hide resolved
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

/** See comp_ops::copy */
int comp_copy(struct comp_dev *dev)
{
int ret = 0;

assert(dev->drv->ops.copy);

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

@mmaka1 mmaka1 Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

perf_cnt_init(&dev->pcd);
#endif

ret = dev->drv->ops.copy(dev);

#if CONFIG_PERFORMANCE_COUNTERS
perf_cnt_stamp(&dev->pcd, comp_perf_info, dev);
perf_cnt_average(&dev->pcd, comp_perf_avg_info, dev);
#endif
}

return ret;
}
5 changes: 4 additions & 1 deletion src/audio/mixer/mixer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,10 @@ static const struct comp_driver comp_mixer = {
},
};

static SHARED_DATA struct comp_driver_info comp_mixer_info = {
#ifndef UNIT_TEST
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Expand Down
29 changes: 1 addition & 28 deletions src/include/sof/audio/component_ext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,35 +171,8 @@ static inline int comp_prepare(struct comp_dev *dev)
return 0;
}

/** See comp_ops::copy */
static inline int comp_copy(struct comp_dev *dev)
{
int ret = 0;

assert(dev->drv->ops.copy);

/* copy only if we are the owner of the component */
if (cpu_is_me(dev->ipc_config.core)) {
/* BugLink: https://github.com/zephyrproject-rtos/zephyr/issues/43786
* TODO: Remove this once the bug gets fixed.
*/
#ifndef __ZEPHYR__
perf_cnt_init(&dev->pcd);
#endif

ret = dev->drv->ops.copy(dev);
int comp_copy(struct comp_dev *dev);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should be in component.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@btian1 btian1 Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.


/* BugLink: https://github.com/zephyrproject-rtos/zephyr/issues/43786
* TODO: Remove this once the bug gets fixed.
*/
#ifndef __ZEPHYR__
perf_cnt_stamp(&dev->pcd, comp_perf_info, dev);
perf_cnt_average(&dev->pcd, comp_perf_avg_info, dev);
#endif
}

return ret;
}

/** See comp_ops::get_attribute */
static inline int comp_get_attribute(struct comp_dev *dev, uint32_t type,
Expand Down
1 change: 1 addition & 0 deletions src/include/sof/audio/mixer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#ifdef UNIT_TEST
void sys_comp_mixer_init(void);
extern struct comp_driver_info comp_mixer_info;
#endif

#define MIXER_GENERIC
Expand Down
4 changes: 4 additions & 0 deletions test/cmocka/src/audio/buffer/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ cmocka_test(buffer_copy
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)

cmocka_test(buffer_new
Expand All @@ -27,6 +28,7 @@ cmocka_test(buffer_new
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)

cmocka_test(buffer_wrap
Expand All @@ -42,6 +44,7 @@ cmocka_test(buffer_wrap
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)

cmocka_test(buffer_write
Expand All @@ -57,4 +60,5 @@ cmocka_test(buffer_write
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)
1 change: 1 addition & 0 deletions test/cmocka/src/audio/mixer/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
cmocka_test(mixer
mixer_test.c
comp_mock.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
${PROJECT_SOURCE_DIR}/test/cmocka/src/notifier_mocks.c
${PROJECT_SOURCE_DIR}/src/audio/buffer.c
${PROJECT_SOURCE_DIR}/src/audio/mixer/mixer.c
Expand Down
2 changes: 1 addition & 1 deletion test/cmocka/src/audio/mixer/comp_mock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,6 @@ static struct comp_driver_info comp_mock_info = {

void sys_comp_mock_init(void)
{
comp_register(&comp_mock_info);
mock_comp_register(&comp_mock_info);
}

1 change: 1 addition & 0 deletions test/cmocka/src/audio/mixer/comp_mock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@
#define SOF_COMP_MOCK ((uint32_t)-1)

void sys_comp_mock_init(void);
int mock_comp_register(struct comp_driver_info *info);
10 changes: 8 additions & 2 deletions test/cmocka/src/audio/mixer/mixer_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ struct comp_dev *post_mixer_comp;
struct comp_buffer *post_mixer_buf;

/* Mocking comp_register here so we can register our components properly */
int comp_register(struct comp_driver_info *info)
int mock_comp_register(struct comp_driver_info *info)
{
void *dst;
int err;
Expand All @@ -63,6 +63,12 @@ int comp_register(struct comp_driver_info *info)
return err;
}

static void ut_comp_mixer_init(void)
{
mock_comp_register(platform_shared_get(&comp_mixer_info,
sizeof(comp_mixer_info)));
}

struct source {
struct comp_dev *comp;
struct comp_buffer *buf;
Expand Down Expand Up @@ -180,7 +186,7 @@ static void activate_periph_comps(struct mix_test_case *tc)

static int test_group_setup(void **state)
{
sys_comp_mixer_init();
ut_comp_mixer_init();
sys_comp_mock_init();

return 0;
Expand Down
6 changes: 6 additions & 0 deletions test/cmocka/src/audio/pipeline/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ endif()

cmocka_test(pipeline_new
pipeline_new.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc3/helper.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc-common.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc-helper.c
Expand All @@ -29,11 +30,13 @@ cmocka_test(pipeline_new
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)

cmocka_test(pipeline_connect_upstream
pipeline_connect_upstream.c
pipeline_connection_mocks.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc3/helper.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc-common.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc-helper.c
Expand All @@ -44,11 +47,13 @@ cmocka_test(pipeline_connect_upstream
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)

cmocka_test(pipeline_free
pipeline_free.c
pipeline_connection_mocks.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc3/helper.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc-common.c
${PROJECT_SOURCE_DIR}/src/ipc/ipc-helper.c
Expand All @@ -59,4 +64,5 @@ cmocka_test(pipeline_free
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)
1 change: 1 addition & 0 deletions test/cmocka/src/audio/selector/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ add_library(audio_for_selector STATIC
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)
sof_append_relative_path_definitions(audio_for_selector)

Expand Down
1 change: 1 addition & 0 deletions test/cmocka/src/audio/volume/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ add_library(audio_for_volume STATIC
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-schedule.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)
sof_append_relative_path_definitions(audio_for_volume)

Expand Down
1 change: 1 addition & 0 deletions test/cmocka/src/math/fft/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ cmocka_test(fft
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-stream.c
${PROJECT_SOURCE_DIR}/src/audio/pipeline/pipeline-xrun.c
${PROJECT_SOURCE_DIR}/test/cmocka/src/common_mocks.c
${PROJECT_SOURCE_DIR}/src/audio/component.c
)