[PATCH v0] ASoC: rsnd: core: Check convert rate in rsnd_hw_params
From: Mikhail Durnev mikhail_durnev@mentor.com
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 mikhail_durnev@mentor.com --- 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);
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-Ch... 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/2ffb6c8ec578fd78d5962f7bc7c38eeb5c6a... 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 lkp@intel.com
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/kbuild-all@lists.01.org
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-Ch... 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/2ffb6c8ec578fd78d5962f7bc7c38eeb5c6a... 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 lkp@intel.com
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/kbuild-all@lists.01.org
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
participants (4)
-
kernel test robot
-
Kuninori Morimoto
-
mdurnev@gmail.com
-
Mikhail Durnev