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

[Vinod Koul <vkoul@kernel.org>]

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