[PATCH] soundwire: intel: trap TRIGGER_SUSPEND in .trigger callback
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,
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
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
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
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
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.
-----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
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...
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.
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?
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?
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
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.
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.
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
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.
participants (6)
-
Bard Liao -
Liao, Bard -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai -
Vinod Koul