[PATCH 0/3] ASoC: soc-pcm: add soc_pcm_hw_update_xxx()

[PATCH 0/3] ASoC: soc-pcm: add soc_pcm_hw_update_xxx()

[Kuninori Morimoto]

Hi Mark

These adds soc_pcm_hw_update_xxx() and cleanup soc-pcm.c

Kuninori Morimoto (3):
  ASoC: soc-pcm: add soc_pcm_hw_update_rate()
  ASoC: soc-pcm: add soc_pcm_hw_update_chan()
  ASoC: soc-pcm: add soc_pcm_hw_update_format()

 sound/soc/soc-pcm.c | 112 ++++++++++++++++++++++----------------------
 1 file changed, 56 insertions(+), 56 deletions(-)

-- 
2.25.1

[PATCH 1/3] ASoC: soc-pcm: add soc_pcm_hw_update_rate()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

To update hw, we need to follow setting order

	1) set hw->rates
	2) call snd_pcm_limit_hw_rates()
	3) update hw->rate_min/max

To avoid random settings, this patch adds new soc_pcm_hw_update_rate()
and share updating code.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index b79f064887d4..eb5b220b189a 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -473,6 +473,26 @@ static void soc_pcm_apply_msb(struct snd_pcm_substream *substream)
 	soc_pcm_set_msb(substream, cpu_bits);
 }
 
+static void soc_pcm_hw_init(struct snd_pcm_hardware *hw)
+{
+	hw->rates		= UINT_MAX;
+	hw->rate_min		= 0;
+	hw->rate_max		= UINT_MAX;
+}
+
+static void soc_pcm_hw_update_rate(struct snd_pcm_hardware *hw,
+				   struct snd_soc_pcm_stream *p)
+{
+	hw->rates = snd_pcm_rate_mask_intersect(hw->rates, p->rates);
+
+	/* setup hw->rate_min/max via hw->rates first */
+	snd_pcm_hw_limit_rates(hw);
+
+	/* update hw->rate_min/max by snd_soc_pcm_stream */
+	hw->rate_min = max(hw->rate_min, p->rate_min);
+	hw->rate_max = min_not_zero(hw->rate_max, p->rate_max);
+}
+
 /**
  * snd_soc_runtime_calc_hw() - Calculate hw limits for a PCM stream
  * @rtd: ASoC PCM runtime
@@ -491,12 +511,11 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 	struct snd_soc_pcm_stream *cpu_stream;
 	unsigned int chan_min = 0, chan_max = UINT_MAX;
 	unsigned int cpu_chan_min = 0, cpu_chan_max = UINT_MAX;
-	unsigned int rate_min = 0, rate_max = UINT_MAX;
-	unsigned int cpu_rate_min = 0, cpu_rate_max = UINT_MAX;
-	unsigned int rates = UINT_MAX, cpu_rates = UINT_MAX;
 	u64 formats = ULLONG_MAX;
 	int i;
 
+	soc_pcm_hw_init(hw);
+
 	/* first calculate min/max only for CPUs in the DAI link */
 	for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
 
@@ -513,11 +532,8 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 
 		cpu_chan_min = max(cpu_chan_min, cpu_stream->channels_min);
 		cpu_chan_max = min(cpu_chan_max, cpu_stream->channels_max);
-		cpu_rate_min = max(cpu_rate_min, cpu_stream->rate_min);
-		cpu_rate_max = min_not_zero(cpu_rate_max, cpu_stream->rate_max);
+		soc_pcm_hw_update_rate(hw, cpu_stream);
 		formats &= cpu_stream->formats;
-		cpu_rates = snd_pcm_rate_mask_intersect(cpu_stream->rates,
-							cpu_rates);
 	}
 
 	/* second calculate min/max only for CODECs in the DAI link */
@@ -536,10 +552,8 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 
 		chan_min = max(chan_min, codec_stream->channels_min);
 		chan_max = min(chan_max, codec_stream->channels_max);
-		rate_min = max(rate_min, codec_stream->rate_min);
-		rate_max = min_not_zero(rate_max, codec_stream->rate_max);
+		soc_pcm_hw_update_rate(hw, codec_stream);
 		formats &= codec_stream->formats;
-		rates = snd_pcm_rate_mask_intersect(codec_stream->rates, rates);
 	}
 
 	/* Verify both a valid CPU DAI and a valid CODEC DAI were found */
@@ -560,14 +574,6 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 	hw->channels_min = max(chan_min, cpu_chan_min);
 	hw->channels_max = min(chan_max, cpu_chan_max);
 	hw->formats = formats;
-	hw->rates = snd_pcm_rate_mask_intersect(rates, cpu_rates);
-
-	snd_pcm_hw_limit_rates(hw);
-
-	hw->rate_min = max(hw->rate_min, cpu_rate_min);
-	hw->rate_min = max(hw->rate_min, rate_min);
-	hw->rate_max = min_not_zero(hw->rate_max, cpu_rate_max);
-	hw->rate_max = min_not_zero(hw->rate_max, rate_max);
 
 	return 0;
 }
@@ -1514,12 +1520,9 @@ int dpcm_be_dai_startup(struct snd_soc_pcm_runtime *fe, int stream)
 static void dpcm_init_runtime_hw(struct snd_pcm_runtime *runtime,
 				 struct snd_soc_pcm_stream *stream)
 {
-	runtime->hw.rates = stream->rates;
-
-	snd_pcm_limit_hw_rates(runtime);
+	struct snd_pcm_hardware *hw = &runtime->hw;
 
-	runtime->hw.rate_min = stream->rate_min;
-	runtime->hw.rate_max = min_not_zero(stream->rate_max, UINT_MAX);
+	soc_pcm_hw_update_rate(hw, stream);
 	runtime->hw.channels_min = stream->channels_min;
 	runtime->hw.channels_max = stream->channels_max;
 	if (runtime->hw.formats)
@@ -1651,12 +1654,7 @@ static void dpcm_runtime_merge_rate(struct snd_pcm_substream *substream,
 
 			pcm = snd_soc_dai_get_pcm_stream(dai, stream);
 
-			hw->rates = snd_pcm_rate_mask_intersect(hw->rates, pcm->rates);
-
-			snd_pcm_limit_hw_rates(runtime);
-
-			hw->rate_min = max(hw->rate_min, pcm->rate_min);
-			hw->rate_max = min_not_zero(hw->rate_max, pcm->rate_max);
+			soc_pcm_hw_update_rate(hw, pcm);
 		}
 	}
 }
-- 
2.25.1

[PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

We have soc_pcm_hw_update_rate() now.
This patch creates same function for chan.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index eb5b220b189a..748265018d1a 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -478,6 +478,8 @@ static void soc_pcm_hw_init(struct snd_pcm_hardware *hw)
 	hw->rates		= UINT_MAX;
 	hw->rate_min		= 0;
 	hw->rate_max		= UINT_MAX;
+	hw->channels_min	= 0;
+	hw->channels_max	= UINT_MAX;
 }
 
 static void soc_pcm_hw_update_rate(struct snd_pcm_hardware *hw,
@@ -493,6 +495,13 @@ static void soc_pcm_hw_update_rate(struct snd_pcm_hardware *hw,
 	hw->rate_max = min_not_zero(hw->rate_max, p->rate_max);
 }
 
+static void soc_pcm_hw_update_chan(struct snd_pcm_hardware *hw,
+				   struct snd_soc_pcm_stream *p)
+{
+	hw->channels_min = max(hw->channels_min, p->channels_min);
+	hw->channels_max = min(hw->channels_max, p->channels_max);
+}
+
 /**
  * snd_soc_runtime_calc_hw() - Calculate hw limits for a PCM stream
  * @rtd: ASoC PCM runtime
@@ -509,7 +518,6 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 	struct snd_soc_dai *cpu_dai;
 	struct snd_soc_pcm_stream *codec_stream;
 	struct snd_soc_pcm_stream *cpu_stream;
-	unsigned int chan_min = 0, chan_max = UINT_MAX;
 	unsigned int cpu_chan_min = 0, cpu_chan_max = UINT_MAX;
 	u64 formats = ULLONG_MAX;
 	int i;
@@ -530,11 +538,12 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 
 		cpu_stream = snd_soc_dai_get_pcm_stream(cpu_dai, stream);
 
-		cpu_chan_min = max(cpu_chan_min, cpu_stream->channels_min);
-		cpu_chan_max = min(cpu_chan_max, cpu_stream->channels_max);
+		soc_pcm_hw_update_chan(hw, cpu_stream);
 		soc_pcm_hw_update_rate(hw, cpu_stream);
 		formats &= cpu_stream->formats;
 	}
+	cpu_chan_min = hw->channels_min;
+	cpu_chan_max = hw->channels_max;
 
 	/* second calculate min/max only for CODECs in the DAI link */
 	for_each_rtd_codec_dais(rtd, i, codec_dai) {
@@ -550,14 +559,13 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 
 		codec_stream = snd_soc_dai_get_pcm_stream(codec_dai, stream);
 
-		chan_min = max(chan_min, codec_stream->channels_min);
-		chan_max = min(chan_max, codec_stream->channels_max);
+		soc_pcm_hw_update_chan(hw, codec_stream);
 		soc_pcm_hw_update_rate(hw, codec_stream);
 		formats &= codec_stream->formats;
 	}
 
 	/* Verify both a valid CPU DAI and a valid CODEC DAI were found */
-	if (!chan_min || !cpu_chan_min)
+	if (!hw->channels_min)
 		return -EINVAL;
 
 	/*
@@ -566,13 +574,11 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 	 * channel allocation be fixed up later
 	 */
 	if (rtd->num_codecs > 1) {
-		chan_min = cpu_chan_min;
-		chan_max = cpu_chan_max;
+		hw->channels_min = cpu_chan_min;
+		hw->channels_max = cpu_chan_max;
 	}
 
 	/* finally find a intersection between CODECs and CPUs */
-	hw->channels_min = max(chan_min, cpu_chan_min);
-	hw->channels_max = min(chan_max, cpu_chan_max);
 	hw->formats = formats;
 
 	return 0;
@@ -1523,8 +1529,7 @@ static void dpcm_init_runtime_hw(struct snd_pcm_runtime *runtime,
 	struct snd_pcm_hardware *hw = &runtime->hw;
 
 	soc_pcm_hw_update_rate(hw, stream);
-	runtime->hw.channels_min = stream->channels_min;
-	runtime->hw.channels_max = stream->channels_max;
+	soc_pcm_hw_update_chan(hw, stream);
 	if (runtime->hw.formats)
 		runtime->hw.formats &= stream->formats;
 	else
@@ -1601,10 +1606,7 @@ static void dpcm_runtime_merge_chan(struct snd_pcm_substream *substream,
 
 			cpu_stream = snd_soc_dai_get_pcm_stream(dai, stream);
 
-			hw->channels_min = max(hw->channels_min,
-					       cpu_stream->channels_min);
-			hw->channels_max = min(hw->channels_max,
-					       cpu_stream->channels_max);
+			soc_pcm_hw_update_chan(hw, cpu_stream);
 		}
 
 		/*
@@ -1614,10 +1616,7 @@ static void dpcm_runtime_merge_chan(struct snd_pcm_substream *substream,
 		if (be->num_codecs == 1) {
 			codec_stream = snd_soc_dai_get_pcm_stream(asoc_rtd_to_codec(be, 0), stream);
 
-			hw->channels_min = max(hw->channels_min,
-					       codec_stream->channels_min);
-			hw->channels_max = min(hw->channels_max,
-					       codec_stream->channels_max);
+			soc_pcm_hw_update_chan(hw, codec_stream);
 		}
 	}
 }
-- 
2.25.1

[PATCH 3/3] ASoC: soc-pcm: add soc_pcm_hw_update_format()

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

We have soc_pcm_hw_update_xxx() now.
This patch creates same function for format.

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

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 748265018d1a..9203b1bf2e0d 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -480,6 +480,7 @@ static void soc_pcm_hw_init(struct snd_pcm_hardware *hw)
 	hw->rate_max		= UINT_MAX;
 	hw->channels_min	= 0;
 	hw->channels_max	= UINT_MAX;
+	hw->formats		= ULLONG_MAX;
 }
 
 static void soc_pcm_hw_update_rate(struct snd_pcm_hardware *hw,
@@ -502,6 +503,12 @@ static void soc_pcm_hw_update_chan(struct snd_pcm_hardware *hw,
 	hw->channels_max = min(hw->channels_max, p->channels_max);
 }
 
+static void soc_pcm_hw_update_format(struct snd_pcm_hardware *hw,
+				     struct snd_soc_pcm_stream *p)
+{
+	hw->formats &= p->formats;
+}
+
 /**
  * snd_soc_runtime_calc_hw() - Calculate hw limits for a PCM stream
  * @rtd: ASoC PCM runtime
@@ -519,7 +526,6 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 	struct snd_soc_pcm_stream *codec_stream;
 	struct snd_soc_pcm_stream *cpu_stream;
 	unsigned int cpu_chan_min = 0, cpu_chan_max = UINT_MAX;
-	u64 formats = ULLONG_MAX;
 	int i;
 
 	soc_pcm_hw_init(hw);
@@ -540,7 +546,7 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 
 		soc_pcm_hw_update_chan(hw, cpu_stream);
 		soc_pcm_hw_update_rate(hw, cpu_stream);
-		formats &= cpu_stream->formats;
+		soc_pcm_hw_update_format(hw, cpu_stream);
 	}
 	cpu_chan_min = hw->channels_min;
 	cpu_chan_max = hw->channels_max;
@@ -561,7 +567,7 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 
 		soc_pcm_hw_update_chan(hw, codec_stream);
 		soc_pcm_hw_update_rate(hw, codec_stream);
-		formats &= codec_stream->formats;
+		soc_pcm_hw_update_format(hw, codec_stream);
 	}
 
 	/* Verify both a valid CPU DAI and a valid CODEC DAI were found */
@@ -578,9 +584,6 @@ int snd_soc_runtime_calc_hw(struct snd_soc_pcm_runtime *rtd,
 		hw->channels_max = cpu_chan_max;
 	}
 
-	/* finally find a intersection between CODECs and CPUs */
-	hw->formats = formats;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_runtime_calc_hw);
@@ -1568,7 +1571,7 @@ static void dpcm_runtime_merge_format(struct snd_pcm_substream *substream,
 
 			codec_stream = snd_soc_dai_get_pcm_stream(dai, stream);
 
-			hw->formats &= codec_stream->formats;
+			soc_pcm_hw_update_format(hw, codec_stream);
 		}
 	}
 }
-- 
2.25.1

Re: [PATCH 0/3] ASoC: soc-pcm: add soc_pcm_hw_update_xxx()

[Kai Vehmanen]

Hey,

On Thu, 4 Feb 2021, Kuninori Morimoto wrote:

> These adds soc_pcm_hw_update_xxx() and cleanup soc-pcm.c
> 
> Kuninori Morimoto (3):
>   ASoC: soc-pcm: add soc_pcm_hw_update_rate()
>   ASoC: soc-pcm: add soc_pcm_hw_update_chan()
>   ASoC: soc-pcm: add soc_pcm_hw_update_format()

the reduction of duplicated code is not a lot, but there's enough that
I think this make sense. Code looks good:

Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Br, Kai

Re: [PATCH 0/3] ASoC: soc-pcm: add soc_pcm_hw_update_xxx()

[Mark Brown]

On 04 Feb 2021 08:50:05 +0900, Kuninori Morimoto wrote:
> These adds soc_pcm_hw_update_xxx() and cleanup soc-pcm.c
> 
> Kuninori Morimoto (3):
>   ASoC: soc-pcm: add soc_pcm_hw_update_rate()
>   ASoC: soc-pcm: add soc_pcm_hw_update_chan()
>   ASoC: soc-pcm: add soc_pcm_hw_update_format()
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/3] ASoC: soc-pcm: add soc_pcm_hw_update_rate()
      commit: f6c04af5dc4b80e70160acd9a7b04b185e093c71
[2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()
      commit: 6cb56a4549e9e2e0f7f67b99cb1887c0e803245a
[3/3] ASoC: soc-pcm: add soc_pcm_hw_update_format()
      commit: debc71f26cdbd45798c63b0dcdabdea93d2f6870

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Pierre-Louis Bossart]

> We have soc_pcm_hw_update_rate() now.
> This patch creates same function for chan.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

This patch seems to break all SOF platforms. I tested manually to try 
and reproduce the CI results and it's indeed not so good:

root:~# speaker-test -Dhw:0,2 -c2 -r48000

speaker-test 1.2.4

Playback device is hw:0,2
Stream parameters are 48000Hz, S16_LE, 2 channels
Using 16 octaves of pink noise
Playback open error: -22,Invalid argument

Git bisect points to this patch as the first bad commit.

I reverted patch 3 and patch 2 in this series in our tests and the 
results are back to normal.

see https://github.com/thesofproject/linux/pull/2755

results with these two patches included:

https://sof-ci.01.org/linuxpr/PR2755/build5289/devicetest/

Results with these two patches reverted:

https://sof-ci.01.org/linuxpr/PR2755/build5290/devicetest/

I will not have the time to dig further before the middle of next week, 
but I wanted to share the information in case Morimoto-san, Ranjani or 
Kai have ideas on what might have gone wrong?

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Kai Vehmanen]

Hi,

On Fri, 12 Feb 2021, Pierre-Louis Bossart wrote:

> > We have soc_pcm_hw_update_rate() now.
> > This patch creates same function for chan.
> 
> This patch seems to break all SOF platforms. I tested manually to try and
> reproduce the CI results and it's indeed not so good:

ouch -- I think this will impact also non-SOF platforms.

The new helper functions seem all correct, but the problem would seem to 
be in the dpcm_init_runtime_hw() as some of the inputs are not initialized 
as expected here. I'll try to send a fixup patch asap. In case 5.11 is 
released later today, this regression needs to be fixed for first 5.12 
pull req.

Br, Kai

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Kuninori Morimoto]

Hi

> > > We have soc_pcm_hw_update_rate() now.
> > > This patch creates same function for chan.
> > 
> > This patch seems to break all SOF platforms. I tested manually to try and
> > reproduce the CI results and it's indeed not so good:
> 
> ouch -- I think this will impact also non-SOF platforms.
> 
> The new helper functions seem all correct, but the problem would seem to 
> be in the dpcm_init_runtime_hw() as some of the inputs are not initialized 
> as expected here. I'll try to send a fixup patch asap. In case 5.11 is 
> released later today, this regression needs to be fixed for first 5.12 
> pull req.

Oops, thank you for your help,
and sorry for my noisy patch.

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

On Sun, Feb 14, 2021 at 11:17:03PM +0200, Kai Vehmanen wrote:
> On Fri, 12 Feb 2021, Pierre-Louis Bossart wrote:

> > > We have soc_pcm_hw_update_rate() now.
> > > This patch creates same function for chan.

> > This patch seems to break all SOF platforms. I tested manually to try and
> > reproduce the CI results and it's indeed not so good:

> ouch -- I think this will impact also non-SOF platforms.

> The new helper functions seem all correct, but the problem would seem to 
> be in the dpcm_init_runtime_hw() as some of the inputs are not initialized 
> as expected here. I'll try to send a fixup patch asap. In case 5.11 is 
> released later today, this regression needs to be fixed for first 5.12 
> pull req.

I've applied the fixup patch, if someone could confirm that the CI looks
good I'll send the pull request for this release.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Kai Vehmanen]

Hey,

On Mon, 15 Feb 2021, Mark Brown wrote:

> On Sun, Feb 14, 2021 at 11:17:03PM +0200, Kai Vehmanen wrote:
> > On Fri, 12 Feb 2021, Pierre-Louis Bossart wrote:
> > > This patch seems to break all SOF platforms. I tested manually to try and
> > > reproduce the CI results and it's indeed not so good:
> > The new helper functions seem all correct, but the problem would seem to 
> > be in the dpcm_init_runtime_hw() as some of the inputs are not initialized 
> 
> I've applied the fixup patch, if someone could confirm that the CI looks
> good I'll send the pull request for this release.

SOF CI now has had multiple all-passed runs with the fixup included while 
it fails systematically without it:
https://sof-ci.01.org/linuxpr/PR2756/build5292/devicetest/

Morimoto-san, did you have a look at the code changes? 

I think we might still have issues if we have multiple CPU DAIs per
runtime and dpcm_init_runtime_hw() is called multiple times. With the 
fixup, the limits are taken from the last CPU DAI.

But if you look at original code, the same issues seems to be there
(rate and channels taken from the stream directly with no consideration
of already set values). Only exception is runtime->hw.formats, which
is handled differently:

»       runtime->hw.rate_min = stream->rate_min;
»       runtime->hw.rate_max = min_not_zero(stream->rate_max, UINT_MAX);
»       runtime->hw.channels_min = stream->channels_min;
»       runtime->hw.channels_max = stream->channels_max;
»       if (runtime->hw.formats)
»       »       runtime->hw.formats &= stream->formats;
»       else
»       »       runtime->hw.formats = stream->formats;
»       runtime->hw.rates = stream->rates;

... so the fixup also resets hw.formats, but it does not seem logical to 
have different treatment only for hw.formats in the mult-CPU-DAI case.

But yes, more reviewers are welcome. 

Br, Kai

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Kuninori Morimoto]

Hi Kai

> I think we might still have issues if we have multiple CPU DAIs per
> runtime and dpcm_init_runtime_hw() is called multiple times. With the 
> fixup, the limits are taken from the last CPU DAI.
> 
> But if you look at original code, the same issues seems to be there
> (rate and channels taken from the stream directly with no consideration
> of already set values). Only exception is runtime->hw.formats, which
> is handled differently:

Actually, when I posted the patch, I removed 1 line which I thought not needed.
But it seems it was necessary...

Current fixup patch always initialize hw at dpcm_set_fe_runtime()'s loop,
but I guess we need is initialize once.

How about this patch ?
It reverts current fixup, and initialize hw once at dpcm_set_fe_runtime().

-----------
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 37094aeff440..14d85ca1e435 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1531,10 +1531,12 @@ static void dpcm_init_runtime_hw(struct snd_pcm_runtime *runtime,
 {
 	struct snd_pcm_hardware *hw = &runtime->hw;
 
-	soc_pcm_hw_init(hw);
 	soc_pcm_hw_update_rate(hw, stream);
 	soc_pcm_hw_update_chan(hw, stream);
-	soc_pcm_hw_update_format(hw, stream);
+	if (runtime->hw.formats)
+		runtime->hw.formats &= stream->formats;
+	else
+		runtime->hw.formats = stream->formats;
 }
 
 static void dpcm_runtime_merge_format(struct snd_pcm_substream *substream,
@@ -1662,10 +1664,13 @@ static void dpcm_runtime_merge_rate(struct snd_pcm_substream *substream,
 static void dpcm_set_fe_runtime(struct snd_pcm_substream *substream)
 {
 	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_pcm_hardware *hw = &runtime->hw;
 	struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream);
 	struct snd_soc_dai *cpu_dai;
 	int i;
 
+	soc_pcm_hw_init(hw);
+
 	for_each_rtd_cpu_dais(rtd, i, cpu_dai) {
 		/*
 		 * Skip CPUs which don't support the current stream


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Kai Vehmanen]

Hey,

On Tue, 16 Feb 2021, Kuninori Morimoto wrote:

> > I think we might still have issues if we have multiple CPU DAIs per
> > runtime and dpcm_init_runtime_hw() is called multiple times. With the 
> 
> Actually, when I posted the patch, I removed 1 line which I thought not needed.
> But it seems it was necessary...
> 
> Current fixup patch always initialize hw at dpcm_set_fe_runtime()'s loop,
> but I guess we need is initialize once.
> 
> How about this patch ?

yes, this is even better! dpcm_init_runtime_hw() is not called from 
anywhere else, so this looks good.

I kicked off SOF CI with this patch applied. It should be all good, but 
testing just in case.

Br, Kai

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Kai Vehmanen]

Hey,

On Tue, 16 Feb 2021, Kai Vehmanen wrote:

> On Tue, 16 Feb 2021, Kuninori Morimoto wrote:
> 
> > Current fixup patch always initialize hw at dpcm_set_fe_runtime()'s loop,
> > but I guess we need is initialize once.
> > 
> > How about this patch ?
> 
> I kicked off SOF CI with this patch applied. It should be all good, but 
> testing just in case.

tests results came back good:
https://sof-ci.01.org/linuxpr/PR2756/build5294/devicetest/

Let's cook up a proper patch for this.

Br, Kai

Re: [PATCH 2/3] ASoC: soc-pcm: add soc_pcm_hw_update_chan()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

On Tue, Feb 16, 2021 at 01:22:46PM +0200, Kai Vehmanen wrote:
> On Tue, 16 Feb 2021, Kai Vehmanen wrote:

> > I kicked off SOF CI with this patch applied. It should be all good, but 
> > testing just in case.

> tests results came back good:
> https://sof-ci.01.org/linuxpr/PR2756/build5294/devicetest/

> Let's cook up a proper patch for this.

OK, I'll wait for confirmation on this one from Morimoto-san just to be
sure.  Thanks both of you for looking into this so quickly.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]