[PATCH] ASoC: qcom: q6asm-dai: Add SNDRV_PCM_INFO_BATCH flag

[PATCH] ASoC: qcom: q6asm-dai: Add SNDRV_PCM_INFO_BATCH flag

[Stephan Gerhold]

At the moment, playing audio with PulseAudio with the qdsp6 driver
results in distorted sound. It seems like its timer-based scheduling
does not work properly with qdsp6 since setting tsched=0 in
the PulseAudio configuration avoids the issue.

Apparently this happens when the pointer() callback is not accurate
enough. There is a SNDRV_PCM_INFO_BATCH flag that can be used to stop
PulseAudio from using timer-based scheduling by default.

According to https://www.alsa-project.org/pipermail/alsa-devel/2014-March/073816.html:

    The flag is being used in the sense explained in the previous audio
    meeting -- the data transfer granularity isn't fine enough but aligned
    to the period size (or less).

q6asm-dai reports the position as multiple of

    prtd->pcm_count = snd_pcm_lib_period_bytes(substream)

so it indeed just a multiple of the period size.

Therefore adding the flag here seems appropriate and makes audio
work out of the box.

Fixes: 2a9e92d371db ("ASoC: qdsp6: q6asm: Add q6asm dai driver")
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
I'm still quite confused about the true meaning of SNDRV_PCM_INFO_BATCH,
so please correct me if I'm wrong :)

The tsched=0 workaround can be found in Linaro distributions
for QCOM devices for example:
  - https://git.linaro.org/ci/fai.git/commit/?id=63494268b654d80df033f4cdeccf8f115801b756
  - https://github.com/ndechesne/meta-qcom/commit/7035dfeadd1c434fc7613730ac38004670553ec0

This patch allows removing that workaround since audio then works
without any configuration changes.
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index f6c7cddf08e8..125af00bba53 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -78,7 +78,7 @@ struct q6asm_dai_data {
 };
 
 static const struct snd_pcm_hardware q6asm_dai_hardware_capture = {
-	.info =                 (SNDRV_PCM_INFO_MMAP |
+	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_BATCH |
 				SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				SNDRV_PCM_INFO_MMAP_VALID |
 				SNDRV_PCM_INFO_INTERLEAVED |
@@ -100,7 +100,7 @@ static const struct snd_pcm_hardware q6asm_dai_hardware_capture = {
 };
 
 static struct snd_pcm_hardware q6asm_dai_hardware_playback = {
-	.info =                 (SNDRV_PCM_INFO_MMAP |
+	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_BATCH |
 				SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				SNDRV_PCM_INFO_MMAP_VALID |
 				SNDRV_PCM_INFO_INTERLEAVED |
-- 
2.26.0

Re: [PATCH] ASoC: qcom: q6asm-dai: Add SNDRV_PCM_INFO_BATCH flag

[Lars-Peter Clausen]

On 3/30/20 7:52 PM, Stephan Gerhold wrote:
> At the moment, playing audio with PulseAudio with the qdsp6 driver
> results in distorted sound. It seems like its timer-based scheduling
> does not work properly with qdsp6 since setting tsched=0 in
> the PulseAudio configuration avoids the issue.
>
> Apparently this happens when the pointer() callback is not accurate
> enough. There is a SNDRV_PCM_INFO_BATCH flag that can be used to stop
> PulseAudio from using timer-based scheduling by default.
>
> According to https://www.alsa-project.org/pipermail/alsa-devel/2014-March/073816.html:
>
>      The flag is being used in the sense explained in the previous audio
>      meeting -- the data transfer granularity isn't fine enough but aligned
>      to the period size (or less).
>
> q6asm-dai reports the position as multiple of
>
>      prtd->pcm_count = snd_pcm_lib_period_bytes(substream)
>
> so it indeed just a multiple of the period size.
>
> Therefore adding the flag here seems appropriate and makes audio
> work out of the box.
>
> Fixes: 2a9e92d371db ("ASoC: qdsp6: q6asm: Add q6asm dai driver")
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> I'm still quite confused about the true meaning of SNDRV_PCM_INFO_BATCH,
> so please correct me if I'm wrong :)

The meaning might have changed over the years, but the way it is used 
right now is that it means that the position pointer has limited 
granularity. With 'limited' being a bit fuzzy, but typically means that 
the granularity is worse than a few samples.

This driver definitely falls into the limited category as the 
granularity seems to be period size.

- Lars

Re: [PATCH] ASoC: qcom: q6asm-dai: Add SNDRV_PCM_INFO_BATCH flag

[Pierre-Louis Bossart]

On 3/30/20 1:15 PM, Lars-Peter Clausen wrote:
> On 3/30/20 7:52 PM, Stephan Gerhold wrote:
>> At the moment, playing audio with PulseAudio with the qdsp6 driver
>> results in distorted sound. It seems like its timer-based scheduling
>> does not work properly with qdsp6 since setting tsched=0 in
>> the PulseAudio configuration avoids the issue.
>>
>> Apparently this happens when the pointer() callback is not accurate
>> enough. There is a SNDRV_PCM_INFO_BATCH flag that can be used to stop
>> PulseAudio from using timer-based scheduling by default.
>>
>> According to 
>> https://www.alsa-project.org/pipermail/alsa-devel/2014-March/073816.html:
>>
>>      The flag is being used in the sense explained in the previous audio
>>      meeting -- the data transfer granularity isn't fine enough but 
>> aligned
>>      to the period size (or less).
>>
>> q6asm-dai reports the position as multiple of
>>
>>      prtd->pcm_count = snd_pcm_lib_period_bytes(substream)
>>
>> so it indeed just a multiple of the period size.
>>
>> Therefore adding the flag here seems appropriate and makes audio
>> work out of the box.
>>
>> Fixes: 2a9e92d371db ("ASoC: qdsp6: q6asm: Add q6asm dai driver")
>> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>> ---
>> I'm still quite confused about the true meaning of SNDRV_PCM_INFO_BATCH,
>> so please correct me if I'm wrong :)
> 
> The meaning might have changed over the years, but the way it is used 
> right now is that it means that the position pointer has limited 
> granularity. With 'limited' being a bit fuzzy, but typically means that 
> the granularity is worse than a few samples.
> 
> This driver definitely falls into the limited category as the 
> granularity seems to be period size.

Agree, we added this INFO_BATCH flag for SOF Broadwell and Baytrail 
platforms as well for the same reason of large granularity.

Re: [PATCH] ASoC: qcom: q6asm-dai: Add SNDRV_PCM_INFO_BATCH flag

[Srinivas Kandagatla]

Thanks Stephan for finding this flag!

On 30/03/2020 18:52, Stephan Gerhold wrote:
> At the moment, playing audio with PulseAudio with the qdsp6 driver
> results in distorted sound. It seems like its timer-based scheduling
> does not work properly with qdsp6 since setting tsched=0 in
> the PulseAudio configuration avoids the issue.
> 
> Apparently this happens when the pointer() callback is not accurate
> enough. There is a SNDRV_PCM_INFO_BATCH flag that can be used to stop
> PulseAudio from using timer-based scheduling by default.
> 
> According to https://www.alsa-project.org/pipermail/alsa-devel/2014-March/073816.html:
> 
>      The flag is being used in the sense explained in the previous audio
>      meeting -- the data transfer granularity isn't fine enough but aligned
>      to the period size (or less).
> 
> q6asm-dai reports the position as multiple of
> 
>      prtd->pcm_count = snd_pcm_lib_period_bytes(substream)
> 
> so it indeed just a multiple of the period size.
> 
> Therefore adding the flag here seems appropriate and makes audio
> work out of the box.
> 
> Fixes: 2a9e92d371db ("ASoC: qdsp6: q6asm: Add q6asm dai driver")
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>

Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Applied "ASoC: qcom: q6asm-dai: Add SNDRV_PCM_INFO_BATCH flag" to the asoc tree

[Mark Brown]

The patch

   ASoC: qcom: q6asm-dai: Add SNDRV_PCM_INFO_BATCH flag

has been applied to the asoc tree at

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

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

From 7f2430cda819a9ecb1df5a0f3ef4f1c20db3f811 Mon Sep 17 00:00:00 2001
From: Stephan Gerhold <stephan@gerhold.net>
Date: Mon, 30 Mar 2020 19:52:10 +0200
Subject: [PATCH] ASoC: qcom: q6asm-dai: Add SNDRV_PCM_INFO_BATCH flag

At the moment, playing audio with PulseAudio with the qdsp6 driver
results in distorted sound. It seems like its timer-based scheduling
does not work properly with qdsp6 since setting tsched=0 in
the PulseAudio configuration avoids the issue.

Apparently this happens when the pointer() callback is not accurate
enough. There is a SNDRV_PCM_INFO_BATCH flag that can be used to stop
PulseAudio from using timer-based scheduling by default.

According to https://www.alsa-project.org/pipermail/alsa-devel/2014-March/073816.html:

    The flag is being used in the sense explained in the previous audio
    meeting -- the data transfer granularity isn't fine enough but aligned
    to the period size (or less).

q6asm-dai reports the position as multiple of

    prtd->pcm_count = snd_pcm_lib_period_bytes(substream)

so it indeed just a multiple of the period size.

Therefore adding the flag here seems appropriate and makes audio
work out of the box.

Fixes: 2a9e92d371db ("ASoC: qdsp6: q6asm: Add q6asm dai driver")
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Link: https://lore.kernel.org/r/20200330175210.47518-1-stephan@gerhold.net
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/qcom/qdsp6/q6asm-dai.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/qcom/qdsp6/q6asm-dai.c b/sound/soc/qcom/qdsp6/q6asm-dai.c
index f6c7cddf08e8..125af00bba53 100644
--- a/sound/soc/qcom/qdsp6/q6asm-dai.c
+++ b/sound/soc/qcom/qdsp6/q6asm-dai.c
@@ -78,7 +78,7 @@ struct q6asm_dai_data {
 };
 
 static const struct snd_pcm_hardware q6asm_dai_hardware_capture = {
-	.info =                 (SNDRV_PCM_INFO_MMAP |
+	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_BATCH |
 				SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				SNDRV_PCM_INFO_MMAP_VALID |
 				SNDRV_PCM_INFO_INTERLEAVED |
@@ -100,7 +100,7 @@ static const struct snd_pcm_hardware q6asm_dai_hardware_capture = {
 };
 
 static struct snd_pcm_hardware q6asm_dai_hardware_playback = {
-	.info =                 (SNDRV_PCM_INFO_MMAP |
+	.info =                 (SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_BATCH |
 				SNDRV_PCM_INFO_BLOCK_TRANSFER |
 				SNDRV_PCM_INFO_MMAP_VALID |
 				SNDRV_PCM_INFO_INTERLEAVED |
-- 
2.20.1