Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1533039 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate multiple times on bus Jiada Wang <jiada_wang@mentor.com> - 2016-11-30 09:00 +0100
    Re: [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate multiple times on bus Takashi Iwai <tiwai@suse.de> - 2016-11-30 10:00 +0100
      Re: [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate  multiple times on bus Jiada Wang <jiada_wang@mentor.com> - 2016-12-01 08:10 +0100
    Re: [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate  multiple times on bus Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2016-11-30 11:50 +0100
      Re: [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate  multiple times on bus Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2016-11-30 23:30 +0100
      Re: [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate  multiple times on bus Jiada Wang <jiada_wang@mentor.com> - 2016-12-05 08:40 +0100
        Re: [PATCH 2/3 v2] ALSA: usb-audio: avoid setting of sample rate  multiple times on bus Takashi Sakamoto <o-takashi@sakamocchi.jp> - 2016-12-05 11:00 +0100

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

From: Daniel Girnus <dgirnus@de.adit-jv.com>

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.
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@de.adit-jv.com>
Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 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;
-- 
2.9.3

[toc] | [next] | [standalone]

#1533076

On Wed, 30 Nov 2016 08:59:22 +0100,
Jiada Wang wrote:
> 
> From: Daniel Girnus <dgirnus@de.adit-jv.com>
> 
> 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.
> 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@de.adit-jv.com>
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>

The sign-off from Daniel seems missing?

The code change looks OK, but it'd be nice to mention in the changelog
that, after this patch, snd_usb_init_sample_rate() is still called
properly whenever the parameter is changed since ep->need_setup_ep is
set in snd_hsb_hw_params().


thanks,

Takashi

> ---
>  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;
> -- 
> 2.9.3
> 
> 

[toc] | [prev] | [next] | [standalone]

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

Hi Takashi

On 11/30/2016 05:51 PM, Takashi Iwai wrote:
> On Wed, 30 Nov 2016 08:59:22 +0100,
> Jiada Wang wrote:
>>
>> From: Daniel Girnus <dgirnus@de.adit-jv.com>
>>
>> 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.
>> 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@de.adit-jv.com>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>
> The sign-off from Daniel seems missing?
>
> The code change looks OK, but it'd be nice to mention in the changelog
> that, after this patch, snd_usb_init_sample_rate() is still called
> properly whenever the parameter is changed since ep->need_setup_ep is
> set in snd_hsb_hw_params().
>
I will add missing sign-off and related information in changelog in v2

Thanks,
Jiada
>
> thanks,
>
> Takashi
>
>> ---
>>  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;
>> --
>> 2.9.3
>>
>>

[toc] | [prev] | [next] | [standalone]

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

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@de.adit-jv.com>
>
> 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.

> 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@de.adit-jv.com>
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

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

On Nov 30 2016 19:45, 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@de.adit-jv.com>
>>
>> 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.

So here you should mention that current USB Audio device class driver 
somewhat ignores state machine of ALSA PCM runtime.

In ALSA PCM core, state of the runtime is described in 'struct 
snd_pcm_runtime.status.state' with macros of 'SNDRV_PCM_STATE_XXX'. 
Applications are allowed to handle the runtime according to the state.

In your issue, the driver is programmed ignoring a case that double 
calls of snd_pcm_prepare(), in short, ioctl(PREPARE) is called in 
'PREPARED' state. This is not only an issue for snd-usb-audio, but also 
for snd-usb-hiface.
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-November/115174.html

For these issue, I have no patch proposals because I have few test 
devices, sorry.


Regards

Takashi Sakamoto

[toc] | [prev] | [next] | [standalone]

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

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@de.adit-jv.com>
>>
>> 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@de.adit-jv.com>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>>  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

[toc] | [prev] | [next] | [standalone]

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

On Dec 5 2016 16:32, Jiada Wang wrote:
> 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@de.adit-jv.com>
>>>
>>> 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,

Exactly. Furthermore, ALSA PCM core has no code to call .prepare() in 
contexts unrelated to SNDRV_PCM_IOCTL_PREPARE.

> some devices are unable to manage this and stop working.
> I will update Changelog in v2 Patchset.


Regards

Takashi Sakamoto

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web