[PATCH 0/3] ASoC: SOF: pcm/Intel: Handle IPC dependent sequencing correctly

[PATCH 0/3] ASoC: SOF: pcm/Intel: Handle IPC dependent sequencing correctly

[Peter Ujfalusi]

Hi,

IPC3 and IPC4 firmwares handle and execute tasks at different stages, like
managing DMAs.
In most cases these are aligned, but we have few exceptions that needs to be
handled differently.

This series introduces flags to handle the differing cases to make sure that
the correct sequencing is used regerless of the IPC version.

Regards,
Peter
---
Ranjani Sridharan (3):
  ASoC: SOF: Intel: hda-dai: Do not perform DMA cleanup during stop
  ASoC: SOF: pcm: Make hw_params reset conditional for IPC3
  ASoC: SOF: pcm: Improve the pcm trigger sequence

 sound/soc/sof/intel/hda-dai.c |  1 -
 sound/soc/sof/ipc3-pcm.c      |  1 +
 sound/soc/sof/ipc4-pcm.c      |  3 ++-
 sound/soc/sof/pcm.c           | 29 ++++++++++++++++++++++-------
 sound/soc/sof/sof-audio.h     |  6 ++++++
 5 files changed, 31 insertions(+), 9 deletions(-)

-- 
2.40.0

[PATCH 1/3] ASoC: SOF: Intel: hda-dai: Do not perform DMA cleanup during stop

[Peter Ujfalusi]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

In the case of repeated start/stop without involving hw_free, the stream
tag needs to be preserved for the subsequent starts. So, skip performing
the DMA clean up during stop and handle it only during suspend or
hw_free.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda-dai.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/sound/soc/sof/intel/hda-dai.c b/sound/soc/sof/intel/hda-dai.c
index 9637f0f44b01..46a17afdd1ea 100644
--- a/sound/soc/sof/intel/hda-dai.c
+++ b/sound/soc/sof/intel/hda-dai.c
@@ -305,7 +305,6 @@ static int hda_dai_trigger(struct snd_pcm_substream *substream, int cmd, struct
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_STOP:
 		ret = hda_link_dma_cleanup(substream, hext_stream, dai, codec_dai);
 		if (ret < 0) {
 			dev_err(sdev->dev, "%s: failed to clean up link DMA\n", __func__);
-- 
2.40.0

[PATCH 2/3] ASoC: SOF: pcm: Make hw_params reset conditional for IPC3

[Peter Ujfalusi]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

In the case of IPC4, since there is no PCM_PARAMS IPC to send the new
stream tag when restarting a stream without a hw_free, the original
stream tag needs to be preserved. So, add new a flag as part of struct
sof_ipc_pcm_ops, reset_hw_params_during_stop and set it only for IPC3.
This will ensure that the host DMA stream tag will not be given up during
the STOP trigger for IPC4.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/ipc3-pcm.c  | 1 +
 sound/soc/sof/pcm.c       | 3 ++-
 sound/soc/sof/sof-audio.h | 3 +++
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/sound/soc/sof/ipc3-pcm.c b/sound/soc/sof/ipc3-pcm.c
index b29d93e0d216..b7f1eb21ca26 100644
--- a/sound/soc/sof/ipc3-pcm.c
+++ b/sound/soc/sof/ipc3-pcm.c
@@ -382,4 +382,5 @@ const struct sof_ipc_pcm_ops ipc3_pcm_ops = {
 	.hw_free = sof_ipc3_pcm_hw_free,
 	.trigger = sof_ipc3_pcm_trigger,
 	.dai_link_fixup = sof_ipc3_pcm_dai_link_fixup,
+	.reset_hw_params_during_stop = true,
 };
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index 445acb5c3a21..f75b161125fa 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -328,7 +328,8 @@ static int sof_pcm_trigger(struct snd_soc_component *component,
 		fallthrough;
 	case SNDRV_PCM_TRIGGER_STOP:
 		ipc_first = true;
-		reset_hw_params = true;
+		if (pcm_ops && pcm_ops->reset_hw_params_during_stop)
+			reset_hw_params = true;
 		break;
 	default:
 		dev_err(component->dev, "Unhandled trigger cmd %d\n", cmd);
diff --git a/sound/soc/sof/sof-audio.h b/sound/soc/sof/sof-audio.h
index d220af5f08fb..81685e778ad6 100644
--- a/sound/soc/sof/sof-audio.h
+++ b/sound/soc/sof/sof-audio.h
@@ -104,6 +104,8 @@ struct snd_sof_dai_config_data {
  * @pcm_free: Function pointer for PCM free that can be used for freeing any
  *	       additional memory in the SOF PCM stream structure
  * @delay: Function pointer for pcm delay calculation
+ * @reset_hw_params_during_stop: Flag indicating whether the hw_params should be reset during the
+ *				 STOP pcm trigger
  */
 struct sof_ipc_pcm_ops {
 	int (*hw_params)(struct snd_soc_component *component, struct snd_pcm_substream *substream,
@@ -117,6 +119,7 @@ struct sof_ipc_pcm_ops {
 	void (*pcm_free)(struct snd_sof_dev *sdev, struct snd_sof_pcm *spcm);
 	snd_pcm_sframes_t (*delay)(struct snd_soc_component *component,
 				   struct snd_pcm_substream *substream);
+	bool reset_hw_params_during_stop;
 };
 
 /**
-- 
2.40.0

[PATCH 3/3] ASoC: SOF: pcm: Improve the pcm trigger sequence

[Peter Ujfalusi]

From: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The recommended sequence for triggering the host DMA is to first program
the DMA in the FW before setting the RUN bit to start the stream in the
host. With IPC3, this sequence is honored because the FW programs the
DMA when the HW_PARAMS IPC is sent during PCM hw_params and then the host
sets the RUN bit during sof_pcm_trigger(). But with IPC4,
sof_pcm_trigger() sends the SET_PIPELINE_STATE IPC to program the DMA in
the FW after the DMA RUN bit is set.

In order to minimize the impact for IPC3, introduce a new flag as part
of struct sof_ipc_pcm_ops, ipc_first_on_start, which will be set for IPC4
only. With this flag set, the SET_PIPELINE_STATE IPC will be sent before
the DMA RUN bit is set by the host during the START/PAUSE_RELEASE
triggers.

Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Peter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/ipc4-pcm.c  |  3 ++-
 sound/soc/sof/pcm.c       | 26 ++++++++++++++++++++------
 sound/soc/sof/sof-audio.h |  3 +++
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c
index 4598057b7f28..b789926dce2a 100644
--- a/sound/soc/sof/ipc4-pcm.c
+++ b/sound/soc/sof/ipc4-pcm.c
@@ -721,5 +721,6 @@ const struct sof_ipc_pcm_ops ipc4_pcm_ops = {
 	.dai_link_fixup = sof_ipc4_pcm_dai_link_fixup,
 	.pcm_setup = sof_ipc4_pcm_setup,
 	.pcm_free = sof_ipc4_pcm_free,
-	.delay = sof_ipc4_pcm_delay
+	.delay = sof_ipc4_pcm_delay,
+	.ipc_first_on_start = true
 };
diff --git a/sound/soc/sof/pcm.c b/sound/soc/sof/pcm.c
index f75b161125fa..d9b4633bba7a 100644
--- a/sound/soc/sof/pcm.c
+++ b/sound/soc/sof/pcm.c
@@ -301,6 +301,8 @@ static int sof_pcm_trigger(struct snd_soc_component *component,
 		ipc_first = true;
 		break;
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+		if (pcm_ops && pcm_ops->ipc_first_on_start)
+			ipc_first = true;
 		break;
 	case SNDRV_PCM_TRIGGER_START:
 		if (spcm->stream[substream->stream].suspend_ignored) {
@@ -312,6 +314,9 @@ static int sof_pcm_trigger(struct snd_soc_component *component,
 			spcm->stream[substream->stream].suspend_ignored = false;
 			return 0;
 		}
+
+		if (pcm_ops && pcm_ops->ipc_first_on_start)
+			ipc_first = true;
 		break;
 	case SNDRV_PCM_TRIGGER_SUSPEND:
 		if (sdev->system_suspend_target == SOF_SUSPEND_S0IX &&
@@ -336,19 +341,28 @@ static int sof_pcm_trigger(struct snd_soc_component *component,
 		return -EINVAL;
 	}
 
-	/*
-	 * DMA and IPC sequence is different for start and stop. Need to send
-	 * STOP IPC before stop DMA
-	 */
 	if (!ipc_first)
 		snd_sof_pcm_platform_trigger(sdev, substream, cmd);
 
 	if (pcm_ops && pcm_ops->trigger)
 		ret = pcm_ops->trigger(component, substream, cmd);
 
-	/* need to STOP DMA even if trigger IPC failed */
-	if (ipc_first)
+	switch (cmd) {
+	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
+	case SNDRV_PCM_TRIGGER_START:
+		/* invoke platform trigger to start DMA only if pcm_ops is successful */
+		if (ipc_first && !ret)
+			snd_sof_pcm_platform_trigger(sdev, substream, cmd);
+		break;
+	case SNDRV_PCM_TRIGGER_SUSPEND:
+	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
+	case SNDRV_PCM_TRIGGER_STOP:
+		/* invoke platform trigger to stop DMA even if pcm_ops failed */
 		snd_sof_pcm_platform_trigger(sdev, substream, cmd);
+		break;
+	default:
+		break;
+	}
 
 	/* free PCM if reset_hw_params is set and the STOP IPC is successful */
 	if (!ret && reset_hw_params)
diff --git a/sound/soc/sof/sof-audio.h b/sound/soc/sof/sof-audio.h
index 81685e778ad6..6c64376858b3 100644
--- a/sound/soc/sof/sof-audio.h
+++ b/sound/soc/sof/sof-audio.h
@@ -106,6 +106,8 @@ struct snd_sof_dai_config_data {
  * @delay: Function pointer for pcm delay calculation
  * @reset_hw_params_during_stop: Flag indicating whether the hw_params should be reset during the
  *				 STOP pcm trigger
+ * @ipc_first_on_start: Send IPC before invoking platform trigger during
+ *				START/PAUSE_RELEASE triggers
  */
 struct sof_ipc_pcm_ops {
 	int (*hw_params)(struct snd_soc_component *component, struct snd_pcm_substream *substream,
@@ -120,6 +122,7 @@ struct sof_ipc_pcm_ops {
 	snd_pcm_sframes_t (*delay)(struct snd_soc_component *component,
 				   struct snd_pcm_substream *substream);
 	bool reset_hw_params_during_stop;
+	bool ipc_first_on_start;
 };
 
 /**
-- 
2.40.0

Re: [PATCH 0/3] ASoC: SOF: pcm/Intel: Handle IPC dependent sequencing correctly

[Mark Brown]

On Wed, 22 Mar 2023 11:43:43 +0200, Peter Ujfalusi wrote:
> IPC3 and IPC4 firmwares handle and execute tasks at different stages, like
> managing DMAs.
> In most cases these are aligned, but we have few exceptions that needs to be
> handled differently.
> 
> This series introduces flags to handle the differing cases to make sure that
> the correct sequencing is used regerless of the IPC version.
> 
> [...]

Applied to

   broonie/sound.git for-next

Thanks!

[1/3] ASoC: SOF: Intel: hda-dai: Do not perform DMA cleanup during stop
      commit: 1bf83fa6654ce8959948878aad14a6db586125b8
[2/3] ASoC: SOF: pcm: Make hw_params reset conditional for IPC3
      commit: 7d6f623c6a9d05195d1b19120383d4f42a1747db
[3/3] ASoC: SOF: pcm: Improve the pcm trigger sequence
      commit: 51ce3e6effab4fd4e13a3f187f4e256259f6e5a4

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 0/3] ASoC: SOF: pcm/Intel: Handle IPC dependent sequencing correctly

[Sam Edwards]

Hi folks,

When I upgraded my system to 6.4.0, I encountered a regression in audio 
output. In regression testing, I found that patch 1/3 here was the 
culprit, and the regression goes away entirely (on 6.4.0 final) when 
applying a patch that reverts this whole patchset. The problem is 
currently still unresolved even in broonie/sound.git.

The regression is an intermittent (few minutes on, few minutes off) 
distortion in audio output on my Tigerlake->ALC298 path. When playing a 
440 Hz test tone, the output spectrum is distorted into 440 Hz, 560 Hz, 
1440 Hz, 1560 Hz, 2440 Hz, 2560 Hz, and so on. Since this is the exact 
spectrum one would get if the output were modulated with a 1000 Hz Dirac 
comb, I interpret this to mean that the audio subsystem is dropping 
(zeroing) 1 sample every 1ms.

There seem to be conditions for this problem to come and go 
spontaneously -- in particular, it won't happen if my nvidia driver is 
unloaded. However, I can make it occur (even with no out-of-tree modules 
loaded) by sending several SIGSTOP->10ms->SIGCONT sequences to my 
pipewire daemon while it's playing audio. The distortion then continues 
until I send several more signals of that same sequence.

Now, aside from having some DSP background, I'm a total outsider to the 
ALSA and SOF world, so what follows is mere speculation on my part: I 
believe the problem has some probability of being "toggled" by a buffer 
underrun, which happens either deliberately by briefly interrupting 
pipewire, or accidentally due to bus contention from having my GPU 
active. Something (userspace? ALSA?) tries to restart the stream in 
response to that underrun, but this patchset makes stream stop+start 
more of a "warm reset," in that it doesn't clean up DMA. As a result, an 
off-by-one error somehow creeps into the DMA size, thus omitting the 
final sample of every 1ms transfer.

I am not sure if this is a regression introduced with this patchset, or 
merely a different bug that became apparent now that DMA isn't being 
reset when underruns happen. If it's the latter case, I'm happy to open 
an issue on Bugzilla instead. In either case, let me know if I can 
provide any additional troubleshooting information.

Cheers,

Sam

Re: [PATCH 0/3] ASoC: SOF: pcm/Intel: Handle IPC dependent sequencing correctly

[Pierre-Louis Bossart]

On 6/30/23 08:33, Sam Edwards wrote:
> Hi folks,
> 
> When I upgraded my system to 6.4.0, I encountered a regression in audio
> output. In regression testing, I found that patch 1/3 here was the
> culprit, and the regression goes away entirely (on 6.4.0 final) when
> applying a patch that reverts this whole patchset. The problem is
> currently still unresolved even in broonie/sound.git.
> 
> The regression is an intermittent (few minutes on, few minutes off)
> distortion in audio output on my Tigerlake->ALC298 path. When playing a
> 440 Hz test tone, the output spectrum is distorted into 440 Hz, 560 Hz,
> 1440 Hz, 1560 Hz, 2440 Hz, 2560 Hz, and so on. Since this is the exact
> spectrum one would get if the output were modulated with a 1000 Hz Dirac
> comb, I interpret this to mean that the audio subsystem is dropping
> (zeroing) 1 sample every 1ms.
> 
> There seem to be conditions for this problem to come and go
> spontaneously -- in particular, it won't happen if my nvidia driver is
> unloaded. However, I can make it occur (even with no out-of-tree modules
> loaded) by sending several SIGSTOP->10ms->SIGCONT sequences to my
> pipewire daemon while it's playing audio. The distortion then continues
> until I send several more signals of that same sequence.
> 
> Now, aside from having some DSP background, I'm a total outsider to the
> ALSA and SOF world, so what follows is mere speculation on my part: I
> believe the problem has some probability of being "toggled" by a buffer
> underrun, which happens either deliberately by briefly interrupting
> pipewire, or accidentally due to bus contention from having my GPU
> active. Something (userspace? ALSA?) tries to restart the stream in
> response to that underrun, but this patchset makes stream stop+start
> more of a "warm reset," in that it doesn't clean up DMA. As a result, an
> off-by-one error somehow creeps into the DMA size, thus omitting the
> final sample of every 1ms transfer.
> 
> I am not sure if this is a regression introduced with this patchset, or
> merely a different bug that became apparent now that DMA isn't being
> reset when underruns happen. If it's the latter case, I'm happy to open
> an issue on Bugzilla instead. In either case, let me know if I can
> provide any additional troubleshooting information.

please file an issue here: https://github.com/thesofproject/linux/issues

It would help if you clarified a bit more what the issue is, it's not
clear to me if the problem happens during a long continuous playback or
when starting/stopping the stream.

Also try to disable SOF with the instructions in
https://thesofproject.github.io/latest/getting_started/intel_debug/suggestions.html
to make sure it's not an HDaudio codec issue.

Thanks!