ASoC: SOF: Fix/stop sequences by plbossart · Pull Request #4816 · thesofproject/linux

plbossart · 2024-02-13T21:11:11Z

tentative solution to address the problems of xruns with IPC4, where we don't properly reset DMAs/PCM.

The alternative suggested by @ujfalusi would be this

diff --git a/sound/soc/sof/ipc4-pcm.c b/sound/soc/sof/ipc4-pcm.c
index 0f332c8cdbe6..004971c8dbb5 100644
--- a/sound/soc/sof/ipc4-pcm.c
+++ b/sound/soc/sof/ipc4-pcm.c
@@ -943,5 +943,7 @@ const struct sof_ipc_pcm_ops ipc4_pcm_ops = {
 	.pcm_free = sof_ipc4_pcm_free,
 	.delay = sof_ipc4_pcm_delay,
 	.ipc_first_on_start = true,
-	.platform_stop_during_hw_free = true,
+	//.platform_stop_during_hw_free = true,
+	.platform_stop_during_hw_free = false,
+	.reset_hw_params_during_stop = true,
 };

~~FIXME: the firmware doesn't seem to like the new transitions and loop on an error 104 which ends-up hanging the system. Probably some weird transitions I missed.~~

Edit: the error 104 was due to the SoundWire node_id issue not reset properly.

ujfalusi · 2024-02-14T05:38:40Z

SoundWire does not like this for sure.

plbossart · 2024-02-15T00:09:03Z

SoundWire does not like this for sure.

yes indeed @ujfalusi ...

Here's the summary of my convoluted day with lots of help from @ranj063.

Freeing the widgets in .prepare is problematic because we only send the alh_index_id in the .hw_params DAI stage.
But not freeing the widgets is problematic as well because as a result we can't do the platform_hw_free either, otherwise there's a risk of the wrong stream tag being used (that would be a corner case where an xrun occurs while starting another stream).

So in the end we only do a pcm_ops->hw_free, which as not indicated by its name is not at all the dual operation to hw_params. pcm_ops->hw_free is really a 'post trigger' operation, and it does make sense to invoke this with the delayed stop.

The last set of commits work for me in all know sof-test cases and the xrun with the gnome UI on a MTL/SoundWire device.

Now it's time to debate if PR #4818 is more elegant/relevant/risky.

ranj063 · 2024-02-15T01:57:23Z

SOFCI TEST

For MeteorLake support, we need this helper back. It's not needed for previous solutions from CometLake to TigerLake, but with the change in IPC additional steps need to be done to clear the gateway node_id. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

The widget_to_sdev() helper can be used in other files, move it as static inline in shared header file. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

When an xrun happens, the BE DAI hw_params doesn't get invoked before the stream restarts with a prepare. In this case, clearing the node ID when the DAI widget is freed and unprepared will result in an error when it is re-initialized. In order to avoid this, move the code to clear the node ID to the BE DAI hw_free op to keep it balanced with the BE DAI hw_params. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

…d logs For some reason the existing code stops on the first error, which potentially leaves the DMA and widgets in a weird state. Change to free-up all resources even in case of errors. Also add a more consistent error handling and logs, with the first error code returned to the caller. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The same sequence is used twice, use common helper. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Add a state variable to keep track of delayed stops, in case pcm_ops->platform_stop_during_hw_free is set. This patch should be iso-functionality, possibly removing no-op cases. The main purpose of this new state variable is to prepare a follow-up patch to reset all PCM and DMAs in case of stop/prepare xrun sequences. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

With the delayed stops, the xrun handling is problematic: the applications expects everything to be reset but the firmware and DMA are still in a PAUSED state. This patch makes sure the prepare while in stream_stopped is special-cased. Co-developed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The node_id for multi-gateway ALH DAI's get overwritten with the group_id during the DAI copier's ipc_prepare op. So, save the ALH dai_index during the BE DAI hw_params in the dai_index field of struct ipc4_copier and use that to set the device ID in the configuration blob. This will avoid errors during copier init after an xrun. Note that the dai_index is typically set in topology for DMIC/SSP, but it's not used for ALH. Reclaiming this dai_index field to store the node_id does not generate a conflict with topology-defined values. Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com>

plbossart · 2024-02-16T18:56:55Z

Issues don't seem to be related to this PR

https://sof-ci.01.org/linuxpr/PR4816/build1454/devicetest/index.html?model=MTLP_RVP_NOCODEC&testcase=check-playback-10sec The MOD_SET_DX looks like a known issue but I'll let @kv2019i and @ujfalusi comment

[  222.458916] kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-mtl 0000:00:1f.3: ipc tx      : 0x47000000|0x0: MOD_SET_DX [data size: 8]
[  222.964581] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ipc timed out for 0x47000000|0x0
[  222.964597] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: Attempting to prevent DSP from entering D3 state to preserve context
[  222.964603] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ------------[ IPC dump start ]------------
[  222.964637] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: Host IPC initiator: 0x47000000|0x0|0x0, target: 0x32000000|0x0|0x0, ctl: 0x3
[  222.964643] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ------------[ IPC dump end ]------------
[  222.964648] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ------------[ DSP dump start ]------------
[  222.964651] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: IPC timeout
[  222.964655] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: fw_state: SOF_FW_BOOT_COMPLETE (7)
[  222.964663] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: 0x50000005: module: ROM_EXT, state: FW_ENTERED, running
[  222.964674] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: Firmware state: 0x0, status/error code: 0x0
[  222.964731] kernel: snd_sof:sof_ipc4_find_debug_slot_offset_by_type: sof-audio-pci-intel-mtl 0000:00:1f.3: Slot type 0x4c455400 is not available in debug window
[  222.964735] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ------------[ DSP dump end ]------------
[  222.964760] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: failed to disable target core: 2 for pipeline pipeline.6
[  222.964766] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: Failed to free connected widgets
[  222.964777] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: sof_pcm_stream_free: sof_widget_list_free failed -110
[  222.964786] kernel: sof-audio-pci-intel-mtl 0000:00:1f.3: ASoC: error at snd_soc_pcm_component_hw_free on 0000:00:1f.3: -110
[  222.964896] kernel: snd_sof:sof_pcm_close: sof-audio-pci-intel-mtl 0000:00:1f.3: pcm: close stream 2 dir 0

https://sof-ci.01.org/linuxpr/PR4816/build1455/devicetest/index.html?model=TGLU_UP_HDA-ipc4&testcase=multiple-pause-resume-50 (filed as #4822)

 1043.495552] kernel: snd_sof_intel_hda_common:hda_dai_trigger: sof-audio-pci-intel-tgl 0000:00:1f.3: cmd=1 dai Analog CPU DAI direction 0
[ 1043.495562] kernel: snd_sof:sof_ipc4_set_pipeline_state: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc4 set pipeline instance 1 state 3
[ 1043.495567] kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx      : 0x13010003|0x0: GLB_SET_PIPELINE_STATE
[ 1043.495907] kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx reply: 0x33000000|0x0: GLB_SET_PIPELINE_STATE
[ 1043.495926] kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx done : 0x13010003|0x0: GLB_SET_PIPELINE_STATE
[ 1043.495930] kernel: snd_sof:sof_ipc4_set_pipeline_state: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc4 set pipeline instance 1 state 4
[ 1043.495934] kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx      : 0x13010004|0x0: GLB_SET_PIPELINE_STATE
[ 1043.496803] kernel: snd_sof:sof_ipc4_log_header: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc tx reply: 0x33000007|0x0: GLB_SET_PIPELINE_STATE
[ 1043.496810] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: FW reported error: 7 - Unsupported operation requested
[ 1043.496944] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: ipc error for msg 0x13010004|0x0
[ 1043.496950] kernel: sof-audio-pci-intel-tgl 0000:00:1f.3: ASoC: error at soc_dai_trigger on Analog CPU DAI: -22
[ 1043.496957] kernel:  HDA Analog: ASoC: trigger FE cmd: 1 failed: -22
[ 1043.496963] kernel: snd_sof:sof_pcm_trigger: sof-audio-pci-intel-tgl 0000:00:1f.3: pcm: trigger stream 0 dir 0 cmd 0
[ 1043.496968] kernel: snd_sof:sof_ipc4_trigger_pipelines: sof-audio-pci-intel-tgl 0000:00:1f.3: trigger cmd: 0 state: 3

ranj063

looks good to me!

bardliao · 2024-02-19T07:44:18Z

-				blob->alh_cfg.mapping[i].device = alh_data->gtw_cfg.node_id;
+				node_type = SOF_IPC4_GET_NODE_TYPE(alh_data->gtw_cfg.node_id);
+				blob->alh_cfg.mapping[i].device =
+					SOF_IPC4_GET_NODE_TYPE(node_type);


Shouldn't this be SOF_IPC4_NODE_TYPE(node_type);?

well spotted @bardliao that would indeed shift the initial value twice.

@ranj063 what do you think?

@bardliao thats correct. Let me fix this

@bardliao @plbossart fixed in #4829

plbossart · 2024-02-21T02:03:03Z

replaced by #4829

ranj063 reviewed Feb 13, 2024

View reviewed changes

Comment thread sound/soc/sof/pcm.c Outdated

plbossart commented Feb 14, 2024

View reviewed changes

Comment thread sound/soc/sof/pcm.c

plbossart force-pushed the fix/stop-sequences branch from df7eeb5 to 5c3ec6e Compare February 14, 2024 20:29

plbossart commented Feb 14, 2024

View reviewed changes

Comment thread sound/soc/sof/sof-audio.c Outdated

plbossart force-pushed the fix/stop-sequences branch from 5c3ec6e to e5e0a24 Compare February 14, 2024 23:21

plbossart mentioned this pull request Feb 15, 2024

[BUG] MTL: sound distortion for SDW devices on SOF topology2 thesofproject/sof#8846

Closed

plbossart force-pushed the fix/stop-sequences branch from ffee9fa to 3cec0be Compare February 15, 2024 20:50

ranj063 and others added 8 commits February 16, 2024 09:21

ASoC: SOF: Intel: hda: move helper to static inline

64c7987

The widget_to_sdev() helper can be used in other files, move it as static inline in shared header file. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

ASoC: SOF: pcm: simplify sof_pcm_hw_free() with helper

7780b61

The same sequence is used twice, use common helper. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart force-pushed the fix/stop-sequences branch from 3cec0be to 6cce39a Compare February 16, 2024 15:53

plbossart marked this pull request as ready for review February 16, 2024 18:58

plbossart requested review from bardliao, dbaluta, kv2019i, lgirdwood and ujfalusi as code owners February 16, 2024 18:58

plbossart changed the title ~~[WIP][DNM] ASoC: SOF: Fix/stop sequences~~ ASoC: SOF: Fix/stop sequences Feb 16, 2024

ranj063 mentioned this pull request Feb 16, 2024

ASoC: SOF: pcm: Always stop DMA in the case of stop trigger #4818

Closed

ranj063 approved these changes Feb 16, 2024

View reviewed changes

ranj063 mentioned this pull request Feb 18, 2024

[RFC] ASoC: SOF: ipc4-topology: reserve alh node_id for dai module #4794

Closed

bardliao reviewed Feb 19, 2024

View reviewed changes

ranj063 mentioned this pull request Feb 20, 2024

Fixes for xrun recovery and Soundwire node_id #4829

Merged

plbossart closed this Feb 21, 2024

Navigation Menu

Repository navigation

ASoC: SOF: Fix/stop sequences - #4816

ASoC: SOF: Fix/stop sequences#4816
plbossart wants to merge 8 commits into
thesofproject:topic/sof-devfrom
plbossart:fix/stop-sequences

plbossart commented Feb 13, 2024 •

edited

Loading

Uh oh!

Uh oh!

ujfalusi commented Feb 14, 2024

Uh oh!

Uh oh!

Uh oh!

plbossart commented Feb 15, 2024

Uh oh!

ranj063 commented Feb 15, 2024

Uh oh!

plbossart commented Feb 16, 2024 •

edited

Loading

Uh oh!

ranj063 left a comment

Uh oh!

bardliao Feb 19, 2024

Uh oh!

plbossart Feb 20, 2024

Uh oh!

ranj063 Feb 20, 2024

Uh oh!

ranj063 Feb 20, 2024

Uh oh!

plbossart commented Feb 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

plbossart commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ujfalusi commented Feb 14, 2024

Uh oh!

Uh oh!

Uh oh!

plbossart commented Feb 15, 2024

Uh oh!

ranj063 commented Feb 15, 2024

Uh oh!

plbossart commented Feb 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ranj063 left a comment

Choose a reason for hiding this comment

Uh oh!

bardliao Feb 19, 2024

Choose a reason for hiding this comment

Uh oh!

plbossart Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

ranj063 Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

ranj063 Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

plbossart commented Feb 21, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

plbossart commented Feb 13, 2024 •

edited

Loading

plbossart commented Feb 16, 2024 •

edited

Loading