thesofproject
[BUG] iterating pipelines with `pipeline_for_each_comp()` can unnecessarily abort early
Describe the bug
pipeline_for_each_comp() is used on multiple code paths to iterate pipeline components and to scan connected pipelines recursively. It uses buffer .walking flags to prevent returning to already scanned paths. However that flag seems to have side effects and in fact might not be needed at all.
First side effect of that flag occurs when using IPC to update stream position, e.g. on platforms like BYT and BDW, where this is the only available method. Sometimes the position update IPC arrives during the copy operation, in which case the respective buffer will have its .walking flag set. That is detected by the timestamp updating routing and the update is aborted. Removing that condition doesn't seem to have any adverse effects.
Another side effect appears during pipeline_comp_params() execution. That function calls pipeline_for_each_comp() twice - once for its actual recursion and once for parameter negotiation with components in the direction, opposite to the data flow. It actually seems to eliminate the negotiation effect. E.g. for playback if that function is entered for the second component in the pipeline from the recursion, its upstream buffer will have its .walking flag set. So an attempt to walk in that direction will fail immediately. Only the first entry to that function will be able to walk upstream, not any of the recursion calls. Is this intentional?
@lyakh we do need a flag to indicate "I've been here before" to avoid infinite loops. Fwiw, the WM9713 codec has such a loop in the audio topology so not this is a real use case.
@lgirdwood I cannot claim that I fully understand what the present code is trying to achieve with the .walking flag, but it is my impression that it's exactly what it is there for. The original commit 968eaf162156 ("pipeline: add a flag to indicate if the buffer is being traversed") doesn't explain a lot apart from saying - don't walk the same buffer twice. But there's no explanation as to which cases it is addressing. So the questions are: which cases is this flag handling? If it is handling cases like WM9713 that you mentioned, then we have to re-use and fix it. If it's handling different cases, we have to find out which ones, and fix it to only handle those cases and not break others.
The primary aim is not to walk the same path more than once, this also avoids loops. Feel free to update and make this clearer.
Preventing pipeline parameter negotiation from walking the pipeline backwards to where we came from seems indeed intentional, negotiation is only supposed to check other branches on multi-source or multi-sink components.
As explained above, .walking is used in cases like on a mixer, when an audio format is configured on one of branches, we will walk all other branches to check format compatibility, then .walking makes sure we don't attempts to check with the current path.