[PATCH] ASoC: dpcm: improve runtime update predictability

[PATCH] ASoC: dpcm: improve runtime update predictability

[Jerome Brunet]

As it is, dpcm_runtime_update() performs the old path and new path
update of a frontend before going on to the next frontend DAI.
Depending the order of the FEs within the rtd list, the result of
the update might be different.

For example:
 * Frontend A connected to backend C, with a 48kHz playback
 * Frontend B connected to backend D, with a 44.1kHz playback
 * FE A appears before FE B in the rtd list of the card.

If we reparent BE C to FE B (disconnecting BE D):
* old path update of FE A will run first, and BE C will get hw_free()
  and shutdown()
* new path update of FE B will run after and BE C, which is stopped,
  so it will be configured at 44.1kHz, as expected

If we reparent BE D to FE A (disconnecting BE C):
* new path update of FE A will run first but since BE D is still running
  at 44.1kHz, it won't be reconfigured (no call to startup() or
  hw_params())
* old path update of FE B runs after, nothing happens
* In this case, we end up with a BE playing at 44.1kHz a stream which is
  supposed to be played at 48Khz (too slow)

To improve this situation, this patch performs all the FE old paths update
before going on to update the new paths. With this, the result should
no longer depend on the order of the FE within the card rtd list.

Please note that there might be a small performance penalty since
dpcm_process_paths() is called twice per stream direction.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 sound/soc/soc-pcm.c | 165 +++++++++++++++++++++++++++-------------------------
 1 file changed, 86 insertions(+), 79 deletions(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 5e7ae47a9658..5c2534414478 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2543,106 +2543,113 @@ static int dpcm_run_old_update(struct snd_soc_pcm_runtime *fe, int stream)
 	return ret;
 }
 
-/* Called by DAPM mixer/mux changes to update audio routing between PCMs and
- * any DAI links.
- */
-int soc_dpcm_runtime_update(struct snd_soc_card *card)
+int soc_dpcm_fe_runtime_update(struct snd_soc_pcm_runtime *fe, int new)
 {
-	struct snd_soc_pcm_runtime *fe;
-	int old, new, paths;
+	struct snd_soc_dapm_widget_list *list;
+	int count, paths;
 
-	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
-	list_for_each_entry(fe, &card->rtd_list, list) {
-		struct snd_soc_dapm_widget_list *list;
+	if (!fe->dai_link->dynamic)
+		return 0;
 
-		/* make sure link is FE */
-		if (!fe->dai_link->dynamic)
-			continue;
+	/* only check active links */
+	if (!fe->cpu_dai->active)
+		return 0;
 
-		/* only check active links */
-		if (!fe->cpu_dai->active)
-			continue;
+	/* DAPM sync will call this to update DSP paths */
+	dev_dbg(fe->dev, "ASoC: DPCM %s runtime update for FE %s\n",
+		new ? "new" : "old", fe->dai_link->name);
 
-		/* DAPM sync will call this to update DSP paths */
-		dev_dbg(fe->dev, "ASoC: DPCM runtime update for FE %s\n",
-			fe->dai_link->name);
+	/* skip if FE doesn't have playback capability */
+	if (!fe->cpu_dai->driver->playback.channels_min ||
+	    !fe->codec_dai->driver->playback.channels_min)
+		goto capture;
 
-		/* skip if FE doesn't have playback capability */
-		if (!fe->cpu_dai->driver->playback.channels_min
-		    || !fe->codec_dai->driver->playback.channels_min)
-			goto capture;
-
-		/* skip if FE isn't currently playing */
-		if (!fe->cpu_dai->playback_active
-		    || !fe->codec_dai->playback_active)
-			goto capture;
-
-		paths = dpcm_path_get(fe, SNDRV_PCM_STREAM_PLAYBACK, &list);
-		if (paths < 0) {
-			dev_warn(fe->dev, "ASoC: %s no valid %s path\n",
-					fe->dai_link->name,  "playback");
-			mutex_unlock(&card->mutex);
-			return paths;
-		}
+	/* skip if FE isn't currently playing */
+	if (!fe->cpu_dai->playback_active || !fe->codec_dai->playback_active)
+		goto capture;
 
-		/* update any new playback paths */
-		new = dpcm_process_paths(fe, SNDRV_PCM_STREAM_PLAYBACK, &list, 1);
-		if (new) {
-			dpcm_run_new_update(fe, SNDRV_PCM_STREAM_PLAYBACK);
-			dpcm_clear_pending_state(fe, SNDRV_PCM_STREAM_PLAYBACK);
-			dpcm_be_disconnect(fe, SNDRV_PCM_STREAM_PLAYBACK);
-		}
+	paths = dpcm_path_get(fe, SNDRV_PCM_STREAM_PLAYBACK, &list);
+	if (paths < 0) {
+		dev_warn(fe->dev, "ASoC: %s no valid %s path\n",
+			 fe->dai_link->name,  "playback");
+		return paths;
+	}
 
-		/* update any old playback paths */
-		old = dpcm_process_paths(fe, SNDRV_PCM_STREAM_PLAYBACK, &list, 0);
-		if (old) {
+	/* update any playback paths */
+	count = dpcm_process_paths(fe, SNDRV_PCM_STREAM_PLAYBACK, &list, new);
+	if (count) {
+		if (new)
+			dpcm_run_new_update(fe, SNDRV_PCM_STREAM_PLAYBACK);
+		else
 			dpcm_run_old_update(fe, SNDRV_PCM_STREAM_PLAYBACK);
-			dpcm_clear_pending_state(fe, SNDRV_PCM_STREAM_PLAYBACK);
-			dpcm_be_disconnect(fe, SNDRV_PCM_STREAM_PLAYBACK);
-		}
 
-		dpcm_path_put(&list);
+		dpcm_clear_pending_state(fe, SNDRV_PCM_STREAM_PLAYBACK);
+		dpcm_be_disconnect(fe, SNDRV_PCM_STREAM_PLAYBACK);
+	}
+
+	dpcm_path_put(&list);
+
 capture:
-		/* skip if FE doesn't have capture capability */
-		if (!fe->cpu_dai->driver->capture.channels_min
-		    || !fe->codec_dai->driver->capture.channels_min)
-			continue;
+	/* skip if FE doesn't have capture capability */
+	if (!fe->cpu_dai->driver->capture.channels_min ||
+	    !fe->codec_dai->driver->capture.channels_min)
+		return 0;
 
-		/* skip if FE isn't currently capturing */
-		if (!fe->cpu_dai->capture_active
-		    || !fe->codec_dai->capture_active)
-			continue;
+	/* skip if FE isn't currently capturing */
+	if (!fe->cpu_dai->capture_active || !fe->codec_dai->capture_active)
+		return 0;
 
-		paths = dpcm_path_get(fe, SNDRV_PCM_STREAM_CAPTURE, &list);
-		if (paths < 0) {
-			dev_warn(fe->dev, "ASoC: %s no valid %s path\n",
-					fe->dai_link->name,  "capture");
-			mutex_unlock(&card->mutex);
-			return paths;
-		}
+	paths = dpcm_path_get(fe, SNDRV_PCM_STREAM_CAPTURE, &list);
+	if (paths < 0) {
+		dev_warn(fe->dev, "ASoC: %s no valid %s path\n",
+			 fe->dai_link->name,  "capture");
+		return paths;
+	}
 
-		/* update any new capture paths */
-		new = dpcm_process_paths(fe, SNDRV_PCM_STREAM_CAPTURE, &list, 1);
-		if (new) {
+	/* update any old capture paths */
+	count = dpcm_process_paths(fe, SNDRV_PCM_STREAM_CAPTURE, &list, new);
+	if (count) {
+		if (new)
 			dpcm_run_new_update(fe, SNDRV_PCM_STREAM_CAPTURE);
-			dpcm_clear_pending_state(fe, SNDRV_PCM_STREAM_CAPTURE);
-			dpcm_be_disconnect(fe, SNDRV_PCM_STREAM_CAPTURE);
-		}
-
-		/* update any old capture paths */
-		old = dpcm_process_paths(fe, SNDRV_PCM_STREAM_CAPTURE, &list, 0);
-		if (old) {
+		else
 			dpcm_run_old_update(fe, SNDRV_PCM_STREAM_CAPTURE);
-			dpcm_clear_pending_state(fe, SNDRV_PCM_STREAM_CAPTURE);
-			dpcm_be_disconnect(fe, SNDRV_PCM_STREAM_CAPTURE);
-		}
 
-		dpcm_path_put(&list);
+		dpcm_clear_pending_state(fe, SNDRV_PCM_STREAM_CAPTURE);
+		dpcm_be_disconnect(fe, SNDRV_PCM_STREAM_CAPTURE);
 	}
 
-	mutex_unlock(&card->mutex);
+	dpcm_path_put(&list);
+
 	return 0;
 }
+
+/* Called by DAPM mixer/mux changes to update audio routing between PCMs and
+ * any DAI links.
+ */
+int soc_dpcm_runtime_update(struct snd_soc_card *card)
+{
+	struct snd_soc_pcm_runtime *fe;
+	int ret = 0;
+
+	mutex_lock_nested(&card->mutex, SND_SOC_CARD_CLASS_RUNTIME);
+	/* shutdown all old paths first */
+	list_for_each_entry(fe, &card->rtd_list, list) {
+		ret = soc_dpcm_fe_runtime_update(fe, 0);
+		if (ret)
+			goto out;
+	}
+
+	/* bring new paths up */
+	list_for_each_entry(fe, &card->rtd_list, list) {
+		ret = soc_dpcm_fe_runtime_update(fe, 1);
+		if (ret)
+			goto out;
+	}
+
+out:
+	mutex_unlock(&card->mutex);
+	return ret;
+}
 int soc_dpcm_be_digital_mute(struct snd_soc_pcm_runtime *fe, int mute)
 {
 	struct snd_soc_dpcm *dpcm;
-- 
2.14.4

[RFC PATCH] ASoC: dpcm: soc_dpcm_fe_runtime_update() can be static

[kbuild test robot]

Fixes: 8d13e13e7869 ("ASoC: dpcm: improve runtime update predictability")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 soc-pcm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 330de23..63f96cd 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2597,7 +2597,7 @@ static int dpcm_run_old_update(struct snd_soc_pcm_runtime *fe, int stream)
 	return ret;
 }
 
-int soc_dpcm_fe_runtime_update(struct snd_soc_pcm_runtime *fe, int new)
+static int soc_dpcm_fe_runtime_update(struct snd_soc_pcm_runtime *fe, int new)
 {
 	struct snd_soc_dapm_widget_list *list;
 	int count, paths;

Re: [PATCH] ASoC: dpcm: improve runtime update predictability

[kbuild test robot]

Hi Jerome,

I love your patch! Perhaps something to improve:

[auto build test WARNING on asoc/for-next]
[also build test WARNING on v4.18-rc2 next-20180625]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jerome-Brunet/ASoC-dpcm-improve-runtime-update-predictability/20180625-172002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

   sound/soc/soc-pcm.c:357:32: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:357:32: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:367:32: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:367:32: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:417:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:417:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:418:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:418:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:419:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:419:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:420:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:420:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:435:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:435:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:436:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:436:28: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:445:24: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:445:24: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:446:24: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:446:24: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:447:24: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:447:24: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:448:24: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:448:24: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1194:39: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1194:39: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1750:41: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1750:41: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1752:41: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1752:41: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1774:36: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1774:36: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1775:36: sparse: expression using sizeof(void)
   sound/soc/soc-pcm.c:1775:36: sparse: expression using sizeof(void)
>> sound/soc/soc-pcm.c:2600:5: sparse: symbol 'soc_dpcm_fe_runtime_update' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation