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

[Pierre-Louis Bossart]

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.

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