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

[Geraldo]

Works for me Dr. Iwai, thank you!

Much more better styled now, and less hacky.

-Geraldo

Em Ter, 6 de abr de 2021 08:48, Takashi Iwai <tiwai at suse.de> escreveu:

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