[PATCH v2] Behringer UFX1604 / UFX1204: get rid of unneeded implicit feedback and pops and clicks while on 96000hz

[Geraldo Nascimento]

Sorry, Dr. Iwai, the truth is that I refuse to install PulseAudio in my
production systems, so I can't comment. To be sure you'd have to ask in
bugzilla #199327

Always a pleasure to work and learn with you, splitting the commits seems
like the ideal choice.

I do hope the fixes work out for others as I put a lot of time in tracking
the problem. Let's hope for the best.

Thank you,
Geraldo

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

> On Sat, 10 Apr 2021 22:27:15 +0200,
> Geraldo Nascimento wrote:
> >
> > OK, Dr. Iwai, only briefly tested but at least on my setup, JACK starts
> at
> > 96000hz and I am able to record my voice in Ardour 6, with no pops and
> clicks
> > detected.
> >
> > I'll record a full acapella just to be sure, but these 96000Hz pops and
> clicks
> > I used to experience - with that implicit feedback hack on by the way -
> tended
> > to be evident within seconds, not intermittent and very annoying.
> >
> > Now I turned the implicit feedback hack off for both the UFX1604 and the
> > UFX1204 on my tree and with your two lines of code I have crystal clear
> sound.
> >
> > I took the liberty to comment your clever hack inside clock.c and the
> pops /
> > clicks problem reappears on 96000Hz. PulseAudio users (see
> > https://bugzilla.kernel.org/show_bug.cgi?id=199327 ) will generally run
> at
> > 44100Hz and won't be affected so much by noise because the clock
> selector will
> > be "tied" to the standard 48000Hz of these mixer desks DAC, in that
> invalid
> > state but close enough to the main clock rate.
> >
> > Henceforth the general confusion where people complain the forced
> implicit
> > feedback made PulseAudio break for their UFX1604 / UFX1204, but they do
> not
> > experience noise. This approach should solve both problems by disabling
> > implicit feedback on the synchronous endpoints but making sure the sound
> is
> > crystal clear even at 96000Hz.
>
> Is the PA breakage still seen in the very latest kernel?  There have
> been a few issues due to the changes but they should have been already
> covered.
>
> Apart from that, the choice of implicit feedback was taken at the
> commit 5e35dc0338d8 in a few years ago for UFX1024.  I don't remember
> the details, but judging from the lsusb output, the devices don't look
> like a typical device that requires the implicit feedback (that has
> usually ASYNC for both directions).  So, as long as it's confirmed
> that the proper clock selector setup fixes the problem, we can drop
> the implicit feedback quirks.
>
> However, it's better to split the patches; one for the fix for the
> missing clock selector setup, another is the drop of the implciit fb
> quirk for Behringer devices.
>
> In anyway, let's see whether the fix works for others.  Then we can go
> forward and apply the fixes to the upstream.
>
>
> thanks,
>
> Takashi
>
> >
> > Thank you,
> > Geraldo
> >
> > Em Sáb, 10 de abr de 2021 05:59, Takashi Iwai <tiwai at suse.de> escreveu:
> >
> >     On Sat, 10 Apr 2021 03:36:14 +0200,
> >     Geraldo Nascimento wrote:
> >     >
> >     > More complete patch disabling unneeded implicit feedback and
> setting
> >     > clock selector to default clock on rate change for UFX1604
> >     >
> >     > After re-reading
> https://bugzilla.kernel.org/show_bug.cgi?id=199327 it
> >     > is even more clear to me that implicit feedback for the
> >     > UFX1604/UFX1204 needs to be disabled.
> >     >
> >     > This is a more complete patch that disables that and for the
> UFX1604
> >     > only sets the clock selector to its pin 1 default clock synced to
> the
> >     > USB SOF upon rate change. This is needed because apparently the
> >     > endpoints are hardwired to the clock selector and after we change
> the
> >     > rate on the main USB SOF synced clock the clock selector is left
> in a
> >     > halfway state in regards to the sampling rate.
> >     >
> >     > That's why the pops and clicks aren't evident at stock 48000Hz,
> become
> >     > slightly audible at 44100Hz and detestable at 96000Hz. Seems the
> clock
> >     > selector needs a nudge or it will screw up the sync.
> >     >
> >     > Unfortunately I don't have access to the lsusb -v of the UFX1204
> soI'm
> >     > waiting for someone to share it here in the list or in the bugzilla
> >     > thread. This patch needs some more love from the community.
> >     >
> >     > ---
> >     >
> >     > This one has been bugging me for quite a while. I went deep hard in
> >     > the guts of ALSA to try to solve it, and it turned out to be a
> minor
> >     > thing apparently. The problem is old, and every UFX1604 Linux user
> can
> >     > attest that it's impossible to use 96000hz in DUPLEX mode without
> >     > annoying pops and clicks on the capture stream.
> >     >
> >     > The fix is simple: after we alter the CLOCK_SOURCE to match our
> sample
> >     > rate, let's tell the CLOCK_SELECTOR we want CLOCK_SOURCE 212
> (synced
> >     > to USB SOF) on pin 1. Solves the problem for me, no more pops and
> >     > clicks while on 96000hz.
> >     >
> >     > ---
> >     >
> >     > Signed-off-by: Geraldo Nascimento <geraldogabriel at gmail.com>
> >
> >     Thanks for the patch.
> >
> >     But we'd like to avoid the setup with a magic ID number.
> >     Judging from what it achieves, does the change like below give the
> >     similar effect?
> >
> >     We might need to apply it conditionally, so this is just meant for
> >     testing.
> >
> >     Takashi
> >
> >     --- a/sound/usb/clock.c
> >     +++ b/sound/usb/clock.c
> >     @@ -324,6 +324,8 @@ static int __uac_clock_find_source(struct
> >     snd_usb_audio *chip,
> >                     ret = __uac_clock_find_source(chip, fmt,
> >
> selector->baCSourceID[ret -
> >     1],
> >                                                   visited, validate);
> >     +               if (ret > 0)
> >     +                       uac_clock_selector_set_val(chip, entity_id,
> cur);
> >                     if (!validate || ret > 0 || !chip->autoclock)
> >                             return ret;
> >
> >
>