Repository navigation
pipeline: fix aborting STOP or PAUSE propagation - #5374
Conversation
|
Addresses #5257 |
When trigger.aborted is set, pipeline_trigger_run() also returns PPL_STATUS_PATH_STOP. So the current test for trigger.aborted in pipeline_task_cmd() after pipeline_trigger_run() is a left-over from an earlier implementation and is never entered with the current flow. In the current implementation, a typical playback topology like [pipeline A] PCM -> ... - [pipeline B] mixer -> ... -> DAI when a pause is triggered mixer will stop its propagation, set trigger.aborted on pipeline B to true and return PPL_STATUS_PATH_STOP. pipeline_task_cmd() is still running in the context of pipeline A, so it will detect PPL_STATUS_PATH_STOP, check that trigger.aborted is not set and terminate the task. Then the scheduler will run the task for pipeline B, that one will verify, that trigger.host == NULL, then find the trigger.aborted to be true and will keep the task running. In a topology, where the PCM, the mixer and the DAI all belong to the same pipeline, when PAUSE is triggered, the whole pipeline should be left running which is what this patch achieves by checkint trigger.aborted when PPL_STATUS_PATH_STOP is returned by pipeline_trigger_run(). BugLink: thesofproject#5257 Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
|
SOFCI TEST |
|
@lyakh I'll give it a double test here to be sure. |
|
we're lucky today. This https://sof-ci.01.org/sofpr/PR5374/build12049/devicetest/?model=APL_UP2_NOCODEC_ZEPHYR&testcase=multiple-pipeline-capture CI failure is identical to https://sof-ci.01.org/linuxpr/PR3409/build7157/devicetest/?model=APL_UP2_NOCODEC_ZEPHYR&testcase=multiple-pipeline-playback also described in #4414 |
|
also ran an internal custom CI test-plan on this PR nr. 10253 to actually test on an UP2-PCM512x, all tests passed. |
|
Impressive find! I'm wondering about test coverage. Is UP2 with pcm512x the only configuration where we have this:
|
We used to have that in all nocodec topologies, then that was changed. We might have a couple more such topologies, or maybe not, I'm not sure I've seen any others like that. OTOH QuickBuild pipelines do that too |
|
@lyakh we don't want to have host and dai in the same pipeline, there's a reason why we changed all the nocodec, hdaudio and SoundWire topologies. Same for cAVS. |
@plbossart hm, wait, I knew you were for splitting pipelines with mixers, mux, demux components in them, but you don't want any pipelines including host and DAI in them? Even without such components? E.g. I just looked at a generic HDA topology in test results of this PR https://sof-ci.01.org/sofpr/PR5374/build12049/devicetest/?model=CML_SKU0955_HDA&testcase=check-capture-10sec and I see simple pipelines with no mixers in them, connecting a host and a PCM - almost all playback pipelines, not to mention capture pipelines. |
|
@lyakh a mixer should be the start of a pipeline. that's how the cAVS firmware works, so not sure what our esteemed firmware friends are doing. in the topology2 we have mixout (end of a pipeline) and mix-in (start of a new one). The mixer in the middle of a pipeline is something that was used in Broadwell, and for his historical reason copy/pasted despite my concerns. It doesn't scale. Now it's time to say game over. See what other OSes are doing with hierarchical mixers here: https://docs.microsoft.com/en-us/windows-hardware/drivers/audio/audio-processing-object-architecture In addition, there's a well-known conceptual bug in SOF where the mixers need to use their own rate, channel map and depth, and not use hw_params/configs that came from sources. @lgirdwood that's something that should be tracked in Jira :-) |
Thanks, Let me track this. |
|
@plbossart thanks for the additional information. I don't have a strong opinion one way or another - whether "embedded" mixers should be considered valid and supported, but I think we should make a design decision and follow it throughout: either we support this option, then we add more such topologies to our testing and make sure they work, or we don't support it, then we remove all such existing topologies and officially declare them invalid and unsupported. And also detect them in the firmware and error out in such cases. @lgirdwood can such a decision be made and implemented? |
|
option b) we don't support this option and convert existing topologies to what we intend on supporting. |
@plbossart Is this because of Microsoft compatibility? @lgirdwood suggested that there should be a public Microsoft document to this respect, do you happen to have a link to it? |
|
@plbossart thanks! |
I didn't understand this, thanks @plbossart for explaining it to me off-line. I couldn't see the difference between:
Answer:
One of the main purposes of this rule is to avoid active/inactive "conflicts" inside the same pipeline. Another one is:
|
When trigger.aborted is set, pipeline_trigger_run() also returns PPL_STATUS_PATH_STOP. Move the aborted test to the correct if-branch.