ASoC: SOF: pcm: Do not invoke sof_pcm_stream_free() during stop trigger by ranj063 · Pull Request #4235 · thesofproject/linux

ranj063 · 2023-03-09T20:59:03Z

Signal start/stop tests perform stream start/stop and restart without doing a hw_free. This means that the widget list associated with a stream is never freed after a stop. Change this to always free the widget list during stop so that the behaviour will be consistent with the regular PCM open/close.

Partially fixes thesofproject/sof#6723 i.e. the check-signal-stop-start-playback-50.sh and check-signal-stop-start-capture-50.sh cases for MTL Nocodec

fredoh9 · 2023-03-09T22:07:01Z

I'm able to verify that this fixes check-signal-stop-start test for playback. looks very solid pass with this fix. tested with NOCODEC mode.

TPLG=/lib/firmware/intel/sof-ace-tplg/sof-mtl-nocodec.tplg ~/sof-test/test-case/check-signal-stop-start.sh -c 10 -m playback

However, same test for capture is still broken.

TPLG=/lib/firmware/intel/sof-ace-tplg/sof-mtl-nocodec.tplg ~/sof-test/test-case/check-signal-stop-start.sh -c 10 -m capture

plbossart · 2023-03-09T23:54:36Z

@ranj063 is this really a MTL problem or more general all IPC4 targets are broken for start/stop?

ranj063 · 2023-03-10T00:22:09Z

@ranj063 is this really a MTL problem or more general all IPC4 targets are broken for start/stop?

@plbossart TGL/ADL seem to be OK. I'm now wondering if they work only by accident

bardliao

LGTM

RanderWang · 2023-03-10T09:42:49Z

 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_STOP:
 		ret = hda_link_dma_cleanup(substream, hext_stream, dai);


hda_link_dma_prepare is called by ASoC framework, not by widget prepare. If we don't clean up hda_link_dma, do we need to skip some hda_link_dma_prepare ?

RanderWang · 2023-03-10T09:43:18Z

 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_STOP:
 		ret = hda_link_dma_cleanup(substream, hext_stream, dai);


hda_link_dma_prepare is called by ASoC framework, not by widget setup. If we don't clean up hda_link_dma, do we need to skip some hda_link_dma_prepare ?

not following @RanderWang 's question. Can you please elaborate?

hda_link_dma_cleanup is not called by TRIGGER STOP now, but where is hda_link_dma_prepare called ? hda_link_dma_prepare is paired with hda_link_dma_cleanup, if we remove one hda_link_dma_cleanup, do we need to remove one hda_link_dma_prepare ? I found hda_link_dma_prepare is called by ASoC framework directly.

what do you mean by hda_link_dma_prepare? There is no such function? Anyway, if I can guess what you're asking, the stream tag is assigned in hda_link_hw_params() and with a repeated start, if the stream tag is not freed, prepare wouldnt really do anything, thus preserving the original stream tag for the start trigger

o, my code is out of date. I can't find it after updating code

ujfalusi · 2023-03-10T11:25:43Z

 	struct snd_sof_dev *sdev = snd_soc_component_get_drvdata(component);
 	const struct sof_ipc_pcm_ops *pcm_ops = sof_ipc_get_ops(sdev, pcm);
 	struct snd_sof_pcm *spcm;
-	bool reset_hw_params = false;


The flag was added for a reason, at least the commit message was very specific on what this is suppose to handle:
04c8027 ("ASoC: SOF: reset DMA state in prepare")

It might lost it's purpose and relevance over time, but aren't we going to break something with this?

@kv2019i ?

@ujfalusi allot of has changed in terms of how we free the widgets with all.thenwork around ipc4. And if you think about it, platform_hw_params gets the stream tag and if with start/stop tests,.there's no setting up widgets again, we need preserve the original tag.

plbossart

Not following if the second patch is redundant or needed. Maybe a host- of link-dma difference between the two?

plbossart · 2023-03-13T17:05:48Z

 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-	case SNDRV_PCM_TRIGGER_STOP:
 		ret = hda_link_dma_cleanup(substream, hext_stream, dai);


not following @RanderWang 's question. Can you please elaborate?

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>

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>

plbossart

Thanks @ranj063

ranj063 requested review from ajitkupandey, bardliao, dbaluta, juimonen, kuanhsuncheng, kv2019i, lgirdwood, lyakh, plbossart, ujfalusi and yaochunhung as code owners March 9, 2023 20:59

ranj063 marked this pull request as draft March 9, 2023 21:37

ranj063 force-pushed the fix/start_stop branch from 0a9af43 to 4b2330f Compare March 9, 2023 21:40

ranj063 force-pushed the fix/start_stop branch from 4b2330f to 0819210 Compare March 10, 2023 00:13

ranj063 force-pushed the fix/start_stop branch from 0819210 to e283ce0 Compare March 10, 2023 00:42

ranj063 changed the title ~~ASoC: SOF: pcm: Always free widget list during stop/suspend trigger~~ ASoC: SOF: pcm: Do not invoke sof_pcm_stream_free() during stop trigger Mar 10, 2023

ranj063 force-pushed the fix/start_stop branch 4 times, most recently from 479e0cf to 6b923fc Compare March 10, 2023 05:04

ranj063 marked this pull request as ready for review March 10, 2023 06:15

ranj063 requested review from RanderWang and libinyang as code owners March 10, 2023 06:15

bardliao previously approved these changes Mar 10, 2023

View reviewed changes

RanderWang reviewed Mar 10, 2023

View reviewed changes

ujfalusi reviewed Mar 10, 2023

View reviewed changes

ranj063 dismissed bardliao’s stale review via 3f26c5d March 11, 2023 02:00

ranj063 force-pushed the fix/start_stop branch from 6b923fc to 3f26c5d Compare March 11, 2023 02:00

plbossart reviewed Mar 13, 2023

View reviewed changes

ranj063 force-pushed the fix/start_stop branch from 3f26c5d to cb4b6f2 Compare March 13, 2023 23:48

ranj063 requested review from RanderWang, bardliao, plbossart and ujfalusi March 13, 2023 23:48

plbossart requested changes Mar 14, 2023

View reviewed changes

Comment thread sound/soc/sof/intel/hda-dai.c

ranj063 added 2 commits March 13, 2023 17:49

ranj063 force-pushed the fix/start_stop branch from cb4b6f2 to feb21d4 Compare March 14, 2023 00:50

ranj063 requested a review from plbossart March 14, 2023 00:51

plbossart approved these changes Mar 14, 2023

View reviewed changes

ranj063 mentioned this pull request Mar 14, 2023

[BUG] I/O error with DMIC capture pause/release on MTL thesofproject/sof#6723

Closed

RanderWang approved these changes Mar 14, 2023

View reviewed changes

bardliao approved these changes Mar 14, 2023

View reviewed changes

plbossart merged commit 63f8775 into thesofproject:topic/sof-dev Mar 14, 2023

Uh oh!

Conversation

ranj063 commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredoh9 commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

plbossart commented Mar 9, 2023

Uh oh!

ranj063 commented Mar 10, 2023

Uh oh!

bardliao left a comment

Choose a reason for hiding this comment

Uh oh!

RanderWang Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

RanderWang Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

plbossart Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

RanderWang Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ranj063 Mar 14, 2023

Choose a reason for hiding this comment

Uh oh!

RanderWang Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ujfalusi Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

ranj063 Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

plbossart Mar 13, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

plbossart left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ranj063 commented Mar 9, 2023 •

edited

Loading

fredoh9 commented Mar 9, 2023 •

edited

Loading

RanderWang Mar 14, 2023 •

edited

Loading

RanderWang Mar 14, 2023 •

edited

Loading