-
Notifications
You must be signed in to change notification settings - Fork 318
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
ipc4: mixin/mixout: Switch to use sink/source API #8537
ipc4: mixin/mixout: Switch to use sink/source API #8537
Conversation
68acb14
to
58ca058
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
later, we can try with hifi4 instructions to optimize those buffer wrappers.
break; | ||
default: | ||
comp_err(dev, "unsupported data format %d", fmt); | ||
return -EINVAL; | ||
} | ||
|
||
if (!md->normal_mix_channel || !md->mute_channel) { | ||
if (!md->normal_mix_channel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggests that md->mute_channel
was never NULL
, which is probably correct
uint16_t sink_index, struct audio_stream *sink, | ||
uint32_t start_frame, uint32_t mixed_frames, | ||
const struct audio_stream *source, uint32_t frame_count) | ||
static int mix(struct comp_dev *dev, const struct mixin_data *mixin_data, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker, but if you do end up doing another round - maybe you could come up with a longer and more unique name, I guess grepping for "mix" might be a bit difficult...
if (!mixout_data->acquired_buf.ptr) { | ||
struct sof_sink *sink = mixout_mod->sinks[0]; | ||
uint32_t free_bytes = sink_get_free_size(sink); | ||
uint32_t buf_size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these byte counts should really be size_t
- we're getting 64-bit architectures already, no need to punish them with 32-bit arithmetics. Throughout the code of course, not just here
static inline void cir_buf_set_zero(void *ptr, void *buf_addr, void *buf_end, uint32_t bytes) | ||
{ | ||
uint32_t head_size = bytes; | ||
uint32_t tail_size = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here too of course - size_t
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I replace those uint32_t with size_t in this PR or that will be done later as part of global update through all code base? That new cir_buf_set_zero() function is mostly a copy-paste of audio_stream_set_zero() located just above it. Those uint32_t came as a copy-paste from audio_stream_set_zero().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't critical either, but it's my personal opinion - I do think that size_t
is a better type for these variables. I won't block this PR if you don't and probably nobody else would (but I might be wrong), but I think it would be a good idea to do the change at least in locations where it wouldn't mean changing any global APIs, like for local variables and parameters of static functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mixin_mixout.c line 593:
list_for_item(blist, &dev->bsink_list) {
don't use dev->bsink_list and dev->bsource_lists
don't assume comp_buffer is used
In general the module should not use any API od audio_stream or comp_buffer because it may be connecxted to something else.
All format settings like audio_stream_set_channels should be changed to sink/src functions (like sink_set_channels/source_set_channels)
58ca058
to
ac78e05
Compare
Done. Please re-review. |
|
||
/* comp_verify_params() does not modify valid_sample_fmt (a BUG?), let's do this here */ | ||
audio_stream_fmt_conversion(mod->priv.cfg.base_cfg.audio_fmt.depth, | ||
mod->priv.cfg.base_cfg.audio_fmt.valid_bit_depth, | ||
&frame_fmt, &valid_fmt, | ||
mod->priv.cfg.base_cfg.audio_fmt.s_type); | ||
|
||
audio_stream_set_valid_fmt(&sink->stream, valid_fmt); | ||
audio_stream_set_channels(&sink->stream, params->channels); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just in case, for eagle-eye reviewers :) , the removal of line with audio_stream_set_channels() is intentional. Channels number is already set above by comp_verify_params(), no need to set it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a pretty massive change. Codewise this seems pretty ok,.
I do note a couple of fails in CI:
- https://sof-ci.01.org/sofpr/PR8537/build638/devicetest/index.html
- https://sof-ci.01.org/sofpr/PR8537/build637/devicetest/index.html
Multiple pipeline checks seem to be failing, but in quite strange way (no errors in FW log, just wrong status reported at end of tests). But definitely fails across large set of DUTs, so needs to be clarified before can be merged.
src/include/ipc4/mixin_mixout.h
Outdated
void *buf_end; | ||
void *ptr; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have cir_buf_wrap() in audio_stream.h, should we have the "cir_" helpers in some common header as well. Seems pretty generic and not limited to mixinmixout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best place is https://github.com/thesofproject/sof/blob/main/src/audio/sink_source_utils.c however we should use it also in loadable modules.
* enum sof_ipc_frame describes both container and sample size and so having one | ||
* variable of this type is enough. frame_fmt is set by comp_verify_params(), however, | ||
* valid_sample_fmt does not. Hence valid_sample_fmt is set below in a loop. If mess | ||
* with having both frame_fmt and valid_sample_fmt in SOF is solved when this loop can |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit scary comment :) but yeah, this is is true. The distinction of container and valid bits was added afterwards to "enum sof_ipc_frame" and as the original definition is part of the IPC interface, IPC4 concepts were added on top. I don't think we need to drag this along all over SOF codebase though. So it should be possible to set/fmt fmt with a single interface. But alas, this is a cleanup for another PR.
src/include/ipc4/mixin_mixout.h
Outdated
void *buf_end; | ||
void *ptr; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
best place is https://github.com/thesofproject/sof/blob/main/src/audio/sink_source_utils.c however we should use it also in loadable modules.
This @RanderWang commit 1855581 fixes the problems reported by CI. Verified on this draft PR #8576. So let's wait for Rander's PR #8580 to be merged (or that commit moved to separate PR if that speeds up merging). |
@RanderWang probably best to make the fix a new PR (without smart amp) as it will merge faster. |
@lgirdwood @serhiy-katsyuba-intel can you help to check this PR ? #8594. Thanks! |
I've cherry-picked commits from your PR and added to this draft PR https://github.com/thesofproject/sof/pull/8576/commits to be verified by CI. There is mixin/mixout re-implementation there which uses sink/source API and so CI will test mixout with multiple sources case (previously it failed without your fix). |
ac78e05
to
0230d97
Compare
Rebased to latest main. |
@serhiy-katsyuba-intel some build errors with stricter host compilation, it picked out some pointer mismatches. See ALSA plugin build in CI.
|
0230d97
to
f45ef06
Compare
Fixed. |
@wszypelt , Internal Intel CI seems stuck. |
@serhiy-katsyuba-intel Thanks for the info, I added it to the queue, the results should be available within 40 minutes |
@wszypelt , when I click on a failed Internal Intel CI System/merge/build -- blank page is shown. How can I find out what (tests?) have failed? |
@serhiy-katsyuba-intel Unfortunately, at the moment, the results are only available at the internal service |
@serhiy-katsyuba-intel btw, if it makes it easier, we could split the PR and merge the simpler patches 1st that pass CI whilst the larger more complex patches are being fixed to pass CI ? |
SOFCI TEST |
Temporary put to draft to investigate CI failure. Extracted cleanup commits to separate PR #8655. |
CI failure is caused by a recent regression introduced by #8594 Previously sinks/sources were setup in .prepare(), after that PR they are setup in .bind() and .unbind(). However, module_adapter_reset() also cleans sinks and sources arrays and set num_of_sources = num_of_sinks = 0. The failed test is a stress test which does many iterations of PAUSE -> RESET -> RUN. First iteration works fine, second iteration leads to DSP panic, as sinks and sources array are cleared on RESET and were never setup back (as they can only be setup on .bind() and .unbind()). ACHTUNG: So if topology uses some module which utilizes sink/source API, after RESET such module will not work properly (or just crash)! cc: @RanderWang , @marcinszkudlinski, @lgirdwood. |
PR with the fix for CI problem: #8657. |
Modifications to mixin/mixout .process() and .reset() implementations to switch to use sink/source API. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
In channel remapping mode mixin sinks could have different number of channels. Since channel remapping mode has been removed, no need to support individual channel number setup for each sink. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
The removed code may falsely fail for freq like 44.1 kHz. Also, we generally do not check for sufficient buffer size in module .prepare() handler. That should be/is done elsewhere. No need to do the exception for mixout component. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
Modification of mixin/mixout .prepare() handler to use sink/source API. Signed-off-by: Serhiy Katsyuba <serhiy.katsyuba@intel.com>
@serhiy-katsyuba-intel now merged, can you rebase. Thanks ! |
f45ef06
to
844cc62
Compare
Rebased. CI has finished with kind of success: timeout on 2 cavs tests. |
The system PM timeouts in https://sof-ci.01.org/sofpr/PR8537/build1400/devicetest/index.html can be ignored, not related to this PR. Proceeding with merge. |
Modifications to mixin/mixout implementation to switch to use sink/source API.