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

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

[Bard Liao]

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.

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

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

[Takashi Iwai]

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
> 

Re: [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

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

[Vinod Koul]

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

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

[Takashi Iwai]

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?


Takashi

Re: [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

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

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Monday, August 2, 2021 2:29 PM
> To: Takashi Iwai <tiwai@suse.de>
> Cc: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>; Bard Liao
> <yung-chuan.liao@linux.intel.com>; broonie@kernel.org; alsa-devel@alsa-
> project.org; Liao, Bard <bard.liao@intel.com>; Ranjani Sridharan
> <ranjani.sridharan@linux.intel.com>
> Subject: Re: [PATCH] soundwire: intel: trap TRIGGER_SUSPEND in .trigger
> callback
> 
> 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.

init_dai_link() is to assign dai link elements only. No IPC is needed.

> 
> --
> ~Vinod

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

[Pierre-Louis Bossart]

>>>>>> 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.
> 
> init_dai_link() is to assign dai link elements only. No IPC is needed.

The 'nonatomic' concept is only used for an FE dailink which expose a
PCM device:

soc-pcm.c:	pcm->nonatomic = rtd->dai_link->nonatomic;

Setting a BE dailink as 'nonatomic' would not accomplish much since BEs
use the 'no_pcm' option.

So the question is: is there any issue with sending an IPC in a DAI
trigger callback? This is not very different from sending a command on a
bus btw, I see a similar example for SLIMbus devices:

wcd9335.c:      case SNDRV_PCM_TRIGGER_SUSPEND:
wcd9335.c-      case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
wcd9335.c-              slim_stream_unprepare(dai_data->sruntime);
wcd9335.c-              slim_stream_disable(dai_data->sruntime);

int slim_stream_unprepare(struct slim_stream_runtime *stream)
{
	int i;

	for (i = 0; i < stream->num_ports; i++)
		slim_disconnect_port(stream, &stream->ports[i]);

static int slim_disconnect_port(struct slim_stream_runtime *stream,
				struct slim_port *port)
{
	struct slim_device *sdev = stream->dev;
	u8 wbuf[1];
	struct slim_val_inf msg = {0, 1, NULL, wbuf, NULL};
	u8 mc = SLIM_MSG_MC_DISCONNECT_PORT;
	DEFINE_SLIM_LDEST_TXN(txn, mc, 5, stream->dev->laddr, &msg);

	wbuf[0] = port->id;
	port->ch.state = SLIM_CH_STATE_DISCONNECTED;
	port->state = SLIM_PORT_DISCONNECTED;

	return slim_do_transfer(sdev->ctrl, &txn);
}

Such commands may take time...

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

[Vinod Koul]

On 02-08-21, 10:46, Pierre-Louis Bossart wrote:
> 
> >>>>>> 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.
> > 
> > init_dai_link() is to assign dai link elements only. No IPC is needed.
> 
> The 'nonatomic' concept is only used for an FE dailink which expose a
> PCM device:
> 
> soc-pcm.c:	pcm->nonatomic = rtd->dai_link->nonatomic;
> 
> Setting a BE dailink as 'nonatomic' would not accomplish much since BEs
> use the 'no_pcm' option.

are no_pcm & nonatomic supposed to be not used together? So if FE is
nonatomic would BE trigger be atomic or nonatomic?

> So the question is: is there any issue with sending an IPC in a DAI
> trigger callback?

Sorry looks like we diverged, orignal question was can we do heavy tasks
in trigger, the answer is no, unless one uses nonatomic flag which was
added so that people can do that work with DSPs like sending IPCs..
Maybe we should add heavy slimbus/soundwire handling to it too...?

> This is not very different from sending a command on a
> bus btw, I see a similar example for SLIMbus devices:
> 
> wcd9335.c:      case SNDRV_PCM_TRIGGER_SUSPEND:
> wcd9335.c-      case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> wcd9335.c-              slim_stream_unprepare(dai_data->sruntime);
> wcd9335.c-              slim_stream_disable(dai_data->sruntime);
> 
> int slim_stream_unprepare(struct slim_stream_runtime *stream)
> {
> 	int i;
> 
> 	for (i = 0; i < stream->num_ports; i++)
> 		slim_disconnect_port(stream, &stream->ports[i]);
> 
> static int slim_disconnect_port(struct slim_stream_runtime *stream,
> 				struct slim_port *port)
> {
> 	struct slim_device *sdev = stream->dev;
> 	u8 wbuf[1];
> 	struct slim_val_inf msg = {0, 1, NULL, wbuf, NULL};
> 	u8 mc = SLIM_MSG_MC_DISCONNECT_PORT;
> 	DEFINE_SLIM_LDEST_TXN(txn, mc, 5, stream->dev->laddr, &msg);
> 
> 	wbuf[0] = port->id;
> 	port->ch.state = SLIM_CH_STATE_DISCONNECTED;
> 	port->state = SLIM_PORT_DISCONNECTED;
> 
> 	return slim_do_transfer(sdev->ctrl, &txn);
> }
> 
> Such commands may take time...

Agree, so users should be recommended to use nonatomic triggers.

-- 
~Vinod

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

[Pierre-Louis Bossart]

On 8/6/21 8:37 AM, Vinod Koul wrote:
> On 02-08-21, 10:46, Pierre-Louis Bossart wrote:
>>
>>>>>>>> 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.
>>>
>>> init_dai_link() is to assign dai link elements only. No IPC is needed.
>>
>> The 'nonatomic' concept is only used for an FE dailink which expose a
>> PCM device:
>>
>> soc-pcm.c:	pcm->nonatomic = rtd->dai_link->nonatomic;
>>
>> Setting a BE dailink as 'nonatomic' would not accomplish much since BEs
>> use the 'no_pcm' option.
> 
> are no_pcm & nonatomic supposed to be not used together? So if FE is
> nonatomic would BE trigger be atomic or nonatomic?

I don't follow the multiple negations, so let me retry:

For Intel machine drivers, all BE dailinks use
.no_pcm = 1 (explicit setting)
.nonatomic = 0 (implicit).

All FE dailinks use
.no_pcm = 0 (implicit)
.nonatomic = 1 (explicit setting)

>> So the question is: is there any issue with sending an IPC in a DAI
>> trigger callback?
> 
> Sorry looks like we diverged, orignal question was can we do heavy tasks
> in trigger, the answer is no, unless one uses nonatomic flag which was
> added so that people can do that work with DSPs like sending IPCs..
> Maybe we should add heavy slimbus/soundwire handling to it too...?

I don't think the answer is as clear as you describe it Vinod.

The .nonatomic field is at the BE dailink level.

Unless I am missing something, I don't see anything that lets me set a
.nonatomic property at the *DAI* level.

The other problem is to define 'heavy task'. In this case, we are
sending an IPC indeed, but the response is immediate, typically in the
next ms tick.

IOW, if the response time is in the ms order of magnitude, is this 'heavy'?

>> This is not very different from sending a command on a
>> bus btw, I see a similar example for SLIMbus devices:
>>
>> wcd9335.c:      case SNDRV_PCM_TRIGGER_SUSPEND:
>> wcd9335.c-      case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
>> wcd9335.c-              slim_stream_unprepare(dai_data->sruntime);
>> wcd9335.c-              slim_stream_disable(dai_data->sruntime);
>>
>> int slim_stream_unprepare(struct slim_stream_runtime *stream)
>> {
>> 	int i;
>>
>> 	for (i = 0; i < stream->num_ports; i++)
>> 		slim_disconnect_port(stream, &stream->ports[i]);
>>
>> static int slim_disconnect_port(struct slim_stream_runtime *stream,
>> 				struct slim_port *port)
>> {
>> 	struct slim_device *sdev = stream->dev;
>> 	u8 wbuf[1];
>> 	struct slim_val_inf msg = {0, 1, NULL, wbuf, NULL};
>> 	u8 mc = SLIM_MSG_MC_DISCONNECT_PORT;
>> 	DEFINE_SLIM_LDEST_TXN(txn, mc, 5, stream->dev->laddr, &msg);
>>
>> 	wbuf[0] = port->id;
>> 	port->ch.state = SLIM_CH_STATE_DISCONNECTED;
>> 	port->state = SLIM_PORT_DISCONNECTED;
>>
>> 	return slim_do_transfer(sdev->ctrl, &txn);
>> }
>>
>> Such commands may take time...
> 
> Agree, so users should be recommended to use nonatomic triggers.

The SLIMbus example relies on DAIs as well, and this option does not
exist, does it?

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

[Vinod Koul]

On 06-08-21, 11:17, Pierre-Louis Bossart wrote:
> 
> 
> On 8/6/21 8:37 AM, Vinod Koul wrote:
> > On 02-08-21, 10:46, Pierre-Louis Bossart wrote:
> >>
> >>>>>>>> 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.
> >>>
> >>> init_dai_link() is to assign dai link elements only. No IPC is needed.
> >>
> >> The 'nonatomic' concept is only used for an FE dailink which expose a
> >> PCM device:
> >>
> >> soc-pcm.c:	pcm->nonatomic = rtd->dai_link->nonatomic;
> >>
> >> Setting a BE dailink as 'nonatomic' would not accomplish much since BEs
> >> use the 'no_pcm' option.
> > 
> > are no_pcm & nonatomic supposed to be not used together? So if FE is
> > nonatomic would BE trigger be atomic or nonatomic?
> 
> I don't follow the multiple negations, so let me retry:
> 
> For Intel machine drivers, all BE dailinks use
> .no_pcm = 1 (explicit setting)
> .nonatomic = 0 (implicit).

that was my question, how is it implicit?
Should be explicitly set, right?

> 
> All FE dailinks use
> .no_pcm = 0 (implicit)
> .nonatomic = 1 (explicit setting)
> 
> >> So the question is: is there any issue with sending an IPC in a DAI
> >> trigger callback?
> > 
> > Sorry looks like we diverged, orignal question was can we do heavy tasks
> > in trigger, the answer is no, unless one uses nonatomic flag which was
> > added so that people can do that work with DSPs like sending IPCs..
> > Maybe we should add heavy slimbus/soundwire handling to it too...?
> 
> I don't think the answer is as clear as you describe it Vinod.
> 
> The .nonatomic field is at the BE dailink level.
> 
> Unless I am missing something, I don't see anything that lets me set a
> .nonatomic property at the *DAI* level.

I would say that was a miss in original design, it should have been set
at dai level or at least allowed to propagate from dai level setting.

Now we are allowed to set it at dai_link but it is governed by dai
behaviour (DSP based DAI etc...)

> 
> The other problem is to define 'heavy task'. In this case, we are
> sending an IPC indeed, but the response is immediate, typically in the
> next ms tick.
> 
> IOW, if the response time is in the ms order of magnitude, is this 'heavy'?

There are two aspects:
1. Does the code involve a sleepy function? Response may be very fast
typically but a code path having sleeping function would stop from using
atomic context
2. Is it designed to be ms always or can be more? We should looks at
worst case rather than typical values for this.

> 
> >> This is not very different from sending a command on a
> >> bus btw, I see a similar example for SLIMbus devices:
> >>
> >> wcd9335.c:      case SNDRV_PCM_TRIGGER_SUSPEND:
> >> wcd9335.c-      case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> >> wcd9335.c-              slim_stream_unprepare(dai_data->sruntime);
> >> wcd9335.c-              slim_stream_disable(dai_data->sruntime);
> >>
> >> int slim_stream_unprepare(struct slim_stream_runtime *stream)
> >> {
> >> 	int i;
> >>
> >> 	for (i = 0; i < stream->num_ports; i++)
> >> 		slim_disconnect_port(stream, &stream->ports[i]);
> >>
> >> static int slim_disconnect_port(struct slim_stream_runtime *stream,
> >> 				struct slim_port *port)
> >> {
> >> 	struct slim_device *sdev = stream->dev;
> >> 	u8 wbuf[1];
> >> 	struct slim_val_inf msg = {0, 1, NULL, wbuf, NULL};
> >> 	u8 mc = SLIM_MSG_MC_DISCONNECT_PORT;
> >> 	DEFINE_SLIM_LDEST_TXN(txn, mc, 5, stream->dev->laddr, &msg);
> >>
> >> 	wbuf[0] = port->id;
> >> 	port->ch.state = SLIM_CH_STATE_DISCONNECTED;
> >> 	port->state = SLIM_PORT_DISCONNECTED;
> >>
> >> 	return slim_do_transfer(sdev->ctrl, &txn);
> >> }
> >>
> >> Such commands may take time...
> > 
> > Agree, so users should be recommended to use nonatomic triggers.
> 
> The SLIMbus example relies on DAIs as well, and this option does not
> exist, does it?

-- 
~Vinod

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

[Takashi Iwai]

On Mon, 09 Aug 2021 06:02:21 +0200,
Vinod Koul wrote:
> 
> On 06-08-21, 11:17, Pierre-Louis Bossart wrote:
> > 
> > 
> > On 8/6/21 8:37 AM, Vinod Koul wrote:
> > > On 02-08-21, 10:46, Pierre-Louis Bossart wrote:
> > >>
> > >>>>>>>> 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.
> > >>>
> > >>> init_dai_link() is to assign dai link elements only. No IPC is needed.
> > >>
> > >> The 'nonatomic' concept is only used for an FE dailink which expose a
> > >> PCM device:
> > >>
> > >> soc-pcm.c:	pcm->nonatomic = rtd->dai_link->nonatomic;
> > >>
> > >> Setting a BE dailink as 'nonatomic' would not accomplish much since BEs
> > >> use the 'no_pcm' option.
> > > 
> > > are no_pcm & nonatomic supposed to be not used together? So if FE is
> > > nonatomic would BE trigger be atomic or nonatomic?
> > 
> > I don't follow the multiple negations, so let me retry:
> > 
> > For Intel machine drivers, all BE dailinks use
> > .no_pcm = 1 (explicit setting)
> > .nonatomic = 0 (implicit).
> 
> that was my question, how is it implicit?
> Should be explicitly set, right?
> 
> > 
> > All FE dailinks use
> > .no_pcm = 0 (implicit)
> > .nonatomic = 1 (explicit setting)
> > 
> > >> So the question is: is there any issue with sending an IPC in a DAI
> > >> trigger callback?
> > > 
> > > Sorry looks like we diverged, orignal question was can we do heavy tasks
> > > in trigger, the answer is no, unless one uses nonatomic flag which was
> > > added so that people can do that work with DSPs like sending IPCs..
> > > Maybe we should add heavy slimbus/soundwire handling to it too...?
> > 
> > I don't think the answer is as clear as you describe it Vinod.
> > 
> > The .nonatomic field is at the BE dailink level.
> > 
> > Unless I am missing something, I don't see anything that lets me set a
> > .nonatomic property at the *DAI* level.
> 
> I would say that was a miss in original design, it should have been set
> at dai level or at least allowed to propagate from dai level setting.
> 
> Now we are allowed to set it at dai_link but it is governed by dai
> behaviour (DSP based DAI etc...)

Actually, there was one big piece I overlooked.  The whole DPCM BE
operation is *always* tied with FE's.  That is, the nonatomic flag is
completely ignored for BE, but just follows what FE sets up.

And that's the very confusing point when reviewing the code.  You
cannot know whether it's written for non-atomic context or not.  This
means that it's also error-prone; the code that assumes the operation
in a certain mode might mismatch with the bound FE.

So, ideally, both FE and BE should set the proper nonatomic flags, and
have a consistency check with WARN_ON() at the run time.


thanks,

Takashi

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

[Pierre-Louis Bossart]

>>> For Intel machine drivers, all BE dailinks use
>>> .no_pcm = 1 (explicit setting)
>>> .nonatomic = 0 (implicit).
>>
>> that was my question, how is it implicit?
>> Should be explicitly set, right?

implicit behavior with C, if you don't set a field its value is zero...

>>> All FE dailinks use
>>> .no_pcm = 0 (implicit)
>>> .nonatomic = 1 (explicit setting)
>>>
>>>>> So the question is: is there any issue with sending an IPC in a DAI
>>>>> trigger callback?
>>>>
>>>> Sorry looks like we diverged, orignal question was can we do heavy tasks
>>>> in trigger, the answer is no, unless one uses nonatomic flag which was
>>>> added so that people can do that work with DSPs like sending IPCs..
>>>> Maybe we should add heavy slimbus/soundwire handling to it too...?
>>>
>>> I don't think the answer is as clear as you describe it Vinod.
>>>
>>> The .nonatomic field is at the BE dailink level.
>>>
>>> Unless I am missing something, I don't see anything that lets me set a
>>> .nonatomic property at the *DAI* level.
>>
>> I would say that was a miss in original design, it should have been set
>> at dai level or at least allowed to propagate from dai level setting.
>>
>> Now we are allowed to set it at dai_link but it is governed by dai
>> behaviour (DSP based DAI etc...)
> 
> Actually, there was one big piece I overlooked.  The whole DPCM BE
> operation is *always* tied with FE's.  That is, the nonatomic flag is
> completely ignored for BE, but just follows what FE sets up.
> 
> And that's the very confusing point when reviewing the code.  You
> cannot know whether it's written for non-atomic context or not.  This
> means that it's also error-prone; the code that assumes the operation
> in a certain mode might mismatch with the bound FE.
> 
> So, ideally, both FE and BE should set the proper nonatomic flags, and
> have a consistency check with WARN_ON() at the run time.

Sorry Takashi, I am not following. Are you asking me to add a .nonatomic
flag in all the exiting BEs along with a WARN_ON?

I can do this, but that's a sure way to trigger massive amounts of
user-reported "regression in kernel 5.1x". Is this really what you want?

Also I don't understand how this would help with the specific problem
raised in this patch: can we yes/no do something 'heavy' in a *DAI*
callback? What is the definition of 'heavy'?

And last, I am not sure it's always the case that a BE follows the FE
configuration. We've had cases of BE->BE loopbacks where the host
doesn't see or configured the data.

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Mon, Aug 09, 2021 at 09:26:51AM -0500, Pierre-Louis Bossart wrote:

> >>> For Intel machine drivers, all BE dailinks use
> >>> .no_pcm = 1 (explicit setting)
> >>> .nonatomic = 0 (implicit).
> >>
> >> that was my question, how is it implicit?
> >> Should be explicitly set, right?

> implicit behavior with C, if you don't set a field its value is zero...

Only for things declared static.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Takashi Iwai]

On Mon, 09 Aug 2021 16:26:51 +0200,
Pierre-Louis Bossart wrote:
> 
> 
> 
> >>> For Intel machine drivers, all BE dailinks use
> >>> .no_pcm = 1 (explicit setting)
> >>> .nonatomic = 0 (implicit).
> >>
> >> that was my question, how is it implicit?
> >> Should be explicitly set, right?
> 
> implicit behavior with C, if you don't set a field its value is zero...
> 
> >>> All FE dailinks use
> >>> .no_pcm = 0 (implicit)
> >>> .nonatomic = 1 (explicit setting)
> >>>
> >>>>> So the question is: is there any issue with sending an IPC in a DAI
> >>>>> trigger callback?
> >>>>
> >>>> Sorry looks like we diverged, orignal question was can we do heavy tasks
> >>>> in trigger, the answer is no, unless one uses nonatomic flag which was
> >>>> added so that people can do that work with DSPs like sending IPCs..
> >>>> Maybe we should add heavy slimbus/soundwire handling to it too...?
> >>>
> >>> I don't think the answer is as clear as you describe it Vinod.
> >>>
> >>> The .nonatomic field is at the BE dailink level.
> >>>
> >>> Unless I am missing something, I don't see anything that lets me set a
> >>> .nonatomic property at the *DAI* level.
> >>
> >> I would say that was a miss in original design, it should have been set
> >> at dai level or at least allowed to propagate from dai level setting.
> >>
> >> Now we are allowed to set it at dai_link but it is governed by dai
> >> behaviour (DSP based DAI etc...)
> > 
> > Actually, there was one big piece I overlooked.  The whole DPCM BE
> > operation is *always* tied with FE's.  That is, the nonatomic flag is
> > completely ignored for BE, but just follows what FE sets up.
> > 
> > And that's the very confusing point when reviewing the code.  You
> > cannot know whether it's written for non-atomic context or not.  This
> > means that it's also error-prone; the code that assumes the operation
> > in a certain mode might mismatch with the bound FE.
> > 
> > So, ideally, both FE and BE should set the proper nonatomic flags, and
> > have a consistency check with WARN_ON() at the run time.
> 
> Sorry Takashi, I am not following. Are you asking me to add a .nonatomic
> flag in all the exiting BEs along with a WARN_ON?
> 
> I can do this, but that's a sure way to trigger massive amounts of
> user-reported "regression in kernel 5.1x". Is this really what you want?

That's why I wrote "ideally".  We all know that the world is no
perfect...  So hardening in that way would be possible, but it has to
be done carefully if we really go for it, and I'm not asking you to do
that now.

> Also I don't understand how this would help with the specific problem
> raised in this patch: can we yes/no do something 'heavy' in a *DAI*
> callback? What is the definition of 'heavy'?

My previous comment wasn't specifically about your patch itself but
rather arguing a generic problem.  We have no notion or matching
mechanism of the atomicity of DPCM BE.

> And last, I am not sure it's always the case that a BE follows the FE
> configuration. We've had cases of BE->BE loopbacks where the host
> doesn't see or configured the data.

Hm, how the trigger and other PCM callbacks for BE get called in that
mode?


Takashi

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

[Pierre-Louis Bossart]

>>> Actually, there was one big piece I overlooked.  The whole DPCM BE
>>> operation is *always* tied with FE's.  That is, the nonatomic flag is
>>> completely ignored for BE, but just follows what FE sets up.
>>>
>>> And that's the very confusing point when reviewing the code.  You
>>> cannot know whether it's written for non-atomic context or not.  This
>>> means that it's also error-prone; the code that assumes the operation
>>> in a certain mode might mismatch with the bound FE.
>>>
>>> So, ideally, both FE and BE should set the proper nonatomic flags, and
>>> have a consistency check with WARN_ON() at the run time.
>>
>> Sorry Takashi, I am not following. Are you asking me to add a .nonatomic
>> flag in all the exiting BEs along with a WARN_ON?
>>
>> I can do this, but that's a sure way to trigger massive amounts of
>> user-reported "regression in kernel 5.1x". Is this really what you want?
> 
> That's why I wrote "ideally".  We all know that the world is no
> perfect...  So hardening in that way would be possible, but it has to
> be done carefully if we really go for it, and I'm not asking you to do
> that now.
> 
>> Also I don't understand how this would help with the specific problem
>> raised in this patch: can we yes/no do something 'heavy' in a *DAI*
>> callback? What is the definition of 'heavy'?
> 
> My previous comment wasn't specifically about your patch itself but
> rather arguing a generic problem.  We have no notion or matching
> mechanism of the atomicity of DPCM BE.

I think the only problem is actually on the SoundWire dailinks.

For SSP/DMIC we don't do anything for BE dailinks, there's no IPC or
waits, only some settings/masks. I don't see any need to set the
.nonatomic field in those cases.

But for SoundWire, we do use the 'stream' functions from the BE ops
callbacks - sdw_prepare_stream, sdw_trigger_stream - which will do a
bank switch operation. That's certainly not an atomic operation, there's
a clear wait_for_completion().

That seems like a miss indeed, I'll add a patch to set the .nonatomic
field for these links.

But for this patch proper, does anyone have an objection? I am still not
clear on what is permissible at the DAI level.

>> And last, I am not sure it's always the case that a BE follows the FE
>> configuration. We've had cases of BE->BE loopbacks where the host
>> doesn't see or configured the data.
> 
> Hm, how the trigger and other PCM callbacks for BE get called in that
> mode?


IIRC everything was handled with DAPM, changing pin states would enable
data transfers. Not 100% sure and that's not really relevant anyways,
you did have a point that the SoundWire BEs are not correctly configured.