Repository navigation
M3 Pro: keep HDMI audio working across pause, rate changes and output switches; add USB-C DisplayPort audio - #265
erykwieliczko wants to merge 9 commits into
Conversation
maralcbr
left a comment
There was a problem hiding this comment.
@erykwieliczko thanks. The write-up is clear and the macOS comparison is convincing. The bitmap fix and the stale-prepare and post-disconnect fixes look right. I have two blockers and a policy question.
Scope, as I read it
- The held link, the rate pin, the 1.5 s delay and the silence only run on the native ops.
audio.c:1205-1210picks those ops only fordcpaud_native_link(). That is fw 14.7 plusexternal_native(dcp-internal.h:741-744), and it needsapple,t6030-dcpext(dcp.c:4240-4241,dcp.c:1393). So the drm/apple part is M3 Pro only (J514S/J516S, HDMI and now USB-C). Good. - The apple-sio change in patch 8 is not scoped like that. The new issue/report accounting runs on every SIO channel, whether or not it sets an idle transfer: one ISSUE in flight (
apple-sio.c:646,660-661), ACK charged to the last ring slot (605), REPORTs charged in ring order (897-905), NACK pops the slot (608). Every M1/M2 family Mac uses SIO for DP/HDMI audio:t8103.dtsi:1007,1028,t8112.dtsi:996,1017,t600x-die0.dtsi:523,t600x-dieX.dtsi:381,402,t602x-die0.dtsi:965. The PR reports no testing on those.
Blockers
1. dmaengine_synchronize() can return while the stream's descriptors still read the PCM buffer.
At handover the stream is retired with no TERMINATE (apple-sio.c:699-714). sio_synchronize() then waits at most 500 ms for it to drain, logs, and returns anyway (apple-sio.c:784-788). Up to four stream periods can be in flight: each ACK refills until the four ring slots are full (apple-sio.c:615-618, 646), and a cyclic transfer wraps, so a two-period buffer still queues four. The PCM does not cap period size (audio.c:399, period_bytes_max = SIZE_MAX). At 48 kHz stereo, with 4-byte DPA words, that is 384 kB/s. Four periods of 125 ms, which is plain aplay's default, already take 500 ms. Larger periods go past it.
After sync_stop returns (audio.c:1097-1099), ALSA runs hw_free and frees the managed buffer (sound/core/pcm_native.c:935-944). Up to 1 MiB is preallocated (audio.c:338-340), so usually the pages stay and the next stream overwrites them while SIO is still reading. A larger buffer, or card teardown, frees them for real. In both cases the header's promise in include/linux/dma/apple-sio.h is broken: synchronize returns before the descriptors are done. Before patch 8 the stop path always sent TERMINATE, so this could not happen.
sio_clear_idle() has the same shape. After a timeout it frees the idle transfer and returns success (apple-sio.c:817-827). The silence buffer is then freed at unbind (audio.c:1457-1458).
What I'd like: on timeout, stop the channel and confirm it is quiet before returning. A second bounded wait is not enough. Also cap period bytes while hold_link is on, so four periods fit well inside the bound at the lowest pinned rate (32 kHz).
2. Patch 8 changes SIO behaviour on M1/M2 and needs one run there before merge.
Edge is where we qualify M3 Pro changes. This one is different: it changes a shared DMA driver for every Apple Silicon Mac with DP/HDMI audio, and it is on by default. Please run a basic HDMI or DP audio check on one M1 or M2 before merge: play, stop, play again, change rate, close and reopen. The other option is to split patch 8 so that channels with no idle transfer keep their old accounting.
One thing to look at while there: the ring is per channel now, and nothing resets it. The old code counted ninflight per sio_tx, so a lost REPORT was gone with the next transfer. Now a REPORT that never arrives leaves a slot taken for the life of the channel (sio_scrub_locked() NULLs the owner but keeps the slot, apple-sio.c:568-575, 760, 823). A lost ISSUE ACK leaves issuing set, which now also blocks refills from the report path (646). I don't think a blind reset after TERMINATE is safe, because a late ACK or REPORT would be charged to the new transfer. But a defined resync point, or at least a rate-limited error when the ring and the transfers disagree, would make this debuggable.
Policy question: maintainer opt-in?
hold_link defaults to on (audio.c:662), with no check on port or sink. After the first stereo stream stops, SIO DMA and the DP audio stream run as long as the display is on: 4 KiB periods, about 94 completions/s at 48 kHz. The rate/format pin is applied at every open (audio.c:825-826, 712-715), so M3 Pro users lose native 44.1 kHz and high-rate playback on HDMI and USB-C. The mute was shown on the PS190 HDMI chain with one TV. USB-C was tested on the earlier base without patches 7-9. This is M3 Pro only, but it is still a default change in behaviour and power. @chriskearney should opt in. One option: hold on the HDMI port (dcpext) by default, and keep USB-C (dcpext1) per-stream until it has been rerun on this series.
Non-blocking
- Pinned rate and format may not exist together.
dcpaud_pin_rate_format()picks the rate and the format separately (audio.c:700-707) fromhits.hitsis a union across elements (parser.c:1725-1729). A sink with stereo 48 kHz/S16 and stereo 44.1 kHz/S24 elements gets 48 kHz/S24, which no element offers, and hw_params fails. Pick a tuple from one element, or pick the rate first and then look up formats at that rate. - The pin also applies to multichannel opens. The sieve is stereo-only (
audio.c:683-687), but the constraint covers the whole runtime (712-715), and only stereo is ever held (1056). Either limit the pin to stereo or say in the comment that it is meant. - NACK during a handover can stall the channel. If the only ring entry is an unacked ISSUE from the old transfer, the idle (or new) transfer can't issue while
issuingis set (apple-sio.c:646). A NACK then clears it and returns without a refill (604-611). With no REPORT pending, nothing restarts the channel. Callingsio_fill_in_locked()on the NACK path fixes it. - Configure before the START-time PREPARE. A held
hw_paramsreturns early (audio.c:908-911), and prepare returns early when a link is prepared (947-948). So the PREPARE at START (1002-1012) runs without the channel configure that prepare's comment promises "before every link prepare" (950). The config is fixed today (459,934-935), so this is probably harmless. Either configure there or fix the comment. - The 1.5 s settle delay ignores
hold_link. It sleeps in trigger START (audio.c:977-982) withhold_link=0too. It only covers what is left of the first 1.5 s after connect, so it is minor. But the PR sayshold_link=0restores the old behaviour. Gate the delay or reword that. It also rests on one boot observation. A comment with the measurement would help. - apple_sio_idle API.
period_len == 0returns -ENOMEM; it should be -EINVAL (apple-sio.c:453-454,839-841).peripheral_configshort-circuits the normal config (1047-1051), so say in the header that the two are separate calls.sio_clear_idle()drops the lock before TERMINATE (806-816), so a transfer issued in that window would be stopped with it. drm/apple serializes this underlink_lock; please write the requirement down ininclude/linux/dma/apple-sio.h. - Fixes: tags. Patch 4 fixes a real bug: the base sized the 64-bit map at 2 bytes and used
unsigned longbit ops on it (apple-sio.c:412-421,1134-1135). Please addFixes:for the commit that added it. Patches 1-3 are fixes too. - checkpatch. The new header needs a MAINTAINERS entry (the ARM/APPLE block around
MAINTAINERS:2630-2642). There is a double blank line att603x-j514s-j516s-usb4.dtsi:72-73. "acknowledgement" gets flagged atapple-sio.c:638,659. ba885d5 has four lines over 100 columns in av.c; they are gone at the head. - DT values. 0x68 and
ps_dpa2follow the in-tree DPAn pattern (t600x-dieX.dtsi:402,404; HDMI is 0x66 andps_dpa1).ps_dpa2exists att6030-pmgr.dtsi:526. I can't check the ADT from the tree. - Trailers and CI. Signed-off-by and
Assisted-by:matchDocumentation/process/coding-assistants.rst:44-46. Kernel CI run 40706626457 (https://github.com/omacom/linux-aurora/actions/runs/40706626457) ran on 2e25a86 and passed, including the requiredci / result. Its per-commit compile builds the changed objects only, not the full kernel at every commit.
Testing
- Before merge: the M1 or M2 HDMI/DP audio check from blocker 2.
- On edge, after merge, as M3 Pro qualification: suspend/resume with a held link, a physical replug, J516S, and USB-C with patches 7-9. Suspend looks covered in code, since ALSA runs sync_stop after suspend and that tears the link down (
audio.c:1102-1106), but it hasn't been run.
| struct sio_chan *siochan = to_sio_chan(chan); | ||
|
|
||
| flush_work(&siochan->terminate_wq); | ||
| if (!wait_event_timeout(siochan->drained, sio_drained(siochan), msecs_to_jiffies(500))) |
There was a problem hiding this comment.
After this timeout, synchronize returns while the retired stream's descriptors can still read the PCM buffer. Up to four stream periods can be in flight, and the PCM doesn't cap period size, so 500 ms is easy to exceed (4 x 125 ms, plain aplay). sync_stop then returns and hw_free reuses or frees the buffer. On timeout, please stop the channel and confirm it is quiet before returning.
| int ret; | ||
|
|
||
| if (tx->ninflight >= SIO_MAX_NINFLIGHT || tx->terminated) | ||
| if (!tx || siochan->issuing || siochan->nissued >= SIO_MAX_NINFLIGHT) |
There was a problem hiding this comment.
This accounting, one ISSUE at a time and REPORTs charged in ring order, now runs on every SIO channel, including M1/M2 DP/HDMI audio that never sets an idle transfer. Please run a basic HDMI audio check on an M1 or M2 before merge, or keep the old path for channels with no idle transfer.
| spin_lock_irqsave(&siochan->vc.lock, flags); | ||
| if (!siochan->current_tx || tx_cookie != siochan->current_tx->vd.tx.cookie || | ||
| siochan->current_tx->terminated) | ||
| siochan->nissued--; |
There was a problem hiding this comment.
If this NACK is for the last entry in the ring, say the old transfer's last ISSUE during a handover, nothing refills the channel afterwards. The idle or new transfer never starts. Calling sio_fill_in_locked() here would fix it.
| u64 format = 0; | ||
| int ret; | ||
|
|
||
| parse_sound_constraints(&elements, &sieve, &hits); |
There was a problem hiding this comment.
hits is a union across elements, so rate and format are picked separately and may not exist together. With stereo 48k/S16 and stereo 44.1k/S24 elements you get 48k/S24, and hw_params fails. Pick the tuple from one element, or pick the rate first and then look up formats at that rate.
| * the channel issues whenever no stream's DMA does, in descriptor order: | ||
| * a stream's DMA takes over from it and hands back to it without a gap. | ||
| */ | ||
| static bool hold_link = true; |
There was a problem hiding this comment.
On by default for every M3 Pro sink and port, including USB-C, which hasn't been rerun with patches 7-9. It means continuous SIO DMA while the display is on and a pinned rate. I'd like a maintainer to opt in. One option is holding only on the HDMI port until USB-C is validated.
| /* The first link of the connection waits for the sink to settle. */ | ||
| scoped_guard(mutex, &dcpaud->data_lock) | ||
| up_ms = ktime_ms_delta(ktime_get(), dcpaud->connected_at); | ||
| if (!READ_ONCE(dcpaud->link_started) && up_ms >= 0 && up_ms < DCPAUD_SINK_SETTLE_MS) { |
There was a problem hiding this comment.
This delay runs with hold_link=0 too, so that setting doesn't fully restore the old behaviour as the PR says. Gate it on hold_link or reword.
| memcmp(&dcpaud->link_cookie, &dcpaud->selected_cookie, sizeof(dcpaud->link_cookie))) { | ||
| if (dcpaud->link_prepared) | ||
| dcpaud_link_teardown(dcpaud); | ||
| ret = dcp_audiosrv_link(dcpaud->dcp_dev, DCP_AUDIO_LINK_PREPARE, |
There was a problem hiding this comment.
This PREPARE runs without the SIO channel configure that prepare's comment says comes before every link prepare (line 950): held hw_params and prepare both return early. The config is fixed today, so it may be fine. Either configure here or fix the comment.
| }; | ||
| }; | ||
|
|
||
|
|
Type-C connectors have no DCP until a pipeline is routed. With DRM_APPLE_DEBUG enabled, registering one dereferences the null platform device. Skip the pipeline debug directory until a DCP is available. Fixes: 2314090 ("drm: apple: route Type-C displays through a shared DCP fabric") Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
Check the published instance before Prepare and after its reply. Record the link owner only after the firmware accepts it. Fixes: f627d77 ("drm/apple: av: carry a Terminate's instance through its whole disconnect") Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
A disconnected PCM can still own a prepared firmware link. Complete that link before closing its client, including when sync_stop wins the race against the deferred owner release. Fixes: 3fe0ef0 ("drm: apple: Add the DCP 14.7 external role's HDMI audio") Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
The 64-slot descriptor bitmap is only allocated two bytes. Use the bitmap allocator so unsigned-long bitmap operations have enough storage. Fixes: 5fdca02 ("dmaengine: apple-sio: Add Apple SIO driver") Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
2e25a86 to
cb89503
Compare
|
@maralcbr thanks for the thorough review. v2 is pushed, rebased on Blockers1.
2. SIO behaviour on M1/M2 (patch 8)
The ring goes back to the old accounting at a defined resync point: once the channel is quiet, with no idle, current or retired transfer and nothing issued. M1/M2 DP audio never sets an idle transfer, so it runs the old code paths unchanged. On the ring accounting:
Policy
A Type-C output ( Non-blocking
Testing
|
maralcbr
left a comment
There was a problem hiding this comment.
@erykwieliczko thanks. v2 is a big step: blocker 2 is closed and the policy split is what I asked for. Re-reviewed at cb89503; refs are at the head. One gap is left in blocker 1, and it's small to fix.
Blocker 1: the stop path treats a NACKed TERMINATE as a stop
sio_call() returns 1 on ACK, 0 on NACK and a negative error on timeout (apple-sio.c:1255-1258). MSG_CONFIGURE already handles this (:1339-1342). sio_stop_quiet() checks only ret < 0 (:928-934). After a NACK it waits, resets the ring, frees the retired transfers and returns 0. So dmaengine_synchronize() and sio_clear_idle() return as if the channel had stopped, while it may still be reading the PCM buffer or the silence.
Two smaller holes in the same path:
- Refills during recovery. While
sio_stop_quiet()waits for the ring to empty (:936), ACK and REPORT handlers keep callingsio_fill_in_locked()(:706,:1120), which issues more of the current or idle transfer after TERMINATE. The ring may never empty, and the reset at:938-947forgets descriptors issued after TERMINATE. Their late REPORTs are then charged, with no warning, to whatever sits in those slots next. Astoppingflag that stops refills until the reset, then onesio_fill_in_locked()afterwards, closes it. sio_synchronize()ignores-EIO(:970-971). It returnsvoid, so the caller frees the buffer anyway. If the stop can't be confirmed, at leastWARNonce there. Better: keep the buffer's owner from freeing it. For the audio silence that already happens, becauseidle_setstays true; for a PCM buffer it can't.
What I'd like: treat only ret == 1 as stopped, and stop refills while recovering. The rest of blocker 1 holds:
- the 500 ms wait, then TERMINATE and confirm (
:963-975); sio_clear_idle()returns the error instead of freeing (:1006-1009);- the 50 ms period cap on holding ports (
audio.c:63,:866-873): four periods take 200 ms; - the
ret > 0KASAN fix (audio.c:872) is right.
Blocker 2: M1/M2 SIO behaviour (closed)
I diffed apple-sio.c against 63a0580. Without an idle transfer, ring stays false and the old accounting runs:
sio_fill_in_old_locked()(:726) is the basesio_fill_in_locked()plus apending++;- the ACK and REPORT handlers are the base ones (
:657,:1082), apart from the NACK now takingvc.lock; sio_terminate_all()goes down its old branch;sio_synchronize()returns afterflush_work();- the scrub and ring-leave added to
sio_terminate_work()do nothing whennissued == 0; sio_clear_idle()returns 0 when there is no idle transfer.
M1/M2 DP audio never sets an idle transfer, so its accounting is unchanged. That covers what I wanted from an M1/M2 run. I haven't run it on hardware.
Should fix: a descriptor freed before its callback ran
sio_terminate_all() frees a quiet retired transfer straight away (apple-sio.c:826-827, :850-851), and REPORTs free settled ones (:1139-1140). Both happen before tasklet_kill(). vchan_complete() takes vc->cyclic under the lock, then calls the callback with &vd->tx_result after unlocking (virt-dma.c:85-100). Clearing vc.cyclic doesn't stop a run that is already in progress. The window is narrow, but it's a use-after-free. The base sio_terminate_work() calls tasklet_kill() before freeing; do the same, or defer these frees to a work item that does.
Policy: maintainer opt-in?
hold_link 0/1/2, defaulting to the HDMI port only (audio.c:669-684), is what I suggested. Opening the PCM on a holding port also changes things for every stream there, multichannel included:
- one rate and sample size (
audio.c:701-755); - periods of 50 ms at most (
:866-873).
@chriskearney, please ack those defaults for M3 Pro HDMI before merge.
Earlier non-blocking items
All addressed:
- the rate and format pin comes from one element, and the comment explains why the pin covers every layout;
- the settle delay only applies while links are held (
audio.c:1029); - NACK refills during a handover (
sio_ring_issue_ack_locked()); period_len == 0returns -EINVAL;- the header documents the separate call and the serialization;
Fixes:tags are on patches 1-4, MAINTAINERS has an entry, and the DTS double blank line is gone.
Minor
hold_linkis 0644. Raising it while a stream runs leaves that stream uncapped, so it goes through the 500 ms wait and the stop path, with a gap. 0444, or a note in the description, would do.dcpaud_silence_stop()drops the PM reference even when removing the idle transfer fails (audio.c:814-820). The buffer stays owned, becauseidle_setstays true, but the channel may still be running while the device autosuspends.
Testing
CI passed at this head. After the fix, qualify on edge on M3 Pro:
- suspend/resume with a held link;
- a physical replug;
- J516S;
- USB-C with
hold_link=2; - a 44.1 kHz stream on HDMI.
The sink behind the J514S HDMI port (PS190 converter, TV) mutes the audio of a link stopped and started again with an unchanged format, until the DP link trains again. macOS never does that: it starts the link once and, between streams, keeps it running on a cleared buffer until the display sleeps or the format changes. Do the same for stereo on the HDMI port: the stopped stream's DMA is replaced by silence and the link is held. The mute was measured behind the HDMI port only, so a Type-C output keeps one link per stream unless hold_link=2; hold_link=0 restores one link per stream everywhere. While links are held, a link is not started in the first 1.5 s after the sink connects: once, at boot, a link started 0.2 s after the display came on was not heard, and holding it kept it silent. A link prepared at START configures the SIO channel first, as the PCM's prepare does. Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
Link the Type-C DisplayPort processor (dcpext1) to DPA2 on SIO channel 0x68, as the ADT links dcpext1 to dp-audio2, so a display on a USB-C port gets an audio device too. Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
PipeWire changes the device rate to follow its streams: 44.1 kHz music, 48 kHz video. A held link of another rate or sample size was replaced at START by one with the same channel layout, which the J514S sink mutes like a same-format restart. After 48 kHz then 44.1 kHz streams, every stream was silent until a replug. macOS runs the link at one nominal rate and format and converts streams to it. While links are held, the PCM now offers one rate (48 kHz, else 44.1 or 32 kHz) and one sample size (24 bits, else 16), taken from one element that offers them together in stereo, and userspace resamples. The pair covers every channel layout: userspace sets the format before the channel count, so a pair applied only to stereo let it rule stereo out. Channel layouts stay free: a layout change is heard. Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
A DP audio sink behind the J514S HDMI port mutes for good when its samples stop for 20 ms or more, until the DP link trains again. Swapping one cyclic transfer for another stops the channel with TERMINATE and starts the next one after its acknowledgment: usually short enough, sometimes not. Let a client set an idle transfer (struct apple_sio_idle, passed as peripheral_config in a slave config call of its own). The channel issues it while no other transfer runs. Terminating a transfer then hands over to it after the issued descriptors, with no TERMINATE, and a transfer issued later follows its issued descriptors the same way. From its first idle transfer until it is quiet again, a channel accounts for its issued descriptors in a ring, seeded with what it has in flight: reports complete them in issue order, so each is charged to the transfer that issued it, and one ISSUE is outstanding at a time. dmaengine_synchronize() waits for an ended transfer's descriptors. If they do not complete within 500 ms, it stops the channel with TERMINATE and waits for their reports again, and resets the ring if they never come. Removing the idle transfer does the same, and fails if the channel cannot be confirmed stopped. A channel that never had an idle transfer keeps the old accounting unchanged: the DP audio of M1 and M2 Macs sets none. Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
The held link's silence replaced a stopped stream's DMA and was replaced by the next stream's with a SIO TERMINATE in between, and closing the PCM terminated it once more. A gap of 20 ms in the samples mutes the J514S sink until the DP link trains again, as a link restart does: with a gap forced to 20 ms, every later stream was silent, and Chromium resuming after a 30 s pause was silent in 2 of 7 runs without one. Make the silence the SIO channel's idle transfer while the link is held. A stream's DMA follows it and hands back to it in descriptor order, and the PCM's terminate at close leaves it running. Its periods shrink to 4 KiB, so a stream starts at most about 85 ms after its START. A stream that ends into the silence leaves up to four periods issued, which dmaengine_synchronize() waits for, 500 ms at most: on a port that holds links, stream periods are kept to 50 ms. A silence whose channel could not be stopped is not freed. Signed-off-by: Eryk Wieliczko <eryk@wieliczko.ninja> Assisted-by: Claude:claude-opus-5-5
cb89503 to
c0b3d14
Compare
|
@maralcbr thanks. v3 is pushed, still on Blocker 1: a NACKed TERMINATE counted as a stop (patch 8)
Should fix: a descriptor freed before its callback ran (patch 8)Ended transfers are no longer freed from
Minor
Testing
|
Problem. On the M3 Pro (J514S; DCP 14.7 external role, PS190 DP→HDMI converter), HDMI audio worked for the first stream after the display link trained, then went silent and stayed silent until a replug. Triggers:
Cause: the sink chain mutes until the DP link trains again whenever:
macOS (same machine, TV and firmware; observed with
log show) never does either:Fix: do what macOS does, and say so in the code.
appledrm.hold_link(read-only at runtime; set at load or on the command line):0: one link per stream;1(default): hold on the HDMI port;2: hold on USB-C too, which hasn't been qualified on this series yet.struct apple_sio_idle, passed as the slave config'speripheral_config):dmaengine_synchronize()waits for an ended transfer's issued descriptors, so the PCM buffer can be freed. If they don't complete within 500 ms, it stops the channel (TERMINATE) and waits for their reports before returning. While links are held, stream periods are capped at 50 ms;tasklet_kill());Patches
2314090e59f0). Without it, integration oopses at boot inapple_connector_debugfs_initwithCONFIG_DRM_APPLE_DEBUG=y, because Type-C connectors have no DCP until routed.f627d7748df5).3fe0ef05013a).5fdca02134fa).Testing (J514S, Sony TV via built-in HDMI and via a USB-C → HDMI 2.1 adapter, 4K):
30a65186+ this series:pw-playwith PipeWire suspends: 18/18;63a0580a(see the review reply for the details):W=1;63a0580a(see the review reply):W=1;hold_link=2, digital silence only, timed and checked in the kernel log; not acoustic, because the TV was on USB-C at night:Not covered:
hold_link=2) by ear: it was only checked with silence, as above;WirePlumber gives the new USB-C sink its default 40% volume.
🤖 Generated with Claude Code