How to use implicit feedback with full duplex?

How to use implicit feedback with full duplex?

[Clemens Ladisch]

Hi,

I thought I'd try to use implicit feedback with my simple audio device:

    Interface Descriptor:
      bInterfaceNumber        1
      bAlternateSetting       0
    Interface Descriptor:
      bInterfaceNumber        1
      bAlternateSetting       1
      AudioStreaming Interface Descriptor:
        ...
      Endpoint Descriptor:
        bEndpointAddress     0x01  EP 1 OUT
    Interface Descriptor:
      bInterfaceNumber        2
      bAlternateSetting       0
    Interface Descriptor:
      bInterfaceNumber        2
      bAlternateSetting       1
      AudioStreaming Interface Descriptor:
        ...
      Endpoint Descriptor:
        bEndpointAddress     0x82  EP 2 IN

So I wrote a patch to configure it like those M-Audio devices:

--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -391,6 +391,17 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 	attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;

 	switch (subs->stream->chip->usb_id) {
+	case USB_ID(0x0582, 0x0018):
+		if (is_playback) {
+			implicit_fb = 1;
+			ep = 0x82;
+			iface = usb_ifnum_to_if(dev, 2);
+			if (!iface || iface->num_altsetting < 2)
+				return -EINVAL;
+			alts = &iface->altsetting[1];
+			goto add_sync_ep;
+		}
+		break;
 	case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
 		if (is_playback) {
 			implicit_fb = 1;

This works fine when playing something:

  kernel: setting usb interface 1:1
  kernel: Creating new playback data endpoint #1
  kernel: Creating new capture data endpoint #82
  kernel: Setting params for ep #1 (type 0, 8 urbs), ret=0
  kernel: match_endpoint_audioformats: (fmt @ffff8801fefa8900) score 2
  kernel: Setting params for ep #82 (type 0, 8 urbs), ret=0
  kernel: Starting data EP @ffff8801fd4e8000
  kernel: Starting sync EP @ffff8801fd2ac000

But when I then try to record at the same time, the driver refuses to
configure the input endpoint (to the only format, which is already set):

  kernel: setting usb interface 2:1
  kernel: Re-using EP 82 in iface 2,1 @ffff8801fd2ac000
  kernel: Unable to change format on ep #82: already in use

And despite that "alreay in use" check, the input endpoint is affected
so much that playback breaks.

Is full duplex supposed to work?  Does it work with other devices?


Regards,
Clemens

Re: How to use implicit feedback with full duplex?

[Eldad Zack]

Hi Clemens,

On Tue, 5 Feb 2013, Clemens Ladisch wrote:
> Hi,
> 
> I thought I'd try to use implicit feedback with my simple audio device:
> 
>     Interface Descriptor:
>       bInterfaceNumber        1
>       bAlternateSetting       0
>     Interface Descriptor:
>       bInterfaceNumber        1
>       bAlternateSetting       1
>       AudioStreaming Interface Descriptor:
>         ...
>       Endpoint Descriptor:
>         bEndpointAddress     0x01  EP 1 OUT
>     Interface Descriptor:
>       bInterfaceNumber        2
>       bAlternateSetting       0
>     Interface Descriptor:
>       bInterfaceNumber        2
>       bAlternateSetting       1
>       AudioStreaming Interface Descriptor:
>         ...
>       Endpoint Descriptor:
>         bEndpointAddress     0x82  EP 2 IN
>
> So I wrote a patch to configure it like those M-Audio devices:
> 
> --- a/sound/usb/pcm.c
> +++ b/sound/usb/pcm.c
> @@ -391,6 +391,17 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
>  	attr = fmt->ep_attr & USB_ENDPOINT_SYNCTYPE;
> 
>  	switch (subs->stream->chip->usb_id) {
> +	case USB_ID(0x0582, 0x0018):
> +		if (is_playback) {
> +			implicit_fb = 1;
> +			ep = 0x82;
> +			iface = usb_ifnum_to_if(dev, 2);
> +			if (!iface || iface->num_altsetting < 2)
> +				return -EINVAL;
> +			alts = &iface->altsetting[1];
> +			goto add_sync_ep;
> +		}
> +		break;

Looks good to me. 

>  	case USB_ID(0x0763, 0x2030): /* M-Audio Fast Track C400 */
>  		if (is_playback) {
>  			implicit_fb = 1;
> 
> This works fine when playing something:
...
> But when I then try to record at the same time, the driver refuses to
> configure the input endpoint (to the only format, which is already set):
> 
>   kernel: setting usb interface 2:1
>   kernel: Re-using EP 82 in iface 2,1 @ffff8801fd2ac000
>   kernel: Unable to change format on ep #82: already in use
> 
> And despite that "alreay in use" check, the input endpoint is affected
> so much that playback breaks.
> 
> Is full duplex supposed to work?  Does it work with other devices?

This is probably a "yes, but" :)
I use my device mostly full duplex, but with jack opening both 
playback and capture at the same time.

I assume you are opening two different streams, one for playback and
one for capture. In that case, the capture EP is of course already
in use, as a source for timing.
If I try opening 2 different streams I get the same here - but playback 
doesn't break for me. I wonder why that happens.

Can you try using jackd -d alsa -d hw:x with the device and see if that 
works for you?

Cheers,
Eldad

Re: How to use implicit feedback with full duplex?

[Clemens Ladisch]

Eldad Zack wrote:
> On Tue, 5 Feb 2013, Clemens Ladisch wrote:
>> I thought I'd try to use implicit feedback with my simple audio device:
>> [...]
>> This works fine when playing something:
> ...
>> But when I then try to record at the same time, the driver refuses to
>> configure the input endpoint (to the only format, which is already set):
>> And despite that "alreay in use" check, the input endpoint is affected
>> so much that playback breaks.
>>
>> Is full duplex supposed to work?  Does it work with other devices?
>
> This is probably a "yes, but" :)
> I use my device mostly full duplex, but with jack opening both
> playback and capture at the same time.
>
> I assume you are opening two different streams, one for playback and
> one for capture.

Jack *also* uses two different streams, but it opens them at the same
time.

> Can you try using jackd -d alsa -d hw:x with the device and see if that
> works for you?

That works.  This means that there is a race condition in the driver, or
that the different open/hw_params/prepare order trips it up.


Anyway, I feel safe now to enable implicit feedback for other devices.


Regards,
Clemens

Re: How to use implicit feedback with full duplex?

[Daniel Mack]

On 06.02.2013 21:13, Clemens Ladisch wrote:
> Eldad Zack wrote:
>> On Tue, 5 Feb 2013, Clemens Ladisch wrote:
>>> I thought I'd try to use implicit feedback with my simple audio device:
>>> [...]
>>> This works fine when playing something:
>> ...
>>> But when I then try to record at the same time, the driver refuses to
>>> configure the input endpoint (to the only format, which is already set):
>>> And despite that "alreay in use" check, the input endpoint is affected
>>> so much that playback breaks.
>>>
>>> Is full duplex supposed to work?  Does it work with other devices?
>>
>> This is probably a "yes, but" :)
>> I use my device mostly full duplex, but with jack opening both
>> playback and capture at the same time.
>>
>> I assume you are opening two different streams, one for playback and
>> one for capture.
> 
> Jack *also* uses two different streams, but it opens them at the same
> time.
> 
>> Can you try using jackd -d alsa -d hw:x with the device and see if that
>> works for you?
> 
> That works.  This means that there is a race condition in the driver, or
> that the different open/hw_params/prepare order trips it up.

I'll look for the FTU and re-try this with Eldad's latest additions once
I find some time. If there are races, we should definitely fix them.

> Anyway, I feel safe now to enable implicit feedback for other devices.

Nice.

On a general note, my idea from the beginning and part of the motivation
to split the driver into the snd_usb_endpoint logic was to pull that
class out to a separate library, so we can use it from other drivers as
well. Much like what snd-usbmidi does.

There are few bits to clean up in order to remove dependencies between
the endpoint code and the rest of the driver, but then your ua101
driver, snd-usb-caiaq and probably some more drivers in the future can
share the generic streaming functionality.


Daniel

Re: How to use implicit feedback with full duplex?

[Felix Homann]

Hi Daniel, Hi Eldad.

2013/2/6 Daniel Mack <zonque@gmail.com>
>
> I'll look for the FTU and re-try this with Eldad's latest additions once
> I find some time.
>
>
I'll probably find some time later next week. I haven't followed thoroughly
what Eldad did for the C400/C600 but wanted to play around with the FTU
anyway (if I remember correctly there was a patch dropping the first n
implicit feedback packets on the C400 that might possibly be of benefit for
the FTUs as well).

@Eldad Have your latest additions already made it into Takashi's tree or
where can I find them?

Re: How to use implicit feedback with full duplex?

[Takashi Iwai]

At Thu, 7 Feb 2013 10:03:20 +0100,
Felix Homann wrote:
> 
> Hi Daniel, Hi Eldad.
> 
> 2013/2/6 Daniel Mack <zonque@gmail.com>
> >
> > I'll look for the FTU and re-try this with Eldad's latest additions once
> > I find some time.
> >
> >
> I'll probably find some time later next week. I haven't followed thoroughly
> what Eldad did for the C400/C600 but wanted to play around with the FTU
> anyway (if I remember correctly there was a patch dropping the first n
> implicit feedback packets on the C400 that might possibly be of benefit for
> the FTUs as well).
> 
> @Eldad Have your latest additions already made it into Takashi's tree or
> where can I find them?

It should be all in my tree.
If not, let me know.  The merge window for 3.9 is pretty close.


thanks,

Takashi 

Re: How to use implicit feedback with full duplex?

[Eldad Zack]

On Wed, 6 Feb 2013, Clemens Ladisch wrote:

> Eldad Zack wrote:
> > On Tue, 5 Feb 2013, Clemens Ladisch wrote:
> >> I thought I'd try to use implicit feedback with my simple audio device:
> >> [...]
> >> This works fine when playing something:
> > ...
> >> But when I then try to record at the same time, the driver refuses to
> >> configure the input endpoint (to the only format, which is already set):
> >> And despite that "alreay in use" check, the input endpoint is affected
> >> so much that playback breaks.
> >>
> >> Is full duplex supposed to work?  Does it work with other devices?
> >
> > This is probably a "yes, but" :)
> > I use my device mostly full duplex, but with jack opening both
> > playback and capture at the same time.
> >
> > I assume you are opening two different streams, one for playback and
> > one for capture.
> 
> Jack *also* uses two different streams, but it opens them at the same
> time.

Oh, right. 

> 
> > Can you try using jackd -d alsa -d hw:x with the device and see if that
> > works for you?
> 
> That works.  This means that there is a race condition in the driver, or
> that the different open/hw_params/prepare order trips it up.

Hmm. I seem to have tested this wrong the other day or just had luck (I 
tried it just once).
But today I can trigger breaking the playback when opening capture 
later every time I try, just like you described.

I get this too when the audio breaks:
[  766.987497] xhci_hcd 0000:00:14.0: shutdown urb ffff880244f89600 ep1in-iso
[  766.987510] xhci_hcd 0000:00:14.0: shutdown urb ffff880244f89400 ep1in-iso
[  766.987519] xhci_hcd 0000:00:14.0: shutdown urb ffff8802438e6000 ep1in-iso
[  766.987527] xhci_hcd 0000:00:14.0: shutdown urb ffff8802438e6200 ep1in-iso
[  766.987534] xhci_hcd 0000:00:14.0: WARN Event TRB for slot 1 ep 2  with no TDs queued?
[  766.987550] xhci_hcd 0000:00:14.0: shutdown urb ffff8802438e6400 ep1in-iso
[  766.987562] xhci_hcd 0000:00:14.0: shutdown urb ffff880244f89000 ep1in-iso
[  766.987576] xhci_hcd 0000:00:14.0: shutdown urb ffff880244f89200 ep1in-iso
[  766.987584] xhci_hcd 0000:00:14.0: shutdown urb ffff880244f88800 ep1in-iso

I will try to figure out what's happening on the weekend.

Cheers,
Eldad

Re: How to use implicit feedback with full duplex?

[Eldad Zack]

Hi Felix,

On Thu, 7 Feb 2013, Felix Homann wrote:

> Hi Daniel, Hi Eldad.
> 
> 2013/2/6 Daniel Mack <zonque@gmail.com>
>       I'll look for the FTU and re-try this with Eldad's latest additions once
>       I find some time.
> 
> 
> I'll probably find some time later next week. I haven't followed thoroughly what Eldad did for the C400/C600 but wanted to play around with the FTU anyway (if I
> remember correctly there was a patch dropping the first n implicit feedback packets on the C400 that might possibly be of benefit for the FTUs as well).

I only added C400 - but I hope the asymmetry-related patches will save 
time with the C600.

Do you get different latency every time you start up a stream on the 
FTU? That's the reason for the skip_packet quirk for the C400.

> @Eldad Have your latest additions already made it into Takashi's tree or where can I find them?

Yes and they are all merged upstream by now. But they are mostly 
small things and specific to the C400.

Cheers,
Eldad

Re: How to use implicit feedback with full duplex?

[Eldad Zack]

On Thu, 7 Feb 2013, Takashi Iwai wrote:

> At Thu, 7 Feb 2013 10:03:20 +0100,
> Felix Homann wrote:
> > 
> > Hi Daniel, Hi Eldad.
> > 
> > 2013/2/6 Daniel Mack <zonque@gmail.com>
> > >
> > > I'll look for the FTU and re-try this with Eldad's latest additions once
> > > I find some time.
> > >
> > >
> > I'll probably find some time later next week. I haven't followed thoroughly
> > what Eldad did for the C400/C600 but wanted to play around with the FTU
> > anyway (if I remember correctly there was a patch dropping the first n
> > implicit feedback packets on the C400 that might possibly be of benefit for
> > the FTUs as well).
> > 
> > @Eldad Have your latest additions already made it into Takashi's tree or
> > where can I find them?
> 
> It should be all in my tree.
> If not, let me know.  The merge window for 3.9 is pretty close.

Thanks for the heads up! I do have some general addition to do automatic 
clock switching in case the clock source currently selected is not valid,
but it's not ready for submission. I just didn't have time to put it
into good shape yet.

Cheers,
Eldad

Re: How to use implicit feedback with full duplex?

[Eldad Zack]

Hi Clemens,

On Wed, 6 Feb 2013, Clemens Ladisch wrote:

> Eldad Zack wrote:
> > On Tue, 5 Feb 2013, Clemens Ladisch wrote:
> >> I thought I'd try to use implicit feedback with my simple audio device:
> >> [...]
> >> This works fine when playing something:
> > ...
> >> But when I then try to record at the same time, the driver refuses to
> >> configure the input endpoint (to the only format, which is already set):
> >> And despite that "alreay in use" check, the input endpoint is affected
> >> so much that playback breaks.
> >>
> >> Is full duplex supposed to work?  Does it work with other devices?
> >
> > This is probably a "yes, but" :)
> > I use my device mostly full duplex, but with jack opening both
> > playback and capture at the same time.
> >
> > I assume you are opening two different streams, one for playback and
> > one for capture.
> 
> Jack *also* uses two different streams, but it opens them at the same
> time.
> 
> > Can you try using jackd -d alsa -d hw:x with the device and see if that
> > works for you?
> 
> That works.  This means that there is a race condition in the driver, or
> that the different open/hw_params/prepare order trips it up.

Yes, and also on closing (close/hw_free).
I finally had some time to look into this - patch below adds some 
checks and with it:
    
* Starting playback, waiting, starting capture: capture doesn't start,
  playback continues without breaking.
    
* Starting capture, waiting, starting playback: playback doesn't start,
  capture breaks - but it is possible to restart the streams afterwards.
  [On my setup (with xhci), when the streams break I must restart my box
  to get them to work again with current code].

* Starting both - no change, works good.

This is the order the ops get called when both are start at once, if it 
helps anyone:

playback	capture
open
		open
		hw_params
		 set_format
 		prepare
 		 set_format
hw_params
 set_format
prepare
 set_format
prepare
 set_format
		prepare
		 set_format
 trigger
		trigger

I'll try to figure out why capture breaks next.
I still quite slow around the code and don't understand some parts of 
it so this might take me a while.

@Daniel, do you have any hints for this case (capture breaking
when starting playback)?

I can also submit this as a proper patch if it makes sense.

Cheers,
Eldad
    
diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c
index 7e9c55a..7cdf9b3 100644
--- a/sound/usb/endpoint.c
+++ b/sound/usb/endpoint.c
@@ -934,12 +934,12 @@ int snd_usb_endpoint_deactivate(struct snd_usb_endpoint *ep)
 	if (!ep)
 		return -EINVAL;
 
-	deactivate_urbs(ep, true);
-	wait_clear_urbs(ep);
-
 	if (ep->use_count != 0)
 		return 0;
 
+	deactivate_urbs(ep, true);
+	wait_clear_urbs(ep);
+
 	clear_bit(EP_FLAG_ACTIVATED, &ep->flags);
 
 	return 0;
diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c
index 9531355..76d7e18 100644
--- a/sound/usb/pcm.c
+++ b/sound/usb/pcm.c
@@ -323,6 +323,19 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 	if (fmt == subs->cur_audiofmt)
 		return 0;
 
+	subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip,
+						   alts, fmt->endpoint, subs->direction,
+						   SND_USB_ENDPOINT_TYPE_DATA);
+	if (!subs->data_endpoint)
+		return -EINVAL;
+
+	if (subs->data_endpoint->use_count != 0) {
+		snd_printk("set_format ep use count !0\n");
+		subs->cur_audiofmt = fmt;
+		snd_usb_set_format_quirk(subs, fmt);
+		return 0;
+	}
+
 	/* close the old interface */
 	if (subs->interface >= 0 && subs->interface != fmt->iface) {
 		err = usb_set_interface(subs->dev, subs->interface, 0);
@@ -350,12 +363,6 @@ static int set_format(struct snd_usb_substream *subs, struct audioformat *fmt)
 		subs->altset_idx = fmt->altset_idx;
 	}
 
-	subs->data_endpoint = snd_usb_add_endpoint(subs->stream->chip,
-						   alts, fmt->endpoint, subs->direction,
-						   SND_USB_ENDPOINT_TYPE_DATA);
-	if (!subs->data_endpoint)
-		return -EINVAL;
-
 	/* we need a sync pipe in async OUT or adaptive IN mode */
 	/* check the number of EP, since some devices have broken
 	 * descriptors which fool us.  if it has only one EP,
@@ -1128,7 +1135,8 @@ static int snd_usb_pcm_close(struct snd_pcm_substream *substream, int direction)
 
 	stop_endpoints(subs, true);
 
-	if (!as->chip->shutdown && subs->interface >= 0) {
+	if (!as->chip->shutdown && subs->interface >= 0 &&
+	    subs->data_endpoint->use_count == 0) {
 		usb_set_interface(subs->dev, subs->interface, 0);
 		subs->interface = -1;
 	}
 

Re: How to use implicit feedback with full duplex?

[Daniel Mack]

On 24.03.2013 23:49, Eldad Zack wrote:
> 
> Hi Clemens,
> 
> On Wed, 6 Feb 2013, Clemens Ladisch wrote:
> 
>> Eldad Zack wrote:
>>> On Tue, 5 Feb 2013, Clemens Ladisch wrote:
>>>> I thought I'd try to use implicit feedback with my simple audio device:
>>>> [...]
>>>> This works fine when playing something:
>>> ...
>>>> But when I then try to record at the same time, the driver refuses to
>>>> configure the input endpoint (to the only format, which is already set):
>>>> And despite that "alreay in use" check, the input endpoint is affected
>>>> so much that playback breaks.
>>>>
>>>> Is full duplex supposed to work?  Does it work with other devices?
>>>
>>> This is probably a "yes, but" :)
>>> I use my device mostly full duplex, but with jack opening both
>>> playback and capture at the same time.
>>>
>>> I assume you are opening two different streams, one for playback and
>>> one for capture.
>>
>> Jack *also* uses two different streams, but it opens them at the same
>> time.
>>
>>> Can you try using jackd -d alsa -d hw:x with the device and see if that
>>> works for you?
>>
>> That works.  This means that there is a race condition in the driver, or
>> that the different open/hw_params/prepare order trips it up.
> 
> Yes, and also on closing (close/hw_free).
> I finally had some time to look into this - patch below adds some 
> checks and with it:
>     
> * Starting playback, waiting, starting capture: capture doesn't start,
>   playback continues without breaking.
>     
> * Starting capture, waiting, starting playback: playback doesn't start,
>   capture breaks - but it is possible to restart the streams afterwards.
>   [On my setup (with xhci), when the streams break I must restart my box
>   to get them to work again with current code].
> 
> * Starting both - no change, works good.
> 
> This is the order the ops get called when both are start at once, if it 
> helps anyone:
> 
> playback	capture
> open
> 		open
> 		hw_params
> 		 set_format
>  		prepare
>  		 set_format
> hw_params
>  set_format
> prepare
>  set_format
> prepare
>  set_format
> 		prepare
> 		 set_format
>  trigger
> 		trigger
> 
> I'll try to figure out why capture breaks next.
> I still quite slow around the code and don't understand some parts of 
> it so this might take me a while.

Thanks for looking into this!

> @Daniel, do you have any hints for this case (capture breaking
> when starting playback)?

Give me some time to catch up and rebuild my FTU setup please. I hope
I'll be able to get back to you by the end of this week.


Daniel

Re: How to use implicit feedback with full duplex?

[Eldad Zack]

On Tue, 26 Mar 2013, Daniel Mack wrote:

> On 24.03.2013 23:49, Eldad Zack wrote:
> > 
> > On Wed, 6 Feb 2013, Clemens Ladisch wrote:
> > 
> >>> Can you try using jackd -d alsa -d hw:x with the device and see if that
> >>> works for you?
> >>
> >> That works.  This means that there is a race condition in the driver, or
> >> that the different open/hw_params/prepare order trips it up.
> > 
> > Yes, and also on closing (close/hw_free).
> > I finally had some time to look into this - patch below adds some 
> > checks and with it:
> >     
> > * Starting playback, waiting, starting capture: capture doesn't start,
> >   playback continues without breaking.
> >     
> > * Starting capture, waiting, starting playback: playback doesn't start,
> >   capture breaks - but it is possible to restart the streams afterwards.
> >   [On my setup (with xhci), when the streams break I must restart my box
> >   to get them to work again with current code].
> > 
> > * Starting both - no change, works good.
> > 
> > This is the order the ops get called when both are start at once, if it 
> > helps anyone:
> > 
> > playback	capture
> > open
> > 		open
> > 		hw_params
> > 		 set_format
> >  		prepare
> >  		 set_format
> > hw_params
> >  set_format
> > prepare
> >  set_format
> > prepare
> >  set_format
> > 		prepare
> > 		 set_format
> >  trigger
> > 		trigger
> > 
> > I'll try to figure out why capture breaks next.
> > I still quite slow around the code and don't understand some parts of 
> > it so this might take me a while.
> 
> Thanks for looking into this!
> 
> > @Daniel, do you have any hints for this case (capture breaking
> > when starting playback)?
> 
> Give me some time to catch up and rebuild my FTU setup please. I hope
> I'll be able to get back to you by the end of this week.

Cool, thank you!

And it just remembered something. I tried to make the "in use"
check non-fatal (return 0) and with a couple of more changes
(very hacky) I got it to start in both orders.

But the problem was that when I stopped the playback* stream my
kernel "exploded" (hard lock up). But if I stopped the capture,
it was ok. Playback didn't even stop.
(It might have been the other way around, but I think it was 
playback)

I can find that branch when I find some time if you think it
might help.

Cheers,
Eldad