Thanks Takashi for the review. >> This patch provides both a simplification of the suspend flows and a >> better balanced operation during suspend/resume transition. >> >> The exiting code relies on a convoluted way of dealing with suspend >> signals. Since there is no .suspend DAI callback, we used the >> component .suspend and marked all the component DAI dmas as >> 'suspended'. The information was used in the .prepare stage to >> differentiate resume operations from xrun handling, and only >> reinitialize SHIM registers and DMA in the former case. >> >> While this solution has been working reliably for about 2 years, there >> is a much better solution consisting in trapping the TRIGGER_SUSPEND >> in the .trigger DAI ops. The DMA is still marked in the same way for >> the .prepare op to run, but in addition the callbacks sent to DSP >> firmware are now balanced. >> >> Normal operation: >> hw_params -> intel_params_stream >> hw_free -> intel_free_stream >> >> suspend -> intel_free_stream >> prepare -> intel_params_stream >> >> This balanced operation was not required with existing SOF firmware >> relying on static pipelines instantiated at every boot. With the >> on-going transition to dynamic pipelines, it's however a requirement >> to keep the use count for the DAI widget balanced across all >> transitions. > > The trigger callback is handled in the stream lock atomically, and are > you sure that you want to operate a possibly heavy task there? It's a good objection that we didn't think of. I guess it depends on the definition of 'heavy' and what is acceptable in such a 'trigger' context. The intel_free_stream() routine only sends an IPC to the firmware to release the DMA resources. It doesn't perform any memory allocation tasks at the kernel level, it only sends information to the firmware that the DMA can be stopped/released. We could trace how much time that really means but I don't expect it to be 'long'. I also don't think the IPC waits for the DMA to be actually stopped/released, the IPC completes when the message is acknowledged with the doorbell registers (I will double-check this point). It's really similar to all the existing IPCs sent in trigger context, we already send an IPC for ALL trigger commands such as START/STOP/PAUSE_PUSH/PAUSE_RELEASE. see e.g. sof_pcm_trigger() in sound/soc/sof/pcm.c What is needed for dynamic pipelines is the ability to deal with suspend-resume, so we would send IPCs in those cases as well. That said, it's true that we marked all the FE dailinks as nonatomic precisely because they would involve IPCs, but here we are dealing with BE dailinks that are typically thought of as 'atomic'. Just thinking aloud, maybe we need to tag all those dailinks as 'nonatomic' as well? > If it's just for releasing before re-acquiring a stream, you can do it > in sync_stop PCM ops instead, too. This is called in prior to prepare > and hw_params ops when a stream has been stopped or suspended > beforehand. Humm, I must admit I have never heard of this sync_stop routine :-) It's not exposed as a DAI callback, I only see this exposed at the component level. That wouldn't be too helpful, the existing solution was based on using the suspend at the component level, which was a bit of a hack - we marked all component DAIs as suspended, including the ones that were never started. The idea of using the DAI seems much better to me, we don't need to track which DAI is started or not, just use the pointer passed by higher layers. Anyways, thanks for the feedback, that gave us a lot of things to think about. -Pierre
