Skip to content
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

ASoC: SOF: Fix suspend while paused corner case #5054

Closed
wants to merge 8 commits into from
Closed

ASoC: SOF: Fix suspend while paused corner case #5054

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5a2443b
ASoC: SOF: Intel: hda-dai: Be really consistent with TRIGGER_SUSPEND …
ujfalusi Jun 10, 2024
9a7c7ab
ASoC: SOF: Intel: hda-dai-ops: ipc4: Reset the paused counter on STOP…
ujfalusi Jun 11, 2024
365cd78
ASoC: SOF: Add new optional suspend_early() dsp_ops callback function
ujfalusi Jun 10, 2024
5b5897a
ASoC: SOF: Intel: hda: Switch to use the suspend_early callback from …
ujfalusi Jun 11, 2024
d897da6
ASoC: SOF: Remove unused set_hw_params_upon_resume callback
ujfalusi Jun 11, 2024
09d8c8a
ASoC: SOF: sof-audio: Send STOP trigger to platforms during system su…
ujfalusi Jun 10, 2024
c3aa4bd
ASoC: SOF: ipc3-topology: Set the suspending flag for paused streams …
ujfalusi Jun 11, 2024
5c6fa89
ASoC: SOF: ipc4: Use the suspending stream flag to handle paused stre…
ujfalusi Jun 11, 2024
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
27 changes: 23 additions & 4 deletions sound/soc/sof/ipc4-pcm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ static void sof_ipc4_add_pipeline_by_priority(struct ipc4_pipeline_set_state_dat

static void
sof_ipc4_add_pipeline_to_trigger_list(struct snd_sof_dev *sdev, int state,
bool suspending,
struct snd_sof_pipeline *spipe,
struct ipc4_pipeline_set_state_data *trigger_list,
s8 *pipe_priority)
Expand All @@ -152,6 +153,18 @@ sof_ipc4_add_pipeline_to_trigger_list(struct snd_sof_dev *sdev, int state,
false);
break;
case SOF_IPC4_PIPE_RESET:
if (suspending) {
if (pipeline->state != SOF_IPC4_PIPE_PAUSED)
dev_warn(sdev->dev,
"%s: %s is not in PAUSED state"
"(state: %d, started/paused count: %d/%d)\n",
__func__, pipe_widget->widget->name,
pipeline->state, spipe->started_count, spipe->paused_count);

spipe->started_count = 0;
spipe->paused_count = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I am still not clear on when this paused_count would be restored when resuming back to a PAUSED state.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, so we have moved the pipelines to PAUSED state, it's started_count and paused_count is not 0, then we suspend the system.
We need to move the pipelines to RESET state to be able to UNBIND and then DELETE them. This is not optional, this is a must.
When the DSP is powered down we will not going to be able to PAUSE_RELEASE (not that we can do that correctly by PAUSE PUSH/RELEASE alone), so we need to reset these counters to 0, nothing will be running or being paused when the system resumes in firmware.

As for the PAUSE_RELEASE, ALSA will notice that we don't support RESUME, so it will re-start the stream.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this has been semi broken always. We had errors on suspend while paused, then we had errors on pause release and more errors when user space gave up, then we rpm suspend and reset the counters and on next try things would work.

Copy link
Member

Choose a reason for hiding this comment

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

My question was "when is paused_count increased again on resume" ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we receive PAUSE_PUSH. After system resume ALSA will not touch a paused PCM, from ALSA pow PAUSED == SUSPENDED and a PAUSED stream can only be changed via PAUSE_RELEASE (even a stop needs intermediate release).

Since we don't support RESUME (from suspend), when user sends the trigger PAUSE_RELEASE then ALSA will return with error and the stream is restarted.

The point is: when we resume from suspend we don't have any active pipelines in DSP, so no pipeline can have a started/paused counter other then 0.

Copy link
Member

Choose a reason for hiding this comment

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

so the pipeline is paused but not paused, depending on which state machine you're looking at, and you're relying on userspace to deal with errors?
Humm...this seems a bit weird if I am honest, we end-up with a non-symmetrical state after resume.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The PCM stream (as in ALSA terms) is paused but the state of the pipelines (as in IPC4 terms) are stopped (stopped and paused count == 0).
If the PCM is paused and the system suspends we will not get any triggers to react, so we end up with stale counters. We don't have PAUSED->SUSPENDED state transition handler (nether ALSA), but:
RUNNING->SUSPENDED is started_count-- (paused_count is 0) and if both 0 then we send the IPCs and stuff
PAUSED->RUNNING is paused_count-- (started_count is > 0)
so
PAUSE->SUSPENDED is rightfully started_count--, paused_count--, unless they were already 0, hrm, what if we have multiple paused streams with common pipelines? The first one will clear the shared portion, the second will clear it's own.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, multiple paused stream with common pipelines indeed is broken. (hw:0,0 and hw:0,31 on HDA machine)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But to be fair without this PR the whole system just locks up, so a bit better than what we have atm.

Copy link
Collaborator Author

@ujfalusi ujfalusi Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So, back again to the same page: #5040 is fixing all of these issues without any side effect.
single paused stream
two paused streams with shared pipeline section

}

/* RESET if the pipeline is neither running nor paused */
if (!spipe->started_count && !spipe->paused_count)
sof_ipc4_add_pipeline_by_priority(trigger_list, pipe_widget, pipe_priority,
Expand Down Expand Up @@ -436,18 +449,24 @@ static int sof_ipc4_trigger_pipelines(struct snd_soc_component *component,
* indeterministic. But the sink->source trigger order sink->source would still be
* guaranteed for each fork independently.
*/
if (state == SOF_IPC4_PIPE_RUNNING || state == SOF_IPC4_PIPE_RESET)
if (state == SOF_IPC4_PIPE_RUNNING || state == SOF_IPC4_PIPE_RESET) {
bool suspending = spcm->stream[substream->stream].suspending;

for (i = pipeline_list->count - 1; i >= 0; i--) {
spipe = pipeline_list->pipelines[i];
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list,
sof_ipc4_add_pipeline_to_trigger_list(sdev, state,
suspending, spipe,
trigger_list,
pipe_priority);
}
else
} else {
for (i = 0; i < pipeline_list->count; i++) {
spipe = pipeline_list->pipelines[i];
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, spipe, trigger_list,
sof_ipc4_add_pipeline_to_trigger_list(sdev, state, false,
spipe, trigger_list,
pipe_priority);
}
}

/* return if all pipelines are in the requested state already */
if (!trigger_list->count) {
Expand Down
2 changes: 2 additions & 0 deletions sound/soc/sof/ipc4-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3278,7 +3278,9 @@ static int sof_ipc4_tear_down_all_pipelines(struct snd_sof_dev *sdev, bool verif
continue;

if (spcm->stream[dir].list) {
spcm->stream[dir].suspending = true;
ret = sof_pcm_stream_free(sdev, substream, spcm, dir, true);
spcm->stream[dir].suspending = false;
if (ret < 0)
return ret;
}
Expand Down
Loading