[PATCH] soundwire: intel: trap TRIGGER_SUSPEND in .trigger callback

[Vinod Koul]

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