[alsa-devel] [PATCH 0/7] ASoC: soc-pcm cleanup step2
Hi Mark
These are soc-pcm cleanup step2. Very random cleanup...
Kuninori Morimoto (7): ASoC: soc-pcm: add snd_soc_runtime_action() ASoC: soc-pcm: adjustment for DAI member 0 reset ASoC: soc-pcm: add for_each_dapm_widgets() macro ASoC: soc-pcm: goto error after trying for_each_rtd_codec_dai ASoC: soc-pcm: goto error after trying all component open ASoC: soc-pcm: move soc_pcm_close() next to soc_pcm_open() ASoC: soc-pcm: tidyup soc_pcm_open() order
include/sound/soc-dapm.h | 5 + sound/soc/soc-dapm.c | 8 +- sound/soc/soc-pcm.c | 266 ++++++++++++++++++++--------------------------- 3 files changed, 118 insertions(+), 161 deletions(-)
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
ALSA SoC has snd_soc_runtime_activate() / snd_soc_runtime_deactivate(). These increment or decrement DAI/Component activity, but the difference is only +1 or -1. This patch adds common snd_soc_runtime_action() which can get +1 or -1 as parameter, and use it from snd_soc_runtime_activate/deactivate() to avoid duplicate implementation.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 67 +++++++++++++++++++++-------------------------------- 1 file changed, 26 insertions(+), 41 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index e66ac9c..8bc6983 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -82,17 +82,8 @@ static int soc_rtd_trigger(struct snd_soc_pcm_runtime *rtd, return 0; }
-/** - * snd_soc_runtime_activate() - Increment active count for PCM runtime components - * @rtd: ASoC PCM runtime that is activated - * @stream: Direction of the PCM stream - * - * Increments the active count for all the DAIs and components attached to a PCM - * runtime. Should typically be called when a stream is opened. - * - * Must be called with the rtd->card->pcm_mutex being held - */ -void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) +static void snd_soc_runtime_action(struct snd_soc_pcm_runtime *rtd, + int stream, int action) { struct snd_soc_dai *cpu_dai = rtd->cpu_dai; struct snd_soc_dai *codec_dai; @@ -101,24 +92,39 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) lockdep_assert_held(&rtd->card->pcm_mutex);
if (stream == SNDRV_PCM_STREAM_PLAYBACK) { - cpu_dai->playback_active++; + cpu_dai->playback_active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->playback_active++; + codec_dai->playback_active += action; } else { - cpu_dai->capture_active++; + cpu_dai->capture_active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->capture_active++; + codec_dai->capture_active += action; }
- cpu_dai->active++; - cpu_dai->component->active++; + cpu_dai->active += action; + cpu_dai->component->active += action; for_each_rtd_codec_dai(rtd, i, codec_dai) { - codec_dai->active++; - codec_dai->component->active++; + codec_dai->active += action; + codec_dai->component->active += action; } }
/** + * snd_soc_runtime_activate() - Increment active count for PCM runtime components + * @rtd: ASoC PCM runtime that is activated + * @stream: Direction of the PCM stream + * + * Increments the active count for all the DAIs and components attached to a PCM + * runtime. Should typically be called when a stream is opened. + * + * Must be called with the rtd->card->pcm_mutex being held + */ +void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) +{ + snd_soc_runtime_action(rtd, stream, 1); +} + +/** * snd_soc_runtime_deactivate() - Decrement active count for PCM runtime components * @rtd: ASoC PCM runtime that is deactivated * @stream: Direction of the PCM stream @@ -130,28 +136,7 @@ void snd_soc_runtime_activate(struct snd_soc_pcm_runtime *rtd, int stream) */ void snd_soc_runtime_deactivate(struct snd_soc_pcm_runtime *rtd, int stream) { - struct snd_soc_dai *cpu_dai = rtd->cpu_dai; - struct snd_soc_dai *codec_dai; - int i; - - lockdep_assert_held(&rtd->card->pcm_mutex); - - if (stream == SNDRV_PCM_STREAM_PLAYBACK) { - cpu_dai->playback_active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->playback_active--; - } else { - cpu_dai->capture_active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) - codec_dai->capture_active--; - } - - cpu_dai->active--; - cpu_dai->component->active--; - for_each_rtd_codec_dai(rtd, i, codec_dai) { - codec_dai->component->active--; - codec_dai->active--; - } + snd_soc_runtime_action(rtd, stream, -1); }
/**
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
commit 3635bf09a89cf ("ASoC: soc-pcm: add symmetry for channels and sample bits") set 0 not only to dai->rate but also to dai->channels and dai->sample_bits if DAI was not active at soc_pcm_close().
and
commit d3383420c969c ("ASoC: soc-pcm: move DAIs parameters cleaning into hw_free()") moved it from soc_pcm_close() to soc_pcm_hw_free().
These happen at v3.14. But, maybe because of branch merge conflict or something similar happen then, soc_pcm_close() still has old settings (care only dai->rate, doesn't care dai->channels/sample_bits). This is 100% duplicated operation.
This patch removes soc_pcm_close() side operation which supposed to already moved to soc_pcm_hw_free().
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 9 --------- 1 file changed, 9 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 8bc6983..690a912 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -687,15 +687,6 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
snd_soc_runtime_deactivate(rtd, substream->stream);
- /* clear the corresponding DAIs rate when inactive */ - if (!cpu_dai->active) - cpu_dai->rate = 0; - - for_each_rtd_codec_dai(rtd, i, codec_dai) { - if (!codec_dai->active) - codec_dai->rate = 0; - } - snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream);
snd_soc_dai_shutdown(cpu_dai, substream);
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch adds new for_each_dapm_widgets() macro and use it.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- include/sound/soc-dapm.h | 5 +++++ sound/soc/soc-dapm.c | 8 ++------ sound/soc/soc-pcm.c | 17 +++++++++-------- 3 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h index 2a306c6..9439e75 100644 --- a/include/sound/soc-dapm.h +++ b/include/sound/soc-dapm.h @@ -693,6 +693,11 @@ struct snd_soc_dapm_widget_list { struct snd_soc_dapm_widget *widgets[0]; };
+#define for_each_dapm_widgets(list, i, widget) \ + for ((i) = 0; \ + (i) < list->num_widgets && (widget = list->widgets[i]); \ + (i)++) + struct snd_soc_dapm_stats { int power_checks; int path_checks; diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index bc20ad9..cc17a37 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1724,9 +1724,7 @@ static void dapm_widget_update(struct snd_soc_card *card)
wlist = dapm_kcontrol_get_wlist(update->kcontrol);
- for (wi = 0; wi < wlist->num_widgets; wi++) { - w = wlist->widgets[wi]; - + for_each_dapm_widgets(wlist, wi, w) { if (w->event && (w->event_flags & SND_SOC_DAPM_PRE_REG)) { ret = w->event(w, update->kcontrol, SND_SOC_DAPM_PRE_REG); if (ret != 0) @@ -1753,9 +1751,7 @@ static void dapm_widget_update(struct snd_soc_card *card) w->name, ret); }
- for (wi = 0; wi < wlist->num_widgets; wi++) { - w = wlist->widgets[wi]; - + for_each_dapm_widgets(wlist, wi, w) { if (w->event && (w->event_flags & SND_SOC_DAPM_POST_REG)) { ret = w->event(w, update->kcontrol, SND_SOC_DAPM_POST_REG); if (ret != 0) diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 690a912..f11c15f 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -1306,12 +1306,12 @@ static inline struct snd_soc_dapm_widget * static int widget_in_list(struct snd_soc_dapm_widget_list *list, struct snd_soc_dapm_widget *widget) { + struct snd_soc_dapm_widget *w; int i;
- for (i = 0; i < list->num_widgets; i++) { - if (widget == list->widgets[i]) + for_each_dapm_widgets(list, i, w) + if (widget == w) return 1; - }
return 0; } @@ -1422,12 +1422,13 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, struct snd_soc_card *card = fe->card; struct snd_soc_dapm_widget_list *list = *list_; struct snd_soc_pcm_runtime *be; + struct snd_soc_dapm_widget *widget; int i, new = 0, err;
/* Create any new FE <--> BE connections */ - for (i = 0; i < list->num_widgets; i++) { + for_each_dapm_widgets(list, i, widget) {
- switch (list->widgets[i]->id) { + switch (widget->id) { case snd_soc_dapm_dai_in: if (stream != SNDRV_PCM_STREAM_PLAYBACK) continue; @@ -1441,10 +1442,10 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, }
/* is there a valid BE rtd for this widget */ - be = dpcm_get_be(card, list->widgets[i], stream); + be = dpcm_get_be(card, widget, stream); if (!be) { dev_err(fe->dev, "ASoC: no BE found for %s\n", - list->widgets[i]->name); + widget->name); continue; }
@@ -1460,7 +1461,7 @@ static int dpcm_add_paths(struct snd_soc_pcm_runtime *fe, int stream, err = dpcm_be_connect(fe, be, stream); if (err < 0) { dev_err(fe->dev, "ASoC: can't connect %s\n", - list->widgets[i]->name); + widget->name); break; } else if (err == 0) /* already connected */ continue;
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. In such case, fallback process need to care about operated/non-operated codec dai.
But, if it goto error process *after* loop even though error happen during loop, it don't need to care about operated/non-operated. In such case code can be more simple. This patch do it. And this is prepare for soc_snd_open() cleanup
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index f11c15f..57d2f00 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -547,25 +547,24 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) goto component_err;
for_each_rtd_codec_dai(rtd, i, codec_dai) { - ret = snd_soc_dai_startup(codec_dai, substream); - if (ret < 0) { - dev_err(codec_dai->dev, - "ASoC: can't open codec %s: %d\n", - codec_dai->name, ret); - goto codec_dai_err; - } + ret |= snd_soc_dai_startup(codec_dai, substream);
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) codec_dai->tx_mask = 0; else codec_dai->rx_mask = 0; } + if (ret < 0) { + dev_err(codec_dai->dev, "ASoC: can't open codec %s: %d\n", + codec_dai->name, ret); + goto codec_dai_err; + }
ret = soc_rtd_startup(rtd, substream); if (ret < 0) { pr_err("ASoC: %s startup failed: %d\n", rtd->dai_link->name, ret); - goto machine_err; + goto codec_dai_err; }
/* Dynamic PCM DAI links compat checks use dynamic capabilities */ @@ -634,11 +633,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) config_err: soc_rtd_shutdown(rtd, substream);
-machine_err: - i = rtd->num_codecs; - codec_dai_err: - for_each_rtd_codec_dai_rollback(rtd, i, codec_dai) + for_each_rtd_codec_dai(rtd, i, codec_dai) snd_soc_dai_shutdown(codec_dai, substream);
component_err:
On Mon, 27 Jan 2020 02:49:22 +0100, Kuninori Morimoto wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. In such case, fallback process need to care about operated/non-operated codec dai.
But, if it goto error process *after* loop even though error happen during loop, it don't need to care about operated/non-operated. In such case code can be more simple. This patch do it. And this is prepare for soc_snd_open() cleanup
This would mean that snd_soc_dai_shutdown() is called even for the stream that returned the error. This isn't the expected behavior.
Also, bit-OR-ing the multiple error codes isn't wise, they may return different error codes, and you'll mixed up to a different number.
thanks,
Takashi
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-pcm.c | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index f11c15f..57d2f00 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -547,25 +547,24 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) goto component_err;
for_each_rtd_codec_dai(rtd, i, codec_dai) {
ret = snd_soc_dai_startup(codec_dai, substream);if (ret < 0) {dev_err(codec_dai->dev,"ASoC: can't open codec %s: %d\n",codec_dai->name, ret);goto codec_dai_err;}
ret |= snd_soc_dai_startup(codec_dai, substream);if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) codec_dai->tx_mask = 0; else codec_dai->rx_mask = 0; }
if (ret < 0) {
dev_err(codec_dai->dev, "ASoC: can't open codec %s: %d\n",codec_dai->name, ret);goto codec_dai_err;}
ret = soc_rtd_startup(rtd, substream); if (ret < 0) { pr_err("ASoC: %s startup failed: %d\n", rtd->dai_link->name, ret);
goto machine_err;
goto codec_dai_err;}
/* Dynamic PCM DAI links compat checks use dynamic capabilities */
@@ -634,11 +633,8 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) config_err: soc_rtd_shutdown(rtd, substream);
-machine_err:
- i = rtd->num_codecs;
codec_dai_err:
- for_each_rtd_codec_dai_rollback(rtd, i, codec_dai)
- for_each_rtd_codec_dai(rtd, i, codec_dai) snd_soc_dai_shutdown(codec_dai, substream);
component_err:
2.7.4
Alsa-devel mailing list Alsa-devel@alsa-project.org https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
Hi Takashi
Thank you for your feedback
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. In such case, fallback process need to care about operated/non-operated codec dai.
But, if it goto error process *after* loop even though error happen during loop, it don't need to care about operated/non-operated. In such case code can be more simple. This patch do it. And this is prepare for soc_snd_open() cleanup
This would mean that snd_soc_dai_shutdown() is called even for the stream that returned the error. This isn't the expected behavior.
Yeah. Actually, I have plan to add such flag by other patch. But indeed order was reversed. Will fixup.
Also, bit-OR-ing the multiple error codes isn't wise, they may return different error codes, and you'll mixed up to a different number.
It is used at some architecture. But yes, let's think about better idea. Will return 1st error.
Thank you for your help !! Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. In such case, fallback process need to care about operated/non-operated codec dai.
But, if it goto error process *after* loop even though error happen during loop, it don't need to care about operated/non-operated. In such case code can be more simple. This patch do it. And this is prepare for soc_snd_open() cleanup
This would mean that snd_soc_dai_shutdown() is called even for the stream that returned the error. This isn't the expected behavior.
Yeah. Actually, I have plan to add such flag by other patch. But indeed order was reversed. Will fixup.
Also, bit-OR-ing the multiple error codes isn't wise, they may return different error codes, and you'll mixed up to a different number.
It is used at some architecture. But yes, let's think about better idea. Will return 1st error.
I also cringed on the bit-OR'ed error codes, but I saw it's already used in soc-pcm.c so thought it was an agreed precedent?
ret |= snd_soc_component_close(component, substream); ret |= snd_soc_component_hw_free(component, substream);
I also don't like the idea of not stopping loops on errors, or releasing things that haven't been properly allocated. It does simplify error handling but it's asking for trouble.
On Tue, 28 Jan 2020 17:31:05 +0100, Pierre-Louis Bossart wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_open() might goto error process *during* for_each_rtd_codec_dai. In such case, fallback process need to care about operated/non-operated codec dai.
But, if it goto error process *after* loop even though error happen during loop, it don't need to care about operated/non-operated. In such case code can be more simple. This patch do it. And this is prepare for soc_snd_open() cleanup
This would mean that snd_soc_dai_shutdown() is called even for the stream that returned the error. This isn't the expected behavior.
Yeah. Actually, I have plan to add such flag by other patch. But indeed order was reversed. Will fixup.
Also, bit-OR-ing the multiple error codes isn't wise, they may return different error codes, and you'll mixed up to a different number.
It is used at some architecture. But yes, let's think about better idea. Will return 1st error.
I also cringed on the bit-OR'ed error codes, but I saw it's already used in soc-pcm.c so thought it was an agreed precedent?
ret |= snd_soc_component_close(component, substream); ret |= snd_soc_component_hw_free(component, substream);
I think it's just an overlook. The driver may return arbitrary error codes so they can conflict. The bit-OR works only if the return code is always consistent.
Takashi
Hi
I also cringed on the bit-OR'ed error codes, but I saw it's already used in soc-pcm.c so thought it was an agreed precedent?
ret |= snd_soc_component_close(component, substream); ret |= snd_soc_component_hw_free(component, substream);I think it's just an overlook. The driver may return arbitrary error codes so they can conflict. The bit-OR works only if the return code is always consistent.
OK. Let's cleanup it, too. If you ara OK, I will do it.
I also don't like the idea of not stopping loops on errors, or releasing things that haven't been properly allocated. It does simplify error handling but it's asking for trouble.
Actually, my local patch which will be posted will solve it. I will re-order patch-set, and post it.
# But, this re-order patch will have conflict in my local tree. # I will solve it, too.
Thank you for your help !! Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_components_open() might goto error process *during* opening component loop. In such case, fallback process need to care about operated/non-operated component.
But, if it goto error process *after* loop even though error happen during loop, it don't need to care about operated/non-operated. In such case code can be more simple. This patch do it. And this is prepare for soc_snd_open() cleanup
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 41 +++++++++++++---------------------------- 1 file changed, 13 insertions(+), 28 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 57d2f00..1e370ef 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -463,47 +463,32 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) hw->rate_max = min_not_zero(hw->rate_max, rate_max); }
-static int soc_pcm_components_open(struct snd_pcm_substream *substream, - struct snd_soc_component **last) +static int soc_pcm_components_open(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, ret = 0;
for_each_rtd_components(rtd, i, component) { - *last = component; + ret |= snd_soc_component_module_get_when_open(component); + ret |= snd_soc_component_open(component, substream); + }
- ret = snd_soc_component_module_get_when_open(component); - if (ret < 0) { - dev_err(component->dev, - "ASoC: can't get module %s\n", - component->name); - return ret; - } + if (ret < 0) + dev_err(component->dev, + "ASoC: error happen during open component %s: %d\n", + component->name, ret);
- ret = snd_soc_component_open(component, substream); - if (ret < 0) { - dev_err(component->dev, - "ASoC: can't open component %s: %d\n", - component->name, ret); - return ret; - } - } - *last = NULL; - return 0; + return ret; }
-static int soc_pcm_components_close(struct snd_pcm_substream *substream, - struct snd_soc_component *last) +static int soc_pcm_components_close(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, ret = 0;
for_each_rtd_components(rtd, i, component) { - if (component == last) - break; - ret |= snd_soc_component_close(component, substream); snd_soc_component_module_put_when_close(component); } @@ -542,7 +527,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) goto out; }
- ret = soc_pcm_components_open(substream, &component); + ret = soc_pcm_components_open(substream); if (ret < 0) goto component_err;
@@ -638,7 +623,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) snd_soc_dai_shutdown(codec_dai, substream);
component_err: - soc_pcm_components_close(substream, component); + soc_pcm_components_close(substream);
snd_soc_dai_shutdown(cpu_dai, substream); out: @@ -692,7 +677,7 @@ static int soc_pcm_close(struct snd_pcm_substream *substream)
soc_rtd_shutdown(rtd, substream);
- soc_pcm_components_close(substream, NULL); + soc_pcm_components_close(substream);
snd_soc_dapm_stream_stop(rtd, substream->stream);
On Sun, Jan 26, 2020 at 5:54 PM Kuninori Morimoto < kuninori.morimoto.gx@renesas.com> wrote:
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_components_open() might goto error process *during* opening component loop. In such case, fallback process need to care about operated/non-operated component.
But, if it goto error process *after* loop even though error happen during loop, it don't need to care about operated/non-operated. In such case code can be more simple. This patch do it. And this is prepare for soc_snd_open() cleanup
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
sound/soc/soc-pcm.c | 41 +++++++++++++---------------------------- 1 file changed, 13 insertions(+), 28 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 57d2f00..1e370ef 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -463,47 +463,32 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) hw->rate_max = min_not_zero(hw->rate_max, rate_max); }
-static int soc_pcm_components_open(struct snd_pcm_substream *substream,
struct snd_soc_component **last)+static int soc_pcm_components_open(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, ret = 0;
for_each_rtd_components(rtd, i, component) {
*last = component;
ret |= snd_soc_component_module_get_when_open(component);ret |= snd_soc_component_open(component, substream);}
ret = snd_soc_component_module_get_when_open(component);if (ret < 0) {dev_err(component->dev,"ASoC: can't get module %s\n",component->name);return ret;}
if (ret < 0)dev_err(component->dev,"ASoC: error happen during open component %s:%d\n",
component->name, ret);
Hi Morimoto-san,
Wouldn't the component here always be the last component in the list of rtd components? Should this error log be moved inside the for_each_rtd_components() {} above?
Thanks, Ranjani
Hi Sridharan
Thank you for your feedback
@@ -463,47 +463,32 @@ static void soc_pcm_init_runtime_hw(struct snd_pcm_substream *substream) hw->rate_max = min_not_zero(hw->rate_max, rate_max); } -static int soc_pcm_components_open(struct snd_pcm_substream *substream, - struct snd_soc_component **last) +static int soc_pcm_components_open(struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = substream->private_data; struct snd_soc_component *component; int i, ret = 0; for_each_rtd_components(rtd, i, component) { - *last = component; + ret |= snd_soc_component_module_get_when_open(component); + ret |= snd_soc_component_open(component, substream); + } - ret = snd_soc_component_module_get_when_open(component); - if (ret < 0) { - dev_err(component->dev, - "ASoC: can't get module %s\n", - component->name); - return ret; - } + if (ret < 0) + dev_err(component->dev, + "ASoC: error happen during open component %s: %d\n", + component->name, ret);Hi Morimoto-san,
Wouldn't the component here always be the last component in the list of rtd components? Should this error log be moved inside the for_each_rtd_components() {} above?
Yeah, indeed. Will fix
Thank you for your help !! Best regards --- Kuninori Morimoto
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
This patch moves soc_pcm_close() next to soc_pcm_open(). This is prepare for soc_pcm_open() cleanup.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 88 ++++++++++++++++++++++++++--------------------------- 1 file changed, 44 insertions(+), 44 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index 1e370ef..b5d2840 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -497,6 +497,50 @@ static int soc_pcm_components_close(struct snd_pcm_substream *substream) }
/* + * 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) +{ + struct snd_soc_pcm_runtime *rtd = substream->private_data; + struct snd_soc_component *component; + struct snd_soc_dai *cpu_dai = rtd->cpu_dai; + struct snd_soc_dai *codec_dai; + int i; + + mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); + + snd_soc_runtime_deactivate(rtd, substream->stream); + + snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream); + + snd_soc_dai_shutdown(cpu_dai, substream); + + for_each_rtd_codec_dai(rtd, i, codec_dai) + snd_soc_dai_shutdown(codec_dai, substream); + + soc_rtd_shutdown(rtd, substream); + + soc_pcm_components_close(substream); + + snd_soc_dapm_stream_stop(rtd, substream->stream); + + mutex_unlock(&rtd->card->pcm_mutex); + + for_each_rtd_components(rtd, i, component) { + pm_runtime_mark_last_busy(component->dev); + pm_runtime_put_autosuspend(component->dev); + } + + for_each_rtd_components(rtd, i, component) + if (!component->active) + pinctrl_pm_select_sleep_state(component->dev); + + return 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 * startup for the cpu DAI, component, machine and codec DAI. @@ -652,50 +696,6 @@ static void codec2codec_close_delayed_work(struct snd_soc_pcm_runtime *rtd) }
/* - * 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) -{ - struct snd_soc_pcm_runtime *rtd = substream->private_data; - struct snd_soc_component *component; - struct snd_soc_dai *cpu_dai = rtd->cpu_dai; - struct snd_soc_dai *codec_dai; - int i; - - mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass); - - snd_soc_runtime_deactivate(rtd, substream->stream); - - snd_soc_dai_digital_mute(cpu_dai, 1, substream->stream); - - snd_soc_dai_shutdown(cpu_dai, substream); - - for_each_rtd_codec_dai(rtd, i, codec_dai) - snd_soc_dai_shutdown(codec_dai, substream); - - soc_rtd_shutdown(rtd, substream); - - soc_pcm_components_close(substream); - - snd_soc_dapm_stream_stop(rtd, substream->stream); - - mutex_unlock(&rtd->card->pcm_mutex); - - for_each_rtd_components(rtd, i, component) { - pm_runtime_mark_last_busy(component->dev); - pm_runtime_put_autosuspend(component->dev); - } - - for_each_rtd_components(rtd, i, component) - if (!component->active) - pinctrl_pm_select_sleep_state(component->dev); - - return 0; -} - -/* * Called by ALSA when the PCM substream is prepared, can set format, sample * rate, etc. This function is non atomic and can be called multiple times, * it can refer to the runtime info.
From: Kuninori Morimoto kuninori.morimoto.gx@renesas.com
soc_pcm_open() operation order is not good. At first, soc_pcm_open() operation order is
1) CPU DAI startup 2) Component open 3) Codec DAI startup 4) rtd startup
But here, 2) will call try_module_get() if component has module_get_upon_open flags. This means 1) CPU DAI startup will be operated *before* its module was loaded. DAI should be called *after* Component.
Second, soc_pcm_close() operation order is 1) CPU DAI shutdown 2) Codec DAI shutdown 3) rtd shutdown 4) Component close
soc_pcm_open() and soc_pcm_close() are paired function, but, its operation order is unbalance. This patch tidyup soc_pcm_open() order to Component -> rtd -> DAI.
Signed-off-by: Kuninori Morimoto kuninori.morimoto.gx@renesas.com --- sound/soc/soc-pcm.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c index b5d2840..d916182 100644 --- a/sound/soc/soc-pcm.c +++ b/sound/soc/soc-pcm.c @@ -563,18 +563,25 @@ static int soc_pcm_open(struct snd_pcm_substream *substream)
mutex_lock_nested(&rtd->card->pcm_mutex, rtd->card->pcm_subclass);
+ ret = soc_pcm_components_open(substream); + if (ret < 0) + goto component_err; + + ret = soc_rtd_startup(rtd, substream); + if (ret < 0) { + pr_err("ASoC: %s startup failed: %d\n", + rtd->dai_link->name, ret); + goto rtd_err; + } + /* startup the audio subsystem */ ret = snd_soc_dai_startup(cpu_dai, substream); if (ret < 0) { dev_err(cpu_dai->dev, "ASoC: can't open interface %s: %d\n", cpu_dai->name, ret); - goto out; + goto cpu_dai_err; }
- ret = soc_pcm_components_open(substream); - if (ret < 0) - goto component_err; - for_each_rtd_codec_dai(rtd, i, codec_dai) { ret |= snd_soc_dai_startup(codec_dai, substream);
@@ -586,14 +593,7 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) if (ret < 0) { dev_err(codec_dai->dev, "ASoC: can't open codec %s: %d\n", codec_dai->name, ret); - goto codec_dai_err; - } - - ret = soc_rtd_startup(rtd, substream); - if (ret < 0) { - pr_err("ASoC: %s startup failed: %d\n", - rtd->dai_link->name, ret); - goto codec_dai_err; + goto config_err; }
/* Dynamic PCM DAI links compat checks use dynamic capabilities */ @@ -660,17 +660,15 @@ static int soc_pcm_open(struct snd_pcm_substream *substream) return 0;
config_err: - soc_rtd_shutdown(rtd, substream); - -codec_dai_err: for_each_rtd_codec_dai(rtd, i, codec_dai) snd_soc_dai_shutdown(codec_dai, substream); - +cpu_dai_err: + snd_soc_dai_shutdown(cpu_dai, substream); +rtd_err: + soc_rtd_shutdown(rtd, substream); component_err: soc_pcm_components_close(substream);
- snd_soc_dai_shutdown(cpu_dai, substream); -out: mutex_unlock(&rtd->card->pcm_mutex);
for_each_rtd_components(rtd, i, component) {
Hi Morimoto-san,
soc_pcm_open() operation order is not good. At first, soc_pcm_open() operation order is
- CPU DAI startup
- Component open
- Codec DAI startup
- rtd startup
But here, 2) will call try_module_get() if component has module_get_upon_open flags. This means 1) CPU DAI startup will be operated *before* its module was loaded. DAI should be called *after* Component.
Second, soc_pcm_close() operation order is
- CPU DAI shutdown
- Codec DAI shutdown
- rtd shutdown
- Component close
soc_pcm_open() and soc_pcm_close() are paired function, but, its operation order is unbalance. This patch tidyup soc_pcm_open() order to Component -> rtd -> DAI.
Maybe a red-herring but while reviewing the other soc-pcm changes I started wondering if this order change is actually valid for all platforms.
If you look at the soc-pcm code, all the way to 2011
ddee627cf6bb60 ('ASoC: core - Separate out PCM operations into new file')
you'll see that the intent was to - start the cpu_dai - open the platform driver to e.g handle DMAs - start the codec_dai.
The second step is not really needed for Intel but might be needed on others where the DMA is centrally handled (Omap, etc).
My concern is that we've modified the order to deal with module dependencies, without necessarily taking into account that DMA setup part
That said I don't understand the initial sequence for soc_pcm_close() so I am not advocating for a revert, but more a clarification of what those component open/close steps are supposed to do.
Thanks -Pierre
On Mon, Feb 24, 2020 at 07:22:25PM -0600, Pierre-Louis Bossart wrote:
you'll see that the intent was to
- start the cpu_dai
- open the platform driver to e.g handle DMAs
- start the codec_dai.
The second step is not really needed for Intel but might be needed on others where the DMA is centrally handled (Omap, etc).
My concern is that we've modified the order to deal with module dependencies, without necessarily taking into account that DMA setup part
That said I don't understand the initial sequence for soc_pcm_close() so I am not advocating for a revert, but more a clarification of what those component open/close steps are supposed to do.
It's possible we're going to shake some issues out here but the ordering has always been a bit arbitrary, especially the CPU vs CODEC ordering. We're basically hoping that the ordering we've picked is the one that causes fewest glitches and upsets on the broad range of hardware. My expectation/hope is that for most hardware the flow control between the DAI and the DMA controller (which has to be there anyway) will mean that it mostly doesn't matter, if there's anything that is too sensitive to it we can always revert and try something else.
Hi Pierre-Louis, Mark
you'll see that the intent was to
- start the cpu_dai
- open the platform driver to e.g handle DMAs
- start the codec_dai.
The second step is not really needed for Intel but might be needed on others where the DMA is centrally handled (Omap, etc).
My concern is that we've modified the order to deal with module dependencies, without necessarily taking into account that DMA setup part
That said I don't understand the initial sequence for soc_pcm_close() so I am not advocating for a revert, but more a clarification of what those component open/close steps are supposed to do.
It's possible we're going to shake some issues out here but the ordering has always been a bit arbitrary, especially the CPU vs CODEC ordering. We're basically hoping that the ordering we've picked is the one that causes fewest glitches and upsets on the broad range of hardware. My expectation/hope is that for most hardware the flow control between the DAI and the DMA controller (which has to be there anyway) will mean that it mostly doesn't matter, if there's anything that is too sensitive to it we can always revert and try something else.
I'm sorry to not clarify the detail. Yes, as you mentioned, the patch exchanged start/stop order, and some platform *might* get damage (I hope not, of course).
But we can't say that the previous order was the best order for all platform. I'm thinking that the current order is also not the best, but is not random. The *best* solution is that we need to have xxx_order flag like for_each_comp_order().
Thank you for your help !! Best regards --- Kuninori Morimoto
participants (5)
-
Kuninori Morimoto -
Mark Brown -
Pierre-Louis Bossart -
Sridharan, Ranjani -
Takashi Iwai