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