[alsa-devel] [PATCH 00/14] ALSA: PCM suspend cleanup
Hi,
this is a patch set to clean up the PCM suspend calls by introducing the PCM device PM ops. This won't change much for ASoC but reduce lots of snd_pcm_suspend*() calls in other sound drivers.
Note that this patch series itself won't fix anything about the recent PM issue reported for Intel ASoC, but it was inspired through the discussion there.
thanks,
Takashi
===
Takashi Iwai (14): ALSA: pcm: Suspend streams globally via device type PM ops ALSA: atiixp: Move PCM suspend/resume code into trigger callback ALSA: isa: Remove superfluous snd_pcm_suspend*() calls ALSA: drivers: Remove superfluous snd_pcm_suspend*() calls ALSA: pci: Remove superfluous snd_pcm_suspend*() calls ALSA: usb: Remove superfluous snd_pcm_suspend*() calls ALSA: x86: Remove superfluous snd_pcm_suspend*() calls ALSA: ppc: Remove superfluous snd_pcm_suspend*() calls ALSA: aoa: Remove superfluous snd_pcm_suspend*() calls ALSA: arm: Remove superfluous snd_pcm_suspend*() calls ALSA: pcmcia: Remove superfluous snd_pcm_suspend*() calls drm: bridge: dw-hdmi: Remove superfluous snd_pcm_suspend*() calls ALSA: doc: Update the description about PCM suspend procedure ALSA: pcm: Make snd_pcm_suspend() local static
.../kernel-api/writing-an-alsa-driver.rst | 25 ++++++------------ .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 1 - include/sound/pcm.h | 6 +---- sound/aoa/soundbus/i2sbus/core.c | 4 --- sound/arm/aaci.c | 1 - sound/arm/pxa2xx-ac97.c | 1 - sound/core/pcm.c | 26 +++++++++++++++++++ sound/core/pcm_native.c | 11 +++----- sound/drivers/aloop.c | 4 --- sound/drivers/dummy.c | 2 -- sound/drivers/pcsp/pcsp.c | 1 - sound/drivers/vx/vx_core.c | 4 --- sound/isa/ad1816a/ad1816a_lib.c | 1 - sound/isa/als100.c | 1 - sound/isa/cmi8328.c | 1 - sound/isa/cmi8330.c | 1 - sound/isa/es1688/es1688.c | 2 -- sound/isa/es18xx.c | 2 -- sound/isa/sb/jazz16.c | 1 - sound/isa/sb/sb16.c | 1 - sound/isa/sb/sb8.c | 1 - sound/isa/wss/wss_lib.c | 1 - sound/pci/ali5451/ali5451.c | 4 +-- sound/pci/als300.c | 1 - sound/pci/als4000.c | 1 - sound/pci/atiixp.c | 19 ++++++-------- sound/pci/atiixp_modem.c | 2 -- sound/pci/azt3328.c | 4 --- sound/pci/ca0106/ca0106_main.c | 3 --- sound/pci/cmipci.c | 4 --- sound/pci/cs4281.c | 2 -- sound/pci/cs46xx/cs46xx_lib.c | 6 ----- sound/pci/cs5535audio/cs5535audio_pm.c | 1 - sound/pci/ctxfi/ctatc.c | 8 ------ sound/pci/echoaudio/echoaudio.c | 3 --- sound/pci/emu10k1/emu10k1.c | 6 ----- sound/pci/ens1370.c | 3 --- sound/pci/es1938.c | 1 - sound/pci/es1968.c | 1 - sound/pci/fm801.c | 1 - sound/pci/hda/hda_codec.c | 2 -- sound/pci/ice1712/ice1712.c | 3 --- sound/pci/ice1712/ice1724.c | 3 --- sound/pci/intel8x0.c | 2 -- sound/pci/intel8x0m.c | 3 --- sound/pci/maestro3.c | 1 - sound/pci/nm256/nm256.c | 1 - sound/pci/oxygen/oxygen_lib.c | 5 +--- sound/pci/riptide/riptide.c | 1 - sound/pci/rme96.c | 2 -- sound/pci/sis7019.c | 1 - sound/pci/trident/trident_main.c | 4 --- sound/pci/via82xx.c | 2 -- sound/pci/via82xx_modem.c | 2 -- sound/pci/ymfpci/ymfpci_main.c | 4 --- sound/pcmcia/pdaudiocf/pdaudiocf_core.c | 1 - sound/ppc/pmac.c | 1 - sound/soc/soc-pcm.c | 1 + sound/usb/card.c | 1 - sound/usb/line6/driver.c | 4 +-- sound/x86/intel_hdmi_audio.c | 12 --------- 61 files changed, 50 insertions(+), 174 deletions(-)
Until now we rely on each driver calling snd_pcm_suspend*() explicitly at its own PM handling. However, this can be done far more easily by setting the PM ops to each actual snd_pcm device object.
This patch adds the device_type object for PCM stream and assigns to each PCM stream object. The type contains only the PM ops for system suspend; we don't need to deal with the resume in general.
The suspend hook simply calls snd_pcm_suspend_all() for the given PCM streams. This implies that the PM order is correctly put, i.e. PCM is suspended before the main (or codec) driver, which should be true in general. If a special ordering is needed, you'd need to adjust the device PM order manually later.
This patch introduces a new flag, snd_pcm.no_device_suspend, too. With this flag set, the PCM device object won't invoke snd_pcm_suspend_all() by itself. This is needed for ASoC who wants to manage the PM call orders in its serialized way, and the flag is set in soc_new_pcm() as default.
For the non-ASoC world, we can get rid of the manual snd_pcm_suspend calls. This will be done in the later patches.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 1 + sound/core/pcm.c | 26 ++++++++++++++++++++++++++ sound/soc/soc-pcm.c | 1 + 3 files changed, 28 insertions(+)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index d6bd3caf6878..04e97564949c 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -538,6 +538,7 @@ struct snd_pcm { void (*private_free) (struct snd_pcm *pcm); bool internal; /* pcm is for internal use only */ bool nonatomic; /* whole PCM operations are in non-atomic context */ + bool no_device_suspend; /* don't invoke device PM suspend */ #if IS_ENABLED(CONFIG_SND_PCM_OSS) struct snd_pcm_oss oss; #endif diff --git a/sound/core/pcm.c b/sound/core/pcm.c index 01b9d62eef14..ca1ea3cf9350 100644 --- a/sound/core/pcm.c +++ b/sound/core/pcm.c @@ -683,6 +683,31 @@ static inline int snd_pcm_substream_proc_done(struct snd_pcm_substream *substrea
static const struct attribute_group *pcm_dev_attr_groups[];
+/* + * PM callbacks: we need to deal only with suspend here, as the resume is + * triggered either from user-space or the driver's resume callback + */ +#ifdef CONFIG_PM_SLEEP +static int do_pcm_suspend(struct device *dev) +{ + struct snd_pcm_str *pstr = container_of(dev, struct snd_pcm_str, dev); + + if (!pstr->pcm->no_device_suspend) + snd_pcm_suspend_all(pstr->pcm); + return 0; +} +#endif + +static const struct dev_pm_ops pcm_dev_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(do_pcm_suspend, NULL) +}; + +/* device type for PCM -- basically only for passing PM callbacks */ +static const struct device_type pcm_dev_type = { + .name = "pcm", + .pm = &pcm_dev_pm_ops, +}; + /** * snd_pcm_new_stream - create a new PCM stream * @pcm: the pcm instance @@ -713,6 +738,7 @@ int snd_pcm_new_stream(struct snd_pcm *pcm, int stream, int substream_count)
snd_device_initialize(&pstr->dev, pcm->card); pstr->dev.groups = pcm_dev_attr_groups; + pstr->dev.type = &pcm_dev_type; dev_set_name(&pstr->dev, "pcmC%iD%i%c", pcm->card->number, pcm->device, stream == SNDRV_PCM_STREAM_PLAYBACK ? 'p' : 'c');
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 03f36e534050..485eec5be608 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -3155,6 +3155,7 @@ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) }
pcm->private_free = soc_pcm_private_free; + pcm->no_device_suspend = true; out: dev_info(rtd->card->dev, "%s <-> %s mapping ok\n", (rtd->num_codecs > 1) ? "multicodec" : rtd->codec_dai->name,
On Tue, Jan 15, 2019 at 05:21:42PM +0100, Takashi Iwai wrote:
Until now we rely on each driver calling snd_pcm_suspend*() explicitly at its own PM handling. However, this can be done far more easily by setting the PM ops to each actual snd_pcm device object.
Acked-by: Mark Brown broonie@kernel.org
ATIIXP driver supports the full PCM resume and saves/restores the running PCM pointer. This used to be done in the suspend and resume callbacks together with snd_pcm_suspend() call. But since we moved the snd_pcm_supsend*() call in PCM device PM ops, this should be moved to a more appropriate place, i.e. the trigger callback.
Along with the movement of the PCM suspend/resume code, remove the superfluous snd_pcm_suspend_all() call, too.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/atiixp.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/sound/pci/atiixp.c b/sound/pci/atiixp.c index 1a41f8c80243..7715d26916ac 100644 --- a/sound/pci/atiixp.c +++ b/sound/pci/atiixp.c @@ -733,6 +733,10 @@ static int snd_atiixp_pcm_trigger(struct snd_pcm_substream *substream, int cmd) case SNDRV_PCM_TRIGGER_START: case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: case SNDRV_PCM_TRIGGER_RESUME: + if (dma->running && dma->suspended && + cmd == SNDRV_PCM_TRIGGER_RESUME) + writel(dma->saved_curptr, chip->remap_addr + + dma->ops->dt_cur); dma->ops->enable_transfer(chip, 1); dma->running = 1; dma->suspended = 0; @@ -740,9 +744,12 @@ static int snd_atiixp_pcm_trigger(struct snd_pcm_substream *substream, int cmd) case SNDRV_PCM_TRIGGER_STOP: case SNDRV_PCM_TRIGGER_PAUSE_PUSH: case SNDRV_PCM_TRIGGER_SUSPEND: + dma->suspended = cmd == SNDRV_PCM_TRIGGER_SUSPEND; + if (dma->running && dma->suspended) + dma->saved_curptr = readl(chip->remap_addr + + dma->ops->dt_cur); dma->ops->enable_transfer(chip, 0); dma->running = 0; - dma->suspended = cmd == SNDRV_PCM_TRIGGER_SUSPEND; break; default: err = -EINVAL; @@ -1479,14 +1486,6 @@ static int snd_atiixp_suspend(struct device *dev) int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - for (i = 0; i < NUM_ATI_PCMDEVS; i++) - if (chip->pcmdevs[i]) { - struct atiixp_dma *dma = &chip->dmas[i]; - if (dma->substream && dma->running) - dma->saved_curptr = readl(chip->remap_addr + - dma->ops->dt_cur); - snd_pcm_suspend_all(chip->pcmdevs[i]); - } for (i = 0; i < NUM_ATI_CODECS; i++) snd_ac97_suspend(chip->ac97[i]); snd_atiixp_aclink_down(chip); @@ -1514,8 +1513,6 @@ static int snd_atiixp_resume(struct device *dev) dma->substream->ops->prepare(dma->substream); writel((u32)dma->desc_buf.addr | ATI_REG_LINKPTR_EN, chip->remap_addr + dma->ops->llp_offset); - writel(dma->saved_curptr, chip->remap_addr + - dma->ops->dt_cur); } }
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/isa/ad1816a/ad1816a_lib.c | 1 - sound/isa/als100.c | 1 - sound/isa/cmi8328.c | 1 - sound/isa/cmi8330.c | 1 - sound/isa/es1688/es1688.c | 2 -- sound/isa/es18xx.c | 2 -- sound/isa/sb/jazz16.c | 1 - sound/isa/sb/sb16.c | 1 - sound/isa/sb/sb8.c | 1 - sound/isa/wss/wss_lib.c | 1 - 10 files changed, 12 deletions(-)
diff --git a/sound/isa/ad1816a/ad1816a_lib.c b/sound/isa/ad1816a/ad1816a_lib.c index fba6d22f7f4b..61e8c7e524db 100644 --- a/sound/isa/ad1816a/ad1816a_lib.c +++ b/sound/isa/ad1816a/ad1816a_lib.c @@ -518,7 +518,6 @@ void snd_ad1816a_suspend(struct snd_ad1816a *chip) int reg; unsigned long flags;
- snd_pcm_suspend_all(chip->pcm); spin_lock_irqsave(&chip->lock, flags); for (reg = 0; reg < 48; reg++) chip->image[reg] = snd_ad1816a_read(chip, reg); diff --git a/sound/isa/als100.c b/sound/isa/als100.c index f63142ec287e..571108021e9d 100644 --- a/sound/isa/als100.c +++ b/sound/isa/als100.c @@ -322,7 +322,6 @@ static int snd_als100_pnp_suspend(struct pnp_card_link *pcard, pm_message_t stat struct snd_sb *chip = acard->chip;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_sbmixer_suspend(chip); return 0; } diff --git a/sound/isa/cmi8328.c b/sound/isa/cmi8328.c index de6ef1b1cf0e..617977516201 100644 --- a/sound/isa/cmi8328.c +++ b/sound/isa/cmi8328.c @@ -434,7 +434,6 @@ static int snd_cmi8328_suspend(struct device *pdev, unsigned int n, cmi = card->private_data; snd_cmi8328_cfg_save(cmi->port, cmi->cfg); snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(cmi->wss->pcm); cmi->wss->suspend(cmi->wss);
return 0; diff --git a/sound/isa/cmi8330.c b/sound/isa/cmi8330.c index 6b8c46942efb..7e5aa06414c4 100644 --- a/sound/isa/cmi8330.c +++ b/sound/isa/cmi8330.c @@ -484,7 +484,6 @@ static int snd_cmi8330_suspend(struct snd_card *card) struct snd_cmi8330 *acard = card->private_data;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(acard->pcm); acard->wss->suspend(acard->wss); snd_sbmixer_suspend(acard->sb); return 0; diff --git a/sound/isa/es1688/es1688.c b/sound/isa/es1688/es1688.c index 3dfe7e592c25..87527627e059 100644 --- a/sound/isa/es1688/es1688.c +++ b/sound/isa/es1688/es1688.c @@ -301,10 +301,8 @@ static int snd_es968_pnp_suspend(struct pnp_card_link *pcard, pm_message_t state) { struct snd_card *card = pnp_get_card_drvdata(pcard); - struct snd_es1688 *chip = card->private_data;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); return 0; }
diff --git a/sound/isa/es18xx.c b/sound/isa/es18xx.c index 0d103d6f805e..77aa9a27fb3b 100644 --- a/sound/isa/es18xx.c +++ b/sound/isa/es18xx.c @@ -1731,8 +1731,6 @@ static int snd_es18xx_suspend(struct snd_card *card, pm_message_t state)
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
- snd_pcm_suspend_all(chip->pcm); - /* power down */ chip->pm_reg = (unsigned char)snd_es18xx_read(chip, ES18XX_PM); chip->pm_reg |= (ES18XX_PM_FM | ES18XX_PM_SUS); diff --git a/sound/isa/sb/jazz16.c b/sound/isa/sb/jazz16.c index bfa0055e1fd6..7a313ff589c7 100644 --- a/sound/isa/sb/jazz16.c +++ b/sound/isa/sb/jazz16.c @@ -356,7 +356,6 @@ static int snd_jazz16_suspend(struct device *pdev, unsigned int n, struct snd_sb *chip = acard->chip;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_sbmixer_suspend(chip); return 0; } diff --git a/sound/isa/sb/sb16.c b/sound/isa/sb/sb16.c index 8f9ebeb998f6..3844d4c02f49 100644 --- a/sound/isa/sb/sb16.c +++ b/sound/isa/sb/sb16.c @@ -471,7 +471,6 @@ static int snd_sb16_suspend(struct snd_card *card, pm_message_t state) struct snd_sb *chip = acard->chip;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_sbmixer_suspend(chip); return 0; } diff --git a/sound/isa/sb/sb8.c b/sound/isa/sb/sb8.c index d77dcba276b5..aa2a83eb81a9 100644 --- a/sound/isa/sb/sb8.c +++ b/sound/isa/sb/sb8.c @@ -218,7 +218,6 @@ static int snd_sb8_suspend(struct device *dev, unsigned int n, struct snd_sb *chip = acard->chip;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_sbmixer_suspend(chip); return 0; } diff --git a/sound/isa/wss/wss_lib.c b/sound/isa/wss/wss_lib.c index 3a5008837576..b11ef97bce1b 100644 --- a/sound/isa/wss/wss_lib.c +++ b/sound/isa/wss/wss_lib.c @@ -1625,7 +1625,6 @@ static void snd_wss_suspend(struct snd_wss *chip) int reg; unsigned long flags;
- snd_pcm_suspend_all(chip->pcm); spin_lock_irqsave(&chip->reg_lock, flags); for (reg = 0; reg < 32; reg++) chip->image[reg] = snd_wss_in(chip, reg);
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/drivers/aloop.c | 4 ---- sound/drivers/dummy.c | 2 -- sound/drivers/pcsp/pcsp.c | 1 - sound/drivers/vx/vx_core.c | 4 ---- 4 files changed, 11 deletions(-)
diff --git a/sound/drivers/aloop.c b/sound/drivers/aloop.c index 1e34e6381baa..65c903b639c2 100644 --- a/sound/drivers/aloop.c +++ b/sound/drivers/aloop.c @@ -1200,12 +1200,8 @@ static int loopback_remove(struct platform_device *devptr) static int loopback_suspend(struct device *pdev) { struct snd_card *card = dev_get_drvdata(pdev); - struct loopback *loopback = card->private_data;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - - snd_pcm_suspend_all(loopback->pcm[0]); - snd_pcm_suspend_all(loopback->pcm[1]); return 0; } diff --git a/sound/drivers/dummy.c b/sound/drivers/dummy.c index 9af154db530a..c8d31550e9a1 100644 --- a/sound/drivers/dummy.c +++ b/sound/drivers/dummy.c @@ -1138,10 +1138,8 @@ static int snd_dummy_remove(struct platform_device *devptr) static int snd_dummy_suspend(struct device *pdev) { struct snd_card *card = dev_get_drvdata(pdev); - struct snd_dummy *dummy = card->private_data;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(dummy->pcm); return 0; } diff --git a/sound/drivers/pcsp/pcsp.c b/sound/drivers/pcsp/pcsp.c index 0dd3f46eb03e..d83ad3820f02 100644 --- a/sound/drivers/pcsp/pcsp.c +++ b/sound/drivers/pcsp/pcsp.c @@ -197,7 +197,6 @@ static int pcsp_suspend(struct device *dev) { struct snd_pcsp *chip = dev_get_drvdata(dev); pcsp_stop_beep(chip); - snd_pcm_suspend_all(chip->pcm); return 0; }
diff --git a/sound/drivers/vx/vx_core.c b/sound/drivers/vx/vx_core.c index 04368dd59a4c..19496fa486aa 100644 --- a/sound/drivers/vx/vx_core.c +++ b/sound/drivers/vx/vx_core.c @@ -732,12 +732,8 @@ EXPORT_SYMBOL(snd_vx_dsp_load); */ int snd_vx_suspend(struct vx_core *chip) { - unsigned int i; - snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot); chip->chip_status |= VX_STAT_IN_SUSPEND; - for (i = 0; i < chip->hw->num_codecs; i++) - snd_pcm_suspend_all(chip->pcm[i]);
return 0; }
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/ali5451/ali5451.c | 4 +--- sound/pci/als300.c | 1 - sound/pci/als4000.c | 1 - sound/pci/atiixp_modem.c | 2 -- sound/pci/azt3328.c | 4 ---- sound/pci/ca0106/ca0106_main.c | 3 --- sound/pci/cmipci.c | 4 ---- sound/pci/cs4281.c | 2 -- sound/pci/cs46xx/cs46xx_lib.c | 6 ------ sound/pci/cs5535audio/cs5535audio_pm.c | 1 - sound/pci/ctxfi/ctatc.c | 8 -------- sound/pci/echoaudio/echoaudio.c | 3 --- sound/pci/emu10k1/emu10k1.c | 6 ------ sound/pci/ens1370.c | 3 --- sound/pci/es1938.c | 1 - sound/pci/es1968.c | 1 - sound/pci/fm801.c | 1 - sound/pci/hda/hda_codec.c | 2 -- sound/pci/ice1712/ice1712.c | 3 --- sound/pci/ice1712/ice1724.c | 3 --- sound/pci/intel8x0.c | 2 -- sound/pci/intel8x0m.c | 3 --- sound/pci/maestro3.c | 1 - sound/pci/nm256/nm256.c | 1 - sound/pci/oxygen/oxygen_lib.c | 5 +---- sound/pci/riptide/riptide.c | 1 - sound/pci/rme96.c | 2 -- sound/pci/sis7019.c | 1 - sound/pci/trident/trident_main.c | 4 ---- sound/pci/via82xx.c | 2 -- sound/pci/via82xx_modem.c | 2 -- sound/pci/ymfpci/ymfpci_main.c | 4 ---- 32 files changed, 2 insertions(+), 85 deletions(-)
diff --git a/sound/pci/ali5451/ali5451.c b/sound/pci/ali5451/ali5451.c index 9f569379b77e..e781ccca1793 100644 --- a/sound/pci/ali5451/ali5451.c +++ b/sound/pci/ali5451/ali5451.c @@ -1882,10 +1882,8 @@ static int ali_suspend(struct device *dev) return 0;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - for (i = 0; i < chip->num_of_codecs; i++) { - snd_pcm_suspend_all(chip->pcm[i]); + for (i = 0; i < chip->num_of_codecs; i++) snd_ac97_suspend(chip->ac97[i]); - }
spin_lock_irq(&chip->reg_lock); diff --git a/sound/pci/als300.c b/sound/pci/als300.c index eaa2d853d922..516b3d9cbfdf 100644 --- a/sound/pci/als300.c +++ b/sound/pci/als300.c @@ -731,7 +731,6 @@ static int snd_als300_suspend(struct device *dev) struct snd_als300 *chip = card->private_data;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_ac97_suspend(chip->ac97); return 0; } diff --git a/sound/pci/als4000.c b/sound/pci/als4000.c index 26b097edec8c..45fa38382e79 100644 --- a/sound/pci/als4000.c +++ b/sound/pci/als4000.c @@ -994,7 +994,6 @@ static int snd_als4000_suspend(struct device *dev)
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_sbmixer_suspend(chip); return 0; } diff --git a/sound/pci/atiixp_modem.c b/sound/pci/atiixp_modem.c index dc1de860cedf..a357a8e2e73d 100644 --- a/sound/pci/atiixp_modem.c +++ b/sound/pci/atiixp_modem.c @@ -1125,8 +1125,6 @@ static int snd_atiixp_suspend(struct device *dev) int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - for (i = 0; i < NUM_ATI_PCMDEVS; i++) - snd_pcm_suspend_all(chip->pcmdevs[i]); for (i = 0; i < NUM_ATI_CODECS; i++) snd_ac97_suspend(chip->ac97[i]); snd_atiixp_aclink_down(chip); diff --git a/sound/pci/azt3328.c b/sound/pci/azt3328.c index fc18c29a8173..90348817f096 100644 --- a/sound/pci/azt3328.c +++ b/sound/pci/azt3328.c @@ -2699,10 +2699,6 @@ snd_azf3328_suspend(struct device *dev)
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
- /* same pcm object for playback/capture */ - snd_pcm_suspend_all(chip->pcm[AZF_CODEC_PLAYBACK]); - snd_pcm_suspend_all(chip->pcm[AZF_CODEC_I2S_OUT]); - snd_azf3328_suspend_ac97(chip);
snd_azf3328_suspend_regs(chip, chip->ctrl_io, diff --git a/sound/pci/ca0106/ca0106_main.c b/sound/pci/ca0106/ca0106_main.c index cd27b5536654..3d1b0bbff33b 100644 --- a/sound/pci/ca0106/ca0106_main.c +++ b/sound/pci/ca0106/ca0106_main.c @@ -1910,11 +1910,8 @@ static int snd_ca0106_suspend(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev); struct snd_ca0106 *chip = card->private_data; - int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - for (i = 0; i < 4; i++) - snd_pcm_suspend_all(chip->pcm[i]); if (chip->details->ac97) snd_ac97_suspend(chip->ac97); snd_ca0106_mixer_suspend(chip); diff --git a/sound/pci/cmipci.c b/sound/pci/cmipci.c index 452cc79b44af..5bbf31c1695c 100644 --- a/sound/pci/cmipci.c +++ b/sound/pci/cmipci.c @@ -3351,10 +3351,6 @@ static int snd_cmipci_suspend(struct device *dev)
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(cm->pcm); - snd_pcm_suspend_all(cm->pcm2); - snd_pcm_suspend_all(cm->pcm_spdif); - /* save registers */ for (i = 0; i < ARRAY_SIZE(saved_regs); i++) cm->saved_regs[i] = snd_cmipci_read(cm, saved_regs[i]); diff --git a/sound/pci/cs4281.c b/sound/pci/cs4281.c index ec4247638fa1..a9fb819cad1d 100644 --- a/sound/pci/cs4281.c +++ b/sound/pci/cs4281.c @@ -2002,8 +2002,6 @@ static int cs4281_suspend(struct device *dev) unsigned int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); - snd_ac97_suspend(chip->ac97); snd_ac97_suspend(chip->ac97_secondary);
diff --git a/sound/pci/cs46xx/cs46xx_lib.c b/sound/pci/cs46xx/cs46xx_lib.c index 750eec437a79..a77d4cc44028 100644 --- a/sound/pci/cs46xx/cs46xx_lib.c +++ b/sound/pci/cs46xx/cs46xx_lib.c @@ -3781,12 +3781,6 @@ static int snd_cs46xx_suspend(struct device *dev)
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); chip->in_suspend = 1; - snd_pcm_suspend_all(chip->pcm); -#ifdef CONFIG_SND_CS46XX_NEW_DSP - snd_pcm_suspend_all(chip->pcm_rear); - snd_pcm_suspend_all(chip->pcm_center_lfe); - snd_pcm_suspend_all(chip->pcm_iec958); -#endif // chip->ac97_powerdown = snd_cs46xx_codec_read(chip, AC97_POWER_CONTROL); // chip->ac97_general_purpose = snd_cs46xx_codec_read(chip, BA0_AC97_GENERAL_PURPOSE);
diff --git a/sound/pci/cs5535audio/cs5535audio_pm.c b/sound/pci/cs5535audio/cs5535audio_pm.c index 82bd10b68a77..446ef1f1b45a 100644 --- a/sound/pci/cs5535audio/cs5535audio_pm.c +++ b/sound/pci/cs5535audio/cs5535audio_pm.c @@ -62,7 +62,6 @@ static int __maybe_unused snd_cs5535audio_suspend(struct device *dev) int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(cs5535au->pcm); snd_ac97_suspend(cs5535au->ac97); for (i = 0; i < NUM_CS5535AUDIO_DMAS; i++) { struct cs5535audio_dma *dma = &cs5535au->dmas[i]; diff --git a/sound/pci/ctxfi/ctatc.c b/sound/pci/ctxfi/ctatc.c index 2ada8444abd9..e622613ea947 100644 --- a/sound/pci/ctxfi/ctatc.c +++ b/sound/pci/ctxfi/ctatc.c @@ -1548,18 +1548,10 @@ static void atc_connect_resources(struct ct_atc *atc) #ifdef CONFIG_PM_SLEEP static int atc_suspend(struct ct_atc *atc) { - int i; struct hw *hw = atc->hw;
snd_power_change_state(atc->card, SNDRV_CTL_POWER_D3hot);
- for (i = FRONT; i < NUM_PCMS; i++) { - if (!atc->pcms[i]) - continue; - - snd_pcm_suspend_all(atc->pcms[i]); - } - atc_release_resources(atc);
hw->suspend(hw); diff --git a/sound/pci/echoaudio/echoaudio.c b/sound/pci/echoaudio/echoaudio.c index 907cf1a46712..18d30d479b6b 100644 --- a/sound/pci/echoaudio/echoaudio.c +++ b/sound/pci/echoaudio/echoaudio.c @@ -2165,9 +2165,6 @@ static int snd_echo_suspend(struct device *dev) { struct echoaudio *chip = dev_get_drvdata(dev);
- snd_pcm_suspend_all(chip->analog_pcm); - snd_pcm_suspend_all(chip->digital_pcm); - #ifdef ECHOCARD_HAS_MIDI /* This call can sleep */ if (chip->midi_out) diff --git a/sound/pci/emu10k1/emu10k1.c b/sound/pci/emu10k1/emu10k1.c index d3203df50a1a..3c41a0edcfb0 100644 --- a/sound/pci/emu10k1/emu10k1.c +++ b/sound/pci/emu10k1/emu10k1.c @@ -224,12 +224,6 @@ static int snd_emu10k1_suspend(struct device *dev)
cancel_delayed_work_sync(&emu->emu1010.firmware_work);
- snd_pcm_suspend_all(emu->pcm); - snd_pcm_suspend_all(emu->pcm_mic); - snd_pcm_suspend_all(emu->pcm_efx); - snd_pcm_suspend_all(emu->pcm_multi); - snd_pcm_suspend_all(emu->pcm_p16v); - snd_ac97_suspend(emu->ac97);
snd_emu10k1_efx_suspend(emu); diff --git a/sound/pci/ens1370.c b/sound/pci/ens1370.c index 727eb3da1fda..1f2960ecc57e 100644 --- a/sound/pci/ens1370.c +++ b/sound/pci/ens1370.c @@ -2037,9 +2037,6 @@ static int snd_ensoniq_suspend(struct device *dev) snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
- snd_pcm_suspend_all(ensoniq->pcm1); - snd_pcm_suspend_all(ensoniq->pcm2); - #ifdef CHIP1371 snd_ac97_suspend(ensoniq->u.es1371.ac97); #else diff --git a/sound/pci/es1938.c b/sound/pci/es1938.c index 9d248eb2e26c..84d07bce581c 100644 --- a/sound/pci/es1938.c +++ b/sound/pci/es1938.c @@ -1475,7 +1475,6 @@ static int es1938_suspend(struct device *dev) unsigned char *s, *d;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm);
/* save mixer-related registers */ for (s = saved_regs, d = chip->saved_regs; *s; s++, d++) diff --git a/sound/pci/es1968.c b/sound/pci/es1968.c index 0b1845ca6005..9dcb698fc8c7 100644 --- a/sound/pci/es1968.c +++ b/sound/pci/es1968.c @@ -2392,7 +2392,6 @@ static int es1968_suspend(struct device *dev) chip->in_suspend = 1; cancel_work_sync(&chip->hwvol_work); snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_ac97_suspend(chip->ac97); snd_es1968_bob_stop(chip); return 0; diff --git a/sound/pci/fm801.c b/sound/pci/fm801.c index e3fb9c61017c..1317f3183eb1 100644 --- a/sound/pci/fm801.c +++ b/sound/pci/fm801.c @@ -1408,7 +1408,6 @@ static int snd_fm801_suspend(struct device *dev) if (chip->tea575x_tuner & TUNER_ONLY) { /* FIXME: tea575x suspend */ } else { - snd_pcm_suspend_all(chip->pcm); snd_ac97_suspend(chip->ac97); snd_ac97_suspend(chip->ac97_sec); } diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 9f8d59e7e89f..ff6dbed4d3cd 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2927,8 +2927,6 @@ static int hda_codec_runtime_suspend(struct device *dev) unsigned int state;
cancel_delayed_work_sync(&codec->jackpoll_work); - list_for_each_entry(pcm, &codec->pcm_list_head, list) - snd_pcm_suspend_all(pcm->pcm); state = hda_call_codec_suspend(codec); if (codec->link_down_at_suspend || (codec_has_clkstop(codec) && codec_has_epss(codec) && diff --git a/sound/pci/ice1712/ice1712.c b/sound/pci/ice1712/ice1712.c index f1fe497c2f9d..dda9b26192cb 100644 --- a/sound/pci/ice1712/ice1712.c +++ b/sound/pci/ice1712/ice1712.c @@ -2792,9 +2792,6 @@ static int snd_ice1712_suspend(struct device *dev)
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
- snd_pcm_suspend_all(ice->pcm); - snd_pcm_suspend_all(ice->pcm_pro); - snd_pcm_suspend_all(ice->pcm_ds); snd_ac97_suspend(ice->ac97);
spin_lock_irq(&ice->reg_lock); diff --git a/sound/pci/ice1712/ice1724.c b/sound/pci/ice1712/ice1724.c index 057c2f394ea7..42994cf36156 100644 --- a/sound/pci/ice1712/ice1724.c +++ b/sound/pci/ice1712/ice1724.c @@ -2804,9 +2804,6 @@ static int snd_vt1724_suspend(struct device *dev)
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
- snd_pcm_suspend_all(ice->pcm); - snd_pcm_suspend_all(ice->pcm_pro); - snd_pcm_suspend_all(ice->pcm_ds); snd_ac97_suspend(ice->ac97);
spin_lock_irq(&ice->reg_lock); diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c index ffddcdfe0c66..885e1d488ed6 100644 --- a/sound/pci/intel8x0.c +++ b/sound/pci/intel8x0.c @@ -2614,8 +2614,6 @@ static int intel8x0_suspend(struct device *dev) int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - for (i = 0; i < chip->pcm_devs; i++) - snd_pcm_suspend_all(chip->pcm[i]); for (i = 0; i < chip->ncodecs; i++) snd_ac97_suspend(chip->ac97[i]); if (chip->device_type == DEVICE_INTEL_ICH4) diff --git a/sound/pci/intel8x0m.c b/sound/pci/intel8x0m.c index c84629190cba..44eb9e28a1eb 100644 --- a/sound/pci/intel8x0m.c +++ b/sound/pci/intel8x0m.c @@ -1025,11 +1025,8 @@ static int intel8x0m_suspend(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev); struct intel8x0m *chip = card->private_data; - int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - for (i = 0; i < chip->pcm_devs; i++) - snd_pcm_suspend_all(chip->pcm[i]); snd_ac97_suspend(chip->ac97); if (chip->irq >= 0) { free_irq(chip->irq, chip); diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c index 62962178a9d7..1a9468c14aaf 100644 --- a/sound/pci/maestro3.c +++ b/sound/pci/maestro3.c @@ -2422,7 +2422,6 @@ static int m3_suspend(struct device *dev) chip->in_suspend = 1; cancel_work_sync(&chip->hwvol_work); snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_ac97_suspend(chip->ac97);
msleep(10); /* give the assp a chance to idle.. */ diff --git a/sound/pci/nm256/nm256.c b/sound/pci/nm256/nm256.c index b97f4ea6b56c..85e46ff44ac3 100644 --- a/sound/pci/nm256/nm256.c +++ b/sound/pci/nm256/nm256.c @@ -1413,7 +1413,6 @@ static int nm256_suspend(struct device *dev) struct nm256 *chip = card->private_data;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_ac97_suspend(chip->ac97); chip->coeffs_current = 0; return 0; diff --git a/sound/pci/oxygen/oxygen_lib.c b/sound/pci/oxygen/oxygen_lib.c index b4ef5804212d..6dce56c209aa 100644 --- a/sound/pci/oxygen/oxygen_lib.c +++ b/sound/pci/oxygen/oxygen_lib.c @@ -744,13 +744,10 @@ static int oxygen_pci_suspend(struct device *dev) { struct snd_card *card = dev_get_drvdata(dev); struct oxygen *chip = card->private_data; - unsigned int i, saved_interrupt_mask; + unsigned int saved_interrupt_mask;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot);
- for (i = 0; i < PCM_COUNT; ++i) - snd_pcm_suspend(chip->streams[i]); - if (chip->model.suspend) chip->model.suspend(chip);
diff --git a/sound/pci/riptide/riptide.c b/sound/pci/riptide/riptide.c index 23017e3bc76c..1d431c8052d6 100644 --- a/sound/pci/riptide/riptide.c +++ b/sound/pci/riptide/riptide.c @@ -1158,7 +1158,6 @@ static int riptide_suspend(struct device *dev)
chip->in_suspend = 1; snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); snd_ac97_suspend(chip->ac97); return 0; } diff --git a/sound/pci/rme96.c b/sound/pci/rme96.c index dcfa4d7a73e2..c56702e6cb60 100644 --- a/sound/pci/rme96.c +++ b/sound/pci/rme96.c @@ -2388,8 +2388,6 @@ static int rme96_suspend(struct device *dev) struct rme96 *rme96 = card->private_data;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend(rme96->playback_substream); - snd_pcm_suspend(rme96->capture_substream);
/* save capture & playback pointers */ rme96->playback_pointer = readl(rme96->iobase + RME96_IO_GET_PLAY_POS) diff --git a/sound/pci/sis7019.c b/sound/pci/sis7019.c index 964acf302479..6b27980d77a8 100644 --- a/sound/pci/sis7019.c +++ b/sound/pci/sis7019.c @@ -1214,7 +1214,6 @@ static int sis_suspend(struct device *dev) int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(sis->pcm); if (sis->codecs_present & SIS_PRIMARY_CODEC_PRESENT) snd_ac97_suspend(sis->ac97[0]); if (sis->codecs_present & SIS_SECONDARY_CODEC_PRESENT) diff --git a/sound/pci/trident/trident_main.c b/sound/pci/trident/trident_main.c index 5523e193d556..f271ea436cff 100644 --- a/sound/pci/trident/trident_main.c +++ b/sound/pci/trident/trident_main.c @@ -3915,10 +3915,6 @@ static int snd_trident_suspend(struct device *dev)
trident->in_suspend = 1; snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(trident->pcm); - snd_pcm_suspend_all(trident->foldback); - snd_pcm_suspend_all(trident->spdif); - snd_ac97_suspend(trident->ac97); snd_ac97_suspend(trident->ac97_sec); return 0; diff --git a/sound/pci/via82xx.c b/sound/pci/via82xx.c index c488c5afa195..736ac79901b3 100644 --- a/sound/pci/via82xx.c +++ b/sound/pci/via82xx.c @@ -2278,8 +2278,6 @@ static int snd_via82xx_suspend(struct device *dev) int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - for (i = 0; i < 2; i++) - snd_pcm_suspend_all(chip->pcms[i]); for (i = 0; i < chip->num_devs; i++) snd_via82xx_channel_reset(chip, &chip->devs[i]); synchronize_irq(chip->irq); diff --git a/sound/pci/via82xx_modem.c b/sound/pci/via82xx_modem.c index b13c8688cc8d..3f59e0279058 100644 --- a/sound/pci/via82xx_modem.c +++ b/sound/pci/via82xx_modem.c @@ -1038,8 +1038,6 @@ static int snd_via82xx_suspend(struct device *dev) int i;
snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - for (i = 0; i < 2; i++) - snd_pcm_suspend_all(chip->pcms[i]); for (i = 0; i < chip->num_devs; i++) snd_via82xx_channel_reset(chip, &chip->devs[i]); synchronize_irq(chip->irq); diff --git a/sound/pci/ymfpci/ymfpci_main.c b/sound/pci/ymfpci/ymfpci_main.c index a4926fb03991..c688b7f481da 100644 --- a/sound/pci/ymfpci/ymfpci_main.c +++ b/sound/pci/ymfpci/ymfpci_main.c @@ -2304,10 +2304,6 @@ static int snd_ymfpci_suspend(struct device *dev) unsigned int i; snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); - snd_pcm_suspend_all(chip->pcm2); - snd_pcm_suspend_all(chip->pcm_spdif); - snd_pcm_suspend_all(chip->pcm_4ch); snd_ac97_suspend(chip->ac97); for (i = 0; i < YDSXGR_NUM_SAVED_REGS; i++) chip->saved_regs[i] = snd_ymfpci_readl(chip, saved_regs_index[i]);
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 9f8d59e7e89f..ff6dbed4d3cd 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2927,8 +2927,6 @@ static int hda_codec_runtime_suspend(struct device *dev) unsigned int state;
cancel_delayed_work_sync(&codec->jackpoll_work);
- list_for_each_entry(pcm, &codec->pcm_list_head, list)
snd_pcm_suspend_all(pcm->pcm);
question: since we now use HDAudio codecs in an ASoC-based implementation (both with the Skylake and SOF drivers), is this creating a case where suspend no longer works? I see no suspend support in sound/soc/codec/hdac_hda.c
I just added a quick trace in the lines being deleted and those lines were definitively executed.
list_for_each_entry(pcm, &codec->pcm_list_head, list) { pr_err("plb: suspending pcm\n"); snd_pcm_suspend_all(pcm->pcm); }
[ 2.516349] plb: suspending pcm [ 2.516350] plb: suspending pcm
On Tue, 15 Jan 2019 17:49:56 +0100, Pierre-Louis Bossart wrote:
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 9f8d59e7e89f..ff6dbed4d3cd 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2927,8 +2927,6 @@ static int hda_codec_runtime_suspend(struct device *dev) unsigned int state; cancel_delayed_work_sync(&codec->jackpoll_work);
- list_for_each_entry(pcm, &codec->pcm_list_head, list)
snd_pcm_suspend_all(pcm->pcm);question: since we now use HDAudio codecs in an ASoC-based implementation (both with the Skylake and SOF drivers), is this creating a case where suspend no longer works? I see no suspend support in sound/soc/codec/hdac_hda.c
As mentioned in patch#1, ASoC calls snd_pcm_suspend_all() in snd_soc_suspend(), which is the canonical place.
But, the suspend / resume for hdac-hda need revisited as well as hdac-hdmi, I suppose. They shouldn't use the device PM ops but just use the ASoC component codec / suspend callbacks. Some more plumbing might be required.
thanks,
Takashi
I just added a quick trace in the lines being deleted and those lines were definitively executed.
list_for_each_entry(pcm, &codec->pcm_list_head, list) { pr_err("plb: suspending pcm\n"); snd_pcm_suspend_all(pcm->pcm); }
[ 2.516349] plb: suspending pcm [ 2.516350] plb: suspending pcm
On Tue, 15 Jan 2019 18:01:51 +0100, Takashi Iwai wrote:
On Tue, 15 Jan 2019 17:49:56 +0100, Pierre-Louis Bossart wrote:
diff --git a/sound/pci/hda/hda_codec.c b/sound/pci/hda/hda_codec.c index 9f8d59e7e89f..ff6dbed4d3cd 100644 --- a/sound/pci/hda/hda_codec.c +++ b/sound/pci/hda/hda_codec.c @@ -2927,8 +2927,6 @@ static int hda_codec_runtime_suspend(struct device *dev) unsigned int state; cancel_delayed_work_sync(&codec->jackpoll_work);
- list_for_each_entry(pcm, &codec->pcm_list_head, list)
snd_pcm_suspend_all(pcm->pcm);question: since we now use HDAudio codecs in an ASoC-based implementation (both with the Skylake and SOF drivers), is this creating a case where suspend no longer works? I see no suspend support in sound/soc/codec/hdac_hda.c
As mentioned in patch#1, ASoC calls snd_pcm_suspend_all() in snd_soc_suspend(), which is the canonical place.
But, the suspend / resume for hdac-hda need revisited as well as hdac-hdmi, I suppose. They shouldn't use the device PM ops but just use the ASoC component codec / suspend callbacks. Some more plumbing might be required.
After further consideration, this seems solvable in a different way.
Basically, we can move some preamble code into the PM prepare callback so that it's executed before the suspend callback. For example, the patch below moves a few things into the prepare callback that was formerly done at the beginning of snd_soc_suspend(). This assures snd_pcm_suspend*() call processed beforehand.
And, another question is whether the snd_pcm_suspend*() call really has to be always after the digital_mute call in ASoC suspend procedure. If this can be done beforehand, we may leave snd_pcm_suspend*() call in the PCM device PM ops like others, while doing the digital_mute & else preamble in the prepare callback. The PCM device PM is called before the machine driver's device PM due to the dependency (the PCM device is always created after the card device). This will make things easier again, we can get rid of the ugly flag in the patch set.
BTW, while checking these things, I noticed that there are three exceptional drivers: sound/soc/intel/atom/sst-mfld-platform-pcm.c, sound/soc/intel/haswell/sst-haswell-pcm.c, and sound/soc/samsung/tm2_wm5110.c. (You can simply grep the external snd_soc_suspend call, and only these match.)
The former two drivers look really weird: they do handle the PM only with PM prepare and complete callbacks, while snd_soc_suspend() and co are called internally from there. The prepare and complete callbacks aren't designed for the complete suspend/resume tasks, so I'd say it's a quite abuse.
The last one has prepare and complete callbacks in addition to the other standard PM calls. And tm2_pm_preapre() stops sysclk and complete() starts sysclk. I don't understand why these are needed in prepare and resume. Can anyone explain?
thanks,
Takashi
---
diff --git a/include/sound/soc.h b/include/sound/soc.h --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -432,9 +432,15 @@ int snd_soc_register_card(struct snd_soc_card *card); int snd_soc_unregister_card(struct snd_soc_card *card); int devm_snd_soc_register_card(struct device *dev, struct snd_soc_card *card); #ifdef CONFIG_PM_SLEEP +int snd_soc_pm_prepare(struct device *dev); int snd_soc_suspend(struct device *dev); int snd_soc_resume(struct device *dev); #else +static inline int snd_soc_pm_prepare(struct device *dev) +{ + return 0; +} + static inline int snd_soc_suspend(struct device *dev) { return 0; diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -436,13 +436,11 @@ static void codec2codec_close_delayed_work(struct work_struct *work) }
#ifdef CONFIG_PM_SLEEP -/* powers down audio subsystem for suspend */ -int snd_soc_suspend(struct device *dev) +/* prepare for suspend */ +int snd_soc_pm_prepare(struct device *dev) { struct snd_soc_card *card = dev_get_drvdata(dev); - struct snd_soc_component *component; struct snd_soc_pcm_runtime *rtd; - int i;
/* If the card is not initialized yet there is nothing to do */ if (!card->instantiated) @@ -480,6 +478,22 @@ int snd_soc_suspend(struct device *dev) snd_pcm_suspend_all(rtd->pcm); }
+ return 0; +} +EXPORT_SYMBOL_GPL(snd_soc_pm_prepare); + +/* powers down audio subsystem for suspend */ +int snd_soc_suspend(struct device *dev) +{ + struct snd_soc_card *card = dev_get_drvdata(dev); + struct snd_soc_component *component; + struct snd_soc_pcm_runtime *rtd; + int i; + + /* If the card is not initialized yet there is nothing to do */ + if (!card->instantiated) + return 0; + if (card->suspend_pre) card->suspend_pre(card);
@@ -2253,6 +2267,7 @@ int snd_soc_poweroff(struct device *dev) EXPORT_SYMBOL_GPL(snd_soc_poweroff);
const struct dev_pm_ops snd_soc_pm_ops = { + .prepare = snd_soc_pm_prepare, .suspend = snd_soc_suspend, .resume = snd_soc_resume, .freeze = snd_soc_suspend,
On Tue, Jan 15, 2019 at 09:42:09PM +0100, Takashi Iwai wrote:
The last one has prepare and complete callbacks in addition to the other standard PM calls. And tm2_pm_preapre() stops sysclk and complete() starts sysclk. I don't understand why these are needed in prepare and resume. Can anyone explain?
AFAICT it's just making sure that they're available ASAP so they look always on to the rest of the system.
On Wed, 16 Jan 2019 16:50:27 +0100, Mark Brown wrote:
On Tue, Jan 15, 2019 at 09:42:09PM +0100, Takashi Iwai wrote:
The last one has prepare and complete callbacks in addition to the other standard PM calls. And tm2_pm_preapre() stops sysclk and complete() starts sysclk. I don't understand why these are needed in prepare and resume. Can anyone explain?
AFAICT it's just making sure that they're available ASAP so they look always on to the rest of the system.
Well, but PM prepare is called before PM suspend call. And the whole ASoC suspend procedure (including PCM suspend, etc) is performed in the PM suspend callback; i.e. we stop sysclk before doing anything else...
Takashi
On Wed, Jan 16, 2019 at 04:52:53PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
AFAICT it's just making sure that they're available ASAP so they look always on to the rest of the system.
Well, but PM prepare is called before PM suspend call. And the whole ASoC suspend procedure (including PCM suspend, etc) is performed in the PM suspend callback; i.e. we stop sysclk before doing anything else...
Ah, got it the wrong way round... in that case I frankly have no idea and would expect it to break if we suspend with active audio - it's possibly a bug.
On Wed, Jan 16, 2019 at 04:52:53PM +0100, Takashi Iwai wrote:
Mark Brown wrote:
On Tue, Jan 15, 2019 at 09:42:09PM +0100, Takashi Iwai wrote:
The last one has prepare and complete callbacks in addition to the other standard PM calls. And tm2_pm_preapre() stops sysclk and complete() starts sysclk. I don't understand why these are needed in prepare and resume. Can anyone explain?
AFAICT it's just making sure that they're available ASAP so they look always on to the rest of the system.
Well, but PM prepare is called before PM suspend call. And the whole ASoC suspend procedure (including PCM suspend, etc) is performed in the PM suspend callback; i.e. we stop sysclk before doing anything else...
Thinking about this some more I'm moderately sure that the calls were intended to do as I described but someone misunderstood what they did and swapped them around. I'm guessing nobody's been testing this.
BTW, while checking these things, I noticed that there are three exceptional drivers: sound/soc/intel/atom/sst-mfld-platform-pcm.c, sound/soc/intel/haswell/sst-haswell-pcm.c, and sound/soc/samsung/tm2_wm5110.c. (You can simply grep the external snd_soc_suspend call, and only these match.)
The former two drivers look really weird: they do handle the PM only with PM prepare and complete callbacks, while snd_soc_suspend() and co are called internally from there. The prepare and complete callbacks aren't designed for the complete suspend/resume tasks, so I'd say it's a quite abuse.
For the Atom/SST driver, I remember there was a need to set/restore the DSP state with a specific command that wasn't handled with regular controls - largely a work-around due to the firmware design.
For the Haswell driver, there was also a need to preserve/restore state and pause/stop pipelines (a recurring issue with the "Made for Windows" firmware).
These drivers are quite old now and it's not clear to me if they are broken or if we are talking of an improvement. Could you clarify what you view as "abuse"?
a) is this the fact that there are prepare/complete callback for those drivers, instead of others such as freeze, thaw, etc.
b) the fact they they call snd_soc_suspend/resume directly?
c) the fact that they suspend the PCM streams?
d) all of the above (which is entirely possible).
Thanks
-Pierre
On Thu, 17 Jan 2019 03:21:57 +0100, Pierre-Louis Bossart wrote:
BTW, while checking these things, I noticed that there are three exceptional drivers: sound/soc/intel/atom/sst-mfld-platform-pcm.c, sound/soc/intel/haswell/sst-haswell-pcm.c, and sound/soc/samsung/tm2_wm5110.c. (You can simply grep the external snd_soc_suspend call, and only these match.)
The former two drivers look really weird: they do handle the PM only with PM prepare and complete callbacks, while snd_soc_suspend() and co are called internally from there. The prepare and complete callbacks aren't designed for the complete suspend/resume tasks, so I'd say it's a quite abuse.
For the Atom/SST driver, I remember there was a need to set/restore the DSP state with a specific command that wasn't handled with regular controls - largely a work-around due to the firmware design.
For the Haswell driver, there was also a need to preserve/restore state and pause/stop pipelines (a recurring issue with the "Made for Windows" firmware).
These drivers are quite old now and it's not clear to me if they are broken or if we are talking of an improvement. Could you clarify what you view as "abuse"?
a) is this the fact that there are prepare/complete callback for those drivers, instead of others such as freeze, thaw, etc.
b) the fact they they call snd_soc_suspend/resume directly?
c) the fact that they suspend the PCM streams?
d) all of the above (which is entirely possible).
The purpose of PM prepare and complete devops aren't for actually do suspend and resume devices there, while these drivers call snd_soc_suspend() and snd_soc_resume() to perform the complete suspend / resume procedure. That's not the way these callbacks are supposed to be used.
The prepare callback is called before the suspend callback of *all* devices on the system. Ditto for complete, it's called after the resume of all devices.
I guess they use prepare/callback to assure some tasks to be performed always suspend and resume. But it's still puzzling, e.g. sst_soc_prepare() has
static int sst_soc_prepare(struct device *dev) { struct sst_data *drv = dev_get_drvdata(dev); struct snd_soc_pcm_runtime *rtd;
if (!drv->soc_card) return 0;
/* suspend all pcms first */ snd_soc_suspend(drv->soc_card->dev); snd_soc_poweroff(drv->soc_card->dev);
/* set the SSPs to idle */ for_each_card_rtds(drv->soc_card, rtd) { struct snd_soc_dai *dai = rtd->cpu_dai;
if (dai->active) { send_ssp_cmd(dai, dai->name, 0); sst_handle_vb_timer(dai, false); } }
return 0; }
... and it calls snd_soc_poweroff() for suspend. That's odd and likely superfluous.
And, the last part ("set the SSPs to idle") can be moved to card->suspend_post hook, and then we can simply call snd_soc_suspend(). Or it can be moved to PM devops suspend_late.
Similarly for sst_soc_complete(), the task "restart SSPs" can be moved to card->resume_pre hook or PM devops resume_pre.
The rest is to make sure the device PM ops order, and that's the hardest part.
Further looking at the code, we can see that several Intel ASoC drivers have device PM ops.
sound/soc/intel/atom/sst/sst.c sound/soc/intel/haswell/sst-haswell-pcm.c sound/soc/intel/skylake/skl.c sound/soc/intel/atom/sst-mfld-platform-pcm.c
... and there are codecs. We need to list up and define the suspend / resume call order.
Takashi
BTW, while checking these things, I noticed that there are three exceptional drivers: sound/soc/intel/atom/sst-mfld-platform-pcm.c, sound/soc/intel/haswell/sst-haswell-pcm.c, and sound/soc/samsung/tm2_wm5110.c. (You can simply grep the external snd_soc_suspend call, and only these match.)
The former two drivers look really weird: they do handle the PM only with PM prepare and complete callbacks, while snd_soc_suspend() and co are called internally from there. The prepare and complete callbacks aren't designed for the complete suspend/resume tasks, so I'd say it's a quite abuse.
For the Atom/SST driver, I remember there was a need to set/restore the DSP state with a specific command that wasn't handled with regular controls - largely a work-around due to the firmware design.
For the Haswell driver, there was also a need to preserve/restore state and pause/stop pipelines (a recurring issue with the "Made for Windows" firmware).
These drivers are quite old now and it's not clear to me if they are broken or if we are talking of an improvement. Could you clarify what you view as "abuse"?
a) is this the fact that there are prepare/complete callback for those drivers, instead of others such as freeze, thaw, etc.
b) the fact they they call snd_soc_suspend/resume directly?
c) the fact that they suspend the PCM streams?
d) all of the above (which is entirely possible).
The purpose of PM prepare and complete devops aren't for actually do suspend and resume devices there, while these drivers call snd_soc_suspend() and snd_soc_resume() to perform the complete suspend / resume procedure. That's not the way these callbacks are supposed to be used.
The prepare callback is called before the suspend callback of *all* devices on the system. Ditto for complete, it's called after the resume of all devices.
I guess they use prepare/callback to assure some tasks to be performed always suspend and resume. But it's still puzzling, e.g. sst_soc_prepare() has
static int sst_soc_prepare(struct device *dev) { struct sst_data *drv = dev_get_drvdata(dev); struct snd_soc_pcm_runtime *rtd;
if (!drv->soc_card) return 0;
/* suspend all pcms first */ snd_soc_suspend(drv->soc_card->dev); snd_soc_poweroff(drv->soc_card->dev);
/* set the SSPs to idle */ for_each_card_rtds(drv->soc_card, rtd) { struct snd_soc_dai *dai = rtd->cpu_dai;
if (dai->active) { send_ssp_cmd(dai, dai->name, 0); sst_handle_vb_timer(dai, false); }}
return 0; }
... and it calls snd_soc_poweroff() for suspend. That's odd and likely superfluous.
And, the last part ("set the SSPs to idle") can be moved to card->suspend_post hook, and then we can simply call snd_soc_suspend(). Or it can be moved to PM devops suspend_late.
Similarly for sst_soc_complete(), the task "restart SSPs" can be moved to card->resume_pre hook or PM devops resume_pre.
The rest is to make sure the device PM ops order, and that's the hardest part.
Further looking at the code, we can see that several Intel ASoC drivers have device PM ops.
sound/soc/intel/atom/sst/sst.c sound/soc/intel/haswell/sst-haswell-pcm.c sound/soc/intel/skylake/skl.c sound/soc/intel/atom/sst-mfld-platform-pcm.c
... and there are codecs. We need to list up and define the suspend / resume call order.
I had an offline discussion with Vinod and here are the key points for the Atom/SST driver
- the DSP isn't completely modeled with DPCM, there are some pipeline management and commands that need to be send manually. This isn't necessarily a perfect design but the one that was defined in 2013
- the choice of the .prepare is intentional. The tasks are split between the SST device (ACPI or PCI) and the platform device it creates. The ACPI/PCI layer handles DSP boot, config, shutdown, fw load, and the platform driver handles PCM/pipelines. The PM starts with the .prepare done in the child before the .suspend done at a higher level.
For Haswell I have no idea, and I wonder if there are actually any devices using this driver. Even from Broadwell we only know of the Pixel 2015 Chromebook and the Dell XPS 13 where the I2S mode is activated (and the latter is deactivated with an ACPI quirk), most devices use HDaudio. Even if we found someone at Intel with bandwidth, changing this part is going to be very difficult between lack of devices and initial teams/individual contributors who have moved on.
SOF will support all these platforms, it might be a better idea to spend time making sure we do the right thing with the newer drivers than try to fix things but actually introduce regressions in legacy code.
-Pierre
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/usb/card.c | 1 - sound/usb/line6/driver.c | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/sound/usb/card.c b/sound/usb/card.c index a105947eaf55..dfa38b78c494 100644 --- a/sound/usb/card.c +++ b/sound/usb/card.c @@ -811,7 +811,6 @@ static int usb_audio_suspend(struct usb_interface *intf, pm_message_t message) snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot); if (!chip->num_suspended_intf++) { list_for_each_entry(as, &chip->pcm_list, list) { - snd_pcm_suspend_all(as->pcm); snd_usb_pcm_suspend(as); as->substream[0].need_setup_ep = as->substream[1].need_setup_ep = true; diff --git a/sound/usb/line6/driver.c b/sound/usb/line6/driver.c index c1376bfdc90b..7afe8fae4939 100644 --- a/sound/usb/line6/driver.c +++ b/sound/usb/line6/driver.c @@ -849,10 +849,8 @@ int line6_suspend(struct usb_interface *interface, pm_message_t message) if (line6->properties->capabilities & LINE6_CAP_CONTROL) line6_stop_listen(line6);
- if (line6pcm != NULL) { - snd_pcm_suspend_all(line6pcm->pcm); + if (line6pcm != NULL) line6pcm->flags = 0; - }
return 0; }
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/x86/intel_hdmi_audio.c | 12 ------------ 1 file changed, 12 deletions(-)
diff --git a/sound/x86/intel_hdmi_audio.c b/sound/x86/intel_hdmi_audio.c index 00c92eb854ce..16ca91f57e7f 100644 --- a/sound/x86/intel_hdmi_audio.c +++ b/sound/x86/intel_hdmi_audio.c @@ -1651,18 +1651,6 @@ static int had_create_jack(struct snd_intelhad *ctx, static int __maybe_unused hdmi_lpe_audio_suspend(struct device *dev) { struct snd_intelhad_card *card_ctx = dev_get_drvdata(dev); - int port; - - for_each_port(card_ctx, port) { - struct snd_intelhad *ctx = &card_ctx->pcm_ctx[port]; - struct snd_pcm_substream *substream; - - substream = had_substream_get(ctx); - if (substream) { - snd_pcm_suspend(substream); - had_substream_put(ctx); - } - }
snd_power_change_state(card_ctx->card, SNDRV_CTL_POWER_D3hot);
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/ppc/pmac.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/ppc/pmac.c b/sound/ppc/pmac.c index d692e4070167..6d420bd3ae17 100644 --- a/sound/ppc/pmac.c +++ b/sound/ppc/pmac.c @@ -1365,7 +1365,6 @@ void snd_pmac_suspend(struct snd_pmac *chip) snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot); if (chip->suspend) chip->suspend(chip); - snd_pcm_suspend_all(chip->pcm); spin_lock_irqsave(&chip->reg_lock, flags); snd_pmac_beep_stop(chip); spin_unlock_irqrestore(&chip->reg_lock, flags);
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/aoa/soundbus/i2sbus/core.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/aoa/soundbus/i2sbus/core.c b/sound/aoa/soundbus/i2sbus/core.c index c3f57a3fb1a5..33e82341c048 100644 --- a/sound/aoa/soundbus/i2sbus/core.c +++ b/sound/aoa/soundbus/i2sbus/core.c @@ -380,10 +380,6 @@ static int i2sbus_suspend(struct macio_dev* dev, pm_message_t state) int err, ret = 0;
list_for_each_entry(i2sdev, &control->list, item) { - /* Notify Alsa */ - /* Suspend PCM streams */ - snd_pcm_suspend_all(i2sdev->sound.pcm); - /* Notify codecs */ list_for_each_entry(cii, &i2sdev->sound.codec_list, list) { err = 0;
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/arm/aaci.c | 1 - sound/arm/pxa2xx-ac97.c | 1 - 2 files changed, 2 deletions(-)
diff --git a/sound/arm/aaci.c b/sound/arm/aaci.c index 0114ffed56dd..0c3f073e2600 100644 --- a/sound/arm/aaci.c +++ b/sound/arm/aaci.c @@ -757,7 +757,6 @@ static int aaci_do_suspend(struct snd_card *card) { struct aaci *aaci = card->private_data; snd_power_change_state(card, SNDRV_CTL_POWER_D3cold); - snd_pcm_suspend_all(aaci->pcm); return 0; }
diff --git a/sound/arm/pxa2xx-ac97.c b/sound/arm/pxa2xx-ac97.c index 1f72672262d0..68fe5bb11eea 100644 --- a/sound/arm/pxa2xx-ac97.c +++ b/sound/arm/pxa2xx-ac97.c @@ -124,7 +124,6 @@ static int pxa2xx_ac97_do_suspend(struct snd_card *card) pxa2xx_audio_ops_t *platform_ops = card->dev->platform_data;
snd_power_change_state(card, SNDRV_CTL_POWER_D3cold); - snd_pcm_suspend_all(pxa2xx_ac97_pcm); snd_ac97_suspend(pxa2xx_ac97_ac97); if (platform_ops && platform_ops->suspend) platform_ops->suspend(platform_ops->priv);
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pcmcia/pdaudiocf/pdaudiocf_core.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/sound/pcmcia/pdaudiocf/pdaudiocf_core.c b/sound/pcmcia/pdaudiocf/pdaudiocf_core.c index d724ab0653cf..eabf29252895 100644 --- a/sound/pcmcia/pdaudiocf/pdaudiocf_core.c +++ b/sound/pcmcia/pdaudiocf/pdaudiocf_core.c @@ -265,7 +265,6 @@ int snd_pdacf_suspend(struct snd_pdacf *chip) u16 val; snd_power_change_state(chip->card, SNDRV_CTL_POWER_D3hot); - snd_pcm_suspend_all(chip->pcm); /* disable interrupts, but use direct write to preserve old register value in chip->regmap */ val = inw(chip->port + PDAUDIOCF_REG_IER); val &= ~(PDAUDIOCF_IRQOVREN|PDAUDIOCF_IRQAKMEN|PDAUDIOCF_IRQLVLEN0|PDAUDIOCF_IRQLVLEN1);
The call of snd_pcm_suspend_all() & co became superfluous since we call it in the PCM PM ops. Let's remove them.
Signed-off-by: Takashi Iwai tiwai@suse.de --- drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 1 - 1 file changed, 1 deletion(-)
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c index cf3f0caf9c63..ed7af7518b52 100644 --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-ahb-audio.c @@ -614,7 +614,6 @@ static int snd_dw_hdmi_suspend(struct device *dev) struct snd_dw_hdmi *dw = dev_get_drvdata(dev);
snd_power_change_state(dw->card, SNDRV_CTL_POWER_D3cold); - snd_pcm_suspend_all(dw->pcm);
return 0; }
The PCM suspend procedure was changed for drivers, so that they don't have to call snd_pcm_suspend*() in each callback any longer. Update the documentation to adapt the changes.
Signed-off-by: Takashi Iwai tiwai@suse.de --- .../kernel-api/writing-an-alsa-driver.rst | 25 ++++++------------- 1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst index b37234afdfa1..7c2f2032d30a 100644 --- a/Documentation/sound/kernel-api/writing-an-alsa-driver.rst +++ b/Documentation/sound/kernel-api/writing-an-alsa-driver.rst @@ -3924,15 +3924,12 @@ The scheme of the real suspend job is as follows. 2. Call :c:func:`snd_power_change_state()` with ``SNDRV_CTL_POWER_D3hot`` to change the power status.
-3. Call :c:func:`snd_pcm_suspend_all()` to suspend the running - PCM streams. - -4. If AC97 codecs are used, call :c:func:`snd_ac97_suspend()` for +3. If AC97 codecs are used, call :c:func:`snd_ac97_suspend()` for each codec.
-5. Save the register values if necessary. +4. Save the register values if necessary.
-6. Stop the hardware if necessary. +5. Stop the hardware if necessary.
A typical code would be like:
@@ -3946,12 +3943,10 @@ A typical code would be like: /* (2) */ snd_power_change_state(card, SNDRV_CTL_POWER_D3hot); /* (3) */ - snd_pcm_suspend_all(chip->pcm); - /* (4) */ snd_ac97_suspend(chip->ac97); - /* (5) */ + /* (4) */ snd_mychip_save_registers(chip); - /* (6) */ + /* (5) */ snd_mychip_stop_hardware(chip); return 0; } @@ -3994,13 +3989,9 @@ A typical code would be like: return 0; }
-As shown in the above, it's better to save registers after suspending -the PCM operations via :c:func:`snd_pcm_suspend_all()` or -:c:func:`snd_pcm_suspend()`. It means that the PCM streams are -already stopped when the register snapshot is taken. But, remember that -you don't have to restart the PCM stream in the resume callback. It'll -be restarted via trigger call with ``SNDRV_PCM_TRIGGER_RESUME`` when -necessary. +Note that, at the time this callback gets called, the PCM stream has +been already suspended via its own PM ops calling +:c:func:`snd_pcm_suspend_all()` internally.
OK, we have all callbacks now. Let's set them up. In the initialization of the card, make sure that you can get the chip data from the card
snd_pcm_suspend() is no longer called from outside, so let's make it local static. Also drop a superfluous NULL check there.
Signed-off-by: Takashi Iwai tiwai@suse.de --- include/sound/pcm.h | 5 ----- sound/core/pcm_native.c | 11 +++-------- 2 files changed, 3 insertions(+), 13 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 04e97564949c..2c30c1ad1b0d 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -582,13 +582,8 @@ int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status); int snd_pcm_drain_done(struct snd_pcm_substream *substream); int snd_pcm_stop_xrun(struct snd_pcm_substream *substream); #ifdef CONFIG_PM -int snd_pcm_suspend(struct snd_pcm_substream *substream); int snd_pcm_suspend_all(struct snd_pcm *pcm); #else -static inline int snd_pcm_suspend(struct snd_pcm_substream *substream) -{ - return 0; -} static inline int snd_pcm_suspend_all(struct snd_pcm *pcm) { return 0; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 818dff1de545..26afb6b0889a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1460,29 +1460,24 @@ static const struct action_ops snd_pcm_action_suspend = { .post_action = snd_pcm_post_suspend };
-/** +/* * snd_pcm_suspend - trigger SUSPEND to all linked streams * @substream: the PCM substream * * After this call, all streams are changed to SUSPENDED state. * - * Return: Zero if successful (or @substream is %NULL), or a negative error - * code. + * Return: Zero if successful, or a negative error code. */ -int snd_pcm_suspend(struct snd_pcm_substream *substream) +static int snd_pcm_suspend(struct snd_pcm_substream *substream) { int err; unsigned long flags;
- if (! substream) - return 0; - snd_pcm_stream_lock_irqsave(substream, flags); err = snd_pcm_action(&snd_pcm_action_suspend, substream, 0); snd_pcm_stream_unlock_irqrestore(substream, flags); return err; } -EXPORT_SYMBOL(snd_pcm_suspend);
/** * snd_pcm_suspend_all - trigger SUSPEND to all substreams in the given pcm
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, January 16, 2019 12:22 AM To: alsa-devel@alsa-project.org Cc: Yang, Libin libin.yang@intel.com; Keyon Jie yang.jie@linux.intel.com; liam.r.girdwood@linux.intel.com; Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com>; broonie@kernel.org; Lin, Mengdong mengdong.lin@intel.com Subject: [PATCH 14/14] ALSA: pcm: Make snd_pcm_suspend() local static
snd_pcm_suspend() is no longer called from outside, so let's make it local static. Also drop a superfluous NULL check there.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/pcm.h | 5 ----- sound/core/pcm_native.c | 11 +++-------- 2 files changed, 3 insertions(+), 13 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 04e97564949c..2c30c1ad1b0d 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -582,13 +582,8 @@ int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status); int snd_pcm_drain_done(struct snd_pcm_substream *substream); int snd_pcm_stop_xrun(struct snd_pcm_substream *substream); #ifdef CONFIG_PM -int snd_pcm_suspend(struct snd_pcm_substream *substream); int snd_pcm_suspend_all(struct snd_pcm *pcm); #else -static inline int snd_pcm_suspend(struct snd_pcm_substream *substream) -{
- return 0;
-} static inline int snd_pcm_suspend_all(struct snd_pcm *pcm) { return 0; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 818dff1de545..26afb6b0889a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1460,29 +1460,24 @@ static const struct action_ops snd_pcm_action_suspend = { .post_action = snd_pcm_post_suspend };
-/** +/*
- snd_pcm_suspend - trigger SUSPEND to all linked streams
- @substream: the PCM substream
- After this call, all streams are changed to SUSPENDED state.
- Return: Zero if successful (or @substream is %NULL), or a negative error
- code.
- Return: Zero if successful, or a negative error code.
*/ -int snd_pcm_suspend(struct snd_pcm_substream *substream) +static int snd_pcm_suspend(struct snd_pcm_substream *substream)
If some drivers may use snd_pcm_suspend() later for corner cases, is it OK to make it be external again?
Regards, Libin
{ int err; unsigned long flags;
- if (! substream)
return 0;- snd_pcm_stream_lock_irqsave(substream, flags); err = snd_pcm_action(&snd_pcm_action_suspend, substream, 0); snd_pcm_stream_unlock_irqrestore(substream, flags); return err;
} -EXPORT_SYMBOL(snd_pcm_suspend);
/**
- snd_pcm_suspend_all - trigger SUSPEND to all substreams in the given pcm
-- 2.20.1
On Thu, 17 Jan 2019 02:50:21 +0100, Yang, Libin wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, January 16, 2019 12:22 AM To: alsa-devel@alsa-project.org Cc: Yang, Libin libin.yang@intel.com; Keyon Jie yang.jie@linux.intel.com; liam.r.girdwood@linux.intel.com; Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com>; broonie@kernel.org; Lin, Mengdong mengdong.lin@intel.com Subject: [PATCH 14/14] ALSA: pcm: Make snd_pcm_suspend() local static
snd_pcm_suspend() is no longer called from outside, so let's make it local static. Also drop a superfluous NULL check there.
Signed-off-by: Takashi Iwai tiwai@suse.de
include/sound/pcm.h | 5 ----- sound/core/pcm_native.c | 11 +++-------- 2 files changed, 3 insertions(+), 13 deletions(-)
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 04e97564949c..2c30c1ad1b0d 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -582,13 +582,8 @@ int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status); int snd_pcm_drain_done(struct snd_pcm_substream *substream); int snd_pcm_stop_xrun(struct snd_pcm_substream *substream); #ifdef CONFIG_PM -int snd_pcm_suspend(struct snd_pcm_substream *substream); int snd_pcm_suspend_all(struct snd_pcm *pcm); #else -static inline int snd_pcm_suspend(struct snd_pcm_substream *substream) -{
- return 0;
-} static inline int snd_pcm_suspend_all(struct snd_pcm *pcm) { return 0; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 818dff1de545..26afb6b0889a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1460,29 +1460,24 @@ static const struct action_ops snd_pcm_action_suspend = { .post_action = snd_pcm_post_suspend };
-/** +/*
- snd_pcm_suspend - trigger SUSPEND to all linked streams
- @substream: the PCM substream
- After this call, all streams are changed to SUSPENDED state.
- Return: Zero if successful (or @substream is %NULL), or a negative error
- code.
- Return: Zero if successful, or a negative error code.
*/ -int snd_pcm_suspend(struct snd_pcm_substream *substream) +static int snd_pcm_suspend(struct snd_pcm_substream *substream)
If some drivers may use snd_pcm_suspend() later for corner cases, is it OK to make it be external again?
Yes. But it means that you're doing something special and often wrong. We can catch such a case more easily by this action :)
thanks,
Takashi
diff --git a/include/sound/pcm.h b/include/sound/pcm.h index 04e97564949c..2c30c1ad1b0d 100644 --- a/include/sound/pcm.h +++ b/include/sound/pcm.h @@ -582,13 +582,8 @@ int snd_pcm_stop(struct snd_pcm_substream *substream, snd_pcm_state_t status); int snd_pcm_drain_done(struct snd_pcm_substream *substream); int snd_pcm_stop_xrun(struct snd_pcm_substream *substream); #ifdef CONFIG_PM -int snd_pcm_suspend(struct snd_pcm_substream *substream); int snd_pcm_suspend_all(struct snd_pcm *pcm); #else -static inline int snd_pcm_suspend(struct snd_pcm_substream *substream) -{
- return 0;
-} static inline int snd_pcm_suspend_all(struct snd_pcm *pcm) { return 0; diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 818dff1de545..26afb6b0889a 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -1460,29 +1460,24 @@ static const struct action_ops snd_pcm_action_suspend = { .post_action = snd_pcm_post_suspend };
-/** +/*
- snd_pcm_suspend - trigger SUSPEND to all linked streams
- @substream: the PCM substream
- After this call, all streams are changed to SUSPENDED state.
- Return: Zero if successful (or @substream is %NULL), or a
negative error
- code.
- Return: Zero if successful, or a negative error code.
*/ -int snd_pcm_suspend(struct snd_pcm_substream *substream) +static int snd_pcm_suspend(struct snd_pcm_substream *substream)
If some drivers may use snd_pcm_suspend() later for corner cases, is it OK to make it be external again?
Yes. But it means that you're doing something special and often wrong. We can catch such a case more easily by this action :)
You are right. I checked SOF (it use snd_pcm_suspend()) and believe we don't have to call snd_pcm_suspend() directly. :-)
Regards, Libin
thanks,
Takashi
Dne 15.1.2019 v 17:21 Takashi Iwai napsal(a):
Hi,
this is a patch set to clean up the PCM suspend calls by introducing the PCM device PM ops. This won't change much for ASoC but reduce lots of snd_pcm_suspend*() calls in other sound drivers.
Note that this patch series itself won't fix anything about the recent PM issue reported for Intel ASoC, but it was inspired through the discussion there.
thanks,
Takashi
This change looks fine.
Reviewed-by: Jaroslav Kysela perex@perex.cz
Thanks, Jaroslav
I have tested these patches on my platform with HD Audio. Suspend => resume => playback Suspend with playback => resume It works!
Regards, Libin
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Wednesday, January 16, 2019 12:22 AM To: alsa-devel@alsa-project.org Cc: Yang, Libin libin.yang@intel.com; Keyon Jie yang.jie@linux.intel.com; liam.r.girdwood@linux.intel.com; Pierre-Louis Bossart <pierre- louis.bossart@linux.intel.com>; broonie@kernel.org; Lin, Mengdong mengdong.lin@intel.com Subject: [PATCH 00/14] ALSA: PCM suspend cleanup
Hi,
this is a patch set to clean up the PCM suspend calls by introducing the PCM device PM ops. This won't change much for ASoC but reduce lots of snd_pcm_suspend*() calls in other sound drivers.
Note that this patch series itself won't fix anything about the recent PM issue reported for Intel ASoC, but it was inspired through the discussion there.
thanks,
Takashi
===
Takashi Iwai (14): ALSA: pcm: Suspend streams globally via device type PM ops ALSA: atiixp: Move PCM suspend/resume code into trigger callback ALSA: isa: Remove superfluous snd_pcm_suspend*() calls ALSA: drivers: Remove superfluous snd_pcm_suspend*() calls ALSA: pci: Remove superfluous snd_pcm_suspend*() calls ALSA: usb: Remove superfluous snd_pcm_suspend*() calls ALSA: x86: Remove superfluous snd_pcm_suspend*() calls ALSA: ppc: Remove superfluous snd_pcm_suspend*() calls ALSA: aoa: Remove superfluous snd_pcm_suspend*() calls ALSA: arm: Remove superfluous snd_pcm_suspend*() calls ALSA: pcmcia: Remove superfluous snd_pcm_suspend*() calls drm: bridge: dw-hdmi: Remove superfluous snd_pcm_suspend*() calls ALSA: doc: Update the description about PCM suspend procedure ALSA: pcm: Make snd_pcm_suspend() local static
.../kernel-api/writing-an-alsa-driver.rst | 25 ++++++------------ .../drm/bridge/synopsys/dw-hdmi-ahb-audio.c | 1 - include/sound/pcm.h | 6 +---- sound/aoa/soundbus/i2sbus/core.c | 4 --- sound/arm/aaci.c | 1 - sound/arm/pxa2xx-ac97.c | 1 - sound/core/pcm.c | 26 +++++++++++++++++++ sound/core/pcm_native.c | 11 +++----- sound/drivers/aloop.c | 4 --- sound/drivers/dummy.c | 2 -- sound/drivers/pcsp/pcsp.c | 1 - sound/drivers/vx/vx_core.c | 4 --- sound/isa/ad1816a/ad1816a_lib.c | 1 - sound/isa/als100.c | 1 - sound/isa/cmi8328.c | 1 - sound/isa/cmi8330.c | 1 - sound/isa/es1688/es1688.c | 2 -- sound/isa/es18xx.c | 2 -- sound/isa/sb/jazz16.c | 1 - sound/isa/sb/sb16.c | 1 - sound/isa/sb/sb8.c | 1 - sound/isa/wss/wss_lib.c | 1 - sound/pci/ali5451/ali5451.c | 4 +-- sound/pci/als300.c | 1 - sound/pci/als4000.c | 1 - sound/pci/atiixp.c | 19 ++++++-------- sound/pci/atiixp_modem.c | 2 -- sound/pci/azt3328.c | 4 --- sound/pci/ca0106/ca0106_main.c | 3 --- sound/pci/cmipci.c | 4 --- sound/pci/cs4281.c | 2 -- sound/pci/cs46xx/cs46xx_lib.c | 6 ----- sound/pci/cs5535audio/cs5535audio_pm.c | 1 - sound/pci/ctxfi/ctatc.c | 8 ------ sound/pci/echoaudio/echoaudio.c | 3 --- sound/pci/emu10k1/emu10k1.c | 6 ----- sound/pci/ens1370.c | 3 --- sound/pci/es1938.c | 1 - sound/pci/es1968.c | 1 - sound/pci/fm801.c | 1 - sound/pci/hda/hda_codec.c | 2 -- sound/pci/ice1712/ice1712.c | 3 --- sound/pci/ice1712/ice1724.c | 3 --- sound/pci/intel8x0.c | 2 -- sound/pci/intel8x0m.c | 3 --- sound/pci/maestro3.c | 1 - sound/pci/nm256/nm256.c | 1 - sound/pci/oxygen/oxygen_lib.c | 5 +--- sound/pci/riptide/riptide.c | 1 - sound/pci/rme96.c | 2 -- sound/pci/sis7019.c | 1 - sound/pci/trident/trident_main.c | 4 --- sound/pci/via82xx.c | 2 -- sound/pci/via82xx_modem.c | 2 -- sound/pci/ymfpci/ymfpci_main.c | 4 --- sound/pcmcia/pdaudiocf/pdaudiocf_core.c | 1 - sound/ppc/pmac.c | 1 - sound/soc/soc-pcm.c | 1 + sound/usb/card.c | 1 - sound/usb/line6/driver.c | 4 +-- sound/x86/intel_hdmi_audio.c | 12 --------- 61 files changed, 50 insertions(+), 174 deletions(-)
-- 2.20.1
participants (5)
-
Jaroslav Kysela -
Mark Brown -
Pierre-Louis Bossart -
Takashi Iwai -
Yang, Libin