[ranj063]

1. snd_pcm_suspend() is going to be a static function. So move to using snd_pcm_suspend_all() instead. But to avoid any race conditions in situations that the streams are stopped prior to suspend (as in the case of chrome), only suspend those pcm's which have at least one running stream.
2. send ipc to stop stream when a suspend trigger is received.
3. Because snd_pcm_suspend_all() suspends all streams in a pcm, unset restore_stream in hw_params to avoid duplicating the call to hw_params for streams that were suspended even though they werent running.

CHanges in V2:

1. add pm ops for bxT_pcm521x machine driver
2. remove suspend streams in SOF suspend callback

[ranj063]

fixes #570

[plbossart]

This doesn't seem aligned with ALSA directions, see below.

[plbossart]

@ranj063 if you look at recent patches from Takashi Iwai on alsa-devel, tracked with issue #550, the direction is to make snd_pcm_suspend a static function and let the framework deal with PCM suspend. Is this whole sequence really necessary?

[ranj063]

@plbossart I'll have to check how the framework handles pcm suspend. let me look into it today.

[ranj063]

@plbossart I've updated the PR now. And we should be good when the changes in Takashi's proposal are merged.

[plbossart]

I am really confused by the approach here.

ASoC defines a set of routines to deal with PM. Most drivers seem to use them, and some Intel boards do

intel/boards/bxt_da7219_max98357a.c:            .pm = &snd_soc_pm_ops,
intel/boards/bxt_rt298.c:               .pm = &snd_soc_pm_ops,
intel/boards/byt-max98090.c:            .pm = &snd_soc_pm_ops,
intel/boards/byt-rt5640.c:              .pm = &snd_soc_pm_ops,
intel/boards/glk_rt5682_max98357a.c:            .pm = &snd_soc_pm_ops,
intel/boards/kbl_da7219_max98357a.c:            .pm = &snd_soc_pm_ops,
intel/boards/kbl_da7219_max98927.c:             .pm = &snd_soc_pm_ops,
intel/boards/kbl_rt5660.c:              .pm = &snd_soc_pm_ops,
intel/boards/kbl_rt5663_max98927.c:             .pm = &snd_soc_pm_ops,
intel/boards/kbl_rt5663_rt5514_max98927.c:              .pm = &snd_soc_pm_ops,
intel/boards/skl_hda_dsp_generic.c:             .pm = &snd_soc_pm_ops,
intel/boards/skl_nau88l25_max98357a.c:          .pm = &snd_soc_pm_ops,
intel/boards/skl_nau88l25_ssm4567.c:            .pm = &snd_soc_pm_ops,
intel/boards/skl_rt286.c:               .pm = &snd_soc_pm_ops,
intel/boards/sof_rt5682.c:              .pm = &snd_soc_pm_ops,

So my problem is why we need to deal with stream suspend/resume if we already have a framework support for it?

[ranj063]

I am really confused by the approach here.

ASoC defines a set of routines to deal with PM. Most drivers seem to use them, and some Intel boards do

intel/boards/bxt_da7219_max98357a.c:            .pm = &snd_soc_pm_ops,
intel/boards/bxt_rt298.c:               .pm = &snd_soc_pm_ops,
intel/boards/byt-max98090.c:            .pm = &snd_soc_pm_ops,
intel/boards/byt-rt5640.c:              .pm = &snd_soc_pm_ops,
intel/boards/glk_rt5682_max98357a.c:            .pm = &snd_soc_pm_ops,
intel/boards/kbl_da7219_max98357a.c:            .pm = &snd_soc_pm_ops,
intel/boards/kbl_da7219_max98927.c:             .pm = &snd_soc_pm_ops,
intel/boards/kbl_rt5660.c:              .pm = &snd_soc_pm_ops,
intel/boards/kbl_rt5663_max98927.c:             .pm = &snd_soc_pm_ops,
intel/boards/kbl_rt5663_rt5514_max98927.c:              .pm = &snd_soc_pm_ops,
intel/boards/skl_hda_dsp_generic.c:             .pm = &snd_soc_pm_ops,
intel/boards/skl_nau88l25_max98357a.c:          .pm = &snd_soc_pm_ops,
intel/boards/skl_nau88l25_ssm4567.c:            .pm = &snd_soc_pm_ops,
intel/boards/skl_rt286.c:               .pm = &snd_soc_pm_ops,
intel/boards/sof_rt5682.c:              .pm = &snd_soc_pm_ops,

So my problem is why we need to deal with stream suspend/resume if we already have a framework support for it?

@plbossart I might be totally wrong here but snd_soc_pm_ops looks like it was defined for the ASoC platform driver. And I believe the machine driver is using the same pm ops instead of defining a new one.

To address you question of handling stream suspend, if I don't do it in SOF, nothing ever happens. I checked the skylake driver and I dont see that it handles stream suspend at all. I am not sure how else to get it working.

[plbossart]

So my problem is why we need to deal with stream suspend/resume if we already have a framework support for it? @plbossart <https://github.com/plbossart> I might be totally wrong here but snd_soc_pm_ops looks like it was defined for the ASoC platform driver. And I believe the machine driver is using the same pm ops instead of defining a new one. To address you question of handling stream suspend, if I don't do it in SOF, nothing ever happens. I checked the skylake driver and I dont see that it handles stream suspend at all. I am not sure how else to get it working.

git grep snd_soc_pm_ops tells me it's a card/machine driver level. And maybe we are missing it e.g. in all our SOF machine drivers...You will note it's enabled in all Chromebooks machine drivers so that's a hint. It was historically not enabled for Atom/haswell/broadwell devices since there were weird things to deal with, but in our case I really wonder if all we need to do is just save/restore context but leave streams alone. That said doing most of the work at the card level is interesting, and maybe not quite aligned with the parent/child relationship we want to have.

[ranj063]

So my problem is why we need to deal with stream suspend/resume if we already have a framework support for it? @plbossart https://github.com/plbossart I might be totally wrong here but snd_soc_pm_ops looks like it was defined for the ASoC platform driver. And I believe the machine driver is using the same pm ops instead of defining a new one. To address you question of handling stream suspend, if I don't do it in SOF, nothing ever happens. I checked the skylake driver and I dont see that it handles stream suspend at all. I am not sure how else to get it working.
git grep snd_soc_pm_ops tells me it's a card/machine driver level. And maybe we are missing it e.g. in all our SOF machine drivers...You will note it's enabled in all Chromebooks machine drivers so that's a hint. It was historically not enabled for Atom/haswell/broadwell devices since there were weird things to deal with, but in our case I really wonder if all we need to do is just save/restore context but leave streams alone. That said doing most of the work at the card level is interesting, and maybe not quite aligned with the parent/child relationship we want to have.

@plbossart the haswell driver is the only one that deals with stream suspend. I'll need to unwrap the mysetery behind how the skylake driver suspends streams without the call to snd_pcm_suspend_all(). Let me talk to some folks tonight and get some information.

Also, on the chromebook, suspending from the UI works just fine. But it fails from the command line. So even though the machine driver includes the pm ops, it doesnt seem to help with the command line case.

[ranj063]

@plbossart i finally got the suspend to work with audio playback from cmd line by making this change and adding snd_soc_pm_ops to the bxt_pcm_512x machine driver:

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index fc7dadfbc10f..93c188fb1400 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -16,6 +16,7 @@
 //   o More testing with other codecs/machines.
 //   o Add more codecs and platforms to ensure good API coverage.
 //   o Support TDM on PCM and I2S
+#define DEBUG
 
 #include <linux/module.h>
 #include <linux/moduleparam.h>
@@ -471,12 +472,13 @@ int snd_soc_suspend(struct device *dev)
                                drv->ops->digital_mute(dai, 1);
                }
        }
+       dev_dbg(card->dev, "ranjani: suspending all pcm's %d\n", card->num_rtd);
 
        /* suspend all pcms */
        for_each_card_rtds(card, rtd) {
-               if (rtd->dai_link->ignore_suspend)
+               if (rtd->dai_link->ignore_suspend || rtd->dai_link->no_pcm)
                        continue;
-
+               dev_dbg(card->dev, "ranjani: suspending all pcm's\n");
                snd_pcm_suspend_all(rtd->pcm);
        }

Basically, the system was rebooting because the rtd_list contained BE dai links and sending the snd_pcm_suspend_all(rtd->pcm) for those BE links fails miserably.

So now the question is:

1. Should the rtd_list contain BE dai_lnks?
2. If so, how do we handle this case? I thought the trigger action for BE dai links does nothing. But looks like there's more to it than meets the eye.
3. If not, why are they getting added in SOF?

@keyonjie could you also comment?

[keyonjie]

@plbossart i finally got the suspend to work with audio playback from cmd line by making this change and adding snd_soc_pm_ops to the bxt_pcm_512x machine driver:

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index fc7dadfbc10f..93c188fb1400 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -16,6 +16,7 @@
// o More testing with other codecs/machines.
// o Add more codecs and platforms to ensure good API coverage.
// o Support TDM on PCM and I2S
+#define DEBUG

#include <linux/module.h>
#include <linux/moduleparam.h>
@@ -471,12 +472,13 @@ int snd_soc_suspend(struct device *dev)
drv->ops->digital_mute(dai, 1);
}
}

•   dev_dbg(card->dev, "ranjani: suspending all pcm's %d\n", card->num_rtd);
  
    /* suspend all pcms */
    for_each_card_rtds(card, rtd) {
  

•           if (rtd->dai_link->ignore_suspend)
  

•           if (rtd->dai_link->ignore_suspend || rtd->dai_link->no_pcm)
                    continue;
  

•           dev_dbg(card->dev, "ranjani: suspending all pcm's\n");
            snd_pcm_suspend_all(rtd->pcm);
    }
  

Basically, the system was rebooting because the rtd_list contained BE dai links and sending the snd_pcm_suspend_all(rtd->pcm) for those BE links fails miserably.

So now the question is:

1. Should the rtd_list contain BE dai_lnks?
2. If so, how do we handle this case? I thought the trigger action for BE dai links does nothing. But looks like there's more to it than meets the eye.
3. If not, why are they getting added in SOF?

@keyonjie could you also comment?

@ranj063 have you printed out rtd->pcm here for BE dai_links, I just went though ASoC code quickly, we are creating internal PCM for BE dai_links, so suspend them looks no problem to me, these internal PCMs don't have trigger ops so trigger suspend should do nothing there.

[ranj063]

@keyonjie its failing precisely because the internal pcm substream's dont have any ops set. In snd_pcm_do_suspend() the call to substream->ops->trigger is invalid because there are no ops set and this causes a null pointer exception that causes the system to reboot.

[plbossart]

Nice catch. I don't think this code ever worked :-(

	if (rtd->dai_link->no_pcm) {
		if (playback)
			pcm->streams[SNDRV_PCM_STREAM_PLAYBACK].substream->private_data = rtd;
		if (capture)
			pcm->streams[SNDRV_PCM_STREAM_CAPTURE].substream->private_data = rtd;
		goto out; //<<<< this bypasses all the ops initializations!!
	}

	/* ASoC PCM operations */
	if (rtd->dai_link->dynamic) {
		rtd->ops.open		= dpcm_fe_dai_open;
		rtd->ops.hw_params	= dpcm_fe_dai_hw_params;
		rtd->ops.prepare	= dpcm_fe_dai_prepare;
		rtd->ops.trigger	= dpcm_fe_dai_trigger;
		rtd->ops.hw_free	= dpcm_fe_dai_hw_free;
		rtd->ops.close		= dpcm_fe_dai_close;
		rtd->ops.pointer	= soc_pcm_pointer;
		rtd->ops.ioctl		= soc_pcm_ioctl;
	} else {
		rtd->ops.open		= soc_pcm_open;
		rtd->ops.hw_params	= soc_pcm_hw_params;
		rtd->ops.prepare	= soc_pcm_prepare;
		rtd->ops.trigger	= soc_pcm_trigger;
		rtd->ops.hw_free	= soc_pcm_hw_free;
		rtd->ops.close		= soc_pcm_close;
		rtd->ops.pointer	= soc_pcm_pointer;
		rtd->ops.ioctl		= soc_pcm_ioctl;
	}

	for_each_rtdcom(rtd, rtdcom) {
		const struct snd_pcm_ops *ops = rtdcom->component->driver->ops;

		if (!ops)
			continue;

		if (ops->ack)
			rtd->ops.ack		= soc_rtdcom_ack;
		if (ops->copy_user)
			rtd->ops.copy_user	= soc_rtdcom_copy_user;
		if (ops->copy_kernel)
			rtd->ops.copy_kernel	= soc_rtdcom_copy_kernel;
		if (ops->fill_silence)
			rtd->ops.fill_silence	= soc_rtdcom_fill_silence;
		if (ops->page)
			rtd->ops.page		= soc_rtdcom_page;
		if (ops->mmap)
			rtd->ops.mmap		= soc_rtdcom_mmap;
	}

	if (playback)
		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops);

	if (capture)
		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops);

	for_each_rtdcom(rtd, rtdcom) {
		component = rtdcom->component;

		if (!component->driver->pcm_new)
			continue;

		ret = component->driver->pcm_new(rtd);
		if (ret < 0) {
			dev_err(component->dev,
				"ASoC: pcm constructor failed: %d\n",
				ret);
			return ret;
		}
	}

	pcm->private_free = soc_pcm_private_free;
out:
	dev_info(rtd->card->dev, "%s <-> %s mapping ok\n",
		 (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name,
		 cpu_dai->name);
	return ret;

[ranj063]

@plbossart @keyonjie checking for ops !- NULL before calling ops->trigger seems to work for getting suspend to work in the case of suspending during I2S playback. But there is still some more work to be done for the case of suspending during HDMI playback. I can see that the skylake driver saves the dpibr and lpib which we dont seem to do. I'm not an expert here. So I could use your help Keyon. I'll post my patches soon.

[plbossart]

But there is still some more work to be done for the case of suspending during HDMI playback. I can see that the skylake driver saves the dpibr and lpib which we dont seem to do. I'm not an expert here. So I could use your help Keyon. I'll post my patches soon.

There should be no difference between outputs? I guess saving/restoring DPIB/LPIB is tied to the effort to restart playback exactly from where the stream stopped, supposedly the DMA can deal with context. I don't think this ever worked and I don't see who would care if you restart 10 days later from the same sample you suspended on. This makes no sense from an audio perspective given the mute ramping that's needed anyways to avoid pops/clicks.

[ranj063]

@plbossart I've updated this PR now. I've added the pm ops to the bxt_pcm512x machine driver.

I think this should fix the following issues:
#592
#489 - fixed partially as only suspending during I2S playback is fixed. suspend during HDMI playback is still broken.

[ranj063]

But there is still some more work to be done for the case of suspending during HDMI playback. I can see that the skylake driver saves the dpibr and lpib which we dont seem to do. I'm not an expert here. So I could use your help Keyon. I'll post my patches soon.

There should be no difference between outputs? I guess saving/restoring DPIB/LPIB is tied to the effort to restart playback exactly from where the stream stopped, supposedly the DMA can deal with context. I don't think this ever worked and I don't see who would care if you restart 10 days later from the same sample you suspended on. This makes no sense from an audio perspective given the mute ramping that's needed anyways to avoid pops/clicks.

@plbossart but it seems to working with the skylake driver. So my guess is we are probably just missing some bits.

[plbossart]

On 1/24/19 12:52 PM, Ranjani Sridharan wrote: But there is still some more work to be done for the case of suspending during HDMI playback. I can see that the skylake driver saves the dpibr and lpib which we dont seem to do. I'm not an expert here. So I could use your help Keyon. I'll post my patches soon. There should be no difference between outputs? I guess saving/restoring DPIB/LPIB is tied to the effort to restart playback exactly from where the stream stopped, supposedly the DMA can deal with context. I don't think this ever worked and I don't see who would care if you restart 10 days later from the same sample you suspended on. This makes no sense from an audio perspective given the mute ramping that's needed anyways to avoid pops/clicks. @plbossart <https://github.com/plbossart> but it seems to working with the skylake driver. So my guess is we are probably just missing some bits.

What works? and what does 'work' mean? The Skylake machine drivers all seem to rely on soc_pm_ops so shouldn't have anything specific to suspend streams?

[ranj063]

On 1/24/19 12:52 PM, Ranjani Sridharan wrote: But there is still some more work to be done for the case of suspending during HDMI playback. I can see that the skylake driver saves the dpibr and lpib which we dont seem to do. I'm not an expert here. So I could use your help Keyon. I'll post my patches soon. There should be no difference between outputs? I guess saving/restoring DPIB/LPIB is tied to the effort to restart playback exactly from where the stream stopped, supposedly the DMA can deal with context. I don't think this ever worked and I don't see who would care if you restart 10 days later from the same sample you suspended on. This makes no sense from an audio perspective given the mute ramping that's needed anyways to avoid pops/clicks. @plbossart https://github.com/plbossart but it seems to working with the skylake driver. So my guess is we are probably just missing some bits.
What works? and what does 'work' mean? The Skylake machine drivers all seem to rely on soc_pm_ops so shouldn't have anything specific to suspend streams?

@plbossart I mean suspending the system during HDMI playback works with the skylake driver. But in our case, we run into IPC errors. I will post the logs on the issue soon.

[plbossart]

What works? and what does 'work' mean? The Skylake machine drivers all seem to rely on soc_pm_ops so shouldn't have anything specific to suspend streams? @plbossart <https://github.com/plbossart> I mean suspending the system during HDMI playback works with the skylake driver. But in our case, we run into IPC errors. I will post the logs on the issue soon.

That's a different story, I get IPC errors even without HDMI on Up2 :-(. Something is seriously broken in our IPC handling, it's just completely unpredictable when changing configurations and/or timing on the host side.

…

[bardliao]

@ranj063 I checked the code flow today. substream->ops will be assigned in soc_new_pcm().
But indeed substream->ops is null when I try to suspend during i2s playback.
So, I am wondering how could it happen.
https://elixir.bootlin.com/linux/latest/source/sound/core/pcm_lib.c#L485

void snd_pcm_set_ops(struct snd_pcm *pcm, int direction,
		     const struct snd_pcm_ops *ops)
{
	struct snd_pcm_str *stream = &pcm->streams[direction];
	struct snd_pcm_substream *substream;
	
	for (substream = stream->substream; substream != NULL; substream = substream->next)
		substream->ops = ops;
}

https://elixir.bootlin.com/linux/latest/source/sound/soc/soc-pcm.c#L3136

	if (playback)
		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &rtd->ops);

	if (capture)
		snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_CAPTURE, &rtd->ops);