On 5/11/20 11:47 PM, Gyeongtaek Lee wrote:
On 5/12/20 03:47 AM, Pierre-Louis Bossart wrote:
On 5/10/20 10:31 PM, Gyeongtaek Lee wrote:
When dpcm_be_dai_hw_params() called, be_hw_params_fixup() callback is
called with same arguments 3times.
It is called in be_hw_params_fixup() itself and in soc_pcm_hw_params()
for cpu dai and codec dai.
Tested in 5.4.
Signed-off-by: Gyeongtaek Lee <gt82.lee@xxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
---
sound/soc/soc-dai.c | 12 ------------
sound/soc/soc-dapm.c | 11 +++++++++++
2 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index 31c41559034b..4785cb6b336f 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -257,20 +257,8 @@ int snd_soc_dai_hw_params(struct snd_soc_dai *dai,
struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)
{
- struct snd_soc_pcm_runtime *rtd = substream->private_data;
int ret;
- /* perform any topology hw_params fixups before DAI */
- if (rtd->dai_link->be_hw_params_fixup) {
- ret = rtd->dai_link->be_hw_params_fixup(rtd, params);
- if (ret < 0) {
- dev_err(rtd->dev,
- "ASoC: hw_params topology fixup failed %d\n",
- ret);
- return ret;
- }
- }
-
Sorry I don't get this change.
If the be_hw_params_fixup() callback is called three times, it's because
the soc_soc_dai_hw_params() routine is called three times, so what is
the problem here?
Also the comment is explicit about doing fixups before calling the dai
hw_params() callback, so that is not longer the case with this change?
Even if the change was legit, the comment is no longer relevant and
should be updated.
Sorry, the comment was too short and inexact to explain the intention of the patch.
When dpcm_be_dai_hw_params() called, be_hw_params_fixup() is called three times
with same substream and params in dpcm_be_dai_hw_params() and
snd_soc_dai_hw_params() in soc_pcm_hw_params() for cpu dai and codec dai.
Calling same code three times may not be a problem in most systems, but in some
system which has limited computing power and changes audio routing frequently,
couple of milliseconds are consumed and make the system a little bit slower to
audio response.
If the be_hw_params_fixup() could be pull out from soc_soc_dai_hw_params(),
and make it call once at pcm start or routing change, response time can be increased.
I accept the argument that be_hw_params_fixup() is called from two
locations, and in the second one there is a iteration to deal with
capture and playback. We should indeed check if calling the same
function from two places is legit or not, you pointed to a legitimate
issue. We currently use this fixup in SOF to make sure the BE
configuration matches what the topology provides, and doing this match 3
times is not very useful indeed.
That said, this is supposed to be a fixup, meaning just a change to the
hw_params structure with no access to hardware. I believe that even if
we reduce the number of calls you are not going to see any improvement
in responses time, the actual time is still going to be spend in the
hw_params() itself.
In my search, the only point that calls snd_soc_dai_hw_params() without
calling be_hw_params_fixup() callback directly is snd_soc_dai_link_event_pre_pmu().
So, I proposed pulling out be_hw_params_fixup() from snd_soc_dai_hw_params() and then
add direct call to be_hw_params_fixup() callback in snd_soc_dai_link_event_pre_pmu()
not to harm current working process.
This change leaves the calls in soc-pcm, so you still have 3 calls to
the same callback - and the same odd pattern where in one case the fixup
is called in a direction-agnostic manner while in the two others it is
called in a loop that's direction-based.
In other words my take on this is: valid problem, but let's try to see
if we can remove redundant calls rather than move one around.
if (dai->driver->ops->hw_params) {
ret = dai->driver->ops->hw_params(substream, params, dai);
if (ret < 0) {
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c
index e2632841b321..d86c1cd4e8fa 100644
--- a/sound/soc/soc-dapm.c
+++ b/sound/soc/soc-dapm.c
@@ -3886,6 +3886,17 @@ snd_soc_dai_link_event_pre_pmu(struct snd_soc_dapm_widget *w,
hw_param_interval(params, SNDRV_PCM_HW_PARAM_CHANNELS)->max
= config->channels_max;
+ /* perform any topology hw_params fixups before DAI */
+ if (rtd->dai_link->be_hw_params_fixup) {
+ ret = rtd->dai_link->be_hw_params_fixup(rtd, params);
+ if (ret < 0) {
+ dev_err(rtd->dev,
+ "ASoC: hw_params topology fixup failed %d\n",
+ ret);
+ return ret;
+ }
+ }
+
substream->stream = SNDRV_PCM_STREAM_CAPTURE;
snd_soc_dapm_widget_for_each_source_path(w, path) {
source = path->source->priv;
base-commit: f3643491bd079c973ac6c693da7966cd17506ca3
