DVR reencode: guarantee <4GB physical placement for RGA-touched buffers

[henkwiedig]

RGA2's MMU can only address physical memory below 4GB. A kernel bug in the Rockchip GEM allocator (__GFP_DMA32 is only ever applied under CONFIG_ARM_LPAE, a 32-bit-only Kconfig symbol never set on arm64) means buffers can silently land above that boundary on >=2GB RAM boards, which a field support-package capture confirmed: RGA rejected a job with "unsupported memory larger than 4G", caught cleanly by the existing fail-fast guard.

MPP's own MPP_BUFFER_FLAGS_CONTIG/DMA32 flags turned out not to reliably route to CMA on this platform -- they silently redirect through the dma-heap backend, which expects a heap name ("cma-dma32" etc.) this kernel's dma-heap driver doesn't expose, so the whole buffer group fails to construct with no diagnostic output. Instead, on boards with enough RAM for this to matter (checked via mem_info.h's runtime /proc/meminfo probe, not a compile-time board flag), the two FrameColorCorrect GBM render targets and FrameProcessor's proc_copy_ working buffer are allocated as raw CMA-backed KMS dumb buffers (ROCKCHIP_BO_CONTIG) and imported directly, bypassing GBM/MPP's allocator abstractions for just these buffers. Falls back to the existing GBM/MPP paths when CONTIG allocation itself fails (e.g. CMA exhausted), consistent with the project's fail-fast philosophy: a transient allocation failure on the working buffer is fatal (stops reencode, notifies via OSD) rather than silently risking a buffer placed >=4GB again; the GBM render targets fall back per-target since RGA doesn't care how a buffer was allocated once it's <4GB.

Hardware-validated via fault injection (forcing the large-RAM path on a 1GB device): CONTIG allocation succeeds until CMA is exhausted, then falls back cleanly; 5x rapid start/stop cycles showed no crashes, fd leaks, or corrupted output. Confirmed inert (unchanged GBM/MPP paths) with the real RAM check on the same 1GB device.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

DVR reencode: force CMA/CONTIG (<4GB) buffers for RGA2 on large-RAM boards

🐞 Bug fix ✨ Enhancement 🕐 40+ Minutes

AI Description

• Detect large-RAM boards at runtime and route RGA-touched buffers to CMA (<4GB).
• Allocate CONTIG KMS dumb-buffers and import via dma-buf, bypassing GBM/MPP abstractions.
• Fail fast or fall back safely when CONTIG allocation/export is unavailable or exhausted.

Diagram

graph TD
  A["FrameProcessor"] --> B{"platform_has_large_ram()"} --> C["DRM dumb-buffer\nCONTIG alloc"] --> D["dma-buf prime fd"] --> E["MPP import\n(proc_copy_)" ] --> F["GL+RGA pipeline"]
  B --> G["MPP hold_grp\nalloc"] --> F
  H["FrameColorCorrect"] --> B --> I["CONTIG RGBA targets"] --> D
  H --> J["GBM RGBA targets"] --> D
  F --> K["RGA2 MMU\n(<4GB req)"]
  subgraph Legend
    direction LR
    _dec{"Decision"} ~~~ _comp["Component"] ~~~ _buf["Buffer/FD"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fix kernel allocator / expose proper DMA32 heap

• ➕ Eliminates userspace workarounds and special-casing
• ➕ Benefits all users of Rockchip GEM / dma-heap, not just this app
• ➕ Keeps allocation logic within standard GBM/MPP abstractions
• ➖ Not actionable for deployed field units without kernel rollout
• ➖ May be gated by vendor kernel constraints and release timelines

2. Use dma-heap directly (e.g., /dev/dma_heap/*) for DMA32/CMA

• ➕ More explicit control than GBM/MPP flags; avoids Rockchip dumb-buffer vendor flags
• ➕ Potentially simpler lifetime model (fd-only) than GEM handle tracking
• ➖ Requires heap naming/availability that varies across kernels (this PR notes missing cma-dma32 exposure)
• ➖ Still a platform-specific integration surface to maintain

3. Always allocate CONTIG/CMA for these buffers

• ➕ Simpler logic; avoids runtime branching and differing behavior across SKUs
• ➖ Unnecessary memory pressure on 1GB boards; higher risk of CMA exhaustion
• ➖ May reduce system stability/performance where contiguous memory is scarce

Recommendation: Given the inability to rely on MPP CONTIG/DMA32 flags and the kernel-side allocator bug on arm64, the PR’s targeted userspace workaround is the most practical near-term fix: it only special-cases the specific RGA2-touched buffers, gates it by actual installed RAM, and defines clear failure behavior (fallback for render targets; fail-fast for the working buffer to avoid silent corruption). Long-term, prefer a kernel fix (proper GFP_DMA32 behavior and/or correct dma-heap exposure) to retire the workaround.

Files changed (6) +220 / -18

Enhancement (2) +47 / -0

mem_info.hAdd runtime /proc/meminfo gate for enabling large-RAM workaround +33/-0

Add runtime /proc/meminfo gate for enabling large-RAM workaround

• Adds platform_has_large_ram(), which reads MemTotal from /proc/meminfo and returns true above a conservative threshold, enabling the <4GB/CMA allocation workaround only where physically relevant.

src/mem_info.h

rockchip_bo.hDefine Rockchip CONTIG dumb-buffer flag missing from sysroot headers +14/-0

Define Rockchip CONTIG dumb-buffer flag missing from sysroot headers

• Adds a local definition of ROCKCHIP_BO_CONTIG (from rockchip_drm uapi) to request CMA-backed contiguous memory via drm_mode_create_dumb.flags.

src/rockchip_bo.h

Bug fix (4) +173 / -18

frame_colorcorrect.cppAllocate GL render targets as CONTIG dumb-buffers on large-RAM boards +82/-16

Allocate GL render targets as CONTIG dumb-buffers on large-RAM boards

• Adds a runtime large-RAM gate and, when enabled, allocates RGBA render targets via DRM dumb-buffers with ROCKCHIP_BO_CONTIG and PRIME-exports them as dma-bufs. Falls back to the prior GBM allocation path when CONTIG creation fails, and extends teardown to close raw GEM handles when GBM isn’t used.

src/frame_colorcorrect.cpp

frame_colorcorrect.hTrack GEM handle for non-GBM (CONTIG) render targets +3/-0

Track GEM handle for non-GBM (CONTIG) render targets

• Extends Target to store a GEM handle for the CONTIG dumb-buffer path so destroy_targets() can close the GEM object when GBM is bypassed.

src/frame_colorcorrect.h

frame_processor.cppImport CMA-backed CONTIG working buffer for proc_copy_ on large-RAM boards +80/-2

Import CMA-backed CONTIG working buffer for proc_copy_ on large-RAM boards

• Introduces alloc_contig_proc_copy() to allocate a ROCKCHIP_BO_CONTIG dumb-buffer, export to dma-buf, and import into MPP as proc_copy_. Updates process_loop() to use this path on large-RAM systems and treat allocation failure as fatal (no fallback to hold_grp) to avoid >=4GB placement risk.

src/frame_processor.cpp

frame_processor.hDeclare CONTIG allocation helper for proc_copy_ +8/-0

Declare CONTIG allocation helper for proc_copy_

• Adds documentation and a private helper declaration describing why proc_copy_ uses a DRM dumb-buffer CMA allocation on large-RAM boards and why failure is not downgraded to the MPP buffer-group fallback.

src/frame_processor.h

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Missing CLOEXEC on PRIME fd ✓ Resolved 🐞 Bug ⛨ Security

Description

The new PRIME_HANDLE_TO_FD exports in FrameColorCorrect::create_targets() (and the similar
FrameProcessor path) omit DRM_CLOEXEC, so dma-buf fds can be inherited across exec() and
unintentionally kept alive. This differs from existing PRIME-export code in the repo that sets
DRM_CLOEXEC | DRM_RDWR for the same ioctl.

Code

src/frame_colorcorrect.cpp[R241-244]

+                dph.handle = dmcd.handle;
+                dph.flags  = DRM_RDWR;
+                dph.fd     = -1;
+                do {

Evidence

The PR adds PRIME exports with DRM_RDWR only, while existing code in the repository exports PRIME
fds with DRM_CLOEXEC | DRM_RDWR, demonstrating the omission in the new paths.

src/frame_colorcorrect.cpp[239-246]
src/frame_processor.cpp[54-61]
src/drm.c[360-367]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New PRIME_HANDLE_TO_FD calls set `dph.flags = DRM_RDWR` only. Without `DRM_CLOEXEC`, the exported dma-buf fd is inheritable across `exec()` unless callers apply `FD_CLOEXEC` later.
## Issue Context
The repo’s existing PRIME export path uses `DRM_CLOEXEC | DRM_RDWR`, so these new allocations should match that behavior for consistency and to prevent fd inheritance.
## Fix Focus Areas
- src/frame_colorcorrect.cpp[239-246]
- src/frame_processor.cpp[54-61]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Dumb-buffer destroy mismatch ✓ Resolved 🐞 Bug ☼ Reliability

Description

FrameColorCorrect::destroy_targets() tears down the CONTIG dumb-buffer path using
DRM_IOCTL_GEM_CLOSE even though the buffer was created via DRM_IOCTL_MODE_CREATE_DUMB. Elsewhere in
this repo, dumb buffers are consistently released with DRM_IOCTL_MODE_DESTROY_DUMB, so this mismatch
risks bypassing driver-specific dumb-buffer cleanup and makes lifetime handling inconsistent.

Code

src/frame_colorcorrect.cpp[R515-518]

+            struct drm_gem_close dgc;
+            memset(&dgc, 0, sizeof(dgc));
+            dgc.handle = t.gem_handle;
+            ioctl(drm_fd_, DRM_IOCTL_GEM_CLOSE, &dgc);

Evidence

The CONTIG path stores a GEM handle from MODE_CREATE_DUMB but later closes it with GEM_CLOSE; in
contrast, other dumb-buffer allocations in the repo are released via MODE_DESTROY_DUMB, indicating
the expected teardown mechanism and the inconsistency introduced here.

src/frame_colorcorrect.cpp[224-260]
src/frame_colorcorrect.cpp[512-520]
src/drm.c[418-431]
src/main.cpp[201-206]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A buffer created via `DRM_IOCTL_MODE_CREATE_DUMB` should be destroyed via `DRM_IOCTL_MODE_DESTROY_DUMB` (or the libdrm equivalent). The current CONTIG path uses `DRM_IOCTL_GEM_CLOSE`, which is inconsistent with other dumb-buffer lifecycle code in this repo.
## Issue Context
`src/drm.c` and `src/main.cpp` both free dumb buffers with `DRM_IOCTL_MODE_DESTROY_DUMB`. Align FrameColorCorrect’s CONTIG path with that pattern.
## Fix Focus Areas
- src/frame_colorcorrect.cpp[224-260]
- src/frame_colorcorrect.cpp[512-520]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. FD leak on import failure ✓ Resolved 🐞 Bug ☼ Reliability

Description

FrameProcessor::alloc_contig_proc_copy() can return after a failed mpp_buffer_import() without
closing the exported PRIME fd when MPP does not replace info.fd, leaking a dma-buf reference and fd.
This is a PR-introduced failure-path leak in the new large-RAM allocation path.

Code

src/frame_processor.cpp[R82-85]

+    MPP_RET mret = mpp_buffer_import(&proc_copy_, &info);
+    if (dph.fd != info.fd) close(dph.fd);  // mpp_buffer_import dups the fd
+    if (mret != MPP_OK) {
+        spdlog::error("FrameProcessor: CONTIG buffer import failed ({})", (int)mret);

Evidence

The code exports dph.fd, then only conditionally closes it before checking mret. When
mpp_buffer_import() fails, the function returns with no unconditional close of the exported fd,
leaving a visible leak path in the new code.

src/frame_processor.cpp[54-61]
src/frame_processor.cpp[77-88]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
After exporting `dph.fd`, `mpp_buffer_import()` is called. On failure, the function returns without guaranteeing the exported fd is closed (it is only closed when `info.fd` is changed by MPP).
## Issue Context
If `mpp_buffer_import()` fails without consuming/closing the provided fd (common API contract), the exported dma-buf fd is leaked. Ensure all failure paths close the original exported fd unless ownership is explicitly transferred.
## Fix Focus Areas
- src/frame_processor.cpp[77-88]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}