[mtl-007] AEC fixes from main branch submission #8603
Conversation
andyross
requested review from
a team,
lgirdwood,
plbossart,
mmaka1,
lbetlej,
dbaluta and
kv2019i
as code owners
andyross
requested review from
marc-hb,
dabekjakub,
ranj063 and
jxstelter
as code owners
| @@ -94,4 +94,7 @@ CONFIG_PROBE=y | |||
| CONFIG_PROBE_DMA_MAX=2 | |||
| CONFIG_KCPS_DYNAMIC_CLOCK_CONTROL=y | |||
| CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING=y | |||
| CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_NUM_CHANNELS=2 | |||
| CONFIG_GOOGLE_RTC_AUDIO_PROCESSING_MIC_BITS=32 | |||
|
|
|||
There was a problem hiding this comment.
Please review in #8571 . This PR is just a place to deliver the same code against mtl-007-drop-stable.
There was a problem hiding this comment.
Oh, actually this patch got dropped from that branch somehow! Will fix. But when I do please review there :)
| @@ -1088,9 +1088,6 @@ static int module_adapter_copy_dp_queues(struct comp_dev *dev) | |||
| struct sof_source *data_src = dp_queue_get_source(dp_queue); | |||
| size_t dp_data_available = source_get_data_available(data_src); | |||
|
|
|||
| if (!dp_data_available) | |||
There was a problem hiding this comment.
this error is a real error - if DP processing is working it will never appear, even during pipelie startup
kv2019i
changed the title
| Object.Control.bytes."1" { | ||
| access [ | ||
| tlv_write | ||
| tlv_callback |
There was a problem hiding this comment.
is callback a kernel or userspace permission?
There was a problem hiding this comment.
This is a cherry pick from main. And "access" in this context refers (I think!) to the API variant used to access the control's data. "callback" is what needs to be used for large TLV blocks because they can't be sent in a single IPC command.
| #define CHAN_MAX CONFIG_COMP_GOOGLE_RTC_AUDIO_PROCESSING_CHANNEL_MAX | ||
|
|
||
| static __aligned(PLATFORM_DCACHE_ALIGN) | ||
| float refoutbuf[CHAN_MAX][NUM_FRAMES]; |
There was a problem hiding this comment.
not a fan of having these here long term. Should we want to start supporting >1 AEC instance (e.g. on HS) this will not end well.
There was a problem hiding this comment.
Please review the main PR. This is just a rebase on top of the device branch for testing. But to answer: Trying to grab half the heap all at once (which is literally what we were doing here) doesn't really scale either. It's just not a great situation, but static allocation in firmware is widely held to be the more reliable. SOF is a little unique in being heavily dynamic, which works fine when you're looking at allocating individual 4-8k buffers. AEC wants 200k by default and last I heard Lionel was saying 390k would be better.
Longer term we could try to integrate the AEC internal allocation with the external heap better (there are hooks already but they only work for overflow use), which might be the best choice overall.
| struct sof_source *ref, | ||
| struct sof_source *mic, | ||
| struct sof_sink *out) | ||
| static ALWAYS_INLINE float s16_to_float(const char *ptr) |
There was a problem hiding this comment.
Why are we not using the macros here?
There was a problem hiding this comment.
(Again, please continue review in the main PR, but answering here):
This was discussed earlier: Those generate horrifyingly pessimal code (runtime division!) and can't be fixed without breaking the ABI (because as specified they work for 64 bit values). These seem like they're intended for generating compile time constants for fixed point use. There's also a pcm_convert API elsewhere in SOF, but that's designed to work in a block on linear arrays of samples (and don't expose inlinable single-sample conversions) where here we need to unify de/interleaving with the conversion.
Basically: SOF just doesn't have an API for this. I could move this somewhere else and try to unify with an existing library.
This reverts commit f24e966.
… extension" This reverts commit f9f0ce3.
This is a weak stub for the Cadence libc's allocator (which is just a newlib build). It's traditionally been provided like this in SOF for the benefit of C++ code where the standard library needs to link to a working malloc() even if it will never call it. Longer term this should be integrated as a working allocator, either unified with the one here or in the Zephyr libc layer. Zephyr already provides a newlib-compatible _sbrk_r(), we just have to tell it to use it when linking against Cadence libc. Signed-off-by: Andy Ross <[email protected]>
Unbreak the Zephyr build when this is enabled, and add the needed bits to produce a working executable. This is mostly just a recapitulation of the existing integration, which means that it's manually pulling in bits from the Cadence toolchain it needs. SOF isn't yet using the Zephyr C++ integration (which isn't xt-clang aware yet), nor does it really want to as SOF itself includes no such code. Zephyr doesn't have a "C++ binary linkage only" feature yet. Signed-off-by: Andy Ross <[email protected]>
Audio stream objects are generally allocated out of zero'd heap memory, but the computed alignment fields need non-zero values to have correct behavior. These were being left as garbage. Set them to a byte and frame alignmnt of 1, as this corresponds to pervasive convention in the tree. But note that a byte alignment of 1 is actually technically incorrect, as all existing DSP targets are on architectures which don't allow misaligned loads. But existing component code is universally coded correctly anyway. (It's worth pointing out that "set_alignment_constants()" is now exposed as an API call on abstracted source/sink objects. But the only current implementation is on audio_stream. Future implementations will need to correctly initialize themselves.) This is a partial fix for Issue thesofproject#8639 Signed-off-by: Andy Ross <[email protected]>
Traditionally audio_stream has failed to initialize its computed alignment fields, forcing components to do this themselves even when they don't actually have special alignment requirements. Remove all the code that was merely setting default values, leaving only a handful of spots with specialr equirements (e.g. eq/area need to treat pairs of samples, a few others have HiFi-optimized variants which need SIMD alignment). Signed-off-by: Andy Ross <[email protected]>
The kernel-provided "base_cfg_ext" data wasn't being handled correctly by the module adapter layer. These structs (packed wire formats) only ever lived in the IPC memory. The module would set them on an "init_data" before calling into driver init, and then clear that poitner afterwards. That's a critical problem, because the values in that struct need to be used at pipeline setup time to configure buffer formats! Save the data correctly. Also pre-parse it so users don't need to do byte math on raw buffers and can just use "in/output_pins[]" arrays on the module_config struct. Signed-off-by: Andy Ross <[email protected]>
The code here never really worked right. The module base config values weren't saved by the core layers, so querying the attribute returned nothing. A few components have been modified with a whitebox workaround where they write directly to the basecfg_ext field in the module_config, but that's not really an API we should want to support. Get the values from the now-correctly-saved-and-parsed fields the module init layer has left for us, with considerable code savings and much less heap thrashing for the temporary copy. Signed-off-by: Andy Ross <[email protected]>
Squashed fixups to this code from thesofproject#8571 Signed-off-by: Andy Ross <[email protected]>
This is down to just one line now with existing (still in review) PRs applied
|
Updated with current versions of all my in-review PRs, unifying the AEC work in main with the MTL branch. |
|
Close this one. Main has now caught up with 007-drop-stable and this works directly on top of it. No need to maintain the separate branch anymore. |
Squashed changes from #8571 , rebased on top of the mtl-007-drop-stable branch so we can fix the divergences in this code.
Review there. Not here.