From: Mikhail Durnev <[email protected]>
snd_pcm_hw_params_set_rate_near can return incorrect sample rate in
some cases, e.g. when the backend output rate is set to some value higher
than 48000 Hz and the input rate is 8000 Hz. So passing the value returned
by snd_pcm_hw_params_set_rate_near to snd_pcm_hw_params will result in
"FSO/FSI ratio error" and playing no audio at all while the userland
is not properly notified about the issue.
If SRC is unable to convert the requested sample rate to the sample rate
the backend is using, then the requested sample rate should be adjusted in
rsnd_hw_params. The userland will be notified about that change in the
returned hw_params structure.
Signed-off-by: Mikhail Durnev <[email protected]>
---
sound/soc/sh/rcar/core.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 1 deletion(-)
diff --git a/sound/soc/sh/rcar/core.c b/sound/soc/sh/rcar/core.c
index 6e670b3..8ca3fb7 100644
--- a/sound/soc/sh/rcar/core.c
+++ b/sound/soc/sh/rcar/core.c
@@ -1428,8 +1428,73 @@ static int rsnd_hw_params(struct snd_soc_component *component,
}
if (io->converted_chan)
dev_dbg(dev, "convert channels = %d\n", io->converted_chan);
- if (io->converted_rate)
+ if (io->converted_rate) {
dev_dbg(dev, "convert rate = %d\n", io->converted_rate);
+
+ /*
+ * SRC supports convert rates from params_rate(hw_params)/k_down
+ * to params_rate(hw_params)*k_up, where k_up is always 6, and
+ * k_down depends on number of channels and SRC unit.
+ * So all SRC units can upsample audio up to 6 times regardless
+ * its number of channels. And all SRC units can downsample
+ * 2 channel audio up to 6 times too.
+ */
+ int k_up = 6;
+ int k_down = 6;
+ int channel;
+ struct rsnd_mod *src_mod = rsnd_io_to_mod_src(io);
+
+ channel = io->converted_chan ? io->converted_chan : fe_channel;
+
+ switch (rsnd_mod_id(src_mod)) {
+ /*
+ * SRC0 can downsample 4, 6 and 8 channel audio up to 4 times.
+ * SRC1, SRC3 and SRC4 can downsample 4 channel audio
+ * up to 4 times.
+ * SRC1, SRC3 and SRC4 can downsample 6 and 8 channel audio
+ * no more than twice.
+ */
+ case 1:
+ case 3:
+ case 4:
+ if (channel > 4) {
+ k_down = 2;
+ break;
+ }
+ case 0:
+ if (channel > 2)
+ k_down = 4;
+ break;
+
+ /* Other SRC units do not support more than 2 channels */
+ default:
+ if (channel > 2)
+ return -EINVAL;
+ }
+
+ if (params_rate(hw_params) > io->converted_rate * k_down) {
+ hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->min =
+ io->converted_rate * k_down;
+ hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->max =
+ io->converted_rate * k_down;
+ hw_params->cmask |= SNDRV_PCM_HW_PARAM_RATE;
+ } else if (params_rate(hw_params) * k_up < io->converted_rate) {
+ hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->min =
+ (io->converted_rate + k_up - 1) / k_up;
+ hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->max =
+ (io->converted_rate + k_up - 1) / k_up;
+ hw_params->cmask |= SNDRV_PCM_HW_PARAM_RATE;
+ }
+
+ /*
+ * TBD: Max SRC input and output rates also depend on number
+ * of channels and SRC unit:
+ * SRC1, SRC3 and SRC4 do not support more than 128kHz
+ * for 6 channel and 96kHz for 8 channel audio.
+ * Perhaps this function should return EINVAL if the input or
+ * the output rate exceeds the limitation.
+ */
+ }
}
return rsnd_dai_call(hw_params, io, substream, hw_params);
--
2.7.4
Hi,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on asoc/for-next]
[also build test WARNING on v5.10-rc6 next-20201201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/mdurnev-gmail-com/ASoC-rsnd-core-Check-convert-rate-in-rsnd_hw_params/20201202-191131
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: riscv-allyesconfig (attached as .config)
compiler: riscv64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/2ffb6c8ec578fd78d5962f7bc7c38eeb5c6a4bd1
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review mdurnev-gmail-com/ASoC-rsnd-core-Check-convert-rate-in-rsnd_hw_params/20201202-191131
git checkout 2ffb6c8ec578fd78d5962f7bc7c38eeb5c6a4bd1
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=riscv
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
sound/soc/sh/rcar/core.c: In function 'rsnd_hw_params':
>> sound/soc/sh/rcar/core.c:1442:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
1442 | int k_up = 6;
| ^~~
sound/soc/sh/rcar/core.c:1447:56: error: 'fe_channel' undeclared (first use in this function); did you mean 'channel'?
1447 | channel = io->converted_chan ? io->converted_chan : fe_channel;
| ^~~~~~~~~~
| channel
sound/soc/sh/rcar/core.c:1447:56: note: each undeclared identifier is reported only once for each function it appears in
>> sound/soc/sh/rcar/core.c:1460:8: warning: this statement may fall through [-Wimplicit-fallthrough=]
1460 | if (channel > 4) {
| ^
sound/soc/sh/rcar/core.c:1464:4: note: here
1464 | case 0:
| ^~~~
vim +1442 sound/soc/sh/rcar/core.c
1391
1392 /*
1393 * pcm ops
1394 */
1395 static int rsnd_hw_params(struct snd_soc_component *component,
1396 struct snd_pcm_substream *substream,
1397 struct snd_pcm_hw_params *hw_params)
1398 {
1399 struct snd_soc_dai *dai = rsnd_substream_to_dai(substream);
1400 struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai);
1401 struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream);
1402 struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
1403
1404 /*
1405 * rsnd assumes that it might be used under DPCM if user want to use
1406 * channel / rate convert. Then, rsnd should be FE.
1407 * And then, this function will be called *after* BE settings.
1408 * this means, each BE already has fixuped hw_params.
1409 * see
1410 * dpcm_fe_dai_hw_params()
1411 * dpcm_be_dai_hw_params()
1412 */
1413 io->converted_rate = 0;
1414 io->converted_chan = 0;
1415 if (fe->dai_link->dynamic) {
1416 struct rsnd_priv *priv = rsnd_io_to_priv(io);
1417 struct device *dev = rsnd_priv_to_dev(priv);
1418 struct snd_soc_dpcm *dpcm;
1419 struct snd_pcm_hw_params *be_params;
1420 int stream = substream->stream;
1421
1422 for_each_dpcm_be(fe, stream, dpcm) {
1423 be_params = &dpcm->hw_params;
1424 if (params_channels(hw_params) != params_channels(be_params))
1425 io->converted_chan = params_channels(be_params);
1426 if (params_rate(hw_params) != params_rate(be_params))
1427 io->converted_rate = params_rate(be_params);
1428 }
1429 if (io->converted_chan)
1430 dev_dbg(dev, "convert channels = %d\n", io->converted_chan);
1431 if (io->converted_rate) {
1432 dev_dbg(dev, "convert rate = %d\n", io->converted_rate);
1433
1434 /*
1435 * SRC supports convert rates from params_rate(hw_params)/k_down
1436 * to params_rate(hw_params)*k_up, where k_up is always 6, and
1437 * k_down depends on number of channels and SRC unit.
1438 * So all SRC units can upsample audio up to 6 times regardless
1439 * its number of channels. And all SRC units can downsample
1440 * 2 channel audio up to 6 times too.
1441 */
> 1442 int k_up = 6;
1443 int k_down = 6;
1444 int channel;
1445 struct rsnd_mod *src_mod = rsnd_io_to_mod_src(io);
1446
1447 channel = io->converted_chan ? io->converted_chan : fe_channel;
1448
1449 switch (rsnd_mod_id(src_mod)) {
1450 /*
1451 * SRC0 can downsample 4, 6 and 8 channel audio up to 4 times.
1452 * SRC1, SRC3 and SRC4 can downsample 4 channel audio
1453 * up to 4 times.
1454 * SRC1, SRC3 and SRC4 can downsample 6 and 8 channel audio
1455 * no more than twice.
1456 */
1457 case 1:
1458 case 3:
1459 case 4:
> 1460 if (channel > 4) {
1461 k_down = 2;
1462 break;
1463 }
1464 case 0:
1465 if (channel > 2)
1466 k_down = 4;
1467 break;
1468
1469 /* Other SRC units do not support more than 2 channels */
1470 default:
1471 if (channel > 2)
1472 return -EINVAL;
1473 }
1474
1475 if (params_rate(hw_params) > io->converted_rate * k_down) {
1476 hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->min =
1477 io->converted_rate * k_down;
1478 hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->max =
1479 io->converted_rate * k_down;
1480 hw_params->cmask |= SNDRV_PCM_HW_PARAM_RATE;
1481 } else if (params_rate(hw_params) * k_up < io->converted_rate) {
1482 hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->min =
1483 (io->converted_rate + k_up - 1) / k_up;
1484 hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->max =
1485 (io->converted_rate + k_up - 1) / k_up;
1486 hw_params->cmask |= SNDRV_PCM_HW_PARAM_RATE;
1487 }
1488
1489 /*
1490 * TBD: Max SRC input and output rates also depend on number
1491 * of channels and SRC unit:
1492 * SRC1, SRC3 and SRC4 do not support more than 128kHz
1493 * for 6 channel and 96kHz for 8 channel audio.
1494 * Perhaps this function should return EINVAL if the input or
1495 * the output rate exceeds the limitation.
1496 */
1497 }
1498 }
1499
1500 return rsnd_dai_call(hw_params, io, substream, hw_params);
1501 }
1502
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on asoc/for-next]
[also build test ERROR on v5.10-rc6 next-20201201]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/mdurnev-gmail-com/ASoC-rsnd-core-Check-convert-rate-in-rsnd_hw_params/20201202-191131
base: https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
config: arm64-randconfig-r012-20201202 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 2671fccf0381769276ca8246ec0499adcb9b0355)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install arm64 cross compiling tool for clang build
# apt-get install binutils-aarch64-linux-gnu
# https://github.com/0day-ci/linux/commit/2ffb6c8ec578fd78d5962f7bc7c38eeb5c6a4bd1
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review mdurnev-gmail-com/ASoC-rsnd-core-Check-convert-rate-in-rsnd_hw_params/20201202-191131
git checkout 2ffb6c8ec578fd78d5962f7bc7c38eeb5c6a4bd1
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>
All error/warnings (new ones prefixed by >>):
>> sound/soc/sh/rcar/core.c:1447:56: error: use of undeclared identifier 'fe_channel'; did you mean 'channel'?
channel = io->converted_chan ? io->converted_chan : fe_channel;
^~~~~~~~~~
channel
sound/soc/sh/rcar/core.c:1444:8: note: 'channel' declared here
int channel;
^
>> sound/soc/sh/rcar/core.c:1442:8: warning: ISO C90 forbids mixing declarations and code [-Wdeclaration-after-statement]
int k_up = 6;
^
1 warning and 1 error generated.
vim +1447 sound/soc/sh/rcar/core.c
1391
1392 /*
1393 * pcm ops
1394 */
1395 static int rsnd_hw_params(struct snd_soc_component *component,
1396 struct snd_pcm_substream *substream,
1397 struct snd_pcm_hw_params *hw_params)
1398 {
1399 struct snd_soc_dai *dai = rsnd_substream_to_dai(substream);
1400 struct rsnd_dai *rdai = rsnd_dai_to_rdai(dai);
1401 struct rsnd_dai_stream *io = rsnd_rdai_to_io(rdai, substream);
1402 struct snd_soc_pcm_runtime *fe = asoc_substream_to_rtd(substream);
1403
1404 /*
1405 * rsnd assumes that it might be used under DPCM if user want to use
1406 * channel / rate convert. Then, rsnd should be FE.
1407 * And then, this function will be called *after* BE settings.
1408 * this means, each BE already has fixuped hw_params.
1409 * see
1410 * dpcm_fe_dai_hw_params()
1411 * dpcm_be_dai_hw_params()
1412 */
1413 io->converted_rate = 0;
1414 io->converted_chan = 0;
1415 if (fe->dai_link->dynamic) {
1416 struct rsnd_priv *priv = rsnd_io_to_priv(io);
1417 struct device *dev = rsnd_priv_to_dev(priv);
1418 struct snd_soc_dpcm *dpcm;
1419 struct snd_pcm_hw_params *be_params;
1420 int stream = substream->stream;
1421
1422 for_each_dpcm_be(fe, stream, dpcm) {
1423 be_params = &dpcm->hw_params;
1424 if (params_channels(hw_params) != params_channels(be_params))
1425 io->converted_chan = params_channels(be_params);
1426 if (params_rate(hw_params) != params_rate(be_params))
1427 io->converted_rate = params_rate(be_params);
1428 }
1429 if (io->converted_chan)
1430 dev_dbg(dev, "convert channels = %d\n", io->converted_chan);
1431 if (io->converted_rate) {
1432 dev_dbg(dev, "convert rate = %d\n", io->converted_rate);
1433
1434 /*
1435 * SRC supports convert rates from params_rate(hw_params)/k_down
1436 * to params_rate(hw_params)*k_up, where k_up is always 6, and
1437 * k_down depends on number of channels and SRC unit.
1438 * So all SRC units can upsample audio up to 6 times regardless
1439 * its number of channels. And all SRC units can downsample
1440 * 2 channel audio up to 6 times too.
1441 */
> 1442 int k_up = 6;
1443 int k_down = 6;
1444 int channel;
1445 struct rsnd_mod *src_mod = rsnd_io_to_mod_src(io);
1446
> 1447 channel = io->converted_chan ? io->converted_chan : fe_channel;
1448
1449 switch (rsnd_mod_id(src_mod)) {
1450 /*
1451 * SRC0 can downsample 4, 6 and 8 channel audio up to 4 times.
1452 * SRC1, SRC3 and SRC4 can downsample 4 channel audio
1453 * up to 4 times.
1454 * SRC1, SRC3 and SRC4 can downsample 6 and 8 channel audio
1455 * no more than twice.
1456 */
1457 case 1:
1458 case 3:
1459 case 4:
1460 if (channel > 4) {
1461 k_down = 2;
1462 break;
1463 }
1464 case 0:
1465 if (channel > 2)
1466 k_down = 4;
1467 break;
1468
1469 /* Other SRC units do not support more than 2 channels */
1470 default:
1471 if (channel > 2)
1472 return -EINVAL;
1473 }
1474
1475 if (params_rate(hw_params) > io->converted_rate * k_down) {
1476 hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->min =
1477 io->converted_rate * k_down;
1478 hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->max =
1479 io->converted_rate * k_down;
1480 hw_params->cmask |= SNDRV_PCM_HW_PARAM_RATE;
1481 } else if (params_rate(hw_params) * k_up < io->converted_rate) {
1482 hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->min =
1483 (io->converted_rate + k_up - 1) / k_up;
1484 hw_param_interval(hw_params, SNDRV_PCM_HW_PARAM_RATE)->max =
1485 (io->converted_rate + k_up - 1) / k_up;
1486 hw_params->cmask |= SNDRV_PCM_HW_PARAM_RATE;
1487 }
1488
1489 /*
1490 * TBD: Max SRC input and output rates also depend on number
1491 * of channels and SRC unit:
1492 * SRC1, SRC3 and SRC4 do not support more than 128kHz
1493 * for 6 channel and 96kHz for 8 channel audio.
1494 * Perhaps this function should return EINVAL if the input or
1495 * the output rate exceeds the limitation.
1496 */
1497 }
1498 }
1499
1500 return rsnd_dai_call(hw_params, io, substream, hw_params);
1501 }
1502
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]
Hi Mikhail
Thank you for your patch
> + switch (rsnd_mod_id(src_mod)) {
> + /*
> + * SRC0 can downsample 4, 6 and 8 channel audio up to 4 times.
> + * SRC1, SRC3 and SRC4 can downsample 4 channel audio
> + * up to 4 times.
> + * SRC1, SRC3 and SRC4 can downsample 6 and 8 channel audio
> + * no more than twice.
> + */
> + case 1:
> + case 3:
> + case 4:
> + if (channel > 4) {
> + k_down = 2;
> + break;
> + }
> + case 0:
> + if (channel > 2)
> + k_down = 4;
> + break;
> +
> + /* Other SRC units do not support more than 2 channels */
> + default:
> + if (channel > 2)
> + return -EINVAL;
> + }
I think you want to add "fallthrough" between case 1/3/4 and case 0 ?
Thank you for your help !!
Best regards
---
Kuninori Morimoto
On 03.12.2020 08:57, Kuninori Morimoto wrote:
> I think you want to add "fallthrough" between case 1/3/4 and case 0 ?
Hi Morimoto-san,
Yes, I will add it in the updated version of the patch. Thanks.
The checkpatch script did not tell me about that.
Best regards,
Misha