[PATCH v2] ALSA: usb-audio: Re-apply implicit feedback mode to Pioneer devices

[Geraldo Nascimento]

Thanks, Dr. Iwai! Applied manually on top of 5.12-rc8 and seems to be
working. My JACK transport is rolling and I have some vinyl spinning on
Mixxx passthrough mode. Will let it spin for a few hours just to be sure.

Em Seg, 19 de abr de 2021 12:41, Takashi Iwai <tiwai at suse.de> escreveu:

> Pioneer devices are supposed to be working with the implicit feedback
> mode, but so far the attempt to apply the implicit feedback 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.
>
> And, later on, Olivia reported later that the fiddling with the
> capture quirk alone doesn't suffice for the test with speaker-test
> program.  This seems to be a similar case like the recently fixed BOSS
> devices.  Indeed, the problem could be addressed by setting
> playback_first flag, which indicates that the playback URBs have to be
> sent out at first even in the implicit feedback mode.
>
> This patch implements the application of the implicit feedback to
> Pioneer devices as described in the above.  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 and set chip->playback_first flag
> if matching, and in audioformat_capture_quirk()(), it returns 1 for
> skipping the generic EP sync handling.
>
> Reported-by: Geraldo <geraldogabriel at gmail.com>
> Tested-by: Olivia Mackintosh <livvy at base.nu>
> Link: https://lore.kernel.org/r/s5ha6pygqfz.wl-tiwai@suse.de
> Signed-off-by: Takashi Iwai <tiwai at suse.de>
> ---
>  sound/usb/implicit.c | 42 +++++++++++++++++++++++++++---------------
>  1 file changed, 27 insertions(+), 15 deletions(-)
>
> diff --git a/sound/usb/implicit.c b/sound/usb/implicit.c
> index 428e3a449f5d..77ffcea294dd 100644
> --- 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 the quirk, also don't handle generic
> sync EP */
>         return 0;
>  }
>
> --
> 2.26.2
>
>