audio_stream: Fix uninitialized data in alignment_constants #8672
Conversation
andyross
force-pushed
the
alignconst-init
branch
from
7af73ea to
bc43097
Compare
See... I think that's just wrong. It's a design flaw to make components responsible for doing this, because by definition it requires coordination with the component on the other side of the buffer, which module code doesn't own and shouldn't have to know about. That's why I'm describing this as an "uninitialized data bug" -- the default stream after init doesn't work correctly. A better scheme would have the component provide "alignment requirements"[1] just after initialization (via a field in the module, API call, whatever), and have the pipeline setup code walk the graph, validate[2] the settings we got from the kernel, and set things up correctly such that the modules can just read/interpret them in prepare(). [1] And all other stream metadata, c.f. the linked bug. [2] Note that the topology layer is ambiguous here: stream formats in IPC4 are stored on the widget, but the buffer connects two widgets. So we're storing the same data twice, and it's possible to have the two settings disagree. AFAICT there's no code layer prepared to detect this. In fact the framework layer seems not to be inspecting some of the data (the "base_cfg_ext" struct) at all. |
|
This is certainly different in IPC4 versus IPC3. With IPC3, the kernel set sparams at the graph edges and FW iterates through the graph and propagates the parameters. With IPC4, it's the host that does the graph walk and sets the formats so that they match and sets the pin configuration explicitly for each pin (one format is set for each pin). So modules should not have to bother with this, they just have to follow what host tells them to do (given it gets the orders correctly, e.g. #8575 for multi pin case). It's possible to mix and match a bit and have e.g. FW do additional logic even when pin configurations come from the host, but so far we have not done a lot of this. |
|
@marcinszkudlinski wrote.
Please check existing uses of audio_stream_init_alignment_constants() like in EQ FIR. We can get rid of this, but then algorithms that have vector optimized loops, need to duplicate the logic to calculate avail alignments themselves. Algorithms that process sample by sample will not need this, but that's not the main audience for this interface. |
There was a problem hiding this comment.
Just to clarify my comment from above: @kv2019i is right and this can't merge in this form as there needs to be a layer that guarantees that alignment values are recomputed at the time they're needed (which includes other changes to frame size!). I have a quick mockup working, will get this reworked and submitted over the weekend.
I'd -1 too, but Github won't allow me to do that to my own patch :)
audio_stream_init_alignment_constants() isn't a particularly small function, isn't used in performance-sensitive contexts, and doesn't really belong in a header. Move to audio_stream.c for hygiene, and because it's about to be modified. Also move the depended-on function audio_stream_frame_align_get(), and (as it has no consumers outside of audio_stream) remove its declaration from the header. Signed-off-by: Andy Ross <[email protected]>
As specified, this function was a bit of a booby trap: it has to be called exactly once, after all other setup that modifies frame size has been done. If it is called in the wrong order, or not at all, any consumers of the frame alignment API on this stream will see garbage data in the computed fields. That's way too much complexity for the component code that needs to use this tool, especially as the impact is not in the component itself (e.g. it's a downstream copier widget with SIMD code that fails). Instead, preserve the two requirements set by this function in the audio_stream struct and recompute them any time any of the upstream values change. That allows this to be safely used from any context. There's a mild complexity with audio_sink/source layer, which is written to directly modify the format (it keeps a pointer into audio_stream of its own) without going through the audio_stream API itself. There, the framework gives us "on_audio_format_set()" callbacks on source and sink, so implement it there. This is a partial fix for Issue thesofproject#8639 Signed-off-by: Andy Ross <[email protected]>
andyross
force-pushed
the
alignconst-init
branch
from
bc43097 to
1cd8de6
Compare
|
OK, reworked version is up. The cleanup patch is identical, but the alignment constant handling is now modified to be a lazy-evaluated version that can be called safely at any time. Please re-review. |
|
And.... of course the rename patch crossed with new work in main that isn't in mtl-007-drop-stable and so isn't directly testable vs. this work. I'm getting pretty fed up with having to work across branches like this. Will fix. |
andyross
force-pushed
the
alignconst-init
branch
2 times, most recently
from
5158b00 to
b73a42a
Compare
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]>
After recent changes, the audio_stream_init_alignment_constants() routine isn't an "init" step anymore, it sets requirements and can be called at any time. Rename it to "audio_stream_set_align()" to better capture its behavior, and rework the documentation to make it clearer how it works. Signed-off-by: Andy Ross <[email protected]>
andyross
force-pushed
the
alignconst-init
branch
from
b73a42a to
541fac3
Compare
@andyross you can convert to draft (right below reviewers list). Would also prevent submission. |
|
@andrula-song @singalsu please recheck |
|
Of the failing CI cases, https://sof-ci.01.org/sofpr/PR8672/build1656/devicetest/index.html is a known fail with system PM and can be ignored. https://sof-ci.01.org/sofpr/PR8672/build1655/devicetest/index.html has a fail on MTL I can't explain (no direct connection to the PR, but also it is suspect to have this fail). Let's see how the other CI checks fair. |
PR was reworked from the version described in the initial version. See comments below and commit message. This text is stale:
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 #8639