diff mbox series

[5/7] ASoC: soc-pcm: add soc_pcm_clean() and call it from soc_pcm_open/close()

Message ID 87ft72bwn4.wl-kuninori.morimoto.gx@renesas.com
State Superseded
Headers show
Series ASoC: merge soc_pcm_open() rollback and soc_pcm_close() | expand

Commit Message

Kuninori Morimoto Sept. 28, 2020, 12:01 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

soc_pcm_open() does rollback when failed (A),
but, it is almost same as soc_pcm_close().

	static int soc_pcm_open(xxx)
	{
		...
		if (ret < 0)
			goto xxx_err;
		...
		return 0;

 ^	config_err:
 |		...
 |	rtd_startup_err:
(A)		...
 |	component_err:
 |		...
 v		return ret;
	}

The difference is
soc_pcm_close() is for all dai/component/substream,
rollback        is for succeeded part only.

This kind of duplicated code can be a hotbed of bugs,
thus, we want to share soc_pcm_close() and rollback.

Now, soc_pcm_open/close() are handling
	1) snd_soc_dai_startup/shutdown()
	2) snd_soc_link_startup/shutdown()
	3) snd_soc_component_module_get/put()
	4) snd_soc_component_open/close()
	5) pm_runtime_put/get()

Now, 1) to 5) are handled.
This patch adds new soc_pcm_clean() and call it from
soc_pcm_open() as rollback, and from soc_pcm_close() as
normal close handler.

One note here is that it don't need to call snd_soc_runtime_deactivate()
when rollback case, because it will be called without
snd_soc_runtime_activate().
It also don't need to call snd_soc_dapm_stream_stop() when rollback case.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/soc-pcm.c | 69 ++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 39 deletions(-)

Comments

Marek Szyprowski Sept. 29, 2020, 1:08 p.m. UTC | #1 
Hi

On 28.09.2020 02:01, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>
> soc_pcm_open() does rollback when failed (A),
> but, it is almost same as soc_pcm_close().
>
> 	static int soc_pcm_open(xxx)
> 	{
> 		...
> 		if (ret < 0)
> 			goto xxx_err;
> 		...
> 		return 0;
>
>   ^	config_err:
>   |		...
>   |	rtd_startup_err:
> (A)		...
>   |	component_err:
>   |		...
>   v		return ret;
> 	}
>
> The difference is
> soc_pcm_close() is for all dai/component/substream,
> rollback        is for succeeded part only.
>
> This kind of duplicated code can be a hotbed of bugs,
> thus, we want to share soc_pcm_close() and rollback.
>
> Now, soc_pcm_open/close() are handling
> 	1) snd_soc_dai_startup/shutdown()
> 	2) snd_soc_link_startup/shutdown()
> 	3) snd_soc_component_module_get/put()
> 	4) snd_soc_component_open/close()
> 	5) pm_runtime_put/get()
>
> Now, 1) to 5) are handled.
> This patch adds new soc_pcm_clean() and call it from
> soc_pcm_open() as rollback, and from soc_pcm_close() as
> normal close handler.
>
> One note here is that it don't need to call snd_soc_runtime_deactivate()
> when rollback case, because it will be called without
> snd_soc_runtime_activate().
> It also don't need to call snd_soc_dapm_stream_stop() when rollback case.
>
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

This patch landed in linux next-20200929 as commit 140a4532cdb8 ("ASoC: 
soc-pcm: add soc_pcm_clean() and call it from soc_pcm_open/close()"). 
Sadly it causes a regression in ALSA operation on my various test 
boards: Exynos4412 based Trats2, Exynos5410 based Odroid XU, Exynos5250 
Snow Chromebook and other. The first app, which tries to open ALSA 
device fails. Then, on the second try, it work.

Here is a log from Odroid XU:

[    3.775032] max98090 1-0010: MAX98090 REVID=0x43
[    3.781958] max98090 1-0010: use default 2.8v micbias
[    3.812813] ALSA device list:
[    3.814448]   #0: Odroid-XU

# speaker-test -l1

speaker-test 1.1.3

Playback device is default
Stream parameters are 48000Hz, S16_LE, 1 channels
Using 16 octaves of pink noise
Playback open error: -22,Invalid argument
# speaker-test -l1

speaker-test 1.1.3

Playback device is default
Stream parameters are 48000Hz, S16_LE, 1 channels
Using 16 octaves of pink noise
Rate set to 48000Hz (requested 48000Hz)
Buffer size range from 128 to 131072
Period size range from 64 to 65536
Using max buffer size 131072
Periods = 4
was set period_size = 32768
was set buffer_size = 131072
  0 - Front Left
Time per period = 0.029512
#

 > ...

Best regards
Kuninori Morimoto Sept. 30, 2020, 11:28 p.m. UTC | #2 
Hi Marek

Thank you for testing.
I will check it

> On 28.09.2020 02:01, Kuninori Morimoto wrote:
> > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >
> > soc_pcm_open() does rollback when failed (A),
> > but, it is almost same as soc_pcm_close().
> >
> > 	static int soc_pcm_open(xxx)
> > 	{
> > 		...
> > 		if (ret < 0)
> > 			goto xxx_err;
> > 		...
> > 		return 0;
> >
> >   ^	config_err:
> >   |		...
> >   |	rtd_startup_err:
> > (A)		...
> >   |	component_err:
> >   |		...
> >   v		return ret;
> > 	}
> >
> > The difference is
> > soc_pcm_close() is for all dai/component/substream,
> > rollback        is for succeeded part only.
> >
> > This kind of duplicated code can be a hotbed of bugs,
> > thus, we want to share soc_pcm_close() and rollback.
> >
> > Now, soc_pcm_open/close() are handling
> > 	1) snd_soc_dai_startup/shutdown()
> > 	2) snd_soc_link_startup/shutdown()
> > 	3) snd_soc_component_module_get/put()
> > 	4) snd_soc_component_open/close()
> > 	5) pm_runtime_put/get()
> >
> > Now, 1) to 5) are handled.
> > This patch adds new soc_pcm_clean() and call it from
> > soc_pcm_open() as rollback, and from soc_pcm_close() as
> > normal close handler.
> >
> > One note here is that it don't need to call snd_soc_runtime_deactivate()
> > when rollback case, because it will be called without
> > snd_soc_runtime_activate().
> > It also don't need to call snd_soc_dapm_stream_stop() when rollback case.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> This patch landed in linux next-20200929 as commit 140a4532cdb8 ("ASoC: 
> soc-pcm: add soc_pcm_clean() and call it from soc_pcm_open/close()"). 
> Sadly it causes a regression in ALSA operation on my various test 
> boards: Exynos4412 based Trats2, Exynos5410 based Odroid XU, Exynos5250 
> Snow Chromebook and other. The first app, which tries to open ALSA 
> device fails. Then, on the second try, it work.
> 
> Here is a log from Odroid XU:
> 
> [    3.775032] max98090 1-0010: MAX98090 REVID=0x43
> [    3.781958] max98090 1-0010: use default 2.8v micbias
> [    3.812813] ALSA device list:
> [    3.814448]   #0: Odroid-XU
> 
> # speaker-test -l1
> 
> speaker-test 1.1.3
> 
> Playback device is default
> Stream parameters are 48000Hz, S16_LE, 1 channels
> Using 16 octaves of pink noise
> Playback open error: -22,Invalid argument
> # speaker-test -l1
> 
> speaker-test 1.1.3
> 
> Playback device is default
> Stream parameters are 48000Hz, S16_LE, 1 channels
> Using 16 octaves of pink noise
> Rate set to 48000Hz (requested 48000Hz)
> Buffer size range from 128 to 131072
> Period size range from 64 to 65536
> Using max buffer size 131072
> Periods = 4
> was set period_size = 32768
> was set buffer_size = 131072
>   0 - Front Left
> Time per period = 0.029512
> #
> 
>  > ...
> 
> Best regards
> -- 
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
diff mbox series

Patch

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 0934a06a9cc9..7f3e44918f19 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -651,12 +651,7 @@  static int soc_pcm_components_close(struct snd_pcm_substream *substream,
 	return ret;
 }
 
-/*
- * Called by ALSA when a PCM substream is closed. Private data can be
- * freed here. The cpu DAI, codec DAI, machine and components are also
- * shutdown.
- */
-static int soc_pcm_close(struct snd_pcm_substream *substream)
+static int soc_pcm_clean(struct snd_pcm_substream *substream, int rollback)
 {
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	struct snd_soc_component *component;
@@ -665,20 +660,22 @@  static int soc_pcm_close(struct snd_pcm_substream *substream)
 
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
-	snd_soc_runtime_deactivate(rtd, substream->stream);
+	if (!rollback)
+		snd_soc_runtime_deactivate(rtd, substream->stream);
 
 	for_each_rtd_dais(rtd, i, dai)
-		snd_soc_dai_shutdown(dai, substream, 0);
+		snd_soc_dai_shutdown(dai, substream, rollback);
 
-	snd_soc_link_shutdown(substream, 0);
+	snd_soc_link_shutdown(substream, rollback);
 
-	soc_pcm_components_close(substream, 0);
+	soc_pcm_components_close(substream, rollback);
 
-	snd_soc_dapm_stream_stop(rtd, substream->stream);
+	if (!rollback)
+		snd_soc_dapm_stream_stop(rtd, substream->stream);
 
 	mutex_unlock(&rtd->card->pcm_mutex);
 
-	snd_soc_pcm_component_pm_runtime_put(rtd, substream, 0);
+	snd_soc_pcm_component_pm_runtime_put(rtd, substream, rollback);
 
 	for_each_rtd_components(rtd, i, component)
 		if (!snd_soc_component_active(component))
@@ -687,6 +684,16 @@  static int soc_pcm_close(struct snd_pcm_substream *substream)
 	return 0;
 }
 
+/*
+ * Called by ALSA when a PCM substream is closed. Private data can be
+ * freed here. The cpu DAI, codec DAI, machine and components are also
+ * shutdown.
+ */
+static int soc_pcm_close(struct snd_pcm_substream *substream)
+{
+	return soc_pcm_clean(substream, 0);
+}
+
 /*
  * Called by ALSA when a PCM substream is opened, the runtime->hw record is
  * then initialized and any private data can be allocated. This also calls
@@ -707,17 +714,17 @@  static int soc_pcm_open(struct snd_pcm_substream *substream)
 
 	ret = snd_soc_pcm_component_pm_runtime_get(rtd, substream);
 	if (ret < 0)
-		goto pm_err;
+		goto err;
 
 	mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
 
 	ret = soc_pcm_components_open(substream);
 	if (ret < 0)
-		goto component_err;
+		goto err;
 
 	ret = snd_soc_link_startup(substream);
 	if (ret < 0)
-		goto rtd_startup_err;
+		goto err;
 
 	/* startup the audio subsystem */
 	for_each_rtd_dais(rtd, i, dai) {
@@ -726,7 +733,7 @@  static int soc_pcm_open(struct snd_pcm_substream *substream)
 			dev_err(dai->dev,
 				"ASoC: can't open DAI %s: %d\n",
 				dai->name, ret);
-			goto config_err;
+			goto err;
 		}
 
 		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
@@ -755,18 +762,18 @@  static int soc_pcm_open(struct snd_pcm_substream *substream)
 	if (!runtime->hw.rates) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching rates\n",
 			codec_dai_name, cpu_dai_name);
-		goto config_err;
+		goto err;
 	}
 	if (!runtime->hw.formats) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching formats\n",
 			codec_dai_name, cpu_dai_name);
-		goto config_err;
+		goto err;
 	}
 	if (!runtime->hw.channels_min || !runtime->hw.channels_max ||
 	    runtime->hw.channels_min > runtime->hw.channels_max) {
 		printk(KERN_ERR "ASoC: %s <-> %s No matching channels\n",
 				codec_dai_name, cpu_dai_name);
-		goto config_err;
+		goto err;
 	}
 
 	soc_pcm_apply_msb(substream);
@@ -776,7 +783,7 @@  static int soc_pcm_open(struct snd_pcm_substream *substream)
 		if (snd_soc_dai_active(dai)) {
 			ret = soc_pcm_apply_symmetry(substream, dai);
 			if (ret != 0)
-				goto config_err;
+				goto err;
 		}
 	}
 
@@ -787,29 +794,13 @@  static int soc_pcm_open(struct snd_pcm_substream *substream)
 		 runtime->hw.channels_max);
 	pr_debug("ASoC: min rate %d max rate %d\n", runtime->hw.rate_min,
 		 runtime->hw.rate_max);
-
 dynamic:
-
 	snd_soc_runtime_activate(rtd, substream->stream);
-
-	mutex_unlock(&rtd->card->pcm_mutex);
-	return 0;
-
-config_err:
-	for_each_rtd_dais_rollback(rtd, i, dai)
-		snd_soc_dai_shutdown(dai, substream, 1);
-
-	snd_soc_link_shutdown(substream, 1);
-rtd_startup_err:
-	soc_pcm_components_close(substream, 1);
-component_err:
+err:
 	mutex_unlock(&rtd->card->pcm_mutex);
-pm_err:
-	snd_soc_pcm_component_pm_runtime_put(rtd, substream, 1);
 
-	for_each_rtd_components(rtd, i, component)
-		if (!snd_soc_component_active(component))
-			pinctrl_pm_select_sleep_state(component->dev);
+	if (ret < 0)
+		soc_pcm_clean(substream, 1);
 
 	return ret;
 }