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

Dear Linux users of Pioneer gear, please disregard v1 of this patch and
test this instead if at all possible.

My initial assessment that we needed to let the capture EP be opened twice
was clearly proven wrong by further testing. This is good because if we
really needed that it would require a lot of reengineering inside ALSA.

One thing that still boggles my mind though is why we need a special
Pioneer case inside snd_usb_parse_implicit_fb_quirk that returns 1 like a
capture quirk was found.

This is a highly experimental patch on top of 5.12-rc6 that's supposed to
engage implicit feedback sync on the playback for Pioneer devices. Without
implicit feedback sync the inputs started glitching due to clock drift in
about an hour in my Pioneer DJ DDJ-SR2.

Once again I'd like to thank Takashi Iwai. He's the true author of the bulk
of this code, I just adapted it and coerced it into working.

Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>

 static int __add_generic_implicit_fb(struct snd_usb_audio *chip,
                                     struct audioformat *fmt,
                                     int iface, int altset)
@@ -306,7 +307,7 @@ static int audioformat_implicit_fb_quirk
            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))
+               if (add_pioneer_implicit_fb(chip, fmt, alts))
                        return 1;
        }

@@ -339,8 +340,20 @@ int snd_usb_parse_implicit_fb_quirk(stru
                                    struct audioformat *fmt,
                                    struct usb_host_interface *alts)
 {
-       if (fmt->endpoint & USB_DIR_IN)
-               return audioformat_capture_quirk(chip, fmt, alts);
+        bool isPioneer;
+
+        if (alts->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC &&
+            (USB_ID_VENDOR(chip->usb_id) == 0x2b73 || /* Pioneer */
+             USB_ID_VENDOR(chip->usb_id) == 0x08e4    /* Pioneer */))
+            isPioneer = true;
+
+       if (fmt->endpoint & USB_DIR_IN) {
+                if (isPioneer == true)
+                    return 1;
+                else
+                   return audioformat_capture_quirk(chip, fmt, alts);
+        }
+
        else
                return audioformat_implicit_fb_quirk(chip, fmt, alts);
 }

On Mon, 05 Apr 2021 15:49:20 +0200,
Geraldo wrote:
> 
> Dear Linux users of Pioneer gear, please disregard v1 of this patch and test
> this instead if at all possible.
> 
> My initial assessment that we needed to let the capture EP be opened twice was
> clearly proven wrong by further testing. This is good because if we really
> needed that it would require a lot of reengineering inside ALSA.
> 
> One thing that still boggles my mind though is why we need a special Pioneer
> case inside snd_usb_parse_implicit_fb_quirk that returns 1 like a capture
> quirk was found.
> 
> This is a highly experimental patch on top of 5.12-rc6 that's supposed to
> engage implicit feedback sync on the playback for Pioneer devices. Without
> implicit feedback sync the inputs started glitching due to clock drift in
> about an hour in my Pioneer DJ DDJ-SR2.
> 
> Once again I'd like to thank Takashi Iwai. He's the true author of the bulk of
> this code, I just adapted it and coerced it into working.
> 
> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>

Thanks for the patch!

It's interesting that Pioneer devices would actually work with the
implicit feedback mode.  It seems that the key point is to skip the
capture side; IIRC, we checked applying the quirk to the playback
side, but this wasn't enough or not properly working on some devices.

If that's the case, I believe a patch like below would be safer and
more inconsistent: it checks the device from both playback and capture
quirks with the same helper function.

Could you check whether this one works?   (It's totally untested.)


thanks,

Takashi

---
--- a/sound/usb/implicit.c
+++ b/sound/usb/implicit.c
@@ -167,18 +167,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) ||
@@ -187,8 +196,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,
@@ -297,13 +307,11 @@ 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))
+		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 */
 	if (chip->generic_implicit_fb)
@@ -324,6 +332,8 @@ static int audioformat_capture_quirk(struct snd_usb_audio *chip,
 	if (p && p->type == IMPLICIT_FB_FIXED)
 		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;
 }

On Fri, 09 Apr 2021 13:07:45 +0200,
Olivia Mackintosh wrote:
> 
> On Mon, Apr 05, 2021 at 10:49:20AM -0300, Geraldo wrote:
> > Dear Linux users of Pioneer gear, please disregard v1 of this patch and
> > test this instead if at all possible.
> > 
> > My initial assessment that we needed to let the capture EP be opened twice
> > was clearly proven wrong by further testing. This is good because if we
> > really needed that it would require a lot of reengineering inside ALSA.
> > 
> > One thing that still boggles my mind though is why we need a special
> > Pioneer case inside snd_usb_parse_implicit_fb_quirk that returns 1 like a
> > capture quirk was found.
> > 
> > This is a highly experimental patch on top of 5.12-rc6 that's supposed to
> > engage implicit feedback sync on the playback for Pioneer devices. Without
> > implicit feedback sync the inputs started glitching due to clock drift in
> > about an hour in my Pioneer DJ DDJ-SR2.
> > 
> > Once again I'd like to thank Takashi Iwai. He's the true author of the bulk
> > of this code, I just adapted it and coerced it into working.
> > 
> > Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> 
> Thank you Geraldo and Takashi for working on this patch. I'm currently
> in the process of testing with the DJM-750 but it currently does not
> work as expected. I'll debug futher and report back but would like to
> make you aware of this in the meantime. It may not be a a fundamental
> issue with the patch, but rather something incidental.

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.

FWIW, below is a proper patch that is applied on top of for-next
branch of sound.git tree (destined for 5.13 kernel merge).


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: usb-audio: Re-apply implicit feedback mode to Pioneer
 devices

Pioneer devices are supposed to be working with the implicit feedback
mode, but so far the attempt to applying this caused issues, hence we
explicitly skipped the implicit feedback mode for them.  Recently,
Geraldo discovered that the device actually works if you skip the
generic matching of the sync EPs for the capture stream.  That is, we
should apply the implicit feedback setup for the playback like other
similar devices, while we need to return 1 from
audioformat_capture_quirk() so that no further matching will be done.

This patch implement the application of the implicit feedback to
Pioneer devices.  The former skip_pioneer_sync_ep() was dropped, and
instead we provide is_pioneer_implicit_fb() to check the Pioneer
devices that need the implicit feedback.  In the
audioformat_implicit_fb_quirk(), simply apply the implicit fb for
playback if matching, and in audioformat_capture_quirk()(), it returns
1 if matching, as mentioned in the above.

Reported-by: Geraldo <geraldogabriel@gmail.com>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/usb/implicit.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/sound/usb/implicit.c b/sound/usb/implicit.c
index 4bd9c105a529..427ed0ff3b7b 100644
--- a/sound/usb/implicit.c
+++ b/sound/usb/implicit.c
@@ -171,18 +171,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) ||
@@ -191,8 +200,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,
@@ -308,13 +318,11 @@ 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))
+		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 */
 	if (chip->generic_implicit_fb)
@@ -335,6 +343,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;
 }

> 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.

Olivia

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

Em Dom, 18 de abr de 2021 09:45, Olivia Mackintosh <livvy@base.nu> escreveu:

> > As a blind shot: might the stall be avoided by passing the recently
> > introduced chip->playback_first flag?  The revised patch is below.
>
> This appears to fix the issue, thank you! I am curious as to why this is
> not required for Geraldo's DDJ-SR2.
>

Heh, sometimes I think Takashi Iwai is psychic ;-)

Just kidding, he's "just" very talented and very experienced. Anyway, the
DDJ-SR2 is originally a Serato controller. It still requires two MIDI SysEx
messages to unmute the RCA inputs - engaging "Serato Mode" in
marketing-speak.

My point is that Pioneer may be a little more compliant / a little less
deviant from the UAC2 standard when designing gear for use with third-party
software.


> My initial thought was that certain devices that restrict capture unless
> regular SysEx/Control URBs are sent may mean that Playback first support
> is required (e.g. 750, 850, 450). Please correct me if this intuition is
> incorrect.


> Olivia
>

On Sun, 18 Apr 2021 16:16:56 +0200,
Geraldo Nascimento wrote:
> 
> Em Dom, 18 de abr de 2021 09:45, Olivia Mackintosh <livvy@base.nu> escreveu:
> 
>     > As a blind shot: might the stall be avoided by passing the recently
>     > introduced chip->playback_first flag?  The revised patch is below.
>    
>     This appears to fix the issue, thank you! I am curious as to why this is
>     not required for Geraldo's DDJ-SR2.
> 
> Heh, sometimes I think Takashi Iwai is psychic ;-)

Heh, it was our luck that we've worked on the similar problem very
recently.

> Just kidding, he's "just" very talented and very experienced. Anyway, the
> DDJ-SR2 is originally a Serato controller. It still requires two MIDI SysEx
> messages to unmute the RCA inputs - engaging "Serato Mode" in marketing-speak.
> 
> My point is that Pioneer may be a little more compliant / a little less
> deviant from the UAC2 standard when designing gear for use with third-party
> software.

I think many devices require the implicit feedback seem following this
pattern: for such devices, the full-duplex streams are mandatory,
i.e. both streams have to be started always.  And the implicit
feedback is triggered at the later point for a proper sync.

>     My initial thought was that certain devices that restrict capture unless
>     regular SysEx/Control URBs are sent may mean that Playback first support
>     is required (e.g. 750, 850, 450). Please correct me if this intuition is
>     incorrect.

As mentioned previously, it might be the difference of the PCM
parameters to be deployed.  I guess Geraldo didn't test speaker-test
program but maybe only the standard sound backends like JACK, etc.

In anyway, I'm going to format and submit the proper patch for
merging.


thanks,

Takashi