Re: [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate multiple times on bus

From: Jiada Wang
Date: Mon Dec 05 2016 - 02:33:22 EST


Hi Sakamoto

On 11/30/2016 02:45 AM, Takashi Sakamoto wrote:
Hi Jiada,

I don't oppose this patch. Nevertheless, your description is not necessarily correct.

On Nov 30 2016 16:59, Jiada Wang wrote:
From: Daniel Girnus <dgirnus@xxxxxxxxxxxxxx>

ALSA usually calls the prepare function twice before starting the playback:
1. On hw_params call from userland and
2. internally when starting the stream.

ALSA PCM core in kernel land doesn't perform like this.

In alsa-lib, 'snd_pcm_hw_params()' calls 'snd_pcm_hw_params_internal()' and 'snd_pcm_prepare()' sequentially.
http://git.alsa-project.org/?p=alsa-lib.git;a=blob;f=src/pcm/pcm.c;h=cd87bc759ded95953e332b7e8d56b0f2d5b4185d;hb=HEAD#l853

In system call level (e.g. see by strace(1)), this looks like two ioctl(2)s with 'SNDRV_PCM_IOCTL_HW_PARAMS' and 'SNDRV_PCM_IOCTL_PREPARE'.

Well, when applications are written to execute 'snd_pcm_hw_params()' and 'snd_pcm_hw_prepare()' sequentially, additional ioctl(2) with 'SNDRV_PCM_IOCTL_PREPARE' appears. PulseAudio is this kind of application. I indicated the useless in 2014, but it still remains:
https://lists.freedesktop.org/archives/pulseaudio-discuss/2014-January/019773.html

You have the misunderstanding due to a nature of alsa-lib and tendency of major applications, from my point of view.

Thanks for your indication, so because some of userland applications call 'snd_pcm_hw_params()' and
'snd_pcm_hw_prepare()' sequentially, means the second 'SNDRV_PCM_IOCTL_PREPARE' be called in 'SNDRV_PCM_STATE_PREPARED' state, some devices are unable to manage this and stop working.

I will update Changelog in v2 Patchset.

Thanks,
Jiada

Some device are not able to manage this and they will stop playback
if the sample rate will be configured several times over USB protocol.

Signed-off-by: Jens Lorenz <jlorenz@xxxxxxxxxxxxxx>
Signed-off-by: Jiada Wang <jiada_wang@xxxxxxxxxx>
---
sound/usb/pcm.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 44d178e..a522c9a 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -806,17 +806,18 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream)
if (ret < 0)
goto unlock;

- iface = usb_ifnum_to_if(subs->dev, subs->cur_audiofmt->iface);
- alts = &iface->altsetting[subs->cur_audiofmt->altset_idx];
- ret = snd_usb_init_sample_rate(subs->stream->chip,
- subs->cur_audiofmt->iface,
- alts,
- subs->cur_audiofmt,
- subs->cur_rate);
- if (ret < 0)
- goto unlock;
-
if (subs->need_setup_ep) {
+
+ iface = usb_ifnum_to_if(subs->dev, subs->cur_audiofmt->iface);
+ alts = &iface->altsetting[subs->cur_audiofmt->altset_idx];
+ ret = snd_usb_init_sample_rate(subs->stream->chip,
+ subs->cur_audiofmt->iface,
+ alts,
+ subs->cur_audiofmt,
+ subs->cur_rate);
+ if (ret < 0)
+ goto unlock;
+
ret = configure_endpoint(subs);
if (ret < 0)
goto unlock;


Regards

Takashi Sakamoto