> -----Original Message----- > From: Vinod Koul <vkoul@xxxxxxxxxx> > Sent: Monday, August 2, 2021 2:29 PM > To: Takashi Iwai <tiwai@xxxxxxx> > Cc: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>; Bard Liao > <yung-chuan.liao@xxxxxxxxxxxxxxx>; broonie@xxxxxxxxxx; alsa-devel@alsa- > project.org; Liao, Bard <bard.liao@xxxxxxxxx>; Ranjani Sridharan > <ranjani.sridharan@xxxxxxxxxxxxxxx> > Subject: Re: [PATCH] soundwire: intel: trap TRIGGER_SUSPEND in .trigger > callback > > 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. init_dai_link() is to assign dai link elements only. No IPC is needed. > > -- > ~Vinod
