QVAC-21914 fix: clip FA AUTO on non-coopmat GPUs + ggml-opencl submission hardening (Pixel 9 OOM, S25 Adreno abort)#181
Merged
Conversation
…udget-aware) The warmup-time hard-disable for GPU projectors without efficient (coopmat) flash attention replaced AUTO/ENABLED with DISABLED, which short-circuited the budget-aware AUTO heuristic in clip_resolve_flash_attn_type(). At high n_pos (image_tile_mode=disabled with image_max_tokens=4096 -> 16384 ViT patches) the forced explicit attention path materializes an O(n^2 * n_head) score matrix, growing RSS to ~12 GB and getting the process lmkd-killed on Pixel 9 Pro (runQwen35ImageTileModeTokensTest). Downgrade to AUTO instead and record the inefficiency in clip_ctx::fa_backend_inefficient, which now also enables the AUTO cutoff default (previously Mali-detection only) so any non-coopmat backend gets the per-image budget decision: explicit attention below the cutoff (fast on scalar-FA GPUs), memory-frugal scalar FA at/above it or when the explicit scratch would not fit device memory. Explicit user DISABLED is still honored, and MTMD_CLIP_AUTO_FA_MIN_KV still overrides the cutoff.
…q-chunking) On Galaxy S25 Ultra (Adreno 830, OpenCL) the monolithic 16384-patch ViT encode (image_tile_mode=disabled, image_max_tokens=4096) faults the GPU near the end of the encode (Adreno-GSL log_gpu_snapshot fires before any decode work reaches the device), after which the driver aborts the process from cl_a8x_cmdbuf_mgr_submit_ibs (os_exit) on the next submission. Two unbounded behaviours plausibly drive the fault and both are bounded here: - ggml_backend_opencl_graph_compute enqueued entire graphs (thousands of nodes, ~48 s of GPU work for the failing encode) with no intra-graph flush. Now clFlush every GGML_OPENCL_FLUSH_INTERVAL nodes (default 64, 0 disables) so the GSL command-buffer manager receives bounded batches. clFlush submits without stalling the host. - ggml_cl_flash_attn issued one dispatch covering all q rows; at n_q = n_kv = 16384 every workgroup loops the full KV, making a single very long kernel. Now chunked along q rows at GGML_OPENCL_FA_MAX_NQ rows per dispatch (default 4096, 0 disables) with a clFlush between chunks. The split is exact: the kernel resolves its q row relative to the Q/O/mask base offsets, is_causal is always 0 (masking is explicit) and alibi/sinks depend only on the head index, so shifting the row base via byte offsets while shrinking n_q is mathematically identical. No .cl kernel changes. The 512-token image-chunk decode is not implicated: the S25 VLM benchmark ran 304 full-512-row ubatch decodes cleanly. Only the giant monolithic encode (5-8x beyond anything previously run on this backend) triggers the fault.
…, tests Addresses the pre-merge review findings on the two QVAC-21914 crash-fix commits (P1/P2 performance, C1/C2 correctness, S1/S2 robustness, K nits): - ggml-opencl: gate the periodic graph flush on accumulated estimated WORK (GGML_OPENCL_FLUSH_WORK_MB, default 512 MB) instead of a bare node counter. Per-token LLM decode graphs never reach the budget by construction, so the decode hot path stays submission-free; the 16k-patch encode still flushes dozens of times. Single touch point in graph_compute (no more per-fusion-branch duplication). - ggml-opencl: both tunables move onto ggml_backend_opencl_context, resolved once at init with strtol-based parsing (clamp, warn on garbage instead of silently disabling the mitigation) and GGML_LOG_INFO'd like the file's other env knobs. FA chunking reads the context field. - ggml-opencl: GGML_ASSERT(is_causal == 0) before the FA chunk loop — the kernel's causal-boundary formula needs the TOTAL n_q, so chunks after the first would silently corrupt output if causal FA were ever enabled here; keep the invariant loud. Explicit n_q == 0 guard. - clip: rework the AUTO cutoff memory clamp. Total memory now provides the STABLE fast-path clamp (explicit scratch <= total/4); free memory (a volatile, load-dependent number) may only lower the cutoff further via the hard-fit requirement (scratch <= free), never the old free/2 heuristic that silently pushed normal-size Mali images onto the ~2.6x slower scalar-FA path under momentary memory pressure. No memory info at all now fails SAFE at a conservative 2048-patch cap instead of trusting the raw 4096 default (~3.2 GB scratch at n_head=16). The arithmetic is extracted into clip_fa_effective_min_kv() (pure, exposed via clip.h for tests). - tests: test-clip-fa-cutoff (pure CPU, locks in the fast path, the P2 regression guard, the fail-safe cap and edge cases; passing) and test-opencl-fa-chunking (chunked-vs-CPU numerical parity over unchunked / exact-chunk / partial-last-chunk / n_q==1, masked and unmasked, with GGML_OPENCL_FA_MAX_NQ=64 and a 1 MB flush budget; self-skips without a capable OpenCL device — PoCL lacks FP16, so it executes on Adreno-class hardware). - clip warmup comment: note ggml-opencl also lands in the "no efficient-FA query" bucket and its giant-encode fault is handled by the submission bounding inside that backend. GGML_OPENCL_FLUSH_INTERVAL (node-count knob) is replaced by GGML_OPENCL_FLUSH_WORK_MB; GGML_OPENCL_FA_MAX_NQ semantics unchanged.
The pure AUTO-budget helper was defined out-of-line in clip.cpp and declared in the internal clip.h. On Windows mtmd builds as a shared library exporting only the MTMD_API-decorated public API; the internal clip_* symbols are absent from mtmd.lib, so test-clip-fa-cutoff (the first cross-DLL-boundary consumer of a clip_* symbol) failed to link (LNK2019). Linux/macOS export all default-visibility symbols, so it linked there. Move the function inline into clip.h (with its NO_MEMINFO_CAP constant); the test and clip.cpp both compile their own copy — no DLL export of an internal helper. CLIP_AUTO_FA_MIN_KV_MALI_DEFAULT stays in clip.cpp (its only user). Verified: mtmd + test-clip-fa-cutoff build and the test passes.
yingying0906
reviewed
Jul 8, 2026
…uard - parse_env_i64: GGML_LOG_WARN when an in-range-but-too-large GGML_OPENCL_FLUSH_WORK_MB / GGML_OPENCL_FA_MAX_NQ is clamped to max, matching the file's convention of logging every overridden value (previously the clamp was silent). - test-clip-fa-cutoff: the n_head=0 case passed total_mem==free_mem==0, which short-circuits to the NO_MEMINFO cap before any sqrt(.../n_head) branch runs — the div-by-zero guard was never exercised. Pass 16 GB total so the total-memory clamp runs with n_head=0; without the guard the (int)sqrt(x/0) path would now fail the assertion. Both from yingying0906's review; Windows/CPU-only surface, no Android behavior change.
yingying0906
approved these changes
Jul 8, 2026
gianni-cor
reviewed
Jul 8, 2026
The graph_compute work-budget flush ran BEFORE the current node was dispatched: it accounted the node's work, and on crossing the budget flushed (submitting only the prior batch) then reset the counter to 0 — so the crossing node started a fresh batch. A large op, or the last large segment of the graph, could therefore begin an unflushed batch and be submitted unbounded at the implicit end-of-graph finish, defeating the bound. Move the budget check below the dispatch (convert the fused-op continue chain to if/else so every path reaches one touch point), so the node that crosses the budget is part of the flushed batch. Reported by @gianni-cor.
Non-behavioral cleanups from the PR #181 review pass: - ggml-opencl graph_compute: correct the flush-cadence comment. The old "per-token decode hot path submission-free by construction" claim was false for multi-GB models — a decode step streams the whole model, so its estimated work crosses the default 512 MB budget a few times per token. Reworded to state that accurately (cost is negligible in practice since clFlush is non-blocking, and it is tunable/zeroable to make decode fully submission-free). Mechanism unchanged. - ggml_cl_flash_attn: GGML_ASSERT(q->ne[1] <= INT32_MAX) before the int n_q truncation, since the q-chunk loop accumulates into an int and derives cl_ulong offsets from it (defensive; not reachable with real shapes). - Consistency: normalize the ticket tag to bare `QVAC-21914` (drop the `qvac ` prefix) in clip.h and tests/CMakeLists.txt, matching the .cpp files and the fork's QVAC-21257 precedent. - tests/CMakeLists.txt: move the unconditional test-opencl-fa-chunking registration up beside test-copy-tbq-subgroups (its self-skipping sibling) instead of sitting right after the LLAMA_MTMD endif() where it read as MTMD-gated; add a comment noting it deliberately does not link mtmd. No functional change to the fix; local mtmd + both tests build, test-clip-fa-cutoff passes.
gianni-cor
approved these changes
Jul 8, 2026
This was referenced Jul 8, 2026
makaveli10
pushed a commit
to makaveli10/qvac-ext-lib-llama.cpp
that referenced
this pull request
Jul 13, 2026
Non-behavioral cleanups from the PR tetherto#181 review pass: - ggml-opencl graph_compute: correct the flush-cadence comment. The old "per-token decode hot path submission-free by construction" claim was false for multi-GB models — a decode step streams the whole model, so its estimated work crosses the default 512 MB budget a few times per token. Reworded to state that accurately (cost is negligible in practice since clFlush is non-blocking, and it is tunable/zeroable to make decode fully submission-free). Mechanism unchanged. - ggml_cl_flash_attn: GGML_ASSERT(q->ne[1] <= INT32_MAX) before the int n_q truncation, since the q-chunk loop accumulates into an int and derives cl_ulong offsets from it (defensive; not reachable with real shapes). - Consistency: normalize the ticket tag to bare `QVAC-21914` (drop the `qvac ` prefix) in clip.h and tests/CMakeLists.txt, matching the .cpp files and the fork's QVAC-21257 precedent. - tests/CMakeLists.txt: move the unconditional test-opencl-fa-chunking registration up beside test-copy-tbq-subgroups (its self-skipping sibling) instead of sitting right after the LLAMA_MTMD endif() where it read as MTMD-gated; add a comment noting it deliberately does not link mtmd. No functional change to the fix; local mtmd + both tests build, test-clip-fa-cutoff passes.
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
3 participants
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.
🎯 What problem does this PR solve?
runQwen35ImageTileModeTokensTest(GPU-default mmproj,image_max_tokens=4096,image_tile_mode=disabled→ a monolithic 16,384-patch ViT encode, the heaviest vision encode in the suite) crashes two of the three Android Device-Farm flagships (QVAC-21914):clip_resolve_flash_attn_type) built precisely for this case — the forced explicit-attention path materializes an O(n²·n_head) score matrix that cannot fit at 16k patches.Adreno-GSL log_gpu_snapshotfires before any decode work reaches the device), after which Qualcomm's UMD kills the process fromcl_a8x_cmdbuf_mgr_submit_ibs(os_exit) on the next submission — no CL error ever surfaces to the app. Logcat forensics on both crashing runs revised the initial "decode abort" attribution: the 512-token image-chunk decode is exonerated (304 clean full-512-row ubatch decodes in the S25 benchmark); only the monolithic encode — 5–8× beyond anything previously run on this backend — triggers the fault.Why S25 and not S26: not a backend split — S26 Ultra (Adreno 840) runs the same ggml-opencl path (
Adreno GPU version 840 found keeping OpenCL backend, 102×Chosen GPU OpenCLin its shard log). S26 completes the identical encode in ~29.5 s on a newer Android-16 UMD; S25 (Android-15 / 830 UMD) faulted ~48 s in. The fault is per-submission-magnitude dependent — fix 2 makes S25 pass without shortening the workload.📝 How does it solve it?
Two independent fixes (the S25 crash reproduced twice with fix 1 alone — original signature — confirming independence):
clip.cpp: downgrade the non-coopmat FA hard-disable to budget-aware AUTO (ef71f2b15). When the backend answersggml_backend_supports_efficient_fa == false, setCLIP_FLASH_ATTN_TYPE_AUTO(not DISABLED) and recordclip_ctx::fa_backend_inefficient, which now also enables the AUTO cutoff default (4096 patches; previously Mali-detection only). Normal images keep the fast explicit path QVAC-21320 feat: Mali GPU projector optimizations — disable FA, warptile, layernorm fusion #174 optimized for; huge encodes fall back to memory-frugal scalar FA — slow-but-alive beats fast-but-OOM. Explicit userDISABLEDandMTMD_CLIP_AUTO_FA_MIN_KVhonored as before; backends without the query (Metal/CUDA/OpenCL) unchanged.ggml-opencl.cpp: bound driver submissions (ee727efe9, refined ind1fe2fad0).graph_computeflushes when the estimated enqueued work since the last flush exceedsGGML_OPENCL_FLUSH_WORK_MB(default 512 MB,0disables) — previously entire multi-thousand-node graphs (~48 s of GPU work) were enqueued with zero intra-graph flushes. Gating on work (not a node count) keeps the per-token LLM decode graph submission-free by construction (it never accumulates near the budget), while the 16k-patch encode flushes dozens of times.ggml_cl_flash_attnchunks dispatches along q-rows atGGML_OPENCL_FA_MAX_NQrows (default 4096,0disables) with a flush between chunks. The split is exact: the kernel resolves its q row relative to the Q/O/mask base byte offsets,is_causalis always 0 (masking is explicit — guarded byGGML_ASSERT), and alibi/sinks depend only on the head index — no.clkernel changes. Forn_q ≤ 4096the dispatch path is byte-identical to before.clip.cpp: memory-clamp rework + review hardening (d1fe2fad0). The AUTO cutoff is now clamped by total memory (stable, session-independent: explicit scratch ≤ total/4); free memory may only lower it via a hard-fit check (scratch ≤ free), never the earlier free/2 heuristic that could push normal-size Mali images onto the slow scalar-FA path under momentary memory pressure. No memory info at all fails safe at a conservative 2048-patch cap instead of the raw 4096 default. Plus: both OpenCL env knobs resolved once at init withstrtol(clamp + warn, not silent-disable) and logged;is_causalassert;n_q == 0guard.CPU paths are untouched: the warmup change is gated on
backend != backend_cpu, and both hardening changes live inside the OpenCL backend module.🧪 How was it tested?
All on-device validation via the consumer branch
tetherto/qvactest/QVAC-21914-tile-mode-fa-fix(GPU-default mmproj) carrying this branch as a self-contained vcpkg overlay port; local compile checks for both the CPU-onlymtmdtarget andggml-opencl(Khronos headers/ICD loader).Crash fix — funcShardE per device, vs the crashing baseline (28865241320, stock v9341.1.5):
cl_a8xsignature; once earlier in MultiInstance — flaky driver heap corruption)crashed:false, zero driver-fault lines in logcat (nolog_gpu_snapshot, noos_exit, no SIGABRT).runImageMmprojGpuTestasserting the GPU projector on all devices).Performance A/B — VLM benchmark (qwen3.5-q8, cognitive preset ×3 samples, CPU+GPU cells, 30/30 samples per device, 0 errors), post-hardening (28900672031) vs pre-hardening baselines (runs ggml-org#252/ggml-org#253):
GPU quality per-task identical to CPU on both Adreno devices; encode/decode within cross-unit noise of the pre-hardening numbers — no measurable overhead from the flushes/chunking (benchmark images are ≤ ~3072 patches, so the chunking path only engages on the huge encodes it exists for). (This A/B ran the
ee727efe9hardening; thed1fe2fad0refinements are behavior-preserving for these shapes — work-budget gating only removes decode-path flushes, the memory clamp only matters at low free memory — and are re-validated by the S25 gate below.)Unit + parity tests (
d1fe2fad0,tests/):test-clip-fa-cutoff— pure-CPU coverage of the AUTO budget arithmetic: fast path survives low free memory, the P2 regression guard (1.5 GB free → hard-fit ~2795, not free/2 ~1976), the no-meminfo fail-safe cap, and edge cases. Passing locally.test-opencl-fa-chunking— chunked-vs-CPU numerical parity (GGML_OPENCL_FA_MAX_NQ=64) over unchunked / exact-chunk / partial-last-chunk /n_q==1, masked and unmasked. Self-skips without an FP16-capable OpenCL device (PoCL lacks FP16), so it runs on Adreno-class hardware / desktop OpenCL — not on the CI Linux host.Re-validation of the refined build (
d1fe2fad0) — all green, no regression:crashed:false, zero driver-fault lines.Windows DLL link fix (
dacb473f9):test-clip-fa-cutofflinked an internalclip_*symbol absent from the Windowsmtmd.dllimport lib (noMTMD_APIexport) →LNK2019on Windows only. Fixed by movingclip_fa_effective_min_kvinline intoclip.h— no export of an internal helper, builds on every platform. Behavior-identical on Android, re-validated end-to-end at the PR tip: S25 gate (28928563147) tile-mode PASS 264 s /crashed:false/ 0 driver-fault lines; full Android integration (28929513759) 18/18 green; benchmark (28929516001) 30/30 per device, GPU quality Δ0, encode within noise (S26 GPU 600 ms).PR review comments (
645e684d7):parse_env_i64now warns when it clamps an oversizedGGML_OPENCL_FLUSH_WORK_MB/GGML_OPENCL_FA_MAX_NQ(was silent);test-clip-fa-cutoff'sn_head=0case reworked to passtotal_mem>0so the div-by-zero guard is actually exercised. Windows/CPU-only; no Android behavior change.Flush-ordering fix (
39e0a8641, @gianni-cor): the work-budget flush ran before the budget-crossing node was dispatched, so that node began a fresh batch that could reach graph end unflushed — defeating the bound. Moved the check below dispatch (fused-opcontinuechain →if/else, single touch point) so the crossing node is in the flushed batch. Behavioral (tighter bound), so re-validated at PR-headc720b2b6b: full Android integration (28938583459) 18/18 green (funcShardE 3/3 × Pixel 9 / S25 / S26) + benchmark (28938585754) 30/30 per device, GPU quality Δ0, encode/decode within noise (S26 GPU 565 ms, S25/S26 decode 29.6/34.6 TPS) — no regression from the reordered flush.Review cleanups (
c720b2b6b): corrected the "submission-free by construction" flush comment to describe the real per-model cadence, addedn_q <= INT32_MAXassert, normalized ticket tags, and relocated the OpenCL FA-chunking test in CMakeLists. Non-behavioral — covered by the same PR-headc720b2b6bon-device runs as the flush-ordering fix above: full Android integration (28938583459) 18/18 green (funcShardE 3/3 × Pixel 9 / S25 / S26) + benchmark (28938585754) 30/30 per device, GPU quality Δ0.None. Fix 1 only changes behavior on backends that affirmatively report inefficient FA (today: Mali via ggml-vulkan); explicit
DISABLEDand the env override are honored, Metal/CUDA/OpenCL projectors unchanged. Fix 2 is OpenCL-module-only, mathematically exact, byte-identical dispatch forn_q ≤ 4096, and both knobs can be disabled at runtime (GGML_OPENCL_FLUSH_WORK_MB=0,GGML_OPENCL_FA_MAX_NQ=0). CPU inference paths are untouched.✅ Consumer overlay validation (Phase A)
All 6 qvac consumers validated against PR head
c720b2b6bvia a self-contained vcpkg overlay port (branchvalidate/QVAC-21914-fabric-overlay-pr181, commitd4dee909— no baseline/version changes).merge-guard / validate-prred is expected on a no-PR dispatch branch; the only retried jobs were infra flakes unrelated to this PR (win32 npm tooling setup, a loaded-runner test timeout):