On 27-07-21, 09:12, Pierre-Louis Bossart wrote: > 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. Doesn't Intel use non atomic trigger to send IPCs which anyway involve code which can sleep..? > > 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 -- ~Vinod
