On Tue, 27 Jul 2021 07:32:56 +0200,
Bard Liao wrote:
>
> From: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
>
> 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?
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.
thanks,
Takashi
>
> Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@xxxxxxxxxxxxxxx>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@xxxxxxxxxxxxxxx>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@xxxxxxxxxxxxxxx>
> Reviewed-by: Rander Wang <rander.wang@xxxxxxxxx>
> Signed-off-by: Bard Liao <yung-chuan.liao@xxxxxxxxxxxxxxx>
> ---
> drivers/soundwire/intel.c | 53 +++++++++++++++++++++------------------
> 1 file changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
> index fb9c23e13206..a0178779a5ba 100644
> --- a/drivers/soundwire/intel.c
> +++ b/drivers/soundwire/intel.c
> @@ -1005,29 +1005,6 @@ static void intel_shutdown(struct snd_pcm_substream *substream,
> pm_runtime_put_autosuspend(cdns->dev);
> }
>
> -static int intel_component_dais_suspend(struct snd_soc_component *component)
> -{
> - struct sdw_cdns_dma_data *dma;
> - struct snd_soc_dai *dai;
> -
> - for_each_component_dais(component, dai) {
> - /*
> - * we don't have a .suspend dai_ops, and we don't have access
> - * to the substream, so let's mark both capture and playback
> - * DMA contexts as suspended
> - */
> - dma = dai->playback_dma_data;
> - if (dma)
> - dma->suspended = true;
> -
> - dma = dai->capture_dma_data;
> - if (dma)
> - dma->suspended = true;
> - }
> -
> - return 0;
> -}
> -
> static int intel_pcm_set_sdw_stream(struct snd_soc_dai *dai,
> void *stream, int direction)
> {
> @@ -1056,11 +1033,39 @@ static void *intel_get_sdw_stream(struct snd_soc_dai *dai,
> return dma->stream;
> }
>
> +static int intel_trigger(struct snd_pcm_substream *substream, int cmd, struct snd_soc_dai *dai)
> +{
> + struct sdw_cdns *cdns = snd_soc_dai_get_drvdata(dai);
> + struct sdw_intel *sdw = cdns_to_intel(cdns);
> + struct sdw_cdns_dma_data *dma;
> +
> + /*
> + * The .prepare callback is used to deal with xruns and resume operations. In the case
> + * of xruns, the DMAs and SHIM registers cannot be touched, but for resume operations the
> + * DMAs and SHIM registers need to be initialized.
> + * the .trigger callback is used to track the suspend case only.
> + */
> + if (cmd != SNDRV_PCM_TRIGGER_SUSPEND)
> + return 0;
> +
> + dma = snd_soc_dai_get_dma_data(dai, substream);
> + if (!dma) {
> + dev_err(dai->dev, "failed to get dma data in %s\n",
> + __func__);
> + return -EIO;
> + }
> +
> + dma->suspended = true;
> +
> + return intel_free_stream(sdw, substream, dai, sdw->instance);
> +}
> +
> static const struct snd_soc_dai_ops intel_pcm_dai_ops = {
> .startup = intel_startup,
> .hw_params = intel_hw_params,
> .prepare = intel_prepare,
> .hw_free = intel_hw_free,
> + .trigger = intel_trigger,
> .shutdown = intel_shutdown,
> .set_sdw_stream = intel_pcm_set_sdw_stream,
> .get_sdw_stream = intel_get_sdw_stream,
> @@ -1071,6 +1076,7 @@ static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
> .hw_params = intel_hw_params,
> .prepare = intel_prepare,
> .hw_free = intel_hw_free,
> + .trigger = intel_trigger,
> .shutdown = intel_shutdown,
> .set_sdw_stream = intel_pdm_set_sdw_stream,
> .get_sdw_stream = intel_get_sdw_stream,
> @@ -1078,7 +1084,6 @@ static const struct snd_soc_dai_ops intel_pdm_dai_ops = {
>
> static const struct snd_soc_component_driver dai_component = {
> .name = "soundwire",
> - .suspend = intel_component_dais_suspend
> };
>
> static int intel_create_dai(struct sdw_cdns *cdns,
> --
> 2.17.1
>
