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

[Takashi Iwai <tiwai@suse.de>]

On Tue, 27 Jul 2021 07:32:56 +0200,
Bard Liao wrote:
> 
> From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> 
> 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@linux.intel.com>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
> Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  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
>