Re: [PATCH] ALSA: usb-audio: Apply implicit feedback mode for BOSS devices

[Takashi Iwai <tiwai@suse.de>]

On Wed, 21 Apr 2021 06:59:51 +0200,
Lucas wrote:
> 
> First, thanks very much for trying to cover all devices through detection!  I
> had hoped something like this could be done, but sadly, it has created a mixed
> result:
> 
> Roland VG-99 Perfect!:
> arecord -D hw:VG99 -f S24_3LE -r 44100 -c 2 ./file.wav
> Recording WAVE './file.wav' : Signed 24 bit Little Endian in 3bytes, Rate
> 44100 Hz, Stereo
> ^CAborted by signal Interrupt...
> 
> aplay -D hw:VG99 -f S24_3LE -r 44100 -c 2 ./file.wav
> Playing WAVE './file.wav' : Signed 24 bit Little Endian in 3bytes, Rate 44100
> Hz, Stereo
> 
> Roland INTEGRA-7 doesn't capture, but plays perfectly (only 96 kHz mode
> tested):
> arecord -D hw:INTEGRA7 -f S32_LE -r 96000 -c 2 ./file.wav
> Recording WAVE './file.wav' : Signed 32 bit Little Endian, Rate 96000 Hz,
> Stereo
> arecord: pcm_read:2153: read error: Input/output error
> 
> aplay -D hw:INTEGRA7 -f S32_LE -r 96000 -c 2 ./other-file.wav
> Playing WAVE './other-file.wav' : Signed 32 bit Little Endian, Rate 96000 Hz,
> Stereo

Hm, that's bad.  And INTEGRA7 did work before the patching?
I saw that this has the right EP numbers (0x0d and 0x8e), so the
static quirk entry was correct.

> 
> Roland R-26 doesn't capture, but plays perfectly (only 96 kHz mode tested):

R-26 seems also to be the one with both ASYNC and EP 0x0d/0x8e, so the
same reason as INTEGRA7.

> arecord -D hw:R26AUDIO -f S32_LE -r 96000 -c 2 ./file.wav
> Recording WAVE './file.wav' : Signed 32 bit Little Endian, Rate 96000 Hz,
> Stereo
> arecord: pcm_read:2153: read error: Input/output error
> 
> aplay -D hw:R26AUDIO -f S32_LE -r 96000 -c 2 ./other-file.wav
> Playing WAVE './other-file.wav' : Signed 32 bit Little Endian, Rate 96000 Hz,
> Stereo
> 
> Roland Boutique D-05 doesn't capture, but plays perfectly:

OK, this one looks slightly different.
D-05 has two EPs in each interface, both are ASYNC and 0x0d/0x8e.
So another adjustment might be needed here.

> EDIROL UA-101 full-speed (USB 1.1) and high-speed (USB 2.0) not detected for
> capture or playback (only 48 kHz mode tested):

This is a different driver, so we can forget for now.

> I'm guessing another look at "lsusb -v" would help, but don't know what to
> look for and have run out of time tonight.

Could you post the archive of your lsusb -v outputs?  Put in a tarball
and post with attachment.

Below is a revised patch.  Let me know if this works better.


thanks,

Takashi


--- a/sound/usb/implicit.c
+++ b/sound/usb/implicit.c
@@ -79,6 +79,7 @@ static const struct snd_usb_implicit_fb_match playback_implicit_fb_quirks[] = {
 
 /* Implicit feedback quirk table for capture: only FIXED type */
 static const struct snd_usb_implicit_fb_match capture_implicit_fb_quirks[] = {
+#if 0
 	IMPLICIT_FB_FIXED_DEV(0x0582, 0x00a6, 0x0d, 0x01), /* Roland JUNO-G */
 	IMPLICIT_FB_FIXED_DEV(0x0582, 0x00a9, 0x0d, 0x01), /* Roland MC-808 */
 	IMPLICIT_FB_FIXED_DEV(0x0582, 0x00ad, 0x0d, 0x01), /* Roland SH-201 */
@@ -146,6 +147,7 @@ static const struct snd_usb_implicit_fb_match capture_implicit_fb_quirks[] = {
 	IMPLICIT_FB_BOTH_DEV(0x0582, 0x01fd, 0x0d, 0x01), /* Roland Boutique SH-01A */
 	IMPLICIT_FB_BOTH_DEV(0x0582, 0x01ff, 0x0d, 0x01), /* Roland Boutique D-05 */
 	IMPLICIT_FB_BOTH_DEV(0x0582, 0x0203, 0x0d, 0x01), /* BOSS AD-10 */
+#endif
 
 	{} /* terminator */
 };
@@ -204,30 +206,70 @@ static int add_generic_uac2_implicit_fb(struct snd_usb_audio *chip,
 				       ifnum, alts);
 }
 
-/* Like the function above, but specific to Roland with vendor class and hack */
+static bool roland_sanity_check_iface(struct usb_host_interface *alts)
+{
+	if (alts->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
+	    (alts->desc.bInterfaceSubClass != 2 &&
+	     alts->desc.bInterfaceProtocol != 2) ||
+	    alts->desc.bNumEndpoints < 1)
+		return false;
+	return true;
+}
+
+/* Like the UAC2 case above, but specific to Roland with vendor class and hack */
 static int add_roland_implicit_fb(struct snd_usb_audio *chip,
 				  struct audioformat *fmt,
-				  unsigned int ifnum,
-				  unsigned int altsetting)
+				  struct usb_host_interface *alts)
 {
-	struct usb_host_interface *alts;
 	struct usb_endpoint_descriptor *epd;
 
-	alts = snd_usb_get_host_interface(chip, ifnum, altsetting);
-	if (!alts)
+	if (!roland_sanity_check_iface(alts))
 		return 0;
-	if (alts->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
-	    (alts->desc.bInterfaceSubClass != 2 &&
-	     alts->desc.bInterfaceProtocol != 2) ||
-	    alts->desc.bNumEndpoints < 1)
+	/* only when both streams are with ASYNC type */
+	epd = get_endpoint(alts, 0);
+	if (!usb_endpoint_is_isoc_out(epd) ||
+	    (epd->bmAttributes & USB_ENDPOINT_SYNCTYPE) != USB_ENDPOINT_SYNC_ASYNC)
+		return 0;
+
+	/* check capture EP */
+	alts = snd_usb_get_host_interface(chip,
+					  alts->desc.bInterfaceNumber + 1,
+					  alts->desc.bAlternateSetting);
+	if (!alts || !roland_sanity_check_iface(alts))
 		return 0;
 	epd = get_endpoint(alts, 0);
 	if (!usb_endpoint_is_isoc_in(epd) ||
-	    (epd->bmAttributes & USB_ENDPOINT_USAGE_MASK) !=
-					USB_ENDPOINT_USAGE_IMPLICIT_FB)
+	    (epd->bmAttributes & USB_ENDPOINT_SYNCTYPE) != USB_ENDPOINT_SYNC_ASYNC)
 		return 0;
+	chip->playback_first = 1;
 	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress, 0,
-				       ifnum, alts);
+				       alts->desc.bInterfaceNumber, alts);
+}
+
+/* capture quirk for Roland device; always full-duplex */
+static int add_roland_capture_quirk(struct snd_usb_audio *chip,
+				    struct audioformat *fmt,
+				    struct usb_host_interface *alts)
+{
+	struct usb_endpoint_descriptor *epd;
+
+	if (!roland_sanity_check_iface(alts))
+		return 0;
+	epd = get_endpoint(alts, 0);
+	if (!usb_endpoint_is_isoc_in(epd) ||
+	    (epd->bmAttributes & USB_ENDPOINT_SYNCTYPE) != USB_ENDPOINT_SYNC_ASYNC)
+		return 0;
+
+	alts = snd_usb_get_host_interface(chip,
+					  alts->desc.bInterfaceNumber - 1,
+					  alts->desc.bAlternateSetting);
+	if (!alts || !roland_sanity_check_iface(alts))
+		return 0;
+	epd = get_endpoint(alts, 0);
+	if (!usb_endpoint_is_isoc_out(epd))
+		return 0;
+	return add_implicit_fb_sync_ep(chip, fmt, epd->bEndpointAddress, 0,
+				       alts->desc.bInterfaceNumber, alts);
 }
 
 /* Playback and capture EPs on Pioneer devices share the same iface/altset
@@ -365,14 +407,8 @@ static int audioformat_implicit_fb_quirk(struct snd_usb_audio *chip,
 	}
 
 	/* Roland/BOSS implicit feedback with vendor spec class */
-	if (attr == USB_ENDPOINT_SYNC_ASYNC &&
-	    alts->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
-	    alts->desc.bInterfaceProtocol == 2 &&
-	    alts->desc.bNumEndpoints == 1 &&
-	    USB_ID_VENDOR(chip->usb_id) == 0x0582 /* Roland */) {
-		if (add_roland_implicit_fb(chip, fmt,
-					   alts->desc.bInterfaceNumber + 1,
-					   alts->desc.bAlternateSetting))
+	if (USB_ID_VENDOR(chip->usb_id) == 0x0582) {
+		if (add_roland_implicit_fb(chip, fmt, alts) > 0)
 			return 1;
 	}
 
@@ -404,6 +440,13 @@ 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);
+
+	/* Roland/BOSS need full-duplex streams */
+	if (USB_ID_VENDOR(chip->usb_id) == 0x0582) {
+		if (add_roland_capture_quirk(chip, fmt, alts) > 0)
+			return 1;
+	}
+
 	if (is_pioneer_implicit_fb(chip, alts))
 		return 1; /* skip the quirk, also don't handle generic sync EP */
 	return 0;