[PATCH v2 0/8] ASoC: soc-pcm cleanup step5
Hi Mark
These are v2 of soc-pcm cleanup step5. These are based on latest for-5.7 branch. Mainly v2 fixed the points reviewed in v1.
[7/8] patch might exchange the behavior
Kuninori Morimoto (8): 1) ASoC: soc-pcm: use defined stream 2) ASoC: soc-pcm: remove duplicate be check from dpcm_add_paths() 3) ASoC: soc-pcm: move dpcm_fe_dai_close() 4) ASoC: soc-pcm: add dpcm_fe_dai_cleanup() 5) ASoC: soc-pcm: use snd_soc_dai_get_pcm_stream() at dpcm_set_fe_runtime() 6) ASoC: soc-pcm: check DAI's activity more simply 7) ASoC: soc-pcm: Do Digital Mute for both CPU/Codec in same timing. 8) ASoC: soc-pcm: tidyup dulicate handing at dpcm_fe_dai_startup()
sound/soc/soc-pcm.c | 114 +++++++++++++++++++------------------------- 1 file changed, 50 insertions(+), 64 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Many functions defines "stream = substream->stream", but some of them is using "substream->stream" instead of "stream". It is pointless. This patch uses defined stream.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v1 -> v2
- add Reviewed-by from Ranjani / Pierre-Louis
sound/soc/soc-pcm.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 90857138c823..8c27eb4d5e4c 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -644,8 +644,7 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) * bailed out on a higher level, since there would be no * CODEC to support the transfer direction in that case. */ - if (!snd_soc_dai_stream_valid(codec_dai, - substream->stream)) + if (!snd_soc_dai_stream_valid(codec_dai, stream)) continue;
codec_stream = snd_soc_dai_get_pcm_stream(codec_dai, stream); @@ -2149,7 +2148,7 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream)
dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
- ret = dpcm_be_dai_startup(fe, fe_substream->stream); + ret = dpcm_be_dai_startup(fe, stream); if (ret < 0) { dev_err(fe->dev,"ASoC: failed to start some BEs %d\n", ret); goto be_err; @@ -2180,7 +2179,7 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream) return 0;
unwind: - dpcm_be_dai_startup_unwind(fe, fe_substream->stream); + dpcm_be_dai_startup_unwind(fe, stream); be_err: dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO); return ret; @@ -2234,7 +2233,7 @@ static int dpcm_fe_dai_shutdown(struct snd_pcm_substream *substream) dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
/* shutdown the BEs */ - dpcm_be_dai_shutdown(fe, substream->stream); + dpcm_be_dai_shutdown(fe, stream);
dev_dbg(fe->dev, "ASoC: close FE %s\n", fe->dai_link->name);
@@ -2412,9 +2411,9 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream, mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME); dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_FE);
- memcpy(&fe->dpcm[substream->stream].hw_params, params, + memcpy(&fe->dpcm[stream].hw_params, params, sizeof(struct snd_pcm_hw_params)); - ret = dpcm_be_dai_hw_params(fe, substream->stream); + ret = dpcm_be_dai_hw_params(fe, stream); if (ret < 0) { dev_err(fe->dev,"ASoC: hw_params BE failed %d\n", ret); goto out; @@ -2736,7 +2735,7 @@ static int dpcm_fe_dai_prepare(struct snd_pcm_substream *substream) goto out; }
- ret = dpcm_be_dai_prepare(fe, substream->stream); + ret = dpcm_be_dai_prepare(fe, stream); if (ret < 0) goto out;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
dpcm_add_paths() checks returned be from dpcm_get_be()
static int dpcm_add_paths(...) { ... for_each_dapm_widgets(list, i, widget) { ... be = dpcm_get_be(...); ... /* make sure BE is a real BE */ => if (!be->dai_link->no_pcm) continue; ... } ... }
But, dpcm_get_be() itself is checking it already.
dpcm_get_be(...) { ... for_each_card_rtds(card, be) { => if (!be->dai_link->no_pcm) continue; ... if (...) => return be; }
return NULL }
This patch removes duplicate check
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v1 -> v2
- add Reviewed-by from Ranjani / Pierre-Louis
sound/soc/soc-pcm.c | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8c27eb4d5e4c..e3a2c4f7757b 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1690,10 +1690,6 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, continue; }
- /* make sure BE is a real BE */ - if (!be->dai_link->no_pcm) - continue; - /* don't connect if FE is not running */ if (!fe->dpcm[stream].runtime && !fe->fe_compr) continue;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
move dpcm_fe_dai_close() next to dpcm_fe_dai_open(). This is prepare for dpcm_fe_dai_open() cleanup
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v1 -> v2
- add Reviewed-by from Ranjani / Pierre-Louis
sound/soc/soc-pcm.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index e3a2c4f7757b..3686dda097e2 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2978,6 +2978,26 @@ int soc_dpcm_runtime_update(struct snd_soc_card *card) return ret; }
+static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) +{ + struct snd_soc_pcm_runtime *fe = fe_substream->private_data; + struct snd_soc_dpcm *dpcm; + int stream = fe_substream->stream, ret; + + mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME); + ret = dpcm_fe_dai_shutdown(fe_substream); + + /* mark FE's links ready to prune */ + for_each_dpcm_be(fe, stream, dpcm) + dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE; + + dpcm_be_disconnect(fe, stream); + + fe->dpcm[stream].runtime = NULL; + mutex_unlock(&fe->card->mutex); + return ret; +} + static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream) { struct snd_soc_pcm_runtime *fe = fe_substream->private_data; @@ -3017,26 +3037,6 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream) return ret; }
-static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) -{ - struct snd_soc_pcm_runtime *fe = fe_substream->private_data; - struct snd_soc_dpcm *dpcm; - int stream = fe_substream->stream, ret; - - mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME); - ret = dpcm_fe_dai_shutdown(fe_substream); - - /* mark FE's links ready to prune */ - for_each_dpcm_be(fe, stream, dpcm) - dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE; - - dpcm_be_disconnect(fe, stream); - - fe->dpcm[stream].runtime = NULL; - mutex_unlock(&fe->card->mutex); - return ret; -} - /* create a new pcm */ int soc_new_pcm(struct snd_soc_pcm_runtime *rtd, int num) {
On Thu, Feb 27, 2020 at 10:54:53AM +0900, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
move dpcm_fe_dai_close() next to dpcm_fe_dai_open(). This is prepare for dpcm_fe_dai_open() cleanup
This doesn't apply against current code, please check and resend.
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
dpcm_fe_dai_close() and error case of dpcm_fe_dai_open() need to do same cleanup operation. To avoid duplicate code, this patch adds dpcm_fe_dai_cleanup() and use it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v1 -> v2
- add Reviewed-by from Ranjani / Pierre-Louis - rename dpcm_fe_dai_clean() -> cleanup()
sound/soc/soc-pcm.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 3686dda097e2..b405fb3a181b 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2978,14 +2978,11 @@ int soc_dpcm_runtime_update(struct snd_soc_card *card) return ret; }
-static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) +static void dpcm_fe_dai_cleanup(struct snd_pcm_substream *fe_substream) { struct snd_soc_pcm_runtime *fe = fe_substream->private_data; struct snd_soc_dpcm *dpcm; - int stream = fe_substream->stream, ret; - - mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME); - ret = dpcm_fe_dai_shutdown(fe_substream); + int stream = fe_substream->stream;
/* mark FE's links ready to prune */ for_each_dpcm_be(fe, stream, dpcm) @@ -2994,6 +2991,18 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) dpcm_be_disconnect(fe, stream);
fe->dpcm[stream].runtime = NULL; +} + +static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) +{ + struct snd_soc_pcm_runtime *fe = fe_substream->private_data; + int ret; + + mutex_lock_nested(&fe->card->mutex, SND_SOC_CARD_CLASS_RUNTIME); + ret = dpcm_fe_dai_shutdown(fe_substream); + + dpcm_fe_dai_cleanup(fe_substream); + mutex_unlock(&fe->card->mutex); return ret; } @@ -3001,7 +3010,6 @@ static int dpcm_fe_dai_close(struct snd_pcm_substream *fe_substream) static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream) { struct snd_soc_pcm_runtime *fe = fe_substream->private_data; - struct snd_soc_dpcm *dpcm; struct snd_soc_dapm_widget_list *list; int ret; int stream = fe_substream->stream; @@ -3021,14 +3029,8 @@ static int dpcm_fe_dai_open(struct snd_pcm_substream *fe_substream) dpcm_process_paths(fe, stream, &list, 1);
ret = dpcm_fe_dai_startup(fe_substream); - if (ret < 0) { - /* clean up all links */ - for_each_dpcm_be(fe, stream, dpcm) - dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE; - - dpcm_be_disconnect(fe, stream); - fe->dpcm[stream].runtime = NULL; - } + if (ret < 0) + dpcm_fe_dai_cleanup(fe_substream);
dpcm_clear_pending_state(fe, stream); dpcm_path_put(&list);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
We already have snd_soc_dai_get_pcm_stream(), let's use it
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v1 -> v2
- add Reviewed-by from Ranjani / Pierre-Louis
sound/soc/soc-pcm.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index b405fb3a181b..3a30776858bf 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2022,7 +2022,6 @@ static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream) struct snd_pcm_runtime *runtime = substream->runtime; struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *cpu_dai; - struct snd_soc_dai_driver *cpu_dai_drv; int i;
for_each_rtd_cpu_dai(rtd, i, cpu_dai) { @@ -2033,11 +2032,9 @@ static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream) if (!snd_soc_dai_stream_valid(cpu_dai, substream->stream)) continue;
- cpu_dai_drv = cpu_dai->driver; - if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) - dpcm_init_runtime_hw(runtime, &cpu_dai_drv->playback); - else - dpcm_init_runtime_hw(runtime, &cpu_dai_drv->capture); + dpcm_init_runtime_hw(runtime, + snd_soc_dai_get_pcm_stream(cpu_dai, + substream->stream)); }
dpcm_runtime_merge_format(substream, &runtime->hw.formats);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
DAI is counting its activity, and both playback/capture directional activity. When considering mute, DAI's activity is enough instead of caring both playback/capture. This patch makes mute check simply.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v1 -> v2
- add Reviewed-by from Ranjani / Pierre-Louis - use "if (codec_dai->active)"
sound/soc/soc-pcm.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 3a30776858bf..b83e13565e2a 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1202,7 +1202,6 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_dai *cpu_dai; struct snd_soc_dai *codec_dai; - bool playback = substream->stream == SNDRV_PCM_STREAM_PLAYBACK; int i;
mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); @@ -1225,15 +1224,10 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) }
/* apply codec digital mute */ - for_each_rtd_codec_dai(rtd, i, codec_dai) { - int playback_active = codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK]; - int capture_active = codec_dai->stream_active[SNDRV_PCM_STREAM_CAPTURE]; - - if ((playback && playback_active == 1) || - (!playback && capture_active == 1)) + for_each_rtd_codec_dai(rtd, i, codec_dai) + if (codec_dai->active) snd_soc_dai_digital_mute(codec_dai, 1, substream->stream); - }
/* free any machine hw params */ soc_rtd_hw_free(rtd, substream);
Hi Mark
@@ -1225,15 +1224,10 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) }
/* apply codec digital mute */
- for_each_rtd_codec_dai(rtd, i, codec_dai) {
int playback_active = codec_dai->stream_active[SNDRV_PCM_STREAM_PLAYBACK];int capture_active = codec_dai->stream_active[SNDRV_PCM_STREAM_CAPTURE];if ((playback && playback_active == 1) ||(!playback && capture_active == 1))
- for_each_rtd_codec_dai(rtd, i, codec_dai)
if (codec_dai->active) snd_soc_dai_digital_mute(codec_dai, 1, substream->stream);
I noticed that it calls snd_soc_dai_digital_mute(xxx, substream->stream). This means, it want to call mute for each Playback/Capture "last user". This patch was total wrong.
Mark, [6/8] is wrong. maybe I need to reconsider [7/8]. Can you please ignore these ? Please let me know if you need v3
Thank you for your help !! Best regards --- Kuninori Morimoto
On Thu, Feb 27, 2020 at 11:38:41AM +0900, Kuninori Morimoto wrote:
@@ -1225,15 +1224,10 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream)
- for_each_rtd_codec_dai(rtd, i, codec_dai)
if (codec_dai->active) snd_soc_dai_digital_mute(codec_dai, 1, substream->stream);
I noticed that it calls snd_soc_dai_digital_mute(xxx, substream->stream). This means, it want to call mute for each Playback/Capture "last user". This patch was total wrong.
Mark, [6/8] is wrong. maybe I need to reconsider [7/8]. Can you please ignore these ? Please let me know if you need v3
I can drop those two, it's fine.
Hi
- for_each_rtd_codec_dai(rtd, i, codec_dai)
if (codec_dai->active) snd_soc_dai_digital_mute(codec_dai, 1, substream->stream);I noticed that it calls snd_soc_dai_digital_mute(xxx, substream->stream). This means, it want to call mute for each Playback/Capture "last user". This patch was total wrong.
Mark, [6/8] is wrong. maybe I need to reconsider [7/8]. Can you please ignore these ? Please let me know if you need v3
I can drop those two, it's fine.
Thanks !
Thank you for your help !! Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Digital Mute for CPU is done at soc_pcm_close(), and Digital Mute for Codec is done at soc_pcm_hw_free(). It is just confusable. This patch do Digital Mute for both CPU/Codec in same timing. Then, it cares DAI activity
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v1 -> v2
- add Reviewed-by from Ranjani / Pierre-Louis
sound/soc/soc-pcm.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index b83e13565e2a..ea900ab06d84 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -760,9 +760,6 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
snd_soc_runtime_deactivate(rtd, substream->stream);
- for_each_rtd_cpu_dai(rtd, i, cpu_dai) - snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream); - for_each_rtd_cpu_dai(rtd, i, cpu_dai) snd_soc_dai_shutdown(cpu_dai, substream);
@@ -1229,6 +1226,11 @@ static int soc_pcm_hw_free(struct snd_pcm_substream *substream) snd_soc_dai_digital_mute(codec_dai, 1, substream->stream);
+ for_each_rtd_cpu_dai(rtd, i, cpu_dai) + if (cpu_dai->active) + snd_soc_dai_digital_mute(cpu_dai, 1, + substream->stream); + /* free any machine hw params */ soc_rtd_hw_free(rtd, substream);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
Error handling at dpcm_fe_dai_startup() has duplicate code. This patch tidyup it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com --- v1 -> v2
- add Reviewed-by from Ranjani / Pierre-Louis - remove unneeded "goto unwind"
sound/soc/soc-pcm.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index ea900ab06d84..5222f758b007 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -2158,17 +2158,13 @@ static int dpcm_fe_dai_startup(struct snd_pcm_substream *fe_substream) snd_pcm_limit_hw_rates(runtime);
ret = dpcm_apply_symmetry(fe_substream, stream); - if (ret < 0) { + if (ret < 0) dev_err(fe->dev, "ASoC: failed to apply dpcm symmetry %d\n", ret); - goto unwind; - } - - dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO); - return 0;
unwind: - dpcm_be_dai_startup_unwind(fe, stream); + if (ret < 0) + dpcm_be_dai_startup_unwind(fe, stream); be_err: dpcm_set_fe_update_state(fe, stream, SND_SOC_DPCM_UPDATE_NO); return ret;
participants (2)
-
Kuninori Morimoto -
Mark Brown