From 5249f622c6e343834abaeb45f74ee28dfb2d6b3b Mon Sep 17 00:00:00 2001 From: David Date: Sun, 11 Oct 2026 01:54:27 -0700 Subject: [PATCH 1/2] feat(lint): INV-4.10 Vulkan frame-submit discipline check (#1917) check_displayxr_app.py gains an advisory (WARN) check for the submit shapes that park a mobile GPU governor at ~1/3 clock: per enclosing function of each vkQueueSubmit (brace-matched, +-60-line fallback), and only when that function is on the frame path (draw / render pass / dispatch / xrWaitFrame-BeginFrame-EndFrame, or a render/draw/frame/present name), it flags (a) vkQueueWaitIdle/vkDeviceWaitIdle in the body, (b) vkWaitForFences after the submit on the fence it just passed, (c) vkAllocateCommandBuffers + vkFreeCommandBuffers in the same body, and as INFO (d) one un-indexed fence waited at the top and signalled by the submit (one frame in flight). One-off upload helpers are left alone; `// dxr-lint: allow-blocking-submit ` on the submit or signature line exempts a deliberate blocking submit. Hermetic unit tests (positive Android/desktop shapes, VkFrameRing and hand-rolled-ring negatives, upload helper, exemptions) run in lint.yml. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/lint.yml | 7 + scripts/check_displayxr_app.py | 254 ++++++++++++++++++ .../test_check_displayxr_app_vk_frame.py | 227 ++++++++++++++++ 3 files changed, 488 insertions(+) create mode 100644 scripts/tests/test_check_displayxr_app_vk_frame.py diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 9efe7671a8..d72141e893 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -246,3 +246,10 @@ jobs: # would have caught the gauss demo's Android leg preferring UNORM after # #1589 made UNORM swapchains linear. run: python3 scripts/tests/test_check_displayxr_app_color.py + + - name: App-linter INV-4.10 Vulkan frame-submit unit tests + # Hermetic (synthetic app trees). Pins the per-function submit/drain, + # wait-own-fence and per-frame command-buffer checks (#1917: a serialised + # submit -> wait loop parks Adreno at ~1/3 clock), and that one-off + # uploads, VkFrameRing-style rings and the exemption comment stay quiet. + run: python3 scripts/tests/test_check_displayxr_app_vk_frame.py diff --git a/scripts/check_displayxr_app.py b/scripts/check_displayxr_app.py index 923c8f8c68..b83704b9db 100755 --- a/scripts/check_displayxr_app.py +++ b/scripts/check_displayxr_app.py @@ -59,6 +59,7 @@ "INV-4.6": "Request an sRGB swapchain (and store a correctly-encoded image); don't double-encode. A UNORM swapchain is read as LINEAR and encoded by the runtime (#1589), so display-referred bytes in it wash out. Checked per leg: every platform directory must choose _SRGB first.", "INV-4.7": "Write every pixel of the imageRect you declare — clear partial-tile renders to (0,0,0,0) first (or shrink the rect); undefined pixels read as opaque magenta on MoltenVK and break transparent-bg.", "INV-4.9": "An app that enables XR_EXT_view_configuration_views_change must not call xrCreateSwapchain from its event handler — move subImage.imageRect instead (an app sized at maxImageRect* per ADR-010 never needs to reallocate).", + "INV-4.10": "(Vulkan) Frame-submit discipline: >=2 frames in flight, ONE vkQueueSubmit per frame per queue, never wait the fence of the frame just submitted, no vkQueueWaitIdle/vkDeviceWaitIdle on the frame path, command buffers allocated once and reset (dxr::VkFrameRing, displayxr-common >= v2.28.0). Advisory (WARN). Heuristic: per enclosing function of each vkQueueSubmit (brace-matched; +-60-line window fallback), judged only when the function is on the frame path (draw/render pass/dispatch/xrWaitFrame/xrBeginFrame/xrEndFrame, or a render/draw/frame/present name); exempt with `// dxr-lint: allow-blocking-submit ` on the submit or function-header line.", "INV-5.9": "VK apps MUST use XR_KHR_vulkan_enable2 (the runtime creates the VkDevice via xrCreateVulkanDeviceKHR); an app-side vkCreateDevice = enable1, which forfeits the #868 weave-rate decoupling and the late-weave pacing.", "INV-7.x": "Capture via xrCaptureAtlasDXR — never reintroduce an app-side CaptureAtlasRegion* readback.", "INV-7.2": "xrCaptureAtlasDXR pathPrefix takes NO extension; the runtime appends _atlas.png.", @@ -781,6 +782,258 @@ def check_views_change_realloc(root: Path, findings: list): break # one finding per handler region is enough +# --------------------------------------------------------------------------- +# INV-4.10 — Vulkan frame-submit discipline (advisory, WARN). +# +# A frame loop that submits, then drains (vkQueueWaitIdle / vkDeviceWaitIdle / +# vkWaitForFences on the fence it just passed), never overlaps CPU and GPU. On a +# desktop dGPU that costs only the overlap; on a mobile GPU governor +# (msm-adreno-tz, Adreno 740) the GPU never looks busy and is parked at about a +# third of its clock (#1917: ~10 fps -> 30 fps in modelviewer#170 once 3 frames +# were kept in flight with ONE submit per frame). Per-frame +# vkAllocateCommandBuffers / vkFreeCommandBuffers adds an alloc + zero cost on +# Adreno (#1935/#1936). The sanctioned shape is displayxr-common's +# dxr::VkFrameRing (common/vk_frame_ring.h, >= v2.28.0). +# --------------------------------------------------------------------------- + +VK_DISCIPLINE_EXTS = {".c", ".cc", ".cpp", ".cxx", ".mm"} +VK_SUBMIT_RE = re.compile(r"\bvkQueueSubmit2?(?:KHR)?\s*\(") +VK_DRAIN_RE = re.compile(r"\b(vkQueueWaitIdle|vkDeviceWaitIdle)\s*\(") +VK_WAIT_FENCES_RE = re.compile(r"\bvkWaitForFences\s*\(") +VK_ALLOC_CB_RE = re.compile(r"\bvkAllocateCommandBuffers\s*\(") +VK_FREE_CB_RE = re.compile(r"\bvkFreeCommandBuffers\s*\(") +# "This function is on the steady-state frame path": it records drawing (a draw, +# a render pass, a dispatch) or drives the OpenXR frame cycle +# (xrWaitFrame/xrBeginFrame/xrEndFrame — NOT the swapchain acquire/release, which +# one-off static quad-layer fills also call), or its +# name says render/draw/frame/present ("Renderer" excluded — that is an init +# function's name, e.g. InitializeVulkanRenderer). A function with none of these +# is treated as a one-off upload/clear helper — the allowed exception. +VK_FRAME_BODY_RE = re.compile( + r"\bvkCmdDraw\w*\s*\(|\bvkCmdBeginRenderPass\w*\s*\(|\bvkCmdBeginRendering\w*\s*\(" + r"|\bvkCmdDispatch\w*\s*\(|\bxr(?:WaitFrame|BeginFrame|EndFrame)\s*\(" +) +VK_FRAME_NAME_RE = re.compile(r"render(?!er)|draw|frame|present", re.IGNORECASE) +VK_DISCIPLINE_EXEMPT = "dxr-lint: allow-blocking-submit" +# Fallback window (lines either side of the submit) when no enclosing function +# body can be delimited. +VK_DISCIPLINE_WINDOW = 60 +_CONTROL_HEADER_RE = re.compile(r"\b(?:if|for|while|switch|catch)\s*\(|^(?:else|do|try)\b|\belse\s*$") +_FUNC_HEADER_RE = re.compile( + r"(?:(\w+)\s*\([^;{}]*\)|\]\s*(?:\([^;{}]*\))?)" # name(args) | lambda ](args) + r"\s*(?:const|noexcept|override|final|mutable|->\s*[\w:<>*&\s]+)*\s*$" +) +_OBJC_METHOD_RE = re.compile(r"^\s*[-+]\s*\(") + + +def _brace_pairs(text: str) -> list: + """(open_index, close_index) for every matched {...} in comment-stripped text, + skipping braces inside string and char literals.""" + pairs, stack = [], [] + i, n = 0, len(text) + while i < n: + c = text[i] + if c in "\"'": + j = i + 1 + while j < n and text[j] != c and text[j] != "\n": + j += 2 if text[j] == "\\" else 1 + i = j + 1 + continue + if c == "{": + stack.append(i) + elif c == "}" and stack: + pairs.append((stack.pop(), i)) + i += 1 + return pairs + + +def _header_before(text: str, open_idx: int) -> tuple: + """The declaration text in front of a '{' (back to the previous ; { or }), + and its start index.""" + j = open_idx - 1 + while j >= 0 and text[j] not in ";{}": + j -= 1 + return text[j + 1:open_idx], j + 1 + + +def _enclosing_function(text: str, pairs: list, pos: int): + """Innermost {...} enclosing `pos` whose header looks like a function (or + lambda / ObjC method) definition rather than a control-flow block or an + aggregate initializer. Returns (body_start, body_end, header_start, name).""" + cands = sorted((p for p in pairs if p[0] < pos < p[1]), key=lambda p: p[0], reverse=True) + for o, c in cands: + header, hstart = _header_before(text, o) + h = " ".join(header.split()) + if not h: + continue + if _OBJC_METHOD_RE.match(h): + m = re.search(r"\)\s*(\w+)", h) + return (o, c, hstart, m.group(1) if m else "") + if _CONTROL_HEADER_RE.search(h): + continue + m = _FUNC_HEADER_RE.search(h) + if m: + return (o, c, hstart, m.group(1) or "") + return None + + +def _call_args(text: str, open_paren: int) -> list: + """Top-level comma-split arguments of the call whose '(' is at open_paren.""" + depth, args, cur = 0, [], [] + for ch in text[open_paren:]: + if ch in "([{": + depth += 1 + if depth == 1: + continue + elif ch in ")]}": + depth -= 1 + if depth == 0: + args.append("".join(cur)) + break + if depth == 1 and ch == ",": + args.append("".join(cur)) + cur = [] + else: + cur.append(ch) + return [" ".join(a.split()) for a in args] + + +def _norm_handle(expr: str) -> str: + return re.sub(r"[\s&()]", "", expr) + + +def check_vk_frame_discipline(root: Path, findings: list): + """INV-4.10 (advisory) — Vulkan frame-submit discipline. + + THE HEURISTIC, stated plainly (a text matcher, not a parser). For every + vkQueueSubmit in a .c/.cc/.cpp/.cxx/.mm source, find the enclosing function + body by brace matching (innermost {...} whose header looks like a function, + lambda or ObjC method; braces in string/char literals are skipped). If none + can be delimited, a +-VK_DISCIPLINE_WINDOW-line window around the submit is + used instead. Only functions on the FRAME PATH are judged — the body records + a draw / render pass / dispatch or calls xrWaitFrame / xrBeginFrame / + xrEndFrame, or the function name contains + render (not "renderer") / draw / frame / present. A submit in any other + function is treated as a one-off upload (the allowed exception) and is not + reported. Within a frame-path body it flags: + (a) vkQueueWaitIdle( or vkDeviceWaitIdle( anywhere in the body; + (b) vkWaitForFences( AFTER the submit whose fence list names the same + handle passed as vkQueueSubmit's fence argument (waiting the frame you + just submitted); + (c) vkAllocateCommandBuffers( AND vkFreeCommandBuffers( both in the body + (per-frame command-buffer churn). + Exemption: the comment `// dxr-lint: allow-blocking-submit ` on the + submit line, or on any line of the function's header (signature through + opening brace), suppresses every finding for that submit. + """ + for path in sorted(root.rglob("*")): + if not path.is_file() or path.suffix.lower() not in VK_DISCIPLINE_EXTS: + continue + if any(part in EXCLUDE_DIRS for part in path.relative_to(root).parts[:-1]): + continue + try: + raw = path.read_text(encoding="utf-8", errors="replace") + except OSError: + continue + if "vkQueueSubmit" not in raw: + continue + text = strip_comments(raw) + raw_lines = raw.splitlines() + pairs = _brace_pairs(text) + reported = set() # (body_start, sub-check) — one finding per function per check + + def line_of(idx): + return text.count("\n", 0, idx) + 1 + + for sm in VK_SUBMIT_RE.finditer(text): + submit_line = line_of(sm.start()) + fn = _enclosing_function(text, pairs, sm.start()) + if fn: + b0, b1, hstart, name = fn + header_lines = range(line_of(hstart), line_of(b0) + 1) + else: + lines_all = text.split("\n") + lo = max(0, submit_line - 1 - VK_DISCIPLINE_WINDOW) + hi = min(len(lines_all), submit_line + VK_DISCIPLINE_WINDOW) + b0 = sum(len(l) + 1 for l in lines_all[:lo]) + b1 = sum(len(l) + 1 for l in lines_all[:hi]) + name, header_lines = "", range(0) + exempt_lines = [submit_line] + list(header_lines) + if any(0 < ln <= len(raw_lines) and VK_DISCIPLINE_EXEMPT in raw_lines[ln - 1] + for ln in exempt_lines): + continue + body = text[b0:b1] + if not (VK_FRAME_BODY_RE.search(body) or (name and VK_FRAME_NAME_RE.search(name))): + continue # one-off upload helper — the allowed exception + where = f" in `{name}()`" if name and name != "" else "" + prel = rel(path, root) + + # (a) queue/device drain in the same frame-path function. + dm = VK_DRAIN_RE.search(body) + if dm and (b0, "a") not in reported: + reported.add((b0, "a")) + findings.append(Finding( + WARN, "INV-4.10", prel, line_of(b0 + dm.start()), + f"{dm.group(1)} in the same frame-path function as vkQueueSubmit{where} — " + "the CPU drains the GPU every frame, so nothing overlaps; on mobile GPU " + "governors (Adreno msm-adreno-tz) the clock collapses to ~1/3 (#1917).", + "Keep >=2 frames in flight with one submit per frame: dxr::VkFrameRing " + "(displayxr-common common/vk_frame_ring.h, >= v2.28.0) — beginFrame() / " + "tileBarrier() between tiles / endFrame(); waitAll() only on resize/teardown. " + f"A genuine one-off: `// {VK_DISCIPLINE_EXEMPT} ` on the submit line.", + )) + + # (b) waiting the fence of the frame just submitted. + args = _call_args(text, sm.end() - 1) + fence = _norm_handle(args[3]) if len(args) >= 4 else "" + if fence and fence not in ("VK_NULL_HANDLE", "nullptr", "NULL", "0") and (b0, "b") not in reported: + for wm in VK_WAIT_FENCES_RE.finditer(text, sm.end(), b1): + wargs = _call_args(text, wm.end() - 1) + if len(wargs) >= 3 and fence in _norm_handle(wargs[2]): + reported.add((b0, "b")) + findings.append(Finding( + WARN, "INV-4.10", prel, line_of(wm.start()), + f"vkWaitForFences on `{fence}` right after submitting with it{where} — " + "waiting the frame you just submitted serialises CPU and GPU (zero " + "frames in flight).", + "Wait the fence of the frame from N slots ago, not this one: " + "dxr::VkFrameRing::beginFrame() does exactly that (displayxr-common " + ">= v2.28.0). Exempt a deliberate one-off with " + f"`// {VK_DISCIPLINE_EXEMPT} `.", + )) + break + + # (d) one un-indexed fence waited BEFORE the submit that signals it: + # the next frame waits the previous one — at most ONE frame in flight. + # INFO only: a fence reached through a slot pointer (frame->fence) looks + # identical to the matcher, so this is a prompt to look, not a verdict. + if (fence and "[" not in fence and fence not in ("VK_NULL_HANDLE", "nullptr", "NULL", "0") + and (b0, "b") not in reported and (b0, "d") not in reported): + for wm in VK_WAIT_FENCES_RE.finditer(text, b0, sm.start()): + wargs = _call_args(text, wm.end() - 1) + if len(wargs) >= 3 and fence == _norm_handle(wargs[2]): + reported.add((b0, "d")) + findings.append(Finding( + INFO, "INV-4.10", prel, line_of(wm.start()), + f"Single fence `{fence}` waited at the top of the frame and signalled " + f"by its submit{where} — at most one frame in flight (INV-4.10 asks for >=2).", + "Use a ring of N>=2 fences/command buffers indexed by frame slot " + "(dxr::VkFrameRing, displayxr-common >= v2.28.0, defaults to 3).", + )) + break + + # (c) per-frame command-buffer allocation. + am = VK_ALLOC_CB_RE.search(body) + if am and VK_FREE_CB_RE.search(body) and (b0, "c") not in reported: + reported.add((b0, "c")) + findings.append(Finding( + WARN, "INV-4.10", prel, line_of(b0 + am.start()), + f"vkAllocateCommandBuffers + vkFreeCommandBuffers in a frame-path function{where} — " + "per-frame command-buffer churn (Adreno pays a fresh zeroed allocation each " + "time, #1935/#1936).", + "Allocate one command buffer per frame slot once and vkResetCommandBuffer() it " + "(dxr::VkFrameRing does this).", + )) # --------------------------------------------------------------------------- @@ -1142,6 +1395,7 @@ def main(argv=None) -> int: check_mcp_pairing(root, findings) check_manual_tracking_event(root, findings) check_views_change_realloc(root, findings) + check_vk_frame_discipline(root, findings) check_android(root, findings) findings = dedupe(findings) print_findings(findings, root) diff --git a/scripts/tests/test_check_displayxr_app_vk_frame.py b/scripts/tests/test_check_displayxr_app_vk_frame.py new file mode 100644 index 0000000000..57b4593a76 --- /dev/null +++ b/scripts/tests/test_check_displayxr_app_vk_frame.py @@ -0,0 +1,227 @@ +#!/usr/bin/env python3 +"""Unit tests for the INV-4.10 Vulkan frame-submit discipline check in +scripts/check_displayxr_app.py. + +Hermetic: each case writes a tiny synthetic app tree to a temp dir and lints it. + +The positive shapes are the ones the in-tree cubes shipped with (runtime #1917, +displayxr-demo-modelviewer#170): Android's record_atlas() allocated a command +buffer, submitted, vkQueueWaitIdle'd and freed it every frame; the Windows / +Linux renderers submit with a fence and immediately wait that same fence. On an +Adreno 740 that serialisation parks the GPU at ~1/3 clock. The negative shape is +dxr::VkFrameRing's (displayxr-common common/vk_frame_ring.h): N slots, one +submit per frame, the only wait is on the slot from N frames ago. + + python3 scripts/tests/test_check_displayxr_app_vk_frame.py +""" +from __future__ import annotations + +import sys +import tempfile +import unittest +from pathlib import Path + +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) +import check_displayxr_app as lint # noqa: E402 + +# The cube_handle_vk_android record_atlas() shape (per-frame alloc + drain + free). +ANDROID_DRAIN = """ +bool record_atlas(uint32_t image_idx, const XrView *views, uint32_t view_count) +{ + VkCommandBufferAllocateInfo ai = {}; + ai.commandPool = g_app_cmd_pool; + VkCommandBuffer cmd = VK_NULL_HANDLE; + if (vkAllocateCommandBuffers(g_vk_device, &ai, &cmd) != VK_SUCCESS) { + return false; + } + vkBeginCommandBuffer(cmd, &bi); + vkCmdBeginRenderPass(cmd, &rp, VK_SUBPASS_CONTENTS_INLINE); + for (uint32_t v = 0; v < view_count; ++v) { + vkCmdDraw(cmd, 36, 1, 0, 0); + } + vkCmdEndRenderPass(cmd); + vkEndCommandBuffer(cmd); + VkResult res = vkQueueSubmit(g_vk_queue, 1, &si, VK_NULL_HANDLE); + if (res == VK_SUCCESS) { + vkQueueWaitIdle(g_vk_queue); + } + vkFreeCommandBuffers(g_vk_device, g_app_cmd_pool, 1, &cmd); + return res == VK_SUCCESS; +} +""" + +# The cube_*_vk_win / linux RenderScene() shape (submit, then wait that fence). +DESKTOP_WAIT_OWN_FENCE = """ +void RenderScene(VkRenderer& renderer, uint32_t imageIndex) +{ + vkWaitForFences(device, 1, &renderer.frameFence, VK_TRUE, UINT64_MAX); + vkResetFences(device, 1, &renderer.frameFence); + vkCmdBeginRenderPass(renderer.commandBuffer, &rp, VK_SUBPASS_CONTENTS_INLINE); + vkCmdDrawIndexed(renderer.commandBuffer, 36, 1, 0, 0, 0); + vkCmdEndRenderPass(renderer.commandBuffer); + VkResult submitResult = vkQueueSubmit(renderer.graphicsQueue, 1, &submitInfo, renderer.frameFence); + if (submitResult != VK_SUCCESS) return; + VkResult waitResult = vkWaitForFences(device, 1, &renderer.frameFence, VK_TRUE, UINT64_MAX); +} +""" + +# The sanctioned shape: dxr::VkFrameRing (no drain, one submit, no wait on it). +FRAME_RING = """ +#include "vk_frame_ring.h" +static dxr::VkFrameRing g_ring; +void RenderFrame(const XrView *views, uint32_t viewCount) +{ + VkCommandBuffer cmd = g_ring.beginFrame(); + vkCmdBeginRenderPass(cmd, &rp, VK_SUBPASS_CONTENTS_INLINE); + for (uint32_t i = 0; i < viewCount; ++i) { + if (i > 0) dxr::VkFrameRing::tileBarrier(cmd); + vkCmdDraw(cmd, 36, 1, 0, 0); + } + vkCmdEndRenderPass(cmd); + VkResult r = g_ring.endFrame(); + (void)r; +} +""" + +# A hand-rolled ring: wait the slot from N frames ago, submit with this slot's +# fence, never wait it. Must NOT fire (b) or the single-fence INFO (d). +HAND_RING = """ +void RenderScene(Renderer& r) +{ + Slot& s = r.slots[r.cur]; + vkWaitForFences(r.device, 1, &r.fences[r.cur], VK_TRUE, UINT64_MAX); + vkResetCommandBuffer(r.cmds[r.cur], 0); + vkCmdDraw(r.cmds[r.cur], 3, 1, 0, 0); + vkQueueSubmit(r.queue, 1, &si, r.fences[r.cur]); + r.cur = (r.cur + 1) % kFramesInFlight; +} +""" + +# A one-off texture upload: alloc + submit + drain + free, no draw, no frame +# call, not a render/draw/frame/present name — the allowed exception. +ONE_OFF_UPLOAD = """ +static bool UploadTexture(VkDevice device, VkQueue queue, VkCommandPool pool, VkImage img) +{ + VkCommandBuffer cmd = VK_NULL_HANDLE; + vkAllocateCommandBuffers(device, &ai, &cmd); + vkCmdCopyBufferToImage(cmd, staging, img, VK_IMAGE_LAYOUT_TRANSFER_DST_OPTIMAL, 1, ®ion); + vkQueueSubmit(queue, 1, &si, VK_NULL_HANDLE); + vkQueueWaitIdle(queue); + vkFreeCommandBuffers(device, pool, 1, &cmd); + return true; +} +""" + +EXEMPT_ON_SUBMIT = ANDROID_DRAIN.replace( + "VkResult res = vkQueueSubmit(g_vk_queue, 1, &si, VK_NULL_HANDLE);", + "VkResult res = vkQueueSubmit(g_vk_queue, 1, &si, VK_NULL_HANDLE); " + "// dxr-lint: allow-blocking-submit capture readback test", +) +EXEMPT_ON_HEADER = DESKTOP_WAIT_OWN_FENCE.replace( + "void RenderScene(VkRenderer& renderer, uint32_t imageIndex)", + "void RenderScene(VkRenderer& renderer, uint32_t imageIndex) " + "// dxr-lint: allow-blocking-submit single-shot screenshot tool", +) + +# A per-frame drain in a frame function whose name says so but which records no +# draw itself (it calls helpers) — the name heuristic must still catch it. +NAMED_FRAME_NO_DRAW = """ +void PresentFrame(App& app) +{ + RecordEverything(app.cmd); + vkQueueSubmit(app.queue, 1, &si, VK_NULL_HANDLE); + vkDeviceWaitIdle(app.device); +} +""" + +# A function named ...Renderer (an init function) is NOT judged by name. +INIT_RENDERER = ONE_OFF_UPLOAD.replace("UploadTexture", "InitializeVulkanRenderer") + + +def lint_tree(files: dict[str, str]) -> list: + with tempfile.TemporaryDirectory() as d: + root = Path(d) + for rel, text in files.items(): + p = root / rel + p.parent.mkdir(parents=True, exist_ok=True) + p.write_text(text) + findings: list = [] + lint.check_vk_frame_discipline(root, findings) + return [f for f in findings if f.rule == "INV-4.10"] + + +def msgs(found): + return [f"{f.level}:{f.line}:{f.msg.split(' — ')[0]}" for f in found] + + +class Inv410(unittest.TestCase): + def test_android_drain_and_alloc_flagged(self): + got = lint_tree({"src/main/cpp/main.cpp": ANDROID_DRAIN}) + self.assertEqual(len(got), 2, msgs(got)) + self.assertTrue(all(f.level == lint.WARN for f in got)) + joined = " ".join(f.msg for f in got) + self.assertIn("vkQueueWaitIdle", joined) + self.assertIn("vkAllocateCommandBuffers + vkFreeCommandBuffers", joined) + self.assertIn("record_atlas", joined) + + def test_wait_on_just_submitted_fence_flagged(self): + got = lint_tree({"vk_renderer.cpp": DESKTOP_WAIT_OWN_FENCE}) + self.assertEqual(len(got), 1, msgs(got)) + self.assertEqual(got[0].level, lint.WARN) + self.assertIn("renderer.frameFence", got[0].msg) + self.assertEqual(got[0].line, 11) # the wait AFTER the submit, not the one before + + def test_frame_ring_clean(self): + self.assertEqual(lint_tree({"main.cpp": FRAME_RING}), []) + + def test_hand_rolled_ring_clean(self): + self.assertEqual(lint_tree({"main.cpp": HAND_RING}), []) + + def test_one_off_upload_not_flagged(self): + self.assertEqual(lint_tree({"main.cpp": ONE_OFF_UPLOAD}), []) + self.assertEqual(lint_tree({"main.cpp": INIT_RENDERER}), []) + + def test_exemption_on_submit_line(self): + self.assertEqual(lint_tree({"main.cpp": EXEMPT_ON_SUBMIT}), []) + + def test_exemption_on_function_header(self): + self.assertEqual(lint_tree({"main.cpp": EXEMPT_ON_HEADER}), []) + + def test_frame_name_without_draw_flagged(self): + got = lint_tree({"main.cpp": NAMED_FRAME_NO_DRAW}) + self.assertEqual(len(got), 1, msgs(got)) + self.assertIn("vkDeviceWaitIdle", got[0].msg) + + def test_single_fence_top_of_frame_is_info(self): + one_in_flight = DESKTOP_WAIT_OWN_FENCE.replace( + " VkResult waitResult = vkWaitForFences(device, 1, &renderer.frameFence, VK_TRUE, UINT64_MAX);\n", "") + got = lint_tree({"main.mm": one_in_flight}) + self.assertEqual(len(got), 1, msgs(got)) + self.assertEqual(got[0].level, lint.INFO) + self.assertIn("one frame in flight", got[0].msg) + + def test_commented_out_drain_ignored(self): + src = ANDROID_DRAIN.replace("\t\tvkQueueWaitIdle(g_vk_queue);", "\t\t// vkQueueWaitIdle(g_vk_queue);") + got = lint_tree({"main.cpp": src}) + self.assertEqual(len(got), 1, msgs(got)) # only the alloc/free churn remains + self.assertIn("vkAllocateCommandBuffers", got[0].msg) + + def test_lazy_alloc_without_free_clean(self): + # Allocate-once-on-first-use, reset every frame: no churn, no finding. + src = """ +void RenderFrame(App& a) +{ + if (a.cmd == VK_NULL_HANDLE) vkAllocateCommandBuffers(a.device, &ai, &a.cmd); + vkResetCommandBuffer(a.cmd, 0); + vkCmdDraw(a.cmd, 3, 1, 0, 0); + vkQueueSubmit(a.queue, 1, &si, a.fences[a.slot]); +} +""" + self.assertEqual(lint_tree({"main.cpp": src}), []) + + def test_headers_and_jvm_not_scanned(self): + self.assertEqual(lint_tree({"renderer.h": ANDROID_DRAIN, "Main.java": ANDROID_DRAIN}), []) + + +if __name__ == "__main__": + unittest.main(verbosity=2) From 4315360a54db7637279467b7022b8c5a7267a183 Mon Sep 17 00:00:00 2001 From: David Date: Sun, 11 Oct 2026 01:54:27 -0700 Subject: [PATCH 2/2] docs(app-rules): INV-4.10 Vulkan frame-submit discipline + scaffolder linux/android MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New INV-4.10 in §4: >=2 frames in flight, one vkQueueSubmit per frame per queue, never wait the frame just submitted, no queue/device drains on the frame path, command buffers allocated once. Why (Adreno msm-adreno-tz DVFS, #1917; per-frame cmd-buffer alloc, #1935/#1936), evidence (modelviewer#170, 13 -> 30 fps), the sanctioned dxr::VkFrameRing pattern (displayxr-common >= v2.28.0), the allowed exceptions and the lint heuristic. Checklist row + a cross-reference from the android build guide's DVFS paragraph. /new-displayxr-app accepts platform=linux|android with the VK cube references mapped, warns not to copy the cubes' current submit loop, and the per-app CLAUDE.md template states INV-4.10. No test app is changed. Co-Authored-By: Claude Opus 5.5 --- .claude/skills/new-displayxr-app/SKILL.md | 33 +++++++-- .../new-displayxr-app/templates/app-CLAUDE.md | 4 ++ docs/getting-started/android-build-guide.md | 2 +- docs/guides/displayxr-app-rules.md | 70 +++++++++++++++++++ 4 files changed, 101 insertions(+), 8 deletions(-) diff --git a/.claude/skills/new-displayxr-app/SKILL.md b/.claude/skills/new-displayxr-app/SKILL.md index 0da073e37a..51a1ed6687 100644 --- a/.claude/skills/new-displayxr-app/SKILL.md +++ b/.claude/skills/new-displayxr-app/SKILL.md @@ -2,11 +2,12 @@ name: new-displayxr-app description: | Scaffold a new DisplayXR OpenXR app from a known-good test app, wired to the authoring rules so it starts correct instead of tripping the common pitfalls. Clones the nearest cube_* reference app, parameterizes it, drops a workspace manifest + icon placeholders + a per-app CLAUDE.md, wires CMake, and runs the app linter until clean. - Usage: /new-displayxr-app [class=handle|texture|hosted] [api=d3d11|d3d12|gl|vk|metal] [platform=win|macos] + Usage: /new-displayxr-app [class=handle|texture|hosted] [api=d3d11|d3d12|gl|vk|metal] [platform=win|macos|linux|android] Examples: /new-displayxr-app photo_viewer class=handle api=d3d11 platform=win /new-displayxr-app stereo_player class=handle api=vk platform=macos /new-displayxr-app editor class=texture api=d3d11 platform=win + /new-displayxr-app tablet_viewer class=handle api=vk platform=android --- # new-displayxr-app — scaffold a correct DisplayXR app @@ -30,7 +31,8 @@ Resolve these; if any is missing or ambiguous, **ask the user** (AskUserQuestion - **class** — `handle` (owns its window; the default and simplest) | `texture` (weaved 3D in a regions expressed via `XR_DXR_display_zones`; the shared texture receives the runtime's zones composite) | `hosted` (runtime creates the window). - **api** — `d3d11` | `d3d12` | `gl` | `vk` | `metal`. -- **platform** — `win` | `macos`. +- **platform** — `win` | `macos` | `linux` | `android`. Linux and Android are **Vulkan-only** + (`api=vk`); anything else there has no reference app — say so and ask. - **location** — default `test_apps//___/` (in-tree; apps are grouped by **handoff class** folder — `handle/ hosted/ texture/ legacy/ probes/` — with `zones` a feature-name modifier, not a folder, so `cube_zones_*` go under `handle/` and @@ -42,11 +44,18 @@ Target name follows the convention `cube_*`-style: `___`), which the linter +checks from its `AndroidManifest.xml`. ## Procedure @@ -124,5 +133,15 @@ Pick the exact match; if none exists, pick the same **class+platform** (swap API `VkPhysicalDevicePresentIdFeaturesKHR` + `PresentWaitFeaturesKHR` yourself. Watch the proc names — the v1 spellings (`xrGetVulkanGraphicsRequirementsKHR`) are not dispatchable under enable2-only, a trap that shipped twice in demo repos. +- **Vulkan frame-submit discipline (INV-4.10):** every `vk` reference app today still submits + then waits (Android: per-frame command-buffer alloc + `vkQueueWaitIdle`; Windows/Linux: wait on + the fence just submitted; macOS: one fence, one frame in flight), and the linter WARNs on each. + Do **not** copy that loop into a new app. Render through `dxr::VkFrameRing` + (displayxr-common `common/vk_frame_ring.h`, ≥ v2.28.0): `beginFrame()` → record every tile into + one command buffer with `tileBarrier()` between tiles → `endFrame()` (the one submit) → + `xrReleaseSwapchainImage` → `xrEndFrame`; one-off uploads through `submitImmediate()`, resize / + teardown through `waitAll()`. On Android this is the difference between ~10 and ~30 fps (#1917). + Until the helper is tagged, copy the header in and note the pin to bump. The cube references are + converted in a follow-up — do not edit them as part of scaffolding. - The linter is advisory on WARNs but **blocking on ERRORs**; treat a clean linter run as the definition of "scaffold complete." diff --git a/.claude/skills/new-displayxr-app/templates/app-CLAUDE.md b/.claude/skills/new-displayxr-app/templates/app-CLAUDE.md index 70fb8cd2d8..69ff14c303 100644 --- a/.claude/skills/new-displayxr-app/templates/app-CLAUDE.md +++ b/.claude/skills/new-displayxr-app/templates/app-CLAUDE.md @@ -16,6 +16,10 @@ They are binding. The traps agents hit most: - **INV-4.3** — per-tile size = **window/canvas × `recommendedViewScaleX/Y`**, never display size. - **INV-4.6** — request an **sRGB swapchain** and store a correctly-encoded image (render linear with GPU sRGB-write, *or* write display-referred bytes — not both). Data textures stay linear. +- **INV-4.10** (Vulkan) — keep **≥2 frames in flight** with **one `vkQueueSubmit` per frame**; + never wait the fence you just submitted, no `vkQueueWaitIdle`/`vkDeviceWaitIdle` on the frame + path, allocate command buffers once and reset them. Use `dxr::VkFrameRing` (displayxr-common + `common/vk_frame_ring.h`). A serialised submit→wait loop parks mobile GPUs at ~1/3 clock. - **INV-2.4** — the runtime owns the active mode; you *request* via `xrRequestDisplayRenderingModeDXR` and update local state only on the change **event**. - **INV-7.1/7.2** — capture via `xrCaptureAtlasDXR` (prefix with **no** extension); never diff --git a/docs/getting-started/android-build-guide.md b/docs/getting-started/android-build-guide.md index ab69893495..b095153768 100644 --- a/docs/getting-started/android-build-guide.md +++ b/docs/getting-started/android-build-guide.md @@ -737,7 +737,7 @@ and `Leia CNSDK DP: alpha-gate pipeline ready`. ADPF hint sessions are **unsupported** on the Lume phone and the NP02J (Android 13, Adreno 740): `dumpsys performance_hint` reports `HAL Support: false`, so every app logs `ADPF: createSession failed` (the line now carries `APerformanceHint_getPreferredUpdateRateNanos` — `-1` confirms the HAL, `>0` would mean our arguments). Without hints the msm-adreno-tz governor parks the GPU near **220 MHz** (floor 124.8, max 680) at ~50% busy; pinning 680 MHz gave the model viewer +60% fps, while `cmd power set-fixed-performance-mode-enabled` changed nothing. Experiment knob, default **off** until measured: `adb shell setprop debug.dxr.sustained_perf 1` makes `MonadoView` request `Window.setSustainedPerformanceMode(true)` on the host Activity (one WARN line reports supported / applied / Activity class; service-owned overlays skip it). -**App rule:** keep **≥2 frames in flight**. A strictly serialised submit → `vkWaitForFences` → submit loop never looks busy to the governor and runs the GPU at about a third of its max clock. +**App rule:** keep **≥2 frames in flight**. A strictly serialised submit → `vkWaitForFences` → submit loop never looks busy to the governor and runs the GPU at about a third of its max clock. The full rule (one submit per frame, no per-frame drains or command-buffer allocation), the shared `dxr::VkFrameRing` helper and the lint check: [INV-4.10](../guides/displayxr-app-rules.md) in the app-authoring rules. ### Device not found by ADB diff --git a/docs/guides/displayxr-app-rules.md b/docs/guides/displayxr-app-rules.md index 1e2d4766bc..ceeb80a4b4 100644 --- a/docs/guides/displayxr-app-rules.md +++ b/docs/guides/displayxr-app-rules.md @@ -601,6 +601,75 @@ re-implementing — see [INV-8.1](#8-app-folder-layout--what-to-include)). most one event per view configuration per second — the runtime coalesces, as the spec requires. Advisory rule (WARN). Ref: `#1488`. +- **INV-4.10 — Vulkan: keep ≥2 frames in flight, ONE `vkQueueSubmit` per frame, never wait the + frame you just submitted.** How you feed the GPU is part of rendering correctly into the + swapchain, not an optimisation, because on mobile it decides the GPU clock. The rule is + platform-neutral: + 1. **≥2 frames in flight** (3 is the measured default) — a ring of command buffers + fences + indexed by frame slot, not one of each. + 2. **Exactly one `vkQueueSubmit` per frame per queue.** All tiles/views of the atlas go into + one command buffer, ordered by a barrier between tiles — never one submit per tile or per eye + (with or without a wait in between). + 3. **Never wait the fence of the frame you just submitted.** The only wait on the hot path is on + the slot's fence from N frames ago (normally long signalled). + 4. **No `vkQueueWaitIdle` / `vkDeviceWaitIdle` on the steady-state frame path.** + 5. **Command buffers are allocated once and reset** (`vkResetCommandBuffer` / a per-slot pool + reset) — never `vkAllocateCommandBuffers` + `vkFreeCommandBuffers` per frame. + + **Why.** Mobile GPU governors pick the clock from GPU *busy %*. A loop that submits, waits, then + submits again leaves the GPU idle while the CPU waits, so the governor reads ~50 % busy and parks + the GPU at about a third of its clock — every frame is then slower, which keeps it idle. Measured + on Adreno 740 (`msm-adreno-tz`, ADPF hint sessions unsupported): the GPU sits near 220 MHz of + 680, ~10 fps ([#1917](https://github.com/DisplayXR/displayxr-runtime/issues/1917); see + [android-build-guide § Low frame rate with an idle-looking GPU](../getting-started/android-build-guide.md#low-frame-rate-with-an-idle-looking-gpu-the-clock-never-ramps-1917)). + Per-frame command-buffer allocation adds a fresh zeroed allocation in the Adreno driver every + frame ([#1935](https://github.com/DisplayXR/displayxr-runtime/issues/1935) / + [#1936](https://github.com/DisplayXR/displayxr-runtime/pull/1936) — the runtime's own `vk_native` + compositor was fixed the same way). On a desktop dGPU the same mistake costs only the lost + CPU↔GPU overlap, which is why it survives desktop testing. + + **Evidence.** `displayxr-demo-modelviewer` on the Leia phone went **13 → 30 fps** by keeping 3 + frames in flight with one submit per frame and nothing else changed + ([modelviewer#170](https://github.com/DisplayXR/displayxr-demo-modelviewer/pull/170)). + + **Sanctioned pattern — `dxr::VkFrameRing`** (displayxr-common `common/vk_frame_ring.h`, + ≥ v2.28.0, header-only; [displayxr-common#74](https://github.com/DisplayXR/displayxr-common/pull/74)): + ```cpp + dxr::VkFrameRing ring; // init(device, physDevice, queue, family, cfg) once + // per frame: + xrWaitFrame / xrBeginFrame / xrLocateViews + xrAcquireSwapchainImage + xrWaitSwapchainImage + VkCommandBuffer cmd = ring.beginFrame(); // waits only the slot from N frames ago + for (uint32_t i = 0; i < viewCount; ++i) { + if (i > 0) dxr::VkFrameRing::tileBarrier(cmd); // shared targets between tiles + // ... record tile i into its atlas sub-rect (per-slot UBO via ring.viewUbo(i, &dynOff)) ... + } + ring.endFrame(); // the ONE vkQueueSubmit; never waits + xrReleaseSwapchainImage // AFTER endFrame: the runtime composes on the same queue + xrEndFrame + ``` + **Allowed exceptions:** genuine one-off uploads (textures, meshes, a static quad-layer fill) go + through `ring.submitImmediate()` — the one named blocking escape hatch; resize and teardown drain + with `ring.waitAll()`; a resource the GPU may still be reading is released with + `ring.deferDestroy()`. `DXR_VK_FRAMES_IN_FLIGHT=1` reproduces the anti-pattern for an A/B + (desktop only — Android processes pass `Config::framesInFlight` explicitly). + + **Lint (WARN, advisory).** `scripts/check_displayxr_app.py` finds the enclosing function of every + `vkQueueSubmit` in a `.c/.cc/.cpp/.cxx/.mm` source by brace matching (falling back to a ±60-line + window when no function body can be delimited) and judges it only if it is on the **frame path** + — it records a draw / render pass / dispatch, calls `xrWaitFrame`/`xrBeginFrame`/`xrEndFrame`, or + is named `*render*` (not `*Renderer*`) / `*draw*` / `*frame*` / `*present*`. Any other function is + treated as a one-off upload and left alone. In a frame-path function it flags: (a) + `vkQueueWaitIdle`/`vkDeviceWaitIdle` anywhere in the body; (b) `vkWaitForFences` *after* the + submit on the same handle passed as its fence; (c) `vkAllocateCommandBuffers` **and** + `vkFreeCommandBuffers` both in the body. As INFO only, (d) one un-indexed fence waited at the top + of the function and signalled by its submit (at most one frame in flight — a fence reached through + a slot pointer looks the same to a text matcher). A deliberate blocking submit (a capture + readback tool, a single-shot test) is exempted with `// dxr-lint: allow-blocking-submit ` + on the submit line or on the function's signature line. It does not see a per-frame drain hidden + behind a helper call in another function, and does not count submits per frame. Ref: `#1917`, + `#1935`, `modelviewer#170`, `displayxr-common#74`. + --- ## 5. Texture apps: express regions via display-zones @@ -1073,6 +1142,7 @@ macOS app (no manifest → no Android findings). - [ ] Color space: request an **sRGB swapchain** and write a correctly-encoded image (linear render + GPU sRGB-write, or display-referred bytes — not both); a linear/UNORM swapchain is read as LINEAR and encoded by the runtime (display-referred bytes in it wash out); same `_SRGB`-first choice on every leg; data textures always linear (INV-4.6) - [ ] Whole declared `imageRect` is written — partial-tile renders clear the full tile to `(0,0,0,0)` first (or shrink the rect); no undefined pixels reach the atlas, esp. transparent-bg (INV-4.7) - [ ] (if `XR_EXT_view_configuration_views_change` is enabled) the handler re-enumerates and **moves `subImage.imageRect`** — no `xrCreateSwapchain` from the event; filters on `systemId` **and** `viewConfigurationType` (INV-4.9) +- [ ] (Vulkan) ≥2 frames in flight, ONE `vkQueueSubmit` per frame, no wait on the frame just submitted, no `vkQueueWaitIdle`/`vkDeviceWaitIdle` on the frame path, command buffers allocated once — `dxr::VkFrameRing` (INV-4.10) - [ ] (texture) regions declared via display-zones — 3D zones + Local2D zones, not output-rect/surround (INV-5.1/5.3) - [ ] (texture) shared surface sized once to worst-case; full composite presented (INV-5.2) - [ ] Window-relative Kooima; matrices transposed for DirectX (INV-6.3/6.4)