| Message ID | 20200724070731.451377-1-tzungbi@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3aecfc72d7ad73330e7a6ebd0005738a8fd417ab |
| Headers | show |
| Series | ASoC: dapm: don't call pm_runtime_* on card device | expand |
On Fri, Jul 24, 2020 at 03:07:31PM +0800, Tzung-Bi Shih wrote: > Commit 4e872a46823c ("ASoC: dapm: Don't force card bias level to be > updated") stops to force update bias_level on card. If card doesn't > provide set_bias_level callback, the snd_soc_dapm_set_bias_level() > is equivalent to NOP for card device. > As a result, dapm_pre_sequence_async() doesn't change the bias_level of > card device correctly. Thus, pm_runtime_get_sync() would be called in > dapm_pre_sequence_async() without symmetric pm_runtime_put() in > dapm_post_sequence_async(). > Don't call pm_runtime_* on card device. Why is this a good fix, as opposed to only skipping the set_bias_level() bit?
On Fri, Jul 24, 2020 at 7:16 PM Mark Brown <broonie@kernel.org> wrote: > > On Fri, Jul 24, 2020 at 03:07:31PM +0800, Tzung-Bi Shih wrote: > > > Commit 4e872a46823c ("ASoC: dapm: Don't force card bias level to be > > updated") stops to force update bias_level on card. If card doesn't > > provide set_bias_level callback, the snd_soc_dapm_set_bias_level() > > is equivalent to NOP for card device. > > > As a result, dapm_pre_sequence_async() doesn't change the bias_level of > > card device correctly. Thus, pm_runtime_get_sync() would be called in > > dapm_pre_sequence_async() without symmetric pm_runtime_put() in > > dapm_post_sequence_async(). > > > Don't call pm_runtime_* on card device. > > Why is this a good fix, as opposed to only skipping the set_bias_level() > bit? Did you mean: skip to call snd_soc_dapm_set_bias_level() in dapm_pre_sequence_async() and dapm_post_sequence_async()? If so, skipping snd_soc_dapm_set_bias_level() won't fix the issue. The runtime_usage increases in pm_runtime_get() and decreases in pm_runtime_put(). Besides, snd_soc_dapm_set_bias_level() calls card->set_bias_level() and card->set_bias_level_post() if any. Skip to call snd_soc_dapm_set_bias_level() couldn't be a good idea. It may change some existing code behavior.
On Fri, Jul 24, 2020 at 08:26:13PM +0800, Tzung-Bi Shih wrote: > On Fri, Jul 24, 2020 at 7:16 PM Mark Brown <broonie@kernel.org> wrote: > > Why is this a good fix, as opposed to only skipping the set_bias_level() > > bit? > Did you mean: skip to call snd_soc_dapm_set_bias_level() in > dapm_pre_sequence_async() and dapm_post_sequence_async()? No, I mean why not just add the missing puts which are currently being skipped due to being caught up with the bias level changes.
On Fri, Jul 24, 2020 at 9:02 PM Mark Brown <broonie@kernel.org> wrote: > On Fri, Jul 24, 2020 at 08:26:13PM +0800, Tzung-Bi Shih wrote: > > On Fri, Jul 24, 2020 at 7:16 PM Mark Brown <broonie@kernel.org> wrote: > > > > Why is this a good fix, as opposed to only skipping the set_bias_level() > > > bit? > > > Did you mean: skip to call snd_soc_dapm_set_bias_level() in > > dapm_pre_sequence_async() and dapm_post_sequence_async()? > > No, I mean why not just add the missing puts which are currently being > skipped due to being caught up with the bias level changes. The challenge I'm facing: (&card->dapm)->bias_level is always SND_SOC_BIAS_OFF. Commit 4e872a46823c stops to update it. It has nowhere to add the missing puts(). Ideally, if the bias_level goes away SND_SOC_BIAS_OFF in dapm_pre_sequence_async(), it calls pm_runtime_get(). If the bias_level goes into SND_SOC_BIAS_OFF in dapm_post_sequence_async(), it calls pm_runtime_put(). I tried to revert commit 4e872a46823c but it seems to screw the card state up. Would you have any suggestions?
On Fri, 24 Jul 2020 15:07:31 +0800, Tzung-Bi Shih wrote: > runtime_usage of sound card has been observed to grow without bound. > For example: > $ cat /sys/devices/platform/sound/power/runtime_usage > 46 > $ sox -n -t s16 -r 48000 -c 2 - synth 1 sine 440 vol 0.1 | \ > aplay -q -D hw:0,0 -f S16_LE -r 48000 -c 2 > $ cat /sys/devices/platform/sound/power/runtime_usage > 52 > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: dapm: don't call pm_runtime_* on card device commit: 3aecfc72d7ad73330e7a6ebd0005738a8fd417ab All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
On Fri, Jul 24, 2020 at 10:41:41PM +0800, Tzung-Bi Shih wrote: > The challenge I'm facing: (&card->dapm)->bias_level is always > SND_SOC_BIAS_OFF. Commit 4e872a46823c stops to update it. It has > nowhere to add the missing puts(). > Ideally, if the bias_level goes away SND_SOC_BIAS_OFF in > dapm_pre_sequence_async(), it calls pm_runtime_get(). If the > bias_level goes into SND_SOC_BIAS_OFF in dapm_post_sequence_async(), > it calls pm_runtime_put(). Right, I see the problem now. This is probably fine.
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index 5076299abf37..3273161e2787 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1798,7 +1798,7 @@ static void dapm_pre_sequence_async(void *data, async_cookie_t cookie) /* If we're off and we're not supposed to go into STANDBY */ if (d->bias_level == SND_SOC_BIAS_OFF && d->target_bias_level != SND_SOC_BIAS_OFF) { - if (d->dev) + if (d->dev && cookie) pm_runtime_get_sync(d->dev); ret = snd_soc_dapm_set_bias_level(d, SND_SOC_BIAS_STANDBY); @@ -1845,7 +1845,7 @@ static void dapm_post_sequence_async(void *data, async_cookie_t cookie) dev_err(d->dev, "ASoC: Failed to turn off bias: %d\n", ret); - if (d->dev) + if (d->dev && cookie) pm_runtime_put(d->dev); }
runtime_usage of sound card has been observed to grow without bound. For example: $ cat /sys/devices/platform/sound/power/runtime_usage 46 $ sox -n -t s16 -r 48000 -c 2 - synth 1 sine 440 vol 0.1 | \ aplay -q -D hw:0,0 -f S16_LE -r 48000 -c 2 $ cat /sys/devices/platform/sound/power/runtime_usage 52 Commit 4e872a46823c ("ASoC: dapm: Don't force card bias level to be updated") stops to force update bias_level on card. If card doesn't provide set_bias_level callback, the snd_soc_dapm_set_bias_level() is equivalent to NOP for card device. As a result, dapm_pre_sequence_async() doesn't change the bias_level of card device correctly. Thus, pm_runtime_get_sync() would be called in dapm_pre_sequence_async() without symmetric pm_runtime_put() in dapm_post_sequence_async(). Don't call pm_runtime_* on card device. Signed-off-by: Tzung-Bi Shih <tzungbi@google.com> --- sound/soc/soc-dapm.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Hi, I'm not very sure if the patch makes sense. I was considering: (1) update the bias_level of card device correctly (2) skip pm_runtime_* for card device Per commit message on 4e872a46823c, I believe there is some context I'm not aware of it. Thus, the patch adopts (2).