Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751266AbcLEHdH (ORCPT ); Mon, 5 Dec 2016 02:33:07 -0500 Received: from relay1.mentorg.com ([192.94.38.131]:39805 "EHLO relay1.mentorg.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750785AbcLEHdG (ORCPT ); Mon, 5 Dec 2016 02:33:06 -0500 Message-ID: <58451823.8040207@mentor.com> Date: Sun, 4 Dec 2016 23:32:51 -0800 From: Jiada Wang User-Agent: Mozilla/5.0 (X11; Linux i686; rv:11.0) Gecko/20120411 Thunderbird/11.0.1 MIME-Version: 1.0 To: Takashi Sakamoto CC: , , , , , Subject: Re: [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate multiple times on bus References: <20161130075923.15205-1-jiada_wang@mentor.com> <20161130075923.15205-3-jiada_wang@mentor.com> In-Reply-To: Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-orw-mbx-04.mgc.mentorg.com (147.34.90.204) To svr-orw-mbx-03.mgc.mentorg.com (147.34.90.203) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3424 Lines: 95 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 >> >> 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 >> Signed-off-by: Jiada Wang >> --- >> 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