Skip to content

ASoC: SOF: pcm: harden PCM STOP sequence #1169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
plbossart merged 1 commit into from
Sep 20, 2019
Merged

ASoC: SOF: pcm: harden PCM STOP sequence #1169

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion sound/soc/sof/pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
struct sof_ipc_stream stream;
struct sof_ipc_reply reply;
bool reset_hw_params = false;
bool ipc_first = false;
int ret;

/* nothing to do for BE */
Expand All @@ -343,6 +344,7 @@ static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
switch (cmd) {
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_PAUSE;
ipc_first = true;
break;
case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_RELEASE;
Expand All @@ -363,19 +365,30 @@ static int sof_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
stream.hdr.cmd |= SOF_IPC_STREAM_TRIG_STOP;
ipc_first = true;
reset_hw_params = true;
break;
default:
dev_err(sdev->dev, "error: unhandled trigger cmd %d\n", cmd);
return -EINVAL;
}

snd_sof_pcm_platform_trigger(sdev, substream, cmd);
/*
* 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);

/* send IPC to the DSP */
ret = sof_ipc_tx_message(sdev->ipc, stream.hdr.cmd, &stream,
sizeof(stream), &reply, sizeof(reply));

/* need to STOP DMA even if STOP IPC failed */
if (ipc_first)

@plbossart plbossart Sep 9, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiulipan in case the STOP IPC fails, is this expected that you still call platform_trigger. If yes you should make it explicit and explain why you are ignoring the return value and using it below.

@xiulipan xiulipan Sep 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart Will add comments. It seems either we fail the IPC or not we must stop the DMA stop.

@xiulipan xiulipan Sep 10, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@plbossart Another question. If IPC stop fails, do we need to free the PCM to make sure the pipeline can be reused?

@plbossart plbossart Sep 10, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i can you look into this? I don't recall the code changes that were committed in July and the test below for the pcm_free case looks suspicious to me.

@kv2019i kv2019i Sep 11, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiulipan @plbossart The fix looks good. The test for pcm_free indeed has the same problem. My assumption was the if the IPC fails for stop, something fatal has happened and no bother sending the dsp_pcm_free(). But if stop will fail in an xrun case like @xiulipan you explained earlier, that changes things. Probably we should use different logic for start and stop -- abort with error if start fails to an intermediate step. For stopping/pausing, we should send all the IPCs still despite errors to ensure we free resources. To me it still sounds a bit fishy that stop would legitimately return an error, but if that's the FW API, then we have to follow it.

That does raise other issue. I tried to think of a nicer way to implement the "ipc_first" logic. If we now have differences in error handling, that will make the two code paths even more different. Maybe time for a different subroutines for starting data transfer and stopping it.:::?

@xiulipan xiulipan Sep 12, 2019

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kv2019i
So you would suggest we will always send FREE IPC no matter if IPC STOP fail
actually, we will only send IPC FREE after IPC STOP.

Actually I tried with some other way like switch case the start and stop in different code. But @ranj063 suggest to go with some simple flag. And this really looks simple but need a few comments to explain why we need do like that.

snd_sof_pcm_platform_trigger(sdev, substream, cmd);
Comment thread
xiulipan marked this conversation as resolved.
Resolved

/* free PCM if reset_hw_params is set and the STOP IPC is successful */
if (!ret && reset_hw_params)
ret = sof_pcm_dsp_pcm_free(substream, sdev, spcm);

Expand Down