Skip to content

M3 Pro: keep HDMI audio working across pause, rate changes and output switches; add USB-C DisplayPort audio - #265

Open
erykwieliczko wants to merge 9 commits into
integration/aurorafrom
fix/m3-pro-hdmi-audio
Open

erykwieliczko wants to merge 9 commits into
integration/aurorafrom
fix/m3-pro-hdmi-audio

Conversation

@erykwieliczko

@erykwieliczko erykwieliczko commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

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:

  • pause/play in a browser;
  • PipeWire suspending the sink;
  • switching outputs;
  • 44.1 kHz music after 48 kHz video.

Cause: the sink chain mutes until the DP link trains again whenever:

  1. the audio link is stopped and started again with the same channel layout. That covers the same format, and also a change of only the sample rate or sample size. A changed channel layout is heard.
  2. the samples stop for 20 ms or more. A forced 20 ms gap at a stream change silenced every later stream.

macOS (same machine, TV and firmware; observed with log show) never does either:

  • it starts the link once at one nominal rate and format and converts streams to it;
  • its DMA never stops: between streams it plays a cleared buffer;
  • it restarts the link only on display sleep or a channel-layout change.

Fix: do what macOS does, and say so in the code.

  • Hold the link (patch 5). On the HDMI port, a stereo link, once started, is held between streams: silence plays while no stream does. A stream of another element replaces the held link at START. Display off, sink change or unplug, and unbind release it. While links are held, the first link of a connection starts no earlier than 1.5 s after the sink connects. 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.
  • One rate and sample size (patch 7). While links are held, the PCM offers one rate (48 kHz, else 44.1 or 32 kHz) and one sample size (24 bits, else 16), both from one element that takes them in stereo, and userspace resamples. The pair covers every channel layout, because userspace sets the format before the channel count. Channel layouts stay free, since a layout change is heard.
  • No gap (patches 8–9). The held silence becomes the SIO channel's idle transfer (struct apple_sio_idle, passed as the slave config's peripheral_config):
    • the channel issues it whenever no stream's DMA runs;
    • a stream's DMA takes over from it, and hands back to it, after the descriptors already issued, with no TERMINATE in between;
    • 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;
    • only an acknowledged TERMINATE counts as a stop. Nothing is issued between the TERMINATE and the channel's resync. A stop that can't be confirmed makes synchronize warn, and leaves the silence's buffer and PM reference held;
    • ended transfers are freed only once no completion callback can still be running (tasklet_kill());
    • a channel keeps the old accounting until its first idle transfer, so M1/M2 DP audio (which never sets one) runs the old code paths unchanged;
    • the PCM's terminate at close leaves the idle transfer running.
  • Cost, as on macOS. Once audio has been used, the SIO DMA and the DP audio stream keep running (silence: 4 KiB periods, about 94 completions/s) while the display is on. A stream starts at most about 85 ms after its START, behind the silence already issued.

Patches

  1. drm/apple: skip AFK debugfs setup for unrouted connectors (Fixes: 2314090e59f0). Without it, integration oopses at boot in apple_connector_debugfs_init with CONFIG_DRM_APPLE_DEBUG=y, because Type-C connectors have no DCP until routed.
  2. drm/apple: reject stale native audio prepares (Fixes: f627d7748df5).
  3. drm/apple: complete live audio links after disconnect (Fixes: 3fe0ef05013a).
  4. dmaengine: apple-sio: allocate the full descriptor bitmap (Fixes: 5fdca02134fa).
  5. drm/apple: hold the native HDMI audio link between streams.
  6. arm64: dts: apple: t603x-j514s-j516s: add Type-C DisplayPort audio. dcpext1 gets dp-audio2 (SIO channel 0x68, power domain DPA2), as in the ADT.
  7. drm/apple: hold the HDMI audio link at one rate and sample size.
  8. dmaengine: apple-sio: hand over to an idle transfer without a gap.
  9. drm/apple: keep the held HDMI audio link's samples flowing.

Testing (J514S, Sony TV via built-in HDMI and via a USB-C → HDMI 2.1 adapter, 4K):

  • Method: quiet DTMF fixtures, a microphone on another PC, six-digit detection on both channels.
  • What reproduced the bugs, before the fix:
    • same-format restarts: silent after the first stream;
    • 48 kHz → 44.1 kHz: silent from the switch on (stable kernel and newest integration alike);
    • a gap forced to 20/50/200 ms: every later stream silent;
    • Chromium resuming after a 30 s pause: silent in 2 of 7 runs.
  • After, on integration 30a65186 + this series:
    • Chromium pause/play with gaps 0/1/8/30/3 s: 25/25;
    • mixed 48/44.1 kHz pw-play with PipeWire suspends: 18/18;
    • ALSA restarts and reopens, before and after display off/on: 10/10;
    • two fresh boots, straight into mixed-rate playback and Chromium pause/play: 22/22. No SIO errors.
    • USB-C, on the earlier base without patches 7–9 (not yet rerun on this one): ALSA restarts/reopens and display off/on 10/10, pw-play across a PipeWire restart 5/5, Chromium 5/5.
  • KUnit: 129/129, including 10 link cases (one new: close leaves the held silence running).
  • v2, on 63a0580a (see the review reply for the details):
    • KUnit 1580 pass, 0 fail, 3 expected skips;
    • every commit builds alone with W=1;
    • a KASAN boot of the J514S with HDMI streams, restarts, reopens, PipeWire restarts and system sleeps with a held link reported nothing;
    • v2's acoustic rerun is still pending.
  • v3 (review fixes), on 63a0580a (see the review reply):
    • KUnit 1589 run, 1586 pass, 3 expected skips (one new case: a silence that can't be removed keeps its PM reference);
    • every commit builds alone with W=1;
    • USB-C with hold_link=2, digital silence only, timed and checked in the kernel log; not acoustic, because the TV was on USB-C at night:
      • streams play at the right speed;
      • each one takes over the held link, also after 45 s and 90 s idle and across reopens;
      • a 44.1 kHz open is refused;
      • 3/3 system sleeps with the link held, the display and a stream back after each;
      • no SIO errors;
    • the same on a KASAN kernel, with no KASAN report.

Not covered:

  • USB-C holding (hold_link=2) by ear: it was only checked with silence, as above;
  • physical replug (KUnit covers the sink-change path);
  • multichannel (not held);
  • J516S, which shares the DT include.

WirePlumber gives the new USB-C sink its default 40% volume.

🤖 Generated with Claude Code

@maralcbr maralcbr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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-1210 picks those ops only for dcpaud_native_link(). That is fw 14.7 plus external_native (dcp-internal.h:741-744), and it needs apple,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) from hits. hits is 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 issuing is 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. Calling sio_fill_in_locked() on the NACK path fixes it.
  • Configure before the START-time PREPARE. A held hw_params returns 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) with hold_link=0 too. It only covers what is left of the first 1.5 s after connect, so it is minor. But the PR says hold_link=0 restores 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 == 0 returns -ENOMEM; it should be -EINVAL (apple-sio.c:453-454, 839-841). peripheral_config short-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 under link_lock; please write the requirement down in include/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 long bit ops on it (apple-sio.c:412-421, 1134-1135). Please add Fixes: 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 at t603x-j514s-j516s-usb4.dtsi:72-73. "acknowledgement" gets flagged at apple-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_dpa2 follow the in-tree DPAn pattern (t600x-dieX.dtsi:402,404; HDMI is 0x66 and ps_dpa1). ps_dpa2 exists at t6030-pmgr.dtsi:526. I can't check the ADT from the tree.
  • Trailers and CI. Signed-off-by and Assisted-by: match Documentation/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 required ci / 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.

Comment thread drivers/dma/apple-sio.c Outdated
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)))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread drivers/dma/apple-sio.c Outdated
int ret;

if (tx->ninflight >= SIO_MAX_NINFLIGHT || tx->terminated)
if (!tx || siochan->issuing || siochan->nissued >= SIO_MAX_NINFLIGHT)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread drivers/dma/apple-sio.c
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--;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread drivers/gpu/drm/apple/audio.c Outdated
* 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;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread drivers/gpu/drm/apple/audio.c Outdated
/* 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) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread drivers/gpu/drm/apple/audio.c Outdated
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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};
};


Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: double blank line.

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
@erykwieliczko

Copy link
Copy Markdown
Collaborator Author

@maralcbr thanks for the thorough review. v2 is pushed, rebased on 63a0580a. Every point is addressed in the commit it belongs to. Commit by commit:

Blockers

1. dmaengine_synchronize() returning while descriptors still read the PCM buffer (patches 8, 9)

  • sio_synchronize() waits 500 ms for the retired transfers. If they haven't drained, it sends TERMINATE (sio_stop_quiet()) and waits for their reports again, and only then returns. If the channel acknowledged TERMINATE but the reports never come, the ring is reset: the channel is stopped, so nothing it held is read anymore. If TERMINATE itself fails, the channel cannot be confirmed stopped, and that is logged as such.
  • sio_clear_idle() works the same way. When the channel cannot be confirmed stopped, it now returns an error and keeps the idle transfer. dcpaud_idle() keeps idle_set, and unbind does not free the silence buffer then (it logs instead).
  • While a port holds links, stream periods are capped at 50 ms (DCPAUD_HELD_PERIOD_MAX_US). That's a period-time constraint rather than period bytes, so it holds for every format and rate: four periods drain in at most 200 ms.

2. SIO behaviour on M1/M2 (patch 8)
Split as you suggested. A channel uses the old accounting, verbatim, until a client sets its first idle transfer. Only the outstanding ISSUEs are now counted, as bookkeeping, so they can seed the ring. On the first apple_sio_idle, the ring is seeded from what the channel has in flight:

  • the current transfer's ninflight, plus at most one ISSUE still waiting for its ack;
  • anything else (two pending ISSUEs, an ended transfer) gets -EBUSY, and the audio driver falls back to terminating the stream.

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:

  • a report charged to a transfer with nothing in flight, or an ack without an issued descriptor, gets a rate-limited error;
  • WARN_ON(!nissued) became one of those;
  • the TERMINATE-confirmed reset above is the resync point for a lost REPORT or ISSUE ack.

Policy

hold_link is now an int:

  • 0: never hold;
  • 1 (default): hold on the HDMI port only;
  • 2: also hold on USB-C.

A Type-C output (dcp_is_typec_output(), exported to the audio driver as dcp_audiosrv_typec()) keeps one link per stream by default, until USB-C is rerun on this series. @chriskearney, your call on the default.

Non-blocking

  • Pinned rate and format (patch 7): the rate is picked first, then the elements are parsed again with the sieve narrowed to that rate, so the sample size comes from an element that takes both in stereo.

  • Pin and multichannel (patch 7): kept on every layout, on purpose, and the comment now says why.

    • I first limited it to stereo with hw rules (rate and format refined once channels were exactly 2, channels moved off 2 once the pair was impossible).
    • On hardware, PipeWire then built no HDMI sink: ALSA userspace sets the format before the channel count, picked S32 (which the sink also offers), and the channel rule then ruled stereo out.
    • With the plain constraints, it falls back to the pinned format, as v1 did.
  • NACK during a handover (patch 8): the NACK path refills when the next source is another transfer (idle or new). A transfer whose own ISSUE was refused is not reissued, which avoids a NACK storm.

  • Configure before the START-time PREPARE (patch 5): dcpaud_link_prepare() configures the SIO channel and prepares the link, and is used both by prepare and at START.

  • 1.5 s settle delay (patch 5): it only applies while links are held. The comment says what it rests on: one boot observation, a margin rather than a measured bound.

  • apple_sio_idle API (patch 8):

    • period_len == 0 (or > len) returns -EINVAL;
    • the header documents that peripheral_config is a call of its own, the error codes, and the timeout behaviour of synchronize and removal;
    • it also states that setting/removing, terminate and synchronize must not run concurrently on a channel, and that the client serializes them.
  • Fixes: tags:

    • patch 1: 2314090e59f0;
    • patch 2: f627d7748df5;
    • patch 3: 3fe0ef05013a;
    • patch 4: 5fdca02134fa.
  • checkpatch:

    • MAINTAINERS lists include/linux/dma/apple-sio.h next to drivers/dma/apple-sio.c;
    • the double blank line is gone;
    • "acknowledgement" is fixed;
    • patch 2's four long lines are wrapped.

    Every commit is checkpatch-clean. The only remaining items are the undocumented apple,dpaudio compatibles the existing DPA nodes already use.

Testing

  • Each commit compiles on its own (W=1), with both the production config and aurora.kunitconfig.
  • KUnit: 1580 pass, 0 fail, 3 expected skips. New cases:
    • dcpaud_typec_stop_ends_stream: USB-C keeps one link per stream unless hold_link=2;
    • dcpaud_pin_is_stereo_only: the rules pin stereo only;
    • the two START cases now expect the channel configure before PREPARE.
  • A KASAN boot on the J514S caught a use-after-free in my first version of the period cap. snd_pcm_hw_constraint_minmax() returns 1 when it narrows, and I treated that as failure after closing the PCM. Fixed before this push.
  • KASAN boot of the final series (with the sleep series on top), J514S with a 4K120 TV on HDMI, no KASAN report through:
    • PipeWire's device probing and restarts;
    • ALSA restarts with 50 ms and 2 s gaps, and reopens (the held link and the idle-transfer handover);
    • a refused 44.1 kHz open;
    • two system sleeps with the TV attached and the link held, and a stream after resume.
      No SIO accounting warnings.
  • Same series without KASAN: the same checks pass.
  • Acoustic (DTMF) checks of v2 are still to come: the TV was muted while I tested.
  • M1/M2: I have no M1 or M2 here. With the split, their channels never leave the old accounting.

@maralcbr maralcbr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 calling sio_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-947 forgets descriptors issued after TERMINATE. Their late REPORTs are then charged, with no warning, to whatever sits in those slots next. A stopping flag that stops refills until the reset, then one sio_fill_in_locked() afterwards, closes it.
  • sio_synchronize() ignores -EIO (:970-971). It returns void, so the caller frees the buffer anyway. If the stop can't be confirmed, at least WARN once there. Better: keep the buffer's owner from freeing it. For the audio silence that already happens, because idle_set stays 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 > 0 KASAN 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 base sio_fill_in_locked() plus a pending++;
  • the ACK and REPORT handlers are the base ones (:657, :1082), apart from the NACK now taking vc.lock;
  • sio_terminate_all() goes down its old branch;
  • sio_synchronize() returns after flush_work();
  • the scrub and ring-leave added to sio_terminate_work() do nothing when nissued == 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 == 0 returns -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_link is 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, because idle_set stays 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
@erykwieliczko

Copy link
Copy Markdown
Collaborator Author

@maralcbr thanks. v3 is pushed, still on 63a0580a. The changes are in patches 5, 8 and 9.

Blocker 1: a NACKed TERMINATE counted as a stop (patch 8)

  • sio_stop_quiet() treats only ret == 1 as stopped. A NACK (0) and no answer (< 0) both return -EIO and leave the transfers as they are, each with its own message.
  • Refills during recovery. A stopping flag is set before the TERMINATE is sent. sio_fill_in_ring_locked() issues nothing while it is set, so the ACK and REPORT handlers can't put descriptors behind the TERMINATE. It is cleared after the reset (or after the failure), followed by one sio_fill_in_locked(). A NACKed ISSUE that empties the ring now also wakes the waiter, which otherwise sat out the 500 ms.
  • sio_synchronize() and -EIO. It can't fail, so it dev_WARN_ONCEs when the stop can't be confirmed. The header says so too. For the silence, the owner already keeps the buffer (idle_set stays true). For a PCM buffer there is nothing the driver can do about it.

Should fix: a descriptor freed before its callback ran (patch 8)

Ended transfers are no longer freed from sio_terminate_all(), the ACK handler or the REPORT handler. Instead:

  • a transfer that settles goes on a per-channel dead list;
  • sio_free_dead() frees that list after tasklet_kill();
  • sio_synchronize() and sio_clear_idle() call it, and so does sio_free_chan_resources() through them;
  • sio_stop_quiet() moves the retired transfers onto the same list instead of freeing them.

sio_settle_locked(), the ring ACK and the REPORT paths return nothing now. On a channel that never had an idle transfer the list stays empty, so sio_synchronize() still only flushes the work there: M1/M2 are unchanged.

Minor

  • hold_link is 0444 (patch 5), with a comment saying why. It's set at load or on the command line (appledrm.hold_link=2).
  • dcpaud_silence_stop() (patch 9): if the idle transfer can't be removed, the stream's PM reference stays with the silence (idle_pm), and the device doesn't autosuspend under a channel that may still run. A later removal that succeeds puts it. New KUnit case: dcpaud_stuck_silence_keeps_pm.

Testing

  • Every commit builds alone with W=1. check-commits.py is clean, and checkpatch is clean apart from the existing undocumented apple,t6030-dpaudio compatible.
  • KUnit: 1589 run, 1586 pass, 3 expected skips (with Make M3 Pro sleep use 1.4 W instead of 2.6 W #274 v3 stacked).
  • J514S, USB-C display, hold_link=2 on the command line (the parameter reads r--r--r--). Digital silence only, timed against the wall clock and read from the kernel log:
    • every stream plays at the right speed (3 s of audio in 3.1 s);
    • each one takes over the held link ("stream follows the held link's silence"), also after 45 s and 90 s idle, and across close/reopen with 2 s and 50 ms gaps;
    • a 44.1 kHz open is refused;
    • 3/3 system sleeps with the link held: the silence stops when the display goes dark, and after resume the display is back, a stream starts a new link, and the next takes it over;
    • no SIO errors (no NACK, stop or ring reset).
  • The same on a KASAN kernel: no KASAN report.
    • Under KASAN, DisplayPort streams complete about 4× too fast on USB-C, and about 2× on HDMI in my earlier v2 runs. v2's KASAN build does the same, and the production builds of v2 and v3 don't, so it's a timing artifact of the KASAN build. I haven't tracked it down.
  • Not exercised on hardware: a refused TERMINATE, since nothing here makes the channel refuse one. Also not done: the acoustic check (the TV was on USB-C at night), a physical replug, J516S and HDMI after these fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants