[alsa-devel] [PATCH 1/5] ASoC: Hold runtime PM references to components of active DAIs
Every device that implements runtime power management for DAIs is doing it in pretty much the same way: in the startup callback they take a runtime PM reference and then in the shutdown callback they release that reference, keeping the device active while the DAI is active. Given the frequency with which this is done and the obviousness of the need to keep the device active in this period factor the code out into the core, taking references on the device for each CPU DAI, CODEC DAI and DMA device in the core.
As runtime PM is reference counted this shouldn't interfere with any other reference holding by the drivers, and since (in common with the existing implementations) we don't check for errors on enabling it shouldn't matter if the device actually has runtime PM enabled or not.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/soc-pcm.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 49aa71e..8aa7cec 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -19,6 +19,7 @@ #include <linux/kernel.h> #include <linux/init.h> #include <linux/delay.h> +#include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/workqueue.h> #include <sound/core.h> @@ -77,6 +78,10 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) struct snd_soc_dai_driver *codec_dai_drv = codec_dai->driver; int ret = 0;
+ pm_runtime_get_sync(cpu_dai->dev); + pm_runtime_get_sync(codec_dai->dev); + pm_runtime_get_sync(platform->dev); + mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
/* startup the audio subsystem */ @@ -233,6 +238,11 @@ platform_err: cpu_dai->driver->ops->shutdown(substream, cpu_dai); out: mutex_unlock(&rtd->pcm_mutex); + + pm_runtime_put(platform->dev); + pm_runtime_put(codec_dai->dev); + pm_runtime_put(cpu_dai->dev); + return ret; }
@@ -339,6 +349,11 @@ static int soc_pcm_close(struct snd_pcm_substream *substream) }
mutex_unlock(&rtd->pcm_mutex); + + pm_runtime_put(platform->dev); + pm_runtime_put(codec_dai->dev); + pm_runtime_put(cpu_dai->dev); + return 0; }
Now that the core holds a pm_runtime reference to the device while the link is active there is no need for the driver to do so.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/omap/omap-dmic.c | 5 +---- 1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/sound/soc/omap/omap-dmic.c b/sound/soc/omap/omap-dmic.c index 9c73c0c..0855c1c 100644 --- a/sound/soc/omap/omap-dmic.c +++ b/sound/soc/omap/omap-dmic.c @@ -114,7 +114,6 @@ static int omap_dmic_dai_startup(struct snd_pcm_substream *substream, mutex_lock(&dmic->mutex);
if (!dai->active) { - pm_runtime_get_sync(dmic->dev); snd_pcm_hw_constraint_msbits(substream->runtime, 0, 32, 24); dmic->active = 1; } else { @@ -133,10 +132,8 @@ static void omap_dmic_dai_shutdown(struct snd_pcm_substream *substream,
mutex_lock(&dmic->mutex);
- if (!dai->active) { - pm_runtime_put_sync(dmic->dev); + if (!dai->active) dmic->active = 0; - }
mutex_unlock(&dmic->mutex); }
On 12/05/2011 02:01 AM, Mark Brown wrote:
Now that the core holds a pm_runtime reference to the device while the link is active there is no need for the driver to do so.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
Acked-by: Peter Ujfalusi peter.ujfalusi@ti.com
Now that the core holds a pm_runtime reference to the device while the link is active there is no need for the driver to do so.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/omap/omap-mcpdm.c | 5 ----- 1 files changed, 0 insertions(+), 5 deletions(-)
diff --git a/sound/soc/omap/omap-mcpdm.c b/sound/soc/omap/omap-mcpdm.c index b50ac60..0e25df4f 100644 --- a/sound/soc/omap/omap-mcpdm.c +++ b/sound/soc/omap/omap-mcpdm.c @@ -266,8 +266,6 @@ static int omap_mcpdm_dai_startup(struct snd_pcm_substream *substream, mutex_lock(&mcpdm->mutex);
if (!dai->active) { - pm_runtime_get_sync(mcpdm->dev); - /* Enable watch dog for ES above ES 1.0 to avoid saturation */ if (omap_rev() != OMAP4430_REV_ES1_0) { u32 ctrl = omap_mcpdm_read(mcpdm, MCPDM_REG_CTRL); @@ -295,9 +293,6 @@ static void omap_mcpdm_dai_shutdown(struct snd_pcm_substream *substream, omap_mcpdm_stop(mcpdm); omap_mcpdm_close_streams(mcpdm); } - - if (!omap_mcpdm_active(mcpdm)) - pm_runtime_put_sync(mcpdm->dev); }
mutex_unlock(&mcpdm->mutex);
On 12/05/2011 02:01 AM, Mark Brown wrote:
Now that the core holds a pm_runtime reference to the device while the link is active there is no need for the driver to do so.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
Acked-by: Peter Ujfalusi peter.ujfalusi@ti.com
Now that the core holds a pm_runtime reference to the device while the link is active there is no need for the driver to do so.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/sh/siu_dai.c | 6 ------ 1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/sound/soc/sh/siu_dai.c b/sound/soc/sh/siu_dai.c index 11c6085..52d4c17 100644 --- a/sound/soc/sh/siu_dai.c +++ b/sound/soc/sh/siu_dai.c @@ -112,9 +112,6 @@ static void siu_dai_start(struct siu_port *port_info)
dev_dbg(port_info->pcm->card->dev, "%s\n", __func__);
- /* Turn on SIU clock */ - pm_runtime_get_sync(info->dev); - /* Issue software reset to siu */ siu_write32(base + SIU_SRCTL, 0);
@@ -158,9 +155,6 @@ static void siu_dai_stop(struct siu_port *port_info)
/* SIU software reset */ siu_write32(base + SIU_SRCTL, 0); - - /* Turn off SIU clock */ - pm_runtime_put_sync(info->dev); }
static void siu_dai_spbAselect(struct siu_port *port_info)
Now that the core holds a pm_runtime reference to the device while the link is active there is no need for the driver to do so.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/sh/fsi.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/sound/soc/sh/fsi.c b/sound/soc/sh/fsi.c index a27c306..db6c89a 100644 --- a/sound/soc/sh/fsi.c +++ b/sound/soc/sh/fsi.c @@ -893,8 +893,6 @@ static int fsi_hw_startup(struct fsi_priv *fsi, u32 flags = fsi_get_info_flags(fsi); u32 data = 0;
- pm_runtime_get_sync(dev); - /* clock setting */ if (fsi_is_clk_master(fsi)) data = DIMD | DOMD; @@ -951,8 +949,6 @@ static void fsi_hw_shutdown(struct fsi_priv *fsi, { if (fsi_is_clk_master(fsi)) fsi_set_master_clk(dev, fsi, fsi->rate, 0); - - pm_runtime_put_sync(dev); }
static int fsi_dai_startup(struct snd_pcm_substream *substream,
On 12/05/2011 02:01 AM, Mark Brown wrote:
Every device that implements runtime power management for DAIs is doing it in pretty much the same way: in the startup callback they take a runtime PM reference and then in the shutdown callback they release that reference, keeping the device active while the DAI is active. Given the frequency with which this is done and the obviousness of the need to keep the device active in this period factor the code out into the core, taking references on the device for each CPU DAI, CODEC DAI and DMA device in the core.
As runtime PM is reference counted this shouldn't interfere with any other reference holding by the drivers, and since (in common with the existing implementations) we don't check for errors on enabling it shouldn't matter if the device actually has runtime PM enabled or not.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
sound/soc/soc-pcm.c | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 49aa71e..8aa7cec 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -19,6 +19,7 @@ #include <linux/kernel.h> #include <linux/init.h> #include <linux/delay.h> +#include <linux/pm_runtime.h> #include <linux/slab.h> #include <linux/workqueue.h> #include <sound/core.h> @@ -77,6 +78,10 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) struct snd_soc_dai_driver *codec_dai_drv = codec_dai->driver; int ret = 0;
- pm_runtime_get_sync(cpu_dai->dev);
- pm_runtime_get_sync(codec_dai->dev);
- pm_runtime_get_sync(platform->dev);
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
I think it is better to move the pm_runtime_get_sync calls after the mutex_lock_nested() to be really safe (and to not change the way DAI drivers were handling the pm_runtime).
On Mon, Dec 05, 2011 at 10:01:07AM +0200, Peter Ujfalusi wrote:
On 12/05/2011 02:01 AM, Mark Brown wrote:
- pm_runtime_get_sync(cpu_dai->dev);
- pm_runtime_get_sync(codec_dai->dev);
- pm_runtime_get_sync(platform->dev);
- mutex_lock_nested(&rtd->pcm_mutex, rtd->pcm_subclass);
I think it is better to move the pm_runtime_get_sync calls after the mutex_lock_nested() to be really safe (and to not change the way DAI drivers were handling the pm_runtime).
This increases the amount of time we are sitting with the pcm_mutex held and I can't see a reason why we should need to do that. If there is a problem here I'd rather improve the drivers so that they can cope with having the power management happening after the lock, the main reason the drivers were doing this previously is that the lock happened to be held by the caller.
On 12/05/2011 01:39 PM, Mark Brown wrote:
This increases the amount of time we are sitting with the pcm_mutex held and I can't see a reason why we should need to do that. If there is a problem here I'd rather improve the drivers so that they can cope with having the power management happening after the lock, the main reason the drivers were doing this previously is that the lock happened to be held by the caller.
It is just that the pm_runtime calls might need the same protection as the other operations for dais/codecs/driver. I just thought about this series, and it might broke the omap-mcpdm driver as soon as we have the BIAS_OFF support for the twl6040 codec driver. The functional clock is coming from the external codec. If we power down the external fclk we will see external abort crash, since the McPDM IP does not have the needed clocks at the time of the pm_runtime_put_sync call (which will try to configure some McPDM registers). Need to check this. Can we hold this series back for a while?
On Mon, Dec 05, 2011 at 03:32:19PM +0200, Peter Ujfalusi wrote:
On 12/05/2011 01:39 PM, Mark Brown wrote:
This increases the amount of time we are sitting with the pcm_mutex held and I can't see a reason why we should need to do that. If there is a problem here I'd rather improve the drivers so that they can cope with having the power management happening after the lock, the main reason the drivers were doing this previously is that the lock happened to be held by the caller.
It is just that the pm_runtime calls might need the same protection as the other operations for dais/codecs/driver.
Well, the pcm_mutex doesn't cover all of those anyway (trigger and pointer in particular) and we've no guarantee that anything will actually happen at the point where the core does the calls as there may be other things holding the device active.
I just thought about this series, and it might broke the omap-mcpdm driver as soon as we have the BIAS_OFF support for the twl6040 codec driver. The functional clock is coming from the external codec. If we power down the external fclk we will see external abort crash, since the McPDM IP does not have the needed clocks at the time of the pm_runtime_put_sync call (which will try to configure some McPDM registers). Need to check this. Can we hold this series back for a while?
I don't understand how this could make the situation any worse than it is already - if nothing else this series will only make the region where we've got the device active slightly wider. There's definitely an issue there, but it seems like it already exists and is orthogonal to this refactoring. The McPDM needs to hold a reference on the CODEC somehow while it is active it seems, either via DAPM or via the runtime_pm APIs.
Also note that as this is a reference counting API there's nothing stopping any of the devices from being kept active independantly of this, your use case would seem to be a good example of the sort of situation where this might be useful.
On 12/05/2011 03:46 PM, Mark Brown wrote:
Well, the pcm_mutex doesn't cover all of those anyway (trigger and pointer in particular) and we've no guarantee that anything will actually happen at the point where the core does the calls as there may be other things holding the device active.
It covers the soc_pcm_open, and soc_pcm_close sequences.
I don't understand how this could make the situation any worse than it is already - if nothing else this series will only make the region where we've got the device active slightly wider.
The ordering of the pm_runtime_get/put will be different. We will have the pm_runtime_put after all other parts of the audio system has been closed, turned off.
There's definitely an issue there, but it seems like it already exists and is orthogonal to this refactoring. The McPDM needs to hold a reference on the CODEC somehow while it is active it seems, either via DAPM or via the runtime_pm APIs.
Yes, this is the reason we do not have the BIAS_OFF support in twl6040 still. I'm working on to integrate the external fclk (bit clock from twl6040) for McPDM with pm_runtime so we are not going to have issues with missing clocks. But as I said at this time we do not have issues since the fclk for McPDM is always present. This change actually gives more incentive to do the pm_runtime support for the McPDM external fclk ;)
On Mon, Dec 05, 2011 at 04:53:00PM +0200, Peter Ujfalusi wrote:
On 12/05/2011 03:46 PM, Mark Brown wrote:
Well, the pcm_mutex doesn't cover all of those anyway (trigger and pointer in particular) and we've no guarantee that anything will actually happen at the point where the core does the calls as there may be other things holding the device active.
It covers the soc_pcm_open, and soc_pcm_close sequences.
It's a pretty limited subset (and of course it won't cover the cases where one DAI is in two PCMs).
I don't understand how this could make the situation any worse than it is already - if nothing else this series will only make the region where we've got the device active slightly wider.
The ordering of the pm_runtime_get/put will be different. We will have the pm_runtime_put after all other parts of the audio system has been closed, turned off.
Hrm, yes. But that's much wider than just the issue with moving inside the pcm_lock - for example, the shutdown calls already come before the DAPM teardowns.
There's definitely an issue there, but it seems like it already exists and is orthogonal to this refactoring. The McPDM needs to hold a reference on the CODEC somehow while it is active it seems, either via DAPM or via the runtime_pm APIs.
Yes, this is the reason we do not have the BIAS_OFF support in twl6040 still. I'm working on to integrate the external fclk (bit clock from twl6040) for McPDM with pm_runtime so we are not going to have issues with missing clocks. But as I said at this time we do not have issues since the fclk for McPDM is always present. This change actually gives more incentive to do the pm_runtime support for the McPDM external fclk ;)
So it sounds like things are OK with the proposed patch then, though there's still some larger issues to work through?
On 12/05/2011 05:07 PM, Mark Brown wrote:
The ordering of the pm_runtime_get/put will be different. We will have the pm_runtime_put after all other parts of the audio system has been closed, turned off.
Hrm, yes. But that's much wider than just the issue with moving inside the pcm_lock - for example, the shutdown calls already come before the DAPM teardowns.
Sure, ideally I would pair the pm_runtime calls with the corresponding startup/shutdown callback.
So it sounds like things are OK with the proposed patch then, though there's still some larger issues to work through?
Yes, correct. I need to have pm_runtime support for the external functional clock for the McPDM (coming as bit clock from twl6040 codec).
On 12/05/2011 02:01 AM, Mark Brown wrote:
Every device that implements runtime power management for DAIs is doing it in pretty much the same way: in the startup callback they take a runtime PM reference and then in the shutdown callback they release that reference, keeping the device active while the DAI is active. Given the frequency with which this is done and the obviousness of the need to keep the device active in this period factor the code out into the core, taking references on the device for each CPU DAI, CODEC DAI and DMA device in the core.
As runtime PM is reference counted this shouldn't interfere with any other reference holding by the drivers, and since (in common with the existing implementations) we don't check for errors on enabling it shouldn't matter if the device actually has runtime PM enabled or not.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
Tested-by: Peter Ujfalusi peter.ujfalusi@ti.com
participants (2)
-
Mark Brown -
Peter Ujfalusi