Re: How to use implicit feedback with full duplex?

[Eldad Zack <eldad@fogrefinery.com>]

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