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 all commits
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
2 changes: 1 addition & 1 deletion sound/soc/sof/intel/hda-common-ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,12 +89,12 @@ const struct snd_sof_dsp_ops sof_hda_common_ops = {
.is_chain_dma_supported = hda_is_chain_dma_supported,

/* PM */
.suspend_early = hda_dsp_suspend_early,
.suspend = hda_dsp_suspend,
.resume = hda_dsp_resume,
.runtime_suspend = hda_dsp_runtime_suspend,
.runtime_resume = hda_dsp_runtime_resume,
.runtime_idle = hda_dsp_runtime_idle,
.set_hw_params_upon_resume = hda_dsp_set_hw_params_upon_resume,
plbossart marked this conversation as resolved.
Show resolved Hide resolved

/* ALSA HW info flags */
.hw_info = SNDRV_PCM_INFO_MMAP |
Expand Down
6 changes: 4 additions & 2 deletions sound/soc/sof/intel/hda-dai-ops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,10 +416,12 @@ static int hda_ipc4_post_trigger(struct snd_sof_dev *sdev, struct snd_soc_dai *c
case SNDRV_PCM_TRIGGER_SUSPEND:
case SNDRV_PCM_TRIGGER_STOP:
/*
* STOP/SUSPEND trigger is invoked only once when all users of this pipeline have
* been stopped. So, clear the started_count so that the pipeline can be reset
* STOP/SUSPEND trigger is invoked only once when all users of
* this pipeline have been stopped. So, clear the started and
* paused count so that the pipeline can be reset
*/
swidget->spipe->started_count = 0;
swidget->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.

what would be the behavior on resume then? The stream would restart? How can we keep the stream paused 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.

we cannot keep the streams paused, we don't support RESUME trigger, so all streams would be restarted anyways, paused or not paused.
In practice: on resume the audio will remain paused (will not run), but when you PAUSE_RELEASE it, then we will restart it. We need to re-initialize everything to get working audio.

Copy link
Member

Choose a reason for hiding this comment

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

how will the audio remain paused if the pause_count remains zero? I don't understand how this counter is used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This counter is used internally to ipc4 (to track the individual pipeline states) , ALSA keeps track of the PCM state. On suspend we need to stop everything, so we can put the DSP to off.
So, when we suspend all pipelines will be reset.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ujfalusi this is too confusing. We increment the paused_count when we pause the stream. So lets assume this sequence:

  1. Start the stream : started_count = 1
  2. Pause the stream : paused_count = 1
  3. Suspend the system: The stream doesnt get the suspend trigger, so the started/paused_counts remain at 1
  4. hda_dai_suspend() gets invoked so both counts get reset to 0.
  5. Resume the system
  6. Pause_release the stream: this will decrement the paused_count to -1?
    Am I understanding this correctly?

break;
case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
break;
Expand Down
12 changes: 6 additions & 6 deletions sound/soc/sof/intel/hda-dai.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,12 +617,6 @@ static int hda_dai_suspend(struct hdac_bus *bus)
sdai = swidget->private;
ops = sdai->platform_private;

ret = hda_link_dma_cleanup(hext_stream->link_substream,
hext_stream,
cpu_dai);
if (ret < 0)
return ret;

/* for consistency with TRIGGER_SUSPEND */
if (ops->post_trigger) {
ret = ops->post_trigger(sdev, cpu_dai,
Expand All @@ -631,6 +625,12 @@ static int hda_dai_suspend(struct hdac_bus *bus)
if (ret < 0)
return ret;
}

ret = hda_link_dma_cleanup(hext_stream->link_substream,
hext_stream,
cpu_dai);
if (ret < 0)
return ret;
Copy link
Member

Choose a reason for hiding this comment

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

I can't recall if this was an editing mistake or intentional. @ranj063 would you happen to remember this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@plbossart I dont think the order really matters in this case. All we do in the post trigger op for suspend is clear the count for pipeline.

}
}

Expand Down
26 changes: 13 additions & 13 deletions sound/soc/sof/intel/hda-dsp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1007,6 +1007,19 @@ int hda_dsp_runtime_suspend(struct snd_sof_dev *sdev)
}
EXPORT_SYMBOL_NS(hda_dsp_runtime_suspend, SND_SOC_SOF_INTEL_HDA_COMMON);

int hda_dsp_suspend_early(struct snd_sof_dev *sdev)
{
int ret;

/* make sure all DAI resources are freed */
ret = hda_dsp_dais_suspend(sdev);
if (ret < 0)
dev_warn(sdev->dev, "%s: failure in hda_dsp_dais_suspend\n", __func__);

return ret;
}
EXPORT_SYMBOL_NS(hda_dsp_suspend_early, SND_SOC_SOF_INTEL_HDA_COMMON);

int hda_dsp_suspend(struct snd_sof_dev *sdev, u32 target_state)
{
struct sof_intel_hda_dev *hda = sdev->pdata->hw_pdata;
Expand Down Expand Up @@ -1148,19 +1161,6 @@ int hda_dsp_shutdown(struct snd_sof_dev *sdev)
}
EXPORT_SYMBOL_NS(hda_dsp_shutdown, SND_SOC_SOF_INTEL_HDA_COMMON);

int hda_dsp_set_hw_params_upon_resume(struct snd_sof_dev *sdev)
{
int ret;

/* make sure all DAI resources are freed */
ret = hda_dsp_dais_suspend(sdev);
if (ret < 0)
dev_warn(sdev->dev, "%s: failure in hda_dsp_dais_suspend\n", __func__);

return ret;
}
EXPORT_SYMBOL_NS(hda_dsp_set_hw_params_upon_resume, SND_SOC_SOF_INTEL_HDA_COMMON);

void hda_dsp_d0i3_work(struct work_struct *work)
{
struct sof_intel_hda_dev *hdev = container_of(work,
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/intel/hda.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,14 +605,14 @@ int hda_dsp_set_power_state_ipc3(struct snd_sof_dev *sdev,
int hda_dsp_set_power_state_ipc4(struct snd_sof_dev *sdev,
const struct sof_dsp_power_state *target_state);

int hda_dsp_suspend_early(struct snd_sof_dev *sdev);
int hda_dsp_suspend(struct snd_sof_dev *sdev, u32 target_state);
int hda_dsp_resume(struct snd_sof_dev *sdev);
int hda_dsp_runtime_suspend(struct snd_sof_dev *sdev);
int hda_dsp_runtime_resume(struct snd_sof_dev *sdev);
int hda_dsp_runtime_idle(struct snd_sof_dev *sdev);
int hda_dsp_shutdown_dma_flush(struct snd_sof_dev *sdev);
int hda_dsp_shutdown(struct snd_sof_dev *sdev);
int hda_dsp_set_hw_params_upon_resume(struct snd_sof_dev *sdev);
void hda_dsp_dump(struct snd_sof_dev *sdev, u32 flags);
void hda_ipc4_dsp_dump(struct snd_sof_dev *sdev, u32 flags);
void hda_ipc_dump(struct snd_sof_dev *sdev);
Expand Down
2 changes: 2 additions & 0 deletions sound/soc/sof/ipc3-topology.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2379,7 +2379,9 @@ static int sof_tear_down_left_over_pipelines(struct snd_sof_dev *sdev)
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
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
15 changes: 8 additions & 7 deletions sound/soc/sof/ops.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,14 @@ static inline int snd_sof_dsp_resume(struct snd_sof_dev *sdev)
return 0;
}

static inline int snd_sof_dsp_suspend_early(struct snd_sof_dev *sdev)
{
if (sof_ops(sdev)->suspend_early)
return sof_ops(sdev)->suspend_early(sdev);

return 0;
}

static inline int snd_sof_dsp_suspend(struct snd_sof_dev *sdev,
u32 target_state)
{
Expand Down Expand Up @@ -268,13 +276,6 @@ static inline int snd_sof_dsp_runtime_idle(struct snd_sof_dev *sdev)
return 0;
}

static inline int snd_sof_dsp_hw_params_upon_resume(struct snd_sof_dev *sdev)
{
if (sof_ops(sdev)->set_hw_params_upon_resume)
return sof_ops(sdev)->set_hw_params_upon_resume(sdev);
return 0;
}

static inline int snd_sof_dsp_set_clk(struct snd_sof_dev *sdev, u32 freq)
{
if (sof_ops(sdev)->set_clk)
Expand Down
21 changes: 10 additions & 11 deletions sound/soc/sof/pm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,16 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
if (runtime_suspend && !sof_ops(sdev)->runtime_suspend)
return 0;

/* Prepare the DSP for system suspend */
Copy link
Member

Choose a reason for hiding this comment

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

What would be the difference with a prepare callback, as we do for SoundWire to go top down and make sure all parent/children are properly pm_runtime resumed before the system suspend.

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 prepare callback happens before ALSA sends the suspend trigger (it is also sent in prepare callback), we use pm callbacks, not ASoC component callbacks..

Copy link
Member

Choose a reason for hiding this comment

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

i was talking about the pm .prepare callback, which is called before suspend. I was not referring to the prepare as a followup to hw_params - naming is confusing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I have tried that exactly as first thing, it does not work, it comes before the trigger suspend and breaks suspend with active audio.

Copy link
Member

Choose a reason for hiding this comment

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

fair enough. There should still be a more detailed description of what this callback should implement vs, what needs to remain in the suspend callback proper.
Last time we introduced the probe_early, it was rather straightforward as it included all the features that could not be done in a workqueue. A suspend_early isn't very clear.

if (!runtime_suspend) {
ret = snd_sof_dsp_suspend_early(sdev);
if (ret) {
dev_err(sdev->dev, "%s: DSP early suspend failed: %d\n",
__func__, ret);
return ret;
}
}

/* we need to tear down pipelines only if the DSP hardware is
* active, which happens for PCI devices. if the device is
* suspended, it is brought back to full power and then
Expand All @@ -221,17 +231,6 @@ static int sof_suspend(struct device *dev, bool runtime_suspend)
if (sdev->fw_state != SOF_FW_BOOT_COMPLETE)
goto suspend;

/* prepare for streams to be resumed properly upon resume */
if (!runtime_suspend) {
ret = snd_sof_dsp_hw_params_upon_resume(sdev);
if (ret < 0) {
dev_err(sdev->dev,
"error: setting hw_params flag during suspend %d\n",
ret);
return ret;
}
}

pm_state.event = target_state;

/* suspend DMA trace */
Expand Down
3 changes: 2 additions & 1 deletion sound/soc/sof/sof-audio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,8 @@ int sof_pcm_stream_free(struct snd_sof_dev *sdev, struct snd_pcm_substream *subs

if (spcm->prepared[substream->stream]) {
/* stop DMA first if needed */
if (pcm_ops && pcm_ops->platform_stop_during_hw_free)
if (spcm->stream[dir].suspending ||
(pcm_ops && pcm_ops->platform_stop_during_hw_free))
snd_sof_pcm_platform_trigger(sdev, substream, SNDRV_PCM_TRIGGER_STOP);

/* free PCM in the DSP */
Expand Down
3 changes: 3 additions & 0 deletions sound/soc/sof/sof-audio.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,9 @@ struct snd_sof_pcm_stream {
bool suspend_ignored;
struct snd_sof_pcm_stream_pipeline_list pipeline_list;

/* Flag to indicate that the stream is prepared for system suspend */
bool suspending;

/* used by IPC implementation and core does not touch it */
void *private;
};
Expand Down
2 changes: 1 addition & 1 deletion sound/soc/sof/sof-priv.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,13 +301,13 @@ struct snd_sof_dsp_ops {
const struct sof_ext_man_elem_header *hdr);

/* DSP PM */
int (*suspend_early)(struct snd_sof_dev *sof_dev); /* optional */
int (*suspend)(struct snd_sof_dev *sof_dev,
u32 target_state); /* optional */
int (*resume)(struct snd_sof_dev *sof_dev); /* optional */
int (*runtime_suspend)(struct snd_sof_dev *sof_dev); /* optional */
int (*runtime_resume)(struct snd_sof_dev *sof_dev); /* optional */
int (*runtime_idle)(struct snd_sof_dev *sof_dev); /* optional */
int (*set_hw_params_upon_resume)(struct snd_sof_dev *sdev); /* optional */
int (*set_power_state)(struct snd_sof_dev *sdev,
const struct sof_dsp_power_state *target_state); /* optional */

Expand Down
Loading