From 9b3ec9204c327d1bf57e5af1bcdd1e5db67335dc Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Tue, 28 Jul 2026 13:15:36 +0200 Subject: [PATCH] =?UTF-8?q?fix(capture):=20a=20refused=20EGL=E2=86=92CUDA?= =?UTF-8?q?=20dmabuf=20offer=20stops=20being=20asked?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The raw-dmabuf passthrough has had a negotiation-timeout latch since the hybrid-Intel case: one conclusive timeout and later captures negotiate the CPU path instead of re-paying it. The EGL→CUDA dmabuf-only offer had no equivalent — a compositor that accepts none of the importer's modifiers timed out the same 10 s negotiation on every session, forever, under the generic 'format negotiation never completed' diagnosis. Now the symmetric latch (note_gpu_dmabuf_negotiation_failed) gates build_importer in the one negotiation resolver, scoped to this offer alone (raw passthrough, worker-death latch, encoder untouched), with the same operator escape as the raw arm: an explicit PUNKTFUNK_ZEROCOPY=1 keeps erroring loudly instead of downgrading. One correctness detail beyond the handoff's sketch: whether the GPU offer was ACTUALLY advertised is a runtime fact of the PipeWire thread (the importer may fail to construct — no CUDA — in which case no dmabuf was offered and a timeout must not latch it off). plan.build_importer alone cannot answer that, so the thread records the made-offer on CaptureSignals (gpu_dmabuf_offer) and the timeout diagnosis branches on the offer that really happened, not the plan's intent. Also renames vaapi_dmabuf_forced → zerocopy_forced (it now guards both timeout arms; single caller). New plan invariant pinned in tests: the latch gates only the importer, never the raw passthrough. (V3 from design/pf-zerocopy-sweep-handoff.md — the deferred design call, now decided in favour of the symmetric latch.) Co-Authored-By: Claude Fable 5 --- crates/pf-capture/src/linux/mod.rs | 30 ++++++++++++- crates/pf-capture/src/linux/pipewire.rs | 56 ++++++++++++++++++++++--- crates/pf-zerocopy/src/imp/mod.rs | 43 ++++++++++++++++--- 3 files changed, 116 insertions(+), 13 deletions(-) diff --git a/crates/pf-capture/src/linux/mod.rs b/crates/pf-capture/src/linux/mod.rs index 01395d9d..4f4efb9c 100644 --- a/crates/pf-capture/src/linux/mod.rs +++ b/crates/pf-capture/src/linux/mod.rs @@ -104,6 +104,13 @@ struct CaptureSignals { /// Set once the stream negotiated one of the 10-bit PQ formats, i.e. frames really are /// PQ/BT.2020 — drives `hdr_meta`. hdr_negotiated: Arc, + /// Set by the PipeWire thread once it ACTUALLY advertised the EGL→CUDA dmabuf-only offer + /// (importer constructed, importable modifiers found, not the raw passthrough, not HDR). + /// `plan.build_importer` alone cannot answer this — the importer may fail to construct (no + /// CUDA on this box), in which case no dmabuf was offered and a negotiation timeout must NOT + /// latch the GPU offer off. Read by `next_frame`'s timeout diagnosis, the EGL→CUDA twin of + /// the raw-passthrough arm. + gpu_dmabuf_offer: Arc, /// The LIVE cursor overlay, published from every buffer's `SPA_META_Cursor` — including the /// cursor-only "corrupted" buffers that never become frames — so the encode loop's forwarder /// tracks pointer-only motion on a static desktop (the frame-attached overlay alone goes stale @@ -123,6 +130,7 @@ impl CaptureSignals { streaming: Arc::new(AtomicBool::new(false)), broken: Arc::new(AtomicBool::new(false)), hdr_negotiated: Arc::new(AtomicBool::new(false)), + gpu_dmabuf_offer: Arc::new(AtomicBool::new(false)), cursor_live: Arc::new(std::sync::Mutex::new(None)), frame_size: Arc::new(std::sync::atomic::AtomicU64::new(0)), } @@ -444,6 +452,7 @@ fn spawn_pipewire( native_nv12_session: policy.native_nv12_session, raw_dmabuf_import_disabled: pf_zerocopy::raw_dmabuf_import_disabled(), gpu_import_disabled: pf_zerocopy::gpu_import_disabled(), + gpu_dmabuf_negotiation_failed: pf_zerocopy::gpu_dmabuf_negotiation_disabled(), // Default ON; `PUNKTFUNK_PIPEWIRE_NV12=0` (or any falsy spelling — the shared parser, not a // bare `!= "0"` string compare) restores the packed-RGB negotiation. native_nv12_env_on: pf_host_config::env_on("PUNKTFUNK_PIPEWIRE_NV12").unwrap_or(true), @@ -721,7 +730,7 @@ impl PortalCapturer { reconnect to stream SDR", self.node_id )) - } else if self.vaapi_dmabuf && !pf_zerocopy::vaapi_dmabuf_forced() { + } else if self.vaapi_dmabuf && !pf_zerocopy::zerocopy_forced() { // The dmabuf-only raw-passthrough offer was never accepted. Latch the // downgrade so the encode loop's pipeline rebuild retries on the CPU offer // instead of failing this same negotiation forever. The latch is SCOPED to the @@ -738,6 +747,25 @@ impl PortalCapturer { rebuild will renegotiate without dmabuf", self.node_id )) + } else if self.signals.gpu_dmabuf_offer.load(Ordering::Relaxed) + && !pf_zerocopy::zerocopy_forced() + { + // The EGL→CUDA dmabuf-only offer was never accepted — the twin of the raw- + // passthrough arm above (the offer the thread ACTUALLY made, per the signal + // it set — see `CaptureSignals::gpu_dmabuf_offer`). One timeout is conclusive: + // a compositor that allocates none of the importer's modifiers refuses them + // identically on every retry, so latch the offer off and let the pipeline + // rebuild renegotiate the CPU path instead of re-running this same 10 s + // timeout on every reconnect. A forced PUNKTFUNK_ZEROCOPY=1 keeps erroring + // loudly instead (same rule as the raw arm). + pf_zerocopy::note_gpu_dmabuf_negotiation_failed(); + Err(anyhow!( + "no PipeWire frame within {within}s (node {}): the compositor never \ + accepted the dmabuf-only offer (EGL→CUDA GPU import) — downgrading THIS \ + offer to the CPU path for the rest of the process; the pipeline rebuild \ + will renegotiate without dmabuf", + self.node_id + )) } else { Err(anyhow!( "no PipeWire frame within {within}s (node {}): format negotiation never \ diff --git a/crates/pf-capture/src/linux/pipewire.rs b/crates/pf-capture/src/linux/pipewire.rs index 4fbd3a53..4a118e65 100644 --- a/crates/pf-capture/src/linux/pipewire.rs +++ b/crates/pf-capture/src/linux/pipewire.rs @@ -127,6 +127,9 @@ pub(super) struct NegotiationInputs { pub raw_dmabuf_import_disabled: bool, /// `pf_zerocopy::gpu_import_disabled()` — repeated import-worker deaths. pub gpu_import_disabled: bool, + /// `pf_zerocopy::gpu_dmabuf_negotiation_disabled()` — a previous EGL→CUDA dmabuf-only offer + /// timed out (the compositor accepts none of the importer's modifiers). + pub gpu_dmabuf_negotiation_failed: bool, /// `PUNKTFUNK_PIPEWIRE_NV12` (default ON) — allow the producer-side NV12 preference. pub native_nv12_env_on: bool, } @@ -154,8 +157,8 @@ pub(super) struct NegotiationPlan { /// Diagnostic: this capture WOULD have taken the raw passthrough, but its scoped latch is /// set (the encoder repeatedly failed to import, or a previous negotiation timed out). pub raw_dmabuf_latched: bool, - /// Diagnostic: this capture WOULD have built the EGL→CUDA importer, but repeated - /// import-worker deaths latched the GPU import off. + /// Diagnostic: this capture WOULD have built the EGL→CUDA importer, but a latch fired — + /// repeated import-worker deaths, or a previous dmabuf-offer negotiation timeout. pub gpu_import_latched: bool, } @@ -176,8 +179,14 @@ pub(super) fn negotiation_plan(i: NegotiationInputs) -> NegotiationPlan { // Building the EGL→CUDA importer would waste a CUDA probe under a raw passthrough — or // worse, succeed and produce CUDA payloads only NVENC can consume. HDR is excluded by // invariant 1. `gpu_import_disabled` is the repeated-worker-death latch (a wedged GPU stack - // must not crash-loop). - let build_importer = i.zerocopy && !raw_passthrough && !i.want_hdr && !i.gpu_import_disabled; + // must not crash-loop); `gpu_dmabuf_negotiation_failed` is the offer's own timeout latch (a + // compositor that accepts none of the importer's modifiers refuses them identically on every + // retry, so the next session negotiates the CPU path instead of re-paying the 10 s timeout). + let build_importer = i.zerocopy + && !raw_passthrough + && !i.want_hdr + && !i.gpu_import_disabled + && !i.gpu_dmabuf_negotiation_failed; // Note there is no `importer.is_none()` term, unlike the expression this replaces: it was // redundant (`build_importer` already excludes `raw_passthrough`, so the importer is // necessarily absent here) and it is what made the decision look impure — the reason @@ -200,7 +209,10 @@ pub(super) fn negotiation_plan(i: NegotiationInputs) -> NegotiationPlan { && !i.force_shm && raw_passthrough && i.raw_dmabuf_import_disabled, - gpu_import_latched: i.zerocopy && !raw_passthrough && !i.want_hdr && i.gpu_import_disabled, + gpu_import_latched: i.zerocopy + && !raw_passthrough + && !i.want_hdr + && (i.gpu_import_disabled || i.gpu_dmabuf_negotiation_failed), } } @@ -810,7 +822,8 @@ pub fn pipewire_thread( // the import off — see `negotiation_plan`). if plan.gpu_import_latched { tracing::warn!( - "zero-copy GPU import disabled after repeated import-worker deaths — using CPU path" + "zero-copy GPU import disabled for this host process (repeated import-worker deaths, \ + or a previous dmabuf negotiation timeout) — using CPU path" ); } let mut importer = if plan.build_importer { @@ -862,6 +875,13 @@ pub fn pipewire_thread( // The one runtime-dependent half of the decision (see `NegotiationPlan::want_dmabuf`): it // needs the modifier list the importer's construction actually yielded. let want_dmabuf = plan.want_dmabuf(importer.is_some(), &modifiers); + // Record whether THIS capture really advertises the EGL→CUDA dmabuf-only offer, for the + // capturer's negotiation-timeout diagnosis (its latch must fire only for an offer that was + // actually made — `plan.build_importer` alone can't know the importer constructed). + signals.gpu_dmabuf_offer.store( + want_dmabuf && !vaapi_passthrough && !want_hdr, + Ordering::Relaxed, + ); if force_shm { tracing::info!( "capture: PUNKTFUNK_FORCE_SHM — race-free SHM download path (no dmabuf, no zero-copy)" @@ -1398,6 +1418,7 @@ mod tests { native_nv12_session: false, raw_dmabuf_import_disabled: false, gpu_import_disabled: false, + gpu_dmabuf_negotiation_failed: false, native_nv12_env_on: true, } } @@ -1512,6 +1533,29 @@ mod tests { } } + /// The EGL→CUDA offer's own negotiation-timeout latch gates `build_importer` — the twin of + /// the raw passthrough's latch, so a compositor that accepts none of the importer's modifiers + /// stops being asked (previously it re-ran the same 10 s timeout on every session, forever). + /// The raw passthrough is untouched by it. + #[test] + fn gpu_dmabuf_negotiation_latch_gates_only_the_importer() { + let p = negotiation_plan(NegotiationInputs { + gpu_dmabuf_negotiation_failed: true, + ..nvenc() + }); + assert!(!p.build_importer, "latched offer must not be re-made"); + assert!(p.gpu_import_latched, "the downgrade must be diagnosable"); + let p = negotiation_plan(NegotiationInputs { + gpu_dmabuf_negotiation_failed: true, + ..vaapi_native_nv12() + }); + assert!( + p.vaapi_passthrough, + "the raw passthrough has its own latch — this one must not touch it" + ); + assert!(!p.gpu_import_latched, "no importer was ever wanted here"); + } + /// A PyroWave session on an NVIDIA box takes the raw passthrough (the wavelet encoder's own /// Vulkan device imports dmabufs on any vendor) and therefore must NOT also build the /// EGL→CUDA importer — whose payloads only NVENC can consume. diff --git a/crates/pf-zerocopy/src/imp/mod.rs b/crates/pf-zerocopy/src/imp/mod.rs index 2e2893ff..f3a4a4d9 100644 --- a/crates/pf-zerocopy/src/imp/mod.rs +++ b/crates/pf-zerocopy/src/imp/mod.rs @@ -60,7 +60,8 @@ fn flag(name: &str) -> bool { /// True when `PUNKTFUNK_ZEROCOPY` is explicitly truthy — the operator forced the dmabuf offer, so /// a failed negotiation keeps erroring loudly instead of silently downgrading to the CPU path. -pub fn vaapi_dmabuf_forced() -> bool { +/// Read by BOTH negotiation-timeout latches (the raw passthrough's and the EGL→CUDA offer's). +pub fn zerocopy_forced() -> bool { flag_opt("PUNKTFUNK_ZEROCOPY") == Some(true) } @@ -70,11 +71,12 @@ pub fn vaapi_dmabuf_forced() -> bool { /// Vulkan (LINEAR) imports now run in a per-capture worker subprocess /// (`design/zerocopy-worker-isolation.md`), so a driver fault on a producer-invalidated dmabuf kills /// the worker and the host degrades to its capture-loss rebuild instead of dying — the reason the -/// NVENC path stayed opt-in is gone. Fallbacks stay in place: VAAPI has a one-shot CPU downgrade if -/// the LINEAR-dmabuf offer never negotiates ([`note_raw_dmabuf_negotiation_failed`]); NVENC falls back per -/// capture when no importer/importable modifier is available and latches the import off after -/// repeated worker deaths. `PUNKTFUNK_ZEROCOPY=0` opts out; `PUNKTFUNK_FORCE_SHM` forces the -/// race-free SHM path. +/// NVENC path stayed opt-in is gone. Fallbacks stay in place: VAAPI has a one-shot CPU downgrade +/// if the LINEAR-dmabuf offer never negotiates ([`note_raw_dmabuf_negotiation_failed`]); NVENC +/// falls back per capture when no importer/importable modifier is available, and latches the +/// import off after repeated worker deaths or its own negotiation timeout +/// ([`note_gpu_dmabuf_negotiation_failed`]). `PUNKTFUNK_ZEROCOPY=0` opts out; +/// `PUNKTFUNK_FORCE_SHM` forces the race-free SHM path. /// /// This is the GLOBAL switch and nothing but the env var moves it. It used to fall back to /// `!VAAPI_DMABUF_FAILED`, a latch a capture-side dmabuf *negotiation* timeout set — which made one @@ -311,6 +313,35 @@ pub fn raw_dmabuf_import_disabled() -> bool { RAW_DMABUF_DISABLED.load(Ordering::Relaxed) } +/// The EGL→CUDA twin of the raw-passthrough negotiation latch: the capture advertised the GPU +/// importer's dmabuf-only offer and the compositor never accepted it. Without this, the raw +/// passthrough stopped being asked after one timeout while the GPU-import offer re-ran the same +/// 10 s negotiation timeout on every session, forever — the mirror image of the hybrid-Intel case +/// [`note_raw_dmabuf_negotiation_failed`] was written for. +static GPU_DMABUF_NEGOTIATION_FAILED: AtomicBool = AtomicBool::new(false); + +/// Latch the EGL→CUDA dmabuf offer off because it *never negotiated*. One timeout is conclusive +/// for this offer too: a compositor that cannot allocate any of the advertised EGL-importable +/// modifiers refuses them identically on every retry. Scoped deliberately — this gates only +/// `build_importer` (the capture-side EGL→CUDA offer); the raw passthrough, the worker-death +/// latch, and the encoder are untouched. +pub fn note_gpu_dmabuf_negotiation_failed() { + if !GPU_DMABUF_NEGOTIATION_FAILED.swap(true, Ordering::Relaxed) { + tracing::warn!( + "zero-copy EGL→CUDA dmabuf offer disabled for this host process: the compositor never \ + accepted the GPU importer's dmabuf-only capture offer, so later captures negotiate \ + the CPU path instead of repeating that timeout (the raw-dmabuf passthrough is NOT \ + affected)" + ); + } +} + +/// True once a negotiation timeout latched the EGL→CUDA dmabuf offer off (see +/// [`note_gpu_dmabuf_negotiation_failed`]). +pub fn gpu_dmabuf_negotiation_disabled() -> bool { + GPU_DMABUF_NEGOTIATION_FAILED.load(Ordering::Relaxed) +} + /// DRM FourCC for a packed 32-bit format name (little-endian, e.g. `b"XR24"`). const fn fourcc(c: &[u8; 4]) -> u32 { (c[0] as u32) | ((c[1] as u32) << 8) | ((c[2] as u32) << 16) | ((c[3] as u32) << 24)