On 02-08-21, 07:49, Takashi Iwai wrote: > On Mon, 02 Aug 2021 06:35:16 +0200, > Vinod Koul wrote: > > > > 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..? > > sof_sdw.c doesn't seem setting it? Yes I think init_dai_link() should set it. Maybe Pierre/Bard would know why. -- ~Vinod
