sound: uaudio: finish the UAC2 shared-clock fix - #2390
Open
delleceste wants to merge 3 commits into
Open
Conversation
|
Thank you for taking the time to contribute to FreeBSD! All issues resolved. |
Contributor
Author
|
cc @christosmarg (committed 755685d via #2323) — commit 2/3 changes isochronous endpoint policy, which is your area. Three commits; 1/3 and 2/3 stand on their own and 3/3 is deliberately last so it can be dropped without a rebase. Built -Werror against 57775b3. |
…owns Follow-up to 755685d (MFC 6886e8a), which stopped an idle capture channel from reprogramming a shared UAC2 Clock Source to a *different* rate. The guard it added still lets the write through when both directions agree on the rate, and that write is not harmless. uaudio_chan_start() auto-starts the capture stream of an asynchronous playback device to harvest jitter information, and uaudio_configure_msg() configures playback first. With the rate alignment added by the same commit, the capture pass therefore issues a second SET_CUR(UA20_CS_SAM_FREQ_CONTROL) to the very same clock entity, with the very same value, *after* the playback alternate setting has already armed the device and its isochronous transfers are running. Several UAC2 implementations (Thesycon/XMOS firmware among them) reload their sample-clock PLL on every SET_CUR regardless of the value written; the device then drops, or silently fails to acquire, the stream it was in the middle of locking to. On an OKTO RESEARCH DAC8 STEREO (0x152a:0x88c5) this shows up as an intermittent silent open after a rate change -- correct rate on the front panel, healthy USB, no audio -- most often when moving into the 44.1 kHz family. Two changes: - The shared-clock guard now skips the write whenever the other direction is streaming at all, not only when it wants a different rate. The first active stream owns the clock; nobody else touches it. - Before programming a clock, read it back with GET_CUR and skip the write when it already holds the wanted rate. This is what Linux's set_sample_rate_v2v3() does, and it also covers the non-shared-clock cases (a stream restarting at the rate it last used). A completed SET_CUR only proves the request was accepted; the read-back is the only evidence that the device changed rate. hw.usb.uaudio.clock_readback=0 restores the unconditional write, for devices that misreport their rate. Linux needs the same escape hatch and spells it QUIRK_FLAG_ALWAYS_SET_RATE; a per-device quirk would be the better long-term shape here too, but a tunable is testable today and needs no quirk-table churn to bring a device back. Also stop uaudio_chan_start() from starting the jitter capture stream at a stale rate: when uaudio_chan_match_rate() finds no capture alternate setting for the playback rate it leaves the previous selection in place, so the stream would be armed at some unrelated rate -- either fighting the playback stream for a shared clock, or feeding the playback path jitter computed from frame sizes that do not belong to it. Run playback alone in that case. PR: 295933 MFC after: 2 weeks Assisted-By: Claude Opus 5 (claude-opus-5) Signed-off-by: Giacomo <delleceste@gmail.com>
…stream uaudio_chan_need_both() starts the capture stream of any asynchronous playback device that also exposes a capture interface, and uses the lengths of the packets it returns as the playback rate-feedback signal. That predates nothing in particular -- it is simply cheaper than a second endpoint when the device has no explicit feedback endpoint. When the device *does* have one, the driver pays for a whole second isochronous stream, arms a second streaming interface immediately after playback was armed, and (on a device with a sample clock shared between the two directions) drags a second clock programming pass along with it -- all to recompute a number the feedback endpoint already reports once a second. It is worst on D/A converters that advertise a UAC2 input terminal they cannot actually source: their capture interface exists only in the descriptors, but uaudio(4) streams it for the lifetime of every playback. Measured on an OKTO RESEARCH DAC8 STEREO (0x152a:0x88c5) at 44.1 kHz, this is ~126 extra isochronous transfers per second, exactly matching the playback transfer rate. Record the explicit feedback endpoint of each alternate setting while walking the descriptors (an isochronous endpoint in the opposite direction with the "feedback" usage type; uaudio_chan_fill_info_sub() skipped it as "sync endpoint information ignored until further"), and do not borrow the capture stream when the playback alternate setting has one. hw.usb.uaudio.prefer_feedback=0 restores the previous behaviour for A/B testing. uaudio_chan_play_sync_callback() decided whether to apply the feedback value from sc_rec_chan[].num_alt, i.e. whether the device has a capture interface at all. Test cur_alt instead, which is the exact complement of the condition uaudio_chan_play_callback() uses before folding capture jitter into the playback rate. Besides matching the new behaviour, this closes a pre-existing hole: when the capture stream exists but was never started, or failed to configure, playback used to free-run with no rate feedback from either source. PR: 295933 MFC after: 2 weeks Assisted-By: Claude Opus 5 (claude-opus-5) Signed-off-by: Giacomo <delleceste@gmail.com>
uaudio_configure_msg_sub() selects the streaming alternate setting first, programs the sample clock afterwards, and starts the isochronous transfers immediately. For UAC2 the clock lives on the AudioControl interface and can be programmed at any time, so that write lands underneath a streaming interface the device has already armed -- the same hazard the shared-clock guard added in 755685d exists to prevent, just reached by a different route. Linux's snd-usb-audio does the opposite: it parks the interface at alternate setting 0, programs the clock while the streaming interface is idle, and only then selects the streaming alternate setting and submits URBs. Do the same. A verified rate change additionally gets hw.usb.uaudio.clock_settle_ms (default 100, RWTUN, clamped to 2000) to settle before the alternate setting arms the stream: a 44.1 kHz <-> 48 kHz family change switches the master crystal on many DACs and the device needs time to relock. Because of the GET_CUR read-back added earlier in this series the delay is applied only when a clock was really written, and the programmed rate is verified afterwards. UAC1 is untouched: its sample-frequency control lives on the streaming endpoint, so there the alternate setting genuinely must be selected first. UAC3 behaviour is unchanged; the misleading /* FALLTHROUGH */ comment is corrected to say that no standard rate control is programmed. Evidence limitation, stated plainly. Unlike the other two patches in this series, this one has no trace or counter behind it. It was written for an intermittent fault on an OKTO RESEARCH DAC8 STEREO (0x152a:0x88c5) where the first open after a sample-rate change renders the whole stream silent -- correct rate on the front panel, healthy USB, feedback endpoint reporting normally, no audio -- and the only evidence that it helps is listening, on that one device. The fault is rare: 42 consecutive open/play/close cycles across all eight supported rates, 21 of them transitions into 44.1 kHz, were all audible, so I cannot presently reproduce it on demand in either direction. The reordering is defensible on its own terms and costs a device nothing when the rate has not changed, but I am not claiming measured proof of the fix. Drop this patch if that is not enough; the first two in the series stand without it. PR: 295933 MFC after: 2 weeks Assisted-By: Claude Opus 5 (claude-opus-5) Signed-off-by: Giacomo <delleceste@gmail.com>
delleceste
force-pushed
the
uaudio-shared-clock-followup
branch
2 times, most recently
from
August 28, 2026 10:56
5d3e216 to
5807e6d
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
sound: uaudio: finish the UAC2 shared-clock fix
Follow-up to 755685d (MFC 6886e8a, PR 295933,
#2323). That commit stopped
an idle capture channel from reprogramming a shared UAC2 Clock Source to a
different rate. Three things it left unfinished, one of which re-creates the
hazard the guard was added to prevent.
Please read commit 3 separately from 1 and 2 — see Evidence below.
1/3 — never rewrite a shared clock another stream owns
The guard skips only on a rate mismatch:
but the rate alignment added by the same commit (
uaudio_chan_match_rate()inuaudio_chan_start()) guarantees a match, so the write always fallsthrough. And
uaudio_chan_start()auto-starts the capture stream for jitterinfo while
uaudio_configure_msg()configures playback first — so the capturepass writes the clock after playback is armed and streaming.
Captured with
hw.usb.uaudio.debug=6, opening at 44.1 kHz on an OKTO DAC8STEREO (one Clock Source, id 41, in both direction bitmaps):
Two writes, one settle, every open. Also adds a
GET_CURread-back so a writethat would change nothing is skipped and the programmed rate is verified —
what
set_sample_rate_v2v3()does in Linux, withhw.usb.uaudio.clock_readback=0as the equivalent ofQUIRK_FLAG_ALWAYS_SET_RATE. A write is only ever skipped on a successfulread-back, so devices without
GET_CURare unaffected. And a faileduaudio_chan_match_rate()no longer leaves a stale capture alternate settingarmed at an unrelated rate.
2/3 — prefer an explicit feedback endpoint over the capture stream
uaudio_chan_need_both()starts the capture stream of any async playbackdevice that also exposes a capture interface, even when the playback alt
carries an explicit feedback endpoint — and
uaudio_chan_play_sync_callback()then discards the feedback value because itgates on
sc_rec_chan[i].num_alt == 0. Measured on the OKTO at 44.1 kHz over20 s:
Half the device's isochronous bandwidth, a second streaming interface armed
right after playback, and (on a shared clock) the extra programming pass from
1/3 — to recompute a number the device already reports. Worst on D/A converters
that advertise a UAC2 input terminal they cannot source: the capture interface
exists only in the descriptors, and we stream it for the lifetime of every
playback.
This is the behaviour change most worth review, hence
hw.usb.uaudio.prefer_feedback(default 1) to A/B it. It also closes apre-existing hole: gating on
cur_altrather thannum_altmeans playback nolonger free-runs with no feedback from either source when the capture stream
exists but never started or failed to configure.
3/3 — program the clock before arming the stream
Stock selects the streaming alt, then programs the clock, then starts
transfers. Linux parks at alt 0, programs, then selects the alt. This does the
same, plus
hw.usb.uaudio.clock_settle_ms(default 100) on a verified ratechange, since a 44.1/48 kHz family change switches the master crystal on many
DACs. UAC1 untouched (its rate control is on the streaming endpoint).
Evidence, and what I am not claiming
1/3 and 2/3 have a trace and a counter behind them and stand as defects
regardless of whether any device audibly misbehaves.
3/3 does not. It was written for an intermittent fault on one DAC where the
first open after a rate change renders the stream silent — correct rate on the
panel, healthy USB, feedback endpoint normal, no audio — and the only evidence
it helps is listening on that device. The fault is rare: 42 consecutive
open/play/close cycles across all eight rates, 21 of them into 44.1 kHz, were
all audible, so I cannot reproduce it on demand in either direction. The
reordering is defensible on its own terms and costs nothing when the rate has
not changed, but there is no measured proof. Drop 3/3 if that is not enough;
1/3 and 2/3 stand without it.
Not fixed here, declared so they are not mistaken for oversights
clock. With
usb_proc_msignal()'s two-entry queue anduaudio_chan_reconfigure()keeping only the latestoperation, aSTOPfollowed quickly by a
STARTcan coalesce and leave the other direction'sinterface armed across a clock write. The real fix is one transaction over
both directions with a generation counter — much larger than this series.
uaudio_dir_running_rate()reads the other channel'srunningandcur_altwith neither the explore lock nor
chan->lockheld, becauseuaudio_configure_msg()deliberately drops the explore lock. Aligned scalarreads, so no tearing, but not a stable snapshot. This came in with
755685d — mine.
declines to reprogram, but the second stream is configured and packetised for
the rate it asked for. Software at rate B, hardware at rate A, silently.
Linux returns
-EBUSY.bmControlsis not consulted; Linux checksuac_v2v3_control_is_writeable(bmControls, UAC2_CS_CONTROL_SAM_FREQ)andverifies rather than writes a read-only clock.
Device coverage
No VID/PID or quirk entries — everything keys off descriptor facts the driver
already parses. The shared-clock guard needs a Clock Source in both direction
bitmaps; the read-back needs UAC2;
prefer_feedbackneeds async playback plus afeedback endpoint plus a capture interface. The topology behind 1/3 (one clock
feeding both directions) is common: XMOS/Thesycon reference designs, most
DAC+ADC interfaces, and every DAC shipping a vestigial capture path. On my host
the OKTO hits all three paths and the ESI U24XL (UAC1) hits none — no
behaviour change at all on that card.
Testing
Builds
-Werrorclean with and withoutUSB_DEBUG, each commit standalone andstacked. 1/3 confirmed on the wire, 2/3 by counter differential. Clock read-back
verified live (
GET_CUR→ 44100,CLOCK_VALID→ 1,RANGE→ the 8 supportedrates). Not listening-tested with the series installed.