thesofproject
replace PAUSE / RELEASE with STOP / START
Currently many components, e.g. DAIs handle STOP and PAUSE differently. Also the mixer component blocks PAUSE while obviously accepting and propagating STOP. This creates a lot of complexity in forking pipelines. With START, STOP, PAUSE and RELEASE now handled in the pipeline task context, it should be possible to handle PAUSE and RELEASE exactly like STOP and START.
FIXME: the last commit breaks several things:
- it adds an unbalanced
notifier_unregister() - it probably breaks (again) the now perfectly correct HDA DMA stop flow
All of those must be fixed before merging this
@lyakh @lgirdwood @kv2019i @ranj063 I would recommend more high-level discussions on what the purpose of PAUSE/RELEASE is at the firmware level, compared to STOP/START. The entire state machine is somewhat odd in that we don't currently have a 'DRAINING' state on stop, so before we equate PAUSE/STOP, we should think a bit more on the implications. Also PAUSE is a fundamental concept in the Intel HDaudio stuff, we have to stop in two steps with an intermediate pause before a complete stop, so we should be cautious.
My 2 cents post winter break.
My two cents: How does this affect pausing decoding a compressed stream? Would a stop/start work correctly (in the sense that stop won't discard the temporary state of the decoder)? And actual stopping would also do a reset which does discard said temporary state or something?
@plbossart @ranj063 @kv2019i @lyakh the general plan is a large simplification and code reduction around our pipeline and component logic. The IPC 3/4 host interface will not change. PAUSE == STOP and RELEASE == START.
Today all streams are stopped and paused at the next pipeline tick after the STOP IPC. This mean that the pipeline stops the DAI presentation at the next DAI DMA block boundary (i.e. its at clean DMA/cache boundaries). This is mostly the same flow as pause today (except pause has a lot more extra state machine logic). There are some pause use case specific corner cases too that need to be simplified or updated as part of this work.
The HDA state machine around stopping wont change and neither should any host IPC flows. We may need a little abstraction there to preserve any host flows but this will be a lot smaller than the code being removed..
@paulstelian97 start/stop should not reset the processing state machine. reset only should do this, we have a feature request for aligning this that I will self assign for 2.1 or 2.2
@paulstelian97 start/stop should not reset the processing state machine. reset only should do this, we have a feature request for aligning this that I will self assign for 2.1 or 2.2
The problem is that 'reset' doesn't map to any ALSA definitions, and we're still missing the nuance between pause, stop (in the drop sense) and drain.
I am all for simplifications but if we don't have a clear view of the end goal we'll redo the work several times.
@paulstelian97 start/stop should not reset the processing state machine. reset only should do this, we have a feature request for aligning this that I will self assign for 2.1 or 2.2
The problem is that 'reset' doesn't map to any ALSA definitions, and we're still missing the nuance between pause, stop (in the drop sense) and drain.
I am all for simplifications but if we don't have a clear view of the end goal we'll redo the work several times.
partial drain is btw a very important feature for compressed data, it's the basis for gapless playback.
@paulstelian97 start/stop should not reset the processing state machine. reset only should do this, we have a feature request for aligning this that I will self assign for 2.1 or 2.2
The problem is that 'reset' doesn't map to any ALSA definitions, and we're still missing the nuance between pause, stop (in the drop sense) and drain.
Yeah, ALSA PCM ops dont have a reset, but I see it as
-
Alsa pause -> IPC STOP
-
Alsa stop -> IPC STOP then IPC RESET.
I am all for simplifications but if we don't have a clear view of the end goal we'll redo the work several times.
partial drain is btw a very important feature for compressed data, it's the basis for gapless playback.
drain is a new case, and it should be added after the simplification work so we dont have to deal with pause and drain state machine transitions.
Sounds good, thanks. Drain is probably good as a distinct state anyway.
I tested this PR locally on a MinnowBoard BYT platform and with it the system locks up hard immediately when "release" is pressed. Debugging
In this version the first patch completely disables PAUSE and RELEASE on legacy ADSPs, the second patch actually replaces the former with the latter, and I let out the HDA patch this time to remind myself what failures it was fixing.
Debugging BYT failures with the previous version resulted in a discovery of a hard system lock up during dma_start() for the host component, when trying to resume after the pause. The major difference with cAVS platforms is, that legacy platforms use DMA scheduling. It should be verified whether this PR causes problems on other DMA-domain platforms @dbaluta
Taking the https://sof-ci.01.org/sofpr/PR5171/build745/devicetest/?model=ADLP_RVP_NOCODEC_ZEPHYR&testcase=check-pause-resume-playback-10 CI failure as an example. This seems to be the same problem as the one on legacy platforms, which is why the respective commit in this series is required. Let me try with the HDA patch now.
@lyakh CI does look good given the Kconfig default in the current PR is yes (to treat PAUSE as STOP)
@lyakh lets figure out the driver changes so that the FW changes are 99% code removal.
@lgirdwood so we should block PAUSE / RELEASE at the driver level? But since IPCs exist in both IPC3 and IPC4 I think the firmware still should be able to handle them, e.g. when loaded with an older kernel. So we can already just modify the firmware to return errors to those IPCs, can we not?
