fix(bnr): gracefully handle GPUs with no published AFX pack (Fixes #3933)#3972
Conversation
There was a problem hiding this comment.
Nice fix — the root cause (gating BNR on cc >= 89 alone, which let sm_120 pass even though only sm_89 ships a pack) is diagnosed correctly, and splitting the check into capability (isAfxCapableGpu) vs pack availability (hasSupportedGpu) is the right factoring. The logic reads cleanly:
hasSupportedGpu() = isAfxCapableGpu() && kPublishedComputeCaps.contains(cc)correctly narrows to archs with a published pack while keeping the existing enable-gate caller unchanged.detectedComputeCap()returning-1for non-sm_/ non-NVIDIA is a clean sentinel, anddetectArch()is cached so the repeated calls are cheap.- Both dead-end surfaces (selector tooltip + Download button) now distinguish "too old" from "AFX-capable but no pack yet," which is exactly the UX the issue asked for. All six CI checks are green.
Two minor, non-blocking notes:
-
Redundant preprocessor guard in
updateBnrStatus(). The new#ifdef HAVE_NVIDIA_AFXblock (around line 1005) is nested inside the function body, which is already wrapped in#ifdef HAVE_NVIDIA_AFXat line 958. The inner guard is harmless but never false here — you can drop it. (The constructor edit doesn't need one since it's already in the#else/HAVE_NVIDIA_AFXbranch.) -
Download button on old/non-NVIDIA GPUs (
!hasSupportedGpu()and!isAfxCapableGpu()): the button is now disabled but keeps its"Download (~1 GB)"text and no explanatory tooltip. In practice this path is unreachable — the BNR selector button itself is disabled for those GPUs, so the page can't be opened — so this is cosmetic only. Worth a one-lineelsefor symmetry if you touch it again, but not required.
Conventions (AppSettings, RAII, C++20) are all fine — no settings or lifetime surface here. Thanks for the thorough live-hardware verification on the actual sm_120 card and for scoping out the NGC-dependent pack publishing cleanly. LGTM.
🤖 aethersdr-agent · cost: $3.5180 · model: claude-opus-4-8
jensenpat
left a comment
There was a problem hiding this comment.
Please revert the CHANGELOG.md change, that is handled by our automated tag process.
…r#3933) On an NVIDIA GPU new enough for AFX but with no published denoiser pack for its exact arch, notably sm_120 (consumer Blackwell / RTX 50-series), the BNR button and Download were offered and then silently failed (the per-arch pack 404s). Gate BNR availability on the set of archs that actually have a published pack, splitting the GPU check into isAfxCapableGpu() (Ada+) vs hasSupportedGpu() (published pack exists), so an AFX-capable-but-unpublished card shows a clear "No BNR pack for your GPU (sm_120) yet, use DFNR" state instead of a dead-end download. Fixes aethersdr#3933. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
58efb12 to
2053071
Compare
|
@jensenpat Done — reverted the CHANGELOG.md change; understood it's handled by the automated tag process. The branch is now source-only ( 73, KM6LFY |
What & why
On an NVIDIA GPU new enough for AFX but with no published denoiser pack for its exact arch — notably sm_120 (consumer Blackwell / RTX 50-series) — the BNR button and its Download were offered and then silently failed: the per-arch
afx-bitspack 404s (onlysm_89is published today). BNR looked supported but could never install, with no message steering the user to DFNR (#3933).Root cause:
hasSupportedGpu()gated only on compute capability (cc >= 89), sosm_120(cc 120) passed the gate even though no pack exists for it.The fix
Split the GPU check into two questions:
isAfxCapableGpu()— is the GPU new enough for AFX at all (Ada / RTX 40-series,cc >= 89)? Says nothing about pack availability.hasSupportedGpu()— is a pack actually published for the detected arch? NowisAfxCapableGpu() && kPublishedComputeCaps.contains(cc), wherekPublishedComputeCaps = { 89 }(kept in sync with theafx-bitsrelease assets + thebuild-afx-bitsValidateSet).The BNR UI enables the feature on
hasSupportedGpu()(unchanged), but now usesisAfxCapableGpu()to tell apart "GPU too old" from "AFX-capable but no pack yet":Scope: this is suggestion #1 from the issue (the part fully in AetherSDR's control). Publishing an actual
sm_120pack (#3933 suggestion #2) is gated on NVIDIA shipping a consumer-Blackwell Maxine model to NGC and is out of scope here — when it lands, add120tokPublishedComputeCapsand upload the assets.Testing
Build: clean (Linux,
-DENABLE_NVIDIA_AFX=ON, Qt 6.8.3). OnlyhasSupportedGpu()'s two callers exist, both in the DSP widget and both updated — no collateral behavior change (a machine with a published pack, e.g.sm_89, still reportshasSupportedGpu()==trueand behaves exactly as before; an RTX 20/30 still gets the "too old" message).Live on the exact issue hardware — RTX 5060 Laptop GPU, compute_cap 12.0 (
sm_120) — via the Automation BridgedumpTree(detectArch()reportssm_120):objectName)enableddspMethodBtnBNR)Both dead-end surfaces are now gated off with a clear message on the real GPU, instead of offering a 404 Download.
Fixes #3933.
🤖 Generated with Claude Code