Re: [PATCH v2] Pioneer devices: engage implicit feedback sync for playback

[Takashi Iwai <tiwai@xxxxxxx>]

On Sun, 18 Apr 2021 00:24:21 +0200,
Olivia Mackintosh wrote:
> 
> > Which kernel version have you tested?  There have been quite a few
> > development about USB-audio recently, so something might be missing or
> > conflicting?  Let's see.
> 
> I have just tested d86f43b17ed4cd751f73d890ea63f818ffa5ef3d with and
> without the patch:
> 
>   - Without the patch, everything works fine.
>   - With the patch, speaker-test times out. I'll try to collect some more
>     infomation from dyndb and try to have a look to see what the problem
>     is.
> 
> There's no obvious mistakes in the patch as far as I can tell.

Thanks for testing.  A possible difference of speaker-test from others
is that speaker-test tends to set up with the larger period and buffer
sizes.  I guess the same problem could be seen when you run aplay with
the same parameters shown in /proc/asound/card*/pcm0p/sub0/hw_params,
too.

As a blind shot: might the stall be avoided by passing the recently
introduced chip->playback_first flag?  The revised patch is below.


Takashi

---
--- a/sound/usb/implicit.c
+++ b/sound/usb/implicit.c
@@ -230,18 +230,27 @@ static int add_roland_implicit_fb(struct snd_usb_audio *chip,
 				       ifnum, alts);
 }
 
-/* Playback and capture EPs on Pioneer devices share the same iface/altset,
- * but they don't seem working with the implicit fb mode well, hence we
- * just return as if the sync were already set up.
+/* Playback and capture EPs on Pioneer devices share the same iface/altset
+ * for the implicit feedback operation
  */
-static int skip_pioneer_sync_ep(struct snd_usb_audio *chip,
-				struct audioformat *fmt,
-				struct usb_host_interface *alts)
+static bool is_pioneer_implicit_fb(struct snd_usb_audio *chip,
+				   struct usb_host_interface *alts)
+
 {
 	struct usb_endpoint_descriptor *epd;
 
+	if (USB_ID_VENDOR(chip->usb_id) != 0x2b73 &&
+	    USB_ID_VENDOR(chip->usb_id) != 0x08e4)
+		return false;
+	if (alts->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC)
+		return false;
 	if (alts->desc.bNumEndpoints != 2)
-		return 0;
+		return false;
+
+	epd = get_endpoint(alts, 0);
+	if (!usb_endpoint_is_isoc_out(epd) ||
+	    (epd->bmAttributes & USB_ENDPOINT_SYNCTYPE) != USB_ENDPOINT_SYNC_ASYNC)
+		return false;
 
 	epd = get_endpoint(alts, 1);
 	if (!usb_endpoint_is_isoc_in(epd) ||
@@ -250,8 +259,9 @@ static int skip_pioneer_sync_ep(struct snd_usb_audio *chip,
 	     USB_ENDPOINT_USAGE_DATA &&
 	     (epd->bmAttributes & USB_ENDPOINT_USAGE_MASK) !=
 	     USB_ENDPOINT_USAGE_IMPLICIT_FB))
-		return 0;
-	return 1; /* don't handle with the implicit fb, just skip sync EP */
+		return false;
+
+	return true;
 }
 
 static int __add_generic_implicit_fb(struct snd_usb_audio *chip,
@@ -367,12 +377,12 @@ static int audioformat_implicit_fb_quirk(struct snd_usb_audio *chip,
 	}
 
 	/* Pioneer devices with vendor spec class */
-	if (attr == USB_ENDPOINT_SYNC_ASYNC &&
-	    alts->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
-	    (USB_ID_VENDOR(chip->usb_id) == 0x2b73 || /* Pioneer */
-	     USB_ID_VENDOR(chip->usb_id) == 0x08e4    /* Pioneer */)) {
-		if (skip_pioneer_sync_ep(chip, fmt, alts))
-			return 1;
+	if (is_pioneer_implicit_fb(chip, alts)) {
+		chip->playback_first = 1;
+		return add_implicit_fb_sync_ep(chip, fmt,
+					       get_endpoint(alts, 1)->bEndpointAddress,
+					       1, alts->desc.bInterfaceNumber,
+					       alts);
 	}
 
 	/* Try the generic implicit fb if available */
@@ -394,6 +404,8 @@ static int audioformat_capture_quirk(struct snd_usb_audio *chip,
 	if (p && (p->type == IMPLICIT_FB_FIXED || p->type == IMPLICIT_FB_BOTH))
 		return add_implicit_fb_sync_ep(chip, fmt, p->ep_num, 0,
 					       p->iface, NULL);
+	if (is_pioneer_implicit_fb(chip, alts))
+		return 1; /* skip */
 	return 0;
 }