Skip to content

sys: add fixed DMA-BUF VDEC input pool - #51

Open
yanyongxian wants to merge 5 commits into
spacemit-com:mainfrom
yanyongxian:feature/vdec-dmabuf-input-pool
Open

yanyongxian wants to merge 5 commits into
spacemit-com:mainfrom
yanyongxian:feature/vdec-dmabuf-input-pool

Conversation

@yanyongxian

@yanyongxian yanyongxian commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Purpose

Make the opt-in fixed DMA-BUF input path safe for sustained decoding and
teardown, not just a one-frame smoke test. Avoid per-frame CMA allocation and
reduce compressed-payload copies; do not claim a throughput improvement or a
kernel CMA allocator fix. This updates the existing fixed-input-pool PR.

The initial implementation could exhaust its own input ring because reclaim
depended on the next packet. Subsequent testing also exposed a poll wait-list
warning during capture queue reconstruction and a receive/unbind lease race.

Changes

  • Keep compressed packets in preallocated mapped SYS slots and pass leased
    DMA-BUF fds to V4L2 OUTPUT; retain the legacy copying path by default.
  • Reap input DQBUF independently in the existing codec event thread, with
    nonblocking device I/O and eventfd wakeup. No extra thread or fixed delay is
    added to normal frame processing.
  • Add a condition-variable handshake around capture queue reconstruction,
    flush/reset and shutdown; wait for poll and event handling to quiesce before
    replacing queues, and join before freeing codec resources.
  • Return unaccepted leases on failure/stop; keep accepted packets, including
    EOS, hardware-owned until input DQBUF or stream-off.
  • Pair CPU DMA-BUF START/END synchronization in fixed-slot send, compatibility
    receive and in-place MJPEG zero-DRI normalization.
  • Detect direct input from the configured V4L2 memory type, not a possibly
    zero-initialized fd, and return the actual legacy QBUF result.
  • Keep the stream queue mutex held from unbind lease validation through reset;
    use a locked reset helper without recursive locking.
  • Enforce token field widths at compile time, including the plugin's signed
    32-bit token limit, and reject unencoded high bits on release.
  • Add sustained decode/pixel benchmarks, concurrent receive/unbind coverage,
    early-stop and EOS hardware tests, and docs/vdec_dmabuf_pool_validation.md.
  • Preserve isolated plugin loading through MPP_PLUGIN_DIR; production library
    installation is not performed by these tests.

Validation

Environment: x86 host Debug build and isolated native K3 RelWithDebInfo build
on 10.0.90.98, Linux 6.18.3-generic; board compilation/tests use CPU 0-7.
Earlier throughput comparisons used 10.0.91.119. No production UAV, system
codec library, kernel, DTB or frequency setting was replaced.

Build (PASS; substitute Debug for the host build):

cmake -S . -B build-fix -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DBUILD_TESTS=ON -DBUILD_ROS2_EXAMPLES=OFF
cmake --build build-fix --parallel 8 --target \
  mpp v4l2_linlonv5v7_codec2 test_sys_stream_dmabuf_pool \
  test_vdec_bound_dmabuf_input test_vdec_input_benchmark \
  test_vdec_input_retry test_mjpeg_zero_dri
export LD_LIBRARY_PATH="$PWD/build-fix/lib"
export MPP_PLUGIN_DIR="$PWD/build-fix/al/vcodec"
ctest --test-dir build-fix \
  -R '^(test_vdec_input_retry|test_mjpeg_zero_dri)$' --output-on-failure
  • Host and K3 retry/zero-DRI unit tests: PASS. Host CMA test is SKIP (77),
    because /dev/dma_heap/linux,cma is absent, not a hardware PASS.
  • test_sys_stream_dmabuf_pool: PASS on K3, covering exhaustion, reuse,
    lease-protected unbind, compatibility receive, malformed high-bit tokens
    and 200 concurrent receive/unbind iterations.
  • test_vdec_bound_dmabuf_input test/assets/1920x1080.jpg MODE: PASS for
    empty/queued/eos, 20 runs each after the review fixes; EOS reaches
    ERR_VDEC_EOS and cleanup/unbind succeeds.
  • Lifecycle handshake validation: 100 startup/initial source-change/33-frame
    decode/teardown cycles; 20 additional decode/stop runs: PASS.
  • One-slot and two-slot DMA pools: 120 measured frames each, PASS.
  • test_vdec_input_benchmark output-240.mjpg 4000 1200 MODE 32 12 0 1:
    legacy and dmabuf both hash 64 visible NV12 frames to 3c5726c46851e96a.
  • test_vdec_input_benchmark output-240.mjpg 4000 1200 dmabuf 5400 12 30
    with 231 MiB additional fixed CMA buffers and 32 MiB/s buffered writes in
    256 KiB chunks for 180 seconds: PASS; 5,400 frames, zero hot-path allocations,
    zero PTS errors. Repeated after the review fixes. Detailed timings are in
    the validation document.
  • test_vdec_input_benchmark output-240.mjpg 4000 1200 dmabuf 3600 12 60:
    PASS, 59.988 FPS, zero hot allocations / PTS errors.
  • Kernel taint remained zero after the lifecycle fix; CMA bitmap usage returned
    to the 1,958-page baseline. git diff --check: PASS.

Review responses

  1. SYS_UnBind: the old unlock/relock sequence was not recursive double locking,
    but the gap did allow an already-looked-up receiver to create a lease before
    reset. Lease checking and reset are now one queue-locked critical section.
  2. DMA synchronization: START/END pairs now surround CPU accesses, including
    the MJPEG normalization write, and failures are handled.
  3. Tokens: current limits are 128 binds and 16 slots, so they did not overflow;
    static assertions now enforce the encoding and signed plugin constraints.
    Release rejects tokens with extra high bits instead of silently truncating.
  4. EOS: do not release a successfully queued input in the MPI EOS branch.
    Hardware still owns it. The independent poll reaper returns it on DQBUF,
    with stream-off as teardown reclamation. Unaccepted inputs are returned by
    MPI. An EOS/drain/cleanup regression is included.

Configuration and compatibility

  • bEnableInputDmaBuf stays false by default. Enable it only for a same-process
    SYS compressed-stream bind configured with SYS_ConfigStreamDmaBufPool.
  • Fixed slot capacity must cover the largest compressed packet AND the driver's
    advertised V4L2 OUTPUT buffer size. Tests use twelve 6 MiB slots for 4000x1200
    MJPEG and twelve 3 MiB slots for 1920x1080 JPEG.
  • DMA-BUF fds/mappings are process-local. Cross-process use remains on the
    existing copy API. Consumers must be rebuilt against the updated headers;
    shared-memory layout version is 5. Do not mix old/new shared-memory users.
  • The 240-frame benchmark fixture contains original MJPEG packets copied from
    an existing recording, not live USB input. No complete UAV/live-camera,
    multi-camera or comprehensive mid-stream resolution-switch test is claimed.
  • Initial testing reproduced kernel list_del/poll_freewait warnings; the
    lifecycle fix avoids the overlap and the subsequent bounded tests did not
    reproduce the warning. It does not prove all driver races absent.
  • Independent dirty-file-folio CMA migration ENOMEM remains a kernel issue.
    Preallocation avoids its steady-state trigger, not allocation failures during
    startup, growth or other components. Fresh-reboot and earlier fragmented
    memory states differ; matching pool occupancy/write rate does not make them
    identical. No global sync/drop_caches workaround is introduced.
  • Earlier A/B throughput was similar (about 104-107 FPS); direct input used
    somewhat more process CPU. The benefit is fewer allocations and copies,
    not a demonstrated FPS gain.

Review follow-up: 2026-09-16

The following two follow-up commits were tested together on K3 10.0.91.119. They update this existing PR; no UAV or camera-sdk changes are included. The details below extend the earlier validation scope and explicitly record remaining live-recording gaps.

uvc: release only owned capture base references

Purpose

Fix the UVC shutdown double release observed while validating PR #51 with a
live 4000x1200 MJPEG stereo camera. Capture can release a slot's base reference
before the recycle worker exits; teardown must not release it a second time.
This occurred on both legacy and fixed DMA-BUF decoder-input paths.

Changes

  • Track capture/driver base-reference ownership separately for each UVC slot.
  • Mark ownership after initial VB acquisition and successful recycle QBUF.
  • Serialize capture release and recycle ownership changes with the UVC mutex.
  • Release only still-owned base references during error cleanup and teardown.
  • Leave depth-queue and external-consumer reference ownership unchanged.

Validation

  • K3 10.0.91.119, Linux 6.18.3-generic, isolated native RelWithDebInfo build
    under /root/uav-dmabuf-validation-20260916; tests pinned to CPU 0-7.
  • Before the fix, both legacy/fixed live tests logged VB_ModReleaseBuffer
    rejection with mod_ref=0 during shutdown. After the fix, 20 SDK camera
    start/capture/stop cycles passed: 1200 frames and 12020 valid IMU samples.
  • External camera integration command: with staged MPP_PLUGIN_DIR and
    LD_LIBRARY_PATH, taskset -c 0-7 ./sdk_live /dev/video13 60 20.
    Evidence: logs/sdk-live-20-cycles.log in the isolated validation directory.
  • Five additional cycles with the full review follow-up candidate passed:
    taskset -c 0-7 ./sdk_live /dev/video13 60 5; 300 frames, 3005 IMU samples,
    no duplicate-release errors or backward timestamps.
  • CMA actual usage returned to the 1346-page baseline; kernel taint stayed 0.
  • git diff --check: PASS. No system MPP/plugin replacement performed.

Configuration and compatibility

No public API or defaults change. This is UVC ownership bookkeeping, not a
kernel allocator fix or a claim of lossless delivery under storage pressure.
Four-camera and full VIO/LAS2/nvblox/DRM tests are outside this validation.
This focused commit is added to the existing fixed-input-pool PR as requested;
no UAV or camera-sdk application changes are included.

vdec: verify failed-init cleanup and DMA pool limits

Purpose

Address the robot review of PR #51 at e2a7ac5 using ownership evidence and
regression tests. Fix an actual device-fd leak on partial initialization, and
distinguish invalid packet capacity from temporary input-pool exhaustion.

Changes

  • Initialize the decoder device fd to -1; close any nonnegative owned fd even
    if codec construction failed, including the valid fd-zero case.
  • Log eventfd failure and document caller-owned create/destroy semantics.
    Do not destroy context locks inside init: its normal destructor still needs
    them to broadcast/join and then destroy synchronization exactly once.
  • Add a hardware-independent failure-injection CTest using the real decoder
    implementation: eventfd failure, device-open failure, codec-create failure,
    and codec-create failure with fd zero; 25 iterations per case.
  • Retain the correct size > capacity comparison, but return SYS_ERR_INVAL for
    oversized fixed-pool packets. SYS_ERR_FULL continues to mean transient slot
    or queue exhaustion. No fallback allocation/growth is introduced.
  • Test capacity-1, capacity and capacity+1, including reuse after rejection.
  • Document the three review responses, UVC fix, live-camera evidence and known
    recording limitations in docs/vdec_dmabuf_pool_review.md.

Validation

Build: x86 Debug and isolated native K3 RelWithDebInfo on 10.0.91.119,
Linux 6.18.3-generic. Board build/tests use CPU 0-7 and staged libraries only.

cmake -S . -B build-review -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DBUILD_TESTS=ON -DBUILD_ROS2_EXAMPLES=OFF
cmake --build build-review --parallel 8
export LD_LIBRARY_PATH="$PWD/build-review/lib"
export MPP_PLUGIN_DIR="$PWD/build-review/al/vcodec"
ctest --test-dir build-review --timeout 20 --output-on-failure \
  -R '^(test_sys|test_vb|test_integration|test_multiproc|test_mux_common|test_mux_socket_disconnect|test_vdec_input_retry|test_vdec_init_cleanup|test_mjpeg_zero_dri)$'
build-review/test/test_sys_stream_dmabuf_pool
for mode in empty queued eos; do
  build-review/test/test_vdec_bound_dmabuf_input test/assets/1920x1080.jpg "$mode"
done
  • Host cleanup/input-retry/zero-DRI: 3/3 PASS. K3 selected CTest: 9/9 PASS.
  • Negative control: reinstating the old fd-close condition makes the new
    cleanup test FAIL with leaked device fd 6. Fixed code passes all 100 cases,
    stable fd counts, exactly two mutex destroys and one condition destroy.
  • K3 capacity boundaries/exhaustion/reuse/200 concurrent unbind races: PASS.
    Host pool test SKIP (77), because the DMA heap is absent, not hardware PASS.
  • K3 bound VDEC empty/queued/EOS hardware tests: PASS.
  • Pixel A/B: test_vdec_input_benchmark output-240.mjpg 4000 1200 MODE 64 12 0 1 1 passes for legacy/dmabuf. The 32 warmup plus 64 measured
    NV12 frames hash to 15d8f2152717f030 in both modes; zero PTS errors;
    measured input allocations are 64 versus zero.
  • Full candidate: five more camera start/capture/stop cycles PASS, 300 frames
    and 3005 IMU samples, no duplicate-release errors or backward timestamps.
  • Full candidate, 60-second live-camera/record/receive run: stereo 59.841 FPS,
    raw IMU 598.448 Hz, fused IMU 598.451 Hz; zero publisher DMA heap allocations
    after 5 seconds; OUTPUT QBUF 3458 DMABUF / 0 MMAP; no publisher/receiver
    error logs. CMA usage returned to 1346 pages; kernel taint remained zero.
  • Recording is NOT claimed lossless: 3430 recorded frames, four missing
    published sequence indices, maximum image gap 249.6 ms and IMU gap 232.96 ms.
    Packet/index counts and sizes matched; timestamps remained monotonic.
  • Earlier write-pressure integration also had recording gaps/expired consumer
    descriptors. Zero hot allocations does not mean complete pipeline immunity.
  • New C tests formatted with clang-format; git diff --check: PASS.

Review responses

  1. Context mutexes/condition are owned by create/destroy, including failed init.
    Destroying them directly in the eventfd failure branch would invalidate the
    caller's later cleanup. Tests verify correct once-only cleanup. The actual
    device-fd leak on partial initialization is fixed instead.
  2. The comparison already allowed exact-fit payloads. Keep it and distinguish
    permanent oversize (INVAL) from temporary exhaustion (FULL), with tests.
  3. The original 2026-09-16 document date matches both the review's own
    2026-09-16T09:14:36Z timestamp and Git history. Keep the factual date.
    Reported CI worker cache/reachability failures are infrastructure issues,
    not altered by this application patch.

Configuration and compatibility

Fixed DMA input remains opt-in and same-process. Existing successful payloads
are unaffected; callers should not retry an oversized packet as a transient
FULL condition. No system library/kernel/DTB/frequency changes and no UAV or
camera-sdk source changes are included in these commits. Full reconstruction,
four-camera, mid-stream resolution-switch and long-duration tests are not
claimed. Full evidence is retained in the isolated board validation logs and
the local camera-fixed-dmabuf-119 artifact directory.

Second-review follow-up (65f3c84)

vdec: roll back failed init and satisfy CI style

Purpose

Address the second robot review on MPP PR #51 at 11ba300 and fix the
actual failing C/C++ style gate. Build and tests in that CI run were skipped
after lint failed, not evidence of a compilation or runtime failure.

Changes

  • Close device/event fds immediately on failed decoder initialization through
    a shared rollback helper; reset them to -1. Keep create-owned locks alive
    until the normal caller-owned destructor, which reuses the same fd helper.
  • Extend the actual-decoder failure-injection test to F_GETFL and F_SETFL,
    assert rollback before destroy, and retain fd-zero/once-only lock cleanup.
  • Explain why SYS_RecvStream needs no lease: it copies to caller-owned storage
    under the queue mutex and only then releases the slot for reuse. Test that
    immediate reuse of the same DMA fd does not alter the caller's copy.
  • Express token bounds on their encoded one-based fields while retaining
    high-bit rejection and compile-time limits. Cover eight malformed tokens
    and verify they do not release the real lease.
  • Replace column-aligned continuation indentation in the four added tests
    with multiples of four, as required by the upstream lint script. No lint
    suppression, rule relaxation, or wholesale production reformatting.
  • Document review responses and retest evidence in
    docs/vdec_dmabuf_pool_review.md.

Validation

Host x86 Debug and isolated K3 native RelWithDebInfo on 10.0.91.119, Linux
6.18.3-generic. Board build/tests use CPU 0-7 and staged libraries only.

cmake -S . -B build-review -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DBUILD_TESTS=ON -DBUILD_ROS2_EXAMPLES=OFF
cmake --build build-review --parallel 8
export LD_LIBRARY_PATH="$PWD/build-review/lib"
export MPP_PLUGIN_DIR="$PWD/build-review/al/vcodec"
ctest --test-dir build-review --timeout 20 --output-on-failure \
  -R '^(test_sys|test_vb|test_integration|test_multiproc|test_mux_common|test_mux_socket_disconnect|test_vdec_input_retry|test_vdec_init_cleanup|test_mjpeg_zero_dri)$'
build-review/test/test_sys_stream_dmabuf_pool
for mode in empty queued eos; do
  build-review/test/test_vdec_bound_dmabuf_input test/assets/1920x1080.jpg "$mode"
done
  • Negative control: the extended test fails against 11ba300 because failed
    init retains an eventfd. Fixed code passes 150 injected failure cases,
    including stable fd counts before destroy and exactly-once lock cleanup.
  • Host cleanup/input-retry/zero-DRI tests: 3/3 PASS. K3 selected CTest: 9/9 PASS.
  • K3 pool capacity/exhaustion, copied-buffer independence, all eight malformed
    tokens and 200 concurrent receive/unbind races: PASS.
  • Bound VDEC empty/queued/EOS hardware tests: PASS.
  • Pixel A/B, 4000x1200 original MJPEG, MODE 64 12 0 1 1: legacy and DMA-BUF
    both hash 32 warmup plus 64 measured NV12 frames to 15d8f2152717f030;
    zero PTS errors. Measured input allocations: legacy 64, DMA-BUF 0.
  • Five camera start/capture/stop cycles: 300 frames and 3005 IMU samples;
    zero invalid metadata or backward timestamps. The first test invocation
    loaded the bridge's parser-only SDK and failed before capture; rerun with
    the standalone MPP-enabled SDK passes. No SDK code change was needed.
  • All 19 PR-changed C/C++ files pass upstream lint_cpp.sh and .cpplintrc
    with cpplint 2.0.2, including custom indentation/header guards. The checked
    script matches current upstream bytes. git diff --check: PASS.
  • Additional 60-second live camera/record/receive run: stereo 60.042 FPS,
    raw/fused IMU 599.822 Hz; OUTPUT QBUF 3477 DMABUF / 0 MMAP; zero heap
    allocations after 5 seconds and no publisher/receiver error logs. CMA used
    returned to 1346 pages; kernel taint remained zero; processes exited normally.
  • Recording is not lossless: 3451 frames with two missing published sequence
    indices, maximum exposure gap 83.2 ms and IMU gap 66.56 ms. Packet/index
    counts and sizes match, timestamps are monotonic, and maximum mux/index
    PTS error is 0.48 ms. Staged library mappings and test logs were retained.

Configuration and compatibility

No token encoding/API change; DMA-BUF input remains opt-in and same-process.
Successful decoding paths retain their behavior. Failed init still requires
destroying the create-owned context but no longer retains its init fds.
No production UAV/camera-sdk sources, system libraries, kernel, DTB or
frequency settings are changed. No full reconstruction, multi-camera,
long-duration reliability, lossless recording or performance gain is claimed.

## Purpose
Eliminate per-packet CMA allocation/free and an avoidable compressed-payload copy on same-process SYS compressed-stream bindings such as UVC to VDEC. Repeated CMA allocation can fragment the CMA heap during long camera runs.

## Changes
- add an opt-in fixed CMA DMA-BUF ring to a SYS compressed-stream bind
- copy each packet once into a preallocated mapped slot, then lease its fd directly to the V4L2 decoder input queue
- return a slot only after V4L2 input DQBUF, and reject unbind while a hardware lease remains
- retain the existing malloc/MMAP decoder-input path as the default and compatibility fallback
- validate that a configured slot is large enough for both the encoded packet and the decoder's V4L2 OUTPUT-buffer requirement
- add isolated plugin loading through MPP_PLUGIN_DIR; normal builds no longer overwrite the installed plugin during a CMake post-build step
- add fixed-pool lifetime and bound JPEG-to-VDEC hardware regression programs

## Validation
- host Debug build: cmake -S . -B build-dmabuf-pool -DBUILD_TESTS=ON -DBUILD_ROS2_EXAMPLES=OFF; cmake --build build-dmabuf-pool --parallel 8 --target mpp v4l2_linlonv5v7_codec2 test_sys_stream_dmabuf_pool test_vdec_bound_dmabuf_input
- host tests: test_vdec_input_retry and test_mjpeg_zero_dri: PASS; fixed-pool test skipped as expected because the host has no /dev/dma_heap/linux,cma
- K3 board 10.0.91.119: test_sys_stream_dmabuf_pool: PASS, including pool exhaustion, lease-protected unbind, and fd reuse
- K3 board 10.0.91.119: test_vdec_bound_dmabuf_input test/assets/1920x1080.jpg: PASS; an isolated MPP_PLUGIN_DIR plugin received a 3 MiB fixed input slot and decoded a 1920x1080 MJPEG frame through V4L2_MEMORY_DMABUF
- verified /usr/lib/libv4l2_linlonv5v7_codec2.so remains byte-identical to its pre-test backup

## Configuration and compatibility
- bEnableInputDmaBuf defaults to false. It requires a same-process SYS bind explicitly configured with SYS_ConfigStreamDmaBufPool before packets are sent.
- Slot capacity must cover the largest compressed frame and the V4L2 decoder output-input buffer requirement; the K3 1920x1080 JPEG test required more than 2,076,672 bytes and uses 3 MiB.
- DMA-BUF fds and mappings are process-local. Cross-process bindings remain on the existing copy-based API.
@spacemit-robot-ci

spacemit-robot-ci Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Robot CI AI Review

结论:发现 2 个需要处理的问题。

发现:

  1. [中等] docs/vdec_dmabuf_pool_review.md(Validation 环境一节的 ctest 命令) ctest -R 使用了锚定正则 ^(test_sys|test_vb|...)$,不会匹配 test_sys_stream_dmabuf_pool、test_vdec_input_retry 等实际注册名,按文档执行只会选中极少数或零个测试,却让读者以为跑了全部硬件回归。
    建议:改为 ^(test_sys.*|test_vb.*|test_vdec_input_retry|test_vdec_init_cleanup|test_mjpeg_zero_dri)$ 等前缀写法,或直接列出准确测试名。

  2. [中等] mpi/sys/sys.c SYS_RecvStream() 固定池分支 拷贝后若 dma_sync_buf(..., DMA_SYNC_READ | DMA_SYNC_END) 返回错误,函数直接 SYS_ERR_BUSY 返回,未复位 slot->queued、未出队 entry,该 slot 将永久保持占用、队列头被卡死(接收方每次重试都撞同一路径),表现为解码流永久停摆。
    建议:失败路径统一回滚 slot/entry 状态后再返回,或对该 DMA错误直接按致命错误处理(复位队列/池),避免半成品状态。

备注:

  • 其余变更(init 失败即时回滚 fd、poll 暂停握手、token 校验、UVC base ref 跟踪)逻辑自洽,未发现明确缺陷。

备注:worker-cache review 失败,已回退 baseline review:worker-cache review failed: snode5: busy; hp-probook: sync worker helper failed: ssh: connect to host 10.0.91.193 port 22: No route to host

yanyongxian added 4 commits September 16, 2026 16:55
## Purpose
Make the opt-in fixed DMA-BUF input path safe for sustained decoding and
teardown, not just a one-frame smoke test. Avoid per-frame CMA allocation and
reduce compressed-payload copies; do not claim a throughput improvement or a
kernel CMA allocator fix. This updates the existing fixed-input-pool PR.

The initial implementation could exhaust its own input ring because reclaim
depended on the next packet. Subsequent testing also exposed a poll wait-list
warning during capture queue reconstruction and a receive/unbind lease race.

## Changes
- Keep compressed packets in preallocated mapped SYS slots and pass leased
  DMA-BUF fds to V4L2 OUTPUT; retain the legacy copying path by default.
- Reap input DQBUF independently in the existing codec event thread, with
  nonblocking device I/O and eventfd wakeup. No extra thread or fixed delay is
  added to normal frame processing.
- Add a condition-variable handshake around capture queue reconstruction,
  flush/reset and shutdown; wait for poll and event handling to quiesce before
  replacing queues, and join before freeing codec resources.
- Return unaccepted leases on failure/stop; keep accepted packets, including
  EOS, hardware-owned until input DQBUF or stream-off.
- Pair CPU DMA-BUF START/END synchronization in fixed-slot send, compatibility
  receive and in-place MJPEG zero-DRI normalization.
- Detect direct input from the configured V4L2 memory type, not a possibly
  zero-initialized fd, and return the actual legacy QBUF result.
- Keep the stream queue mutex held from unbind lease validation through reset;
  use a locked reset helper without recursive locking.
- Enforce token field widths at compile time, including the plugin's signed
  32-bit token limit, and reject unencoded high bits on release.
- Add sustained decode/pixel benchmarks, concurrent receive/unbind coverage,
  early-stop and EOS hardware tests, and docs/vdec_dmabuf_pool_validation.md.
- Preserve isolated plugin loading through MPP_PLUGIN_DIR; production library
  installation is not performed by these tests.

## Validation
Environment: x86 host Debug build and isolated native K3 RelWithDebInfo build
on 10.0.90.98, Linux 6.18.3-generic; board compilation/tests use CPU 0-7.
Earlier throughput comparisons used 10.0.91.119. No production UAV, system
codec library, kernel, DTB or frequency setting was replaced.

Build (PASS; substitute Debug for the host build):

```sh
cmake -S . -B build-fix -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DBUILD_TESTS=ON -DBUILD_ROS2_EXAMPLES=OFF
cmake --build build-fix --parallel 8 --target \
  mpp v4l2_linlonv5v7_codec2 test_sys_stream_dmabuf_pool \
  test_vdec_bound_dmabuf_input test_vdec_input_benchmark \
  test_vdec_input_retry test_mjpeg_zero_dri
export LD_LIBRARY_PATH="$PWD/build-fix/lib"
export MPP_PLUGIN_DIR="$PWD/build-fix/al/vcodec"
ctest --test-dir build-fix \
  -R '^(test_vdec_input_retry|test_mjpeg_zero_dri)$' --output-on-failure
```

- Host and K3 retry/zero-DRI unit tests: PASS. Host CMA test is SKIP (77),
  because /dev/dma_heap/linux,cma is absent, not a hardware PASS.
- `test_sys_stream_dmabuf_pool`: PASS on K3, covering exhaustion, reuse,
  lease-protected unbind, compatibility receive, malformed high-bit tokens
  and 200 concurrent receive/unbind iterations.
- `test_vdec_bound_dmabuf_input test/assets/1920x1080.jpg MODE`: PASS for
  empty/queued/eos, 20 runs each after the review fixes; EOS reaches
  ERR_VDEC_EOS and cleanup/unbind succeeds.
- Lifecycle handshake validation: 100 startup/initial source-change/33-frame
  decode/teardown cycles; 20 additional decode/stop runs: PASS.
- One-slot and two-slot DMA pools: 120 measured frames each, PASS.
- `test_vdec_input_benchmark output-240.mjpg 4000 1200 MODE 32 12 0 1`:
  legacy and dmabuf both hash 64 visible NV12 frames to 3c5726c46851e96a.
- `test_vdec_input_benchmark output-240.mjpg 4000 1200 dmabuf 5400 12 30`
  with 231 MiB additional fixed CMA buffers and 32 MiB/s buffered writes in
  256 KiB chunks for 180 seconds: PASS; 5,400 frames, zero hot-path allocations,
  zero PTS errors. Repeated after the review fixes. Detailed timings are in
  the validation document.
- `test_vdec_input_benchmark output-240.mjpg 4000 1200 dmabuf 3600 12 60`:
  PASS, 59.988 FPS, zero hot allocations / PTS errors.
- Kernel taint remained zero after the lifecycle fix; CMA bitmap usage returned
  to the 1,958-page baseline. `git diff --check`: PASS.

## Review responses
1. SYS_UnBind: the old unlock/relock sequence was not recursive double locking,
   but the gap did allow an already-looked-up receiver to create a lease before
   reset. Lease checking and reset are now one queue-locked critical section.
2. DMA synchronization: START/END pairs now surround CPU accesses, including
   the MJPEG normalization write, and failures are handled.
3. Tokens: current limits are 128 binds and 16 slots, so they did not overflow;
   static assertions now enforce the encoding and signed plugin constraints.
   Release rejects tokens with extra high bits instead of silently truncating.
4. EOS: do not release a successfully queued input in the MPI EOS branch.
   Hardware still owns it. The independent poll reaper returns it on DQBUF,
   with stream-off as teardown reclamation. Unaccepted inputs are returned by
   MPI. An EOS/drain/cleanup regression is included.

## Configuration and compatibility
- bEnableInputDmaBuf stays false by default. Enable it only for a same-process
  SYS compressed-stream bind configured with SYS_ConfigStreamDmaBufPool.
- Fixed slot capacity must cover the largest compressed packet AND the driver's
  advertised V4L2 OUTPUT buffer size. Tests use twelve 6 MiB slots for 4000x1200
  MJPEG and twelve 3 MiB slots for 1920x1080 JPEG.
- DMA-BUF fds/mappings are process-local. Cross-process use remains on the
  existing copy API. Consumers must be rebuilt against the updated headers;
  shared-memory layout version is 5. Do not mix old/new shared-memory users.
- The 240-frame benchmark fixture contains original MJPEG packets copied from
  an existing recording, not live USB input. No complete UAV/live-camera,
  multi-camera or comprehensive mid-stream resolution-switch test is claimed.
- Initial testing reproduced kernel list_del/poll_freewait warnings; the
  lifecycle fix avoids the overlap and the subsequent bounded tests did not
  reproduce the warning. It does not prove all driver races absent.
- Independent dirty-file-folio CMA migration ENOMEM remains a kernel issue.
  Preallocation avoids its steady-state trigger, not allocation failures during
  startup, growth or other components. Fresh-reboot and earlier fragmented
  memory states differ; matching pool occupancy/write rate does not make them
  identical. No global sync/drop_caches workaround is introduced.
- Earlier A/B throughput was similar (about 104-107 FPS); direct input used
  somewhat more process CPU. The benefit is fewer allocations and copies,
  not a demonstrated FPS gain.
## Purpose
Fix the UVC shutdown double release observed while validating PR spacemit-com#51 with a
live 4000x1200 MJPEG stereo camera. Capture can release a slot's base reference
before the recycle worker exits; teardown must not release it a second time.
This occurred on both legacy and fixed DMA-BUF decoder-input paths.

## Changes
- Track capture/driver base-reference ownership separately for each UVC slot.
- Mark ownership after initial VB acquisition and successful recycle QBUF.
- Serialize capture release and recycle ownership changes with the UVC mutex.
- Release only still-owned base references during error cleanup and teardown.
- Leave depth-queue and external-consumer reference ownership unchanged.

## Validation
- K3 10.0.91.119, Linux 6.18.3-generic, isolated native RelWithDebInfo build
  under /root/uav-dmabuf-validation-20260916; tests pinned to CPU 0-7.
- Before the fix, both legacy/fixed live tests logged VB_ModReleaseBuffer
  rejection with mod_ref=0 during shutdown. After the fix, 20 SDK camera
  start/capture/stop cycles passed: 1200 frames and 12020 valid IMU samples.
- External camera integration command: with staged MPP_PLUGIN_DIR and
  LD_LIBRARY_PATH, `taskset -c 0-7 ./sdk_live /dev/video13 60 20`.
  Evidence: logs/sdk-live-20-cycles.log in the isolated validation directory.
- Five additional cycles with the full review follow-up candidate passed:
  `taskset -c 0-7 ./sdk_live /dev/video13 60 5`; 300 frames, 3005 IMU samples,
  no duplicate-release errors or backward timestamps.
- CMA actual usage returned to the 1346-page baseline; kernel taint stayed 0.
- git diff --check: PASS. No system MPP/plugin replacement performed.

## Configuration and compatibility
No public API or defaults change. This is UVC ownership bookkeeping, not a
kernel allocator fix or a claim of lossless delivery under storage pressure.
Four-camera and full VIO/LAS2/nvblox/DRM tests are outside this validation.
This focused commit is added to the existing fixed-input-pool PR as requested;
no UAV or camera-sdk application changes are included.
## Purpose
Address the robot review of PR spacemit-com#51 at e2a7ac5 using ownership evidence and
regression tests. Fix an actual device-fd leak on partial initialization, and
distinguish invalid packet capacity from temporary input-pool exhaustion.

## Changes
- Initialize the decoder device fd to -1; close any nonnegative owned fd even
  if codec construction failed, including the valid fd-zero case.
- Log eventfd failure and document caller-owned create/destroy semantics.
  Do not destroy context locks inside init: its normal destructor still needs
  them to broadcast/join and then destroy synchronization exactly once.
- Add a hardware-independent failure-injection CTest using the real decoder
  implementation: eventfd failure, device-open failure, codec-create failure,
  and codec-create failure with fd zero; 25 iterations per case.
- Retain the correct size > capacity comparison, but return SYS_ERR_INVAL for
  oversized fixed-pool packets. SYS_ERR_FULL continues to mean transient slot
  or queue exhaustion. No fallback allocation/growth is introduced.
- Test capacity-1, capacity and capacity+1, including reuse after rejection.
- Document the three review responses, UVC fix, live-camera evidence and known
  recording limitations in docs/vdec_dmabuf_pool_review.md.

## Validation
Build: x86 Debug and isolated native K3 RelWithDebInfo on 10.0.91.119,
Linux 6.18.3-generic. Board build/tests use CPU 0-7 and staged libraries only.

```sh
cmake -S . -B build-review -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DBUILD_TESTS=ON -DBUILD_ROS2_EXAMPLES=OFF
cmake --build build-review --parallel 8
export LD_LIBRARY_PATH="$PWD/build-review/lib"
export MPP_PLUGIN_DIR="$PWD/build-review/al/vcodec"
ctest --test-dir build-review --timeout 20 --output-on-failure \
  -R '^(test_sys|test_vb|test_integration|test_multiproc|test_mux_common|test_mux_socket_disconnect|test_vdec_input_retry|test_vdec_init_cleanup|test_mjpeg_zero_dri)$'
build-review/test/test_sys_stream_dmabuf_pool
for mode in empty queued eos; do
  build-review/test/test_vdec_bound_dmabuf_input test/assets/1920x1080.jpg "$mode"
done
```

- Host cleanup/input-retry/zero-DRI: 3/3 PASS. K3 selected CTest: 9/9 PASS.
- Negative control: reinstating the old fd-close condition makes the new
  cleanup test FAIL with leaked device fd 6. Fixed code passes all 100 cases,
  stable fd counts, exactly two mutex destroys and one condition destroy.
- K3 capacity boundaries/exhaustion/reuse/200 concurrent unbind races: PASS.
  Host pool test SKIP (77), because the DMA heap is absent, not hardware PASS.
- K3 bound VDEC empty/queued/EOS hardware tests: PASS.
- Pixel A/B: `test_vdec_input_benchmark output-240.mjpg 4000 1200 MODE
  64 12 0 1 1` passes for legacy/dmabuf. The 32 warmup plus 64 measured
  NV12 frames hash to 15d8f2152717f030 in both modes; zero PTS errors;
  measured input allocations are 64 versus zero.
- Full candidate: five more camera start/capture/stop cycles PASS, 300 frames
  and 3005 IMU samples, no duplicate-release errors or backward timestamps.
- Full candidate, 60-second live-camera/record/receive run: stereo 59.841 FPS,
  raw IMU 598.448 Hz, fused IMU 598.451 Hz; zero publisher DMA heap allocations
  after 5 seconds; OUTPUT QBUF 3458 DMABUF / 0 MMAP; no publisher/receiver
  error logs. CMA usage returned to 1346 pages; kernel taint remained zero.
- Recording is NOT claimed lossless: 3430 recorded frames, four missing
  published sequence indices, maximum image gap 249.6 ms and IMU gap 232.96 ms.
  Packet/index counts and sizes matched; timestamps remained monotonic.
- Earlier write-pressure integration also had recording gaps/expired consumer
  descriptors. Zero hot allocations does not mean complete pipeline immunity.
- New C tests formatted with clang-format; git diff --check: PASS.

## Review responses
1. Context mutexes/condition are owned by create/destroy, including failed init.
   Destroying them directly in the eventfd failure branch would invalidate the
   caller's later cleanup. Tests verify correct once-only cleanup. The actual
   device-fd leak on partial initialization is fixed instead.
2. The comparison already allowed exact-fit payloads. Keep it and distinguish
   permanent oversize (INVAL) from temporary exhaustion (FULL), with tests.
3. The original 2026-09-16 document date matches both the review's own
   2026-09-16T09:14:36Z timestamp and Git history. Keep the factual date.
   Reported CI worker cache/reachability failures are infrastructure issues,
   not altered by this application patch.

## Configuration and compatibility
Fixed DMA input remains opt-in and same-process. Existing successful payloads
are unaffected; callers should not retry an oversized packet as a transient
FULL condition. No system library/kernel/DTB/frequency changes and no UAV or
camera-sdk source changes are included in these commits. Full reconstruction,
four-camera, mid-stream resolution-switch and long-duration tests are not
claimed. Full evidence is retained in the isolated board validation logs and
the local camera-fixed-dmabuf-119 artifact directory.
## Purpose
Address the second robot review on MPP PR spacemit-com#51 at 11ba300 and fix the
actual failing C/C++ style gate. Build and tests in that CI run were skipped
after lint failed, not evidence of a compilation or runtime failure.

## Changes
- Close device/event fds immediately on failed decoder initialization through
  a shared rollback helper; reset them to -1. Keep create-owned locks alive
  until the normal caller-owned destructor, which reuses the same fd helper.
- Extend the actual-decoder failure-injection test to F_GETFL and F_SETFL,
  assert rollback before destroy, and retain fd-zero/once-only lock cleanup.
- Explain why SYS_RecvStream needs no lease: it copies to caller-owned storage
  under the queue mutex and only then releases the slot for reuse. Test that
  immediate reuse of the same DMA fd does not alter the caller's copy.
- Express token bounds on their encoded one-based fields while retaining
  high-bit rejection and compile-time limits. Cover eight malformed tokens
  and verify they do not release the real lease.
- Replace column-aligned continuation indentation in the four added tests
  with multiples of four, as required by the upstream lint script. No lint
  suppression, rule relaxation, or wholesale production reformatting.
- Document review responses and retest evidence in
  docs/vdec_dmabuf_pool_review.md.

## Validation
Host x86 Debug and isolated K3 native RelWithDebInfo on 10.0.91.119, Linux
6.18.3-generic. Board build/tests use CPU 0-7 and staged libraries only.

```sh
cmake -S . -B build-review -DCMAKE_BUILD_TYPE=RelWithDebInfo \
  -DBUILD_TESTS=ON -DBUILD_ROS2_EXAMPLES=OFF
cmake --build build-review --parallel 8
export LD_LIBRARY_PATH="$PWD/build-review/lib"
export MPP_PLUGIN_DIR="$PWD/build-review/al/vcodec"
ctest --test-dir build-review --timeout 20 --output-on-failure \
  -R '^(test_sys|test_vb|test_integration|test_multiproc|test_mux_common|test_mux_socket_disconnect|test_vdec_input_retry|test_vdec_init_cleanup|test_mjpeg_zero_dri)$'
build-review/test/test_sys_stream_dmabuf_pool
for mode in empty queued eos; do
  build-review/test/test_vdec_bound_dmabuf_input test/assets/1920x1080.jpg "$mode"
done
```

- Negative control: the extended test fails against 11ba300 because failed
  init retains an eventfd. Fixed code passes 150 injected failure cases,
  including stable fd counts before destroy and exactly-once lock cleanup.
- Host cleanup/input-retry/zero-DRI tests: 3/3 PASS. K3 selected CTest: 9/9 PASS.
- K3 pool capacity/exhaustion, copied-buffer independence, all eight malformed
  tokens and 200 concurrent receive/unbind races: PASS.
- Bound VDEC empty/queued/EOS hardware tests: PASS.
- Pixel A/B, 4000x1200 original MJPEG, MODE 64 12 0 1 1: legacy and DMA-BUF
  both hash 32 warmup plus 64 measured NV12 frames to 15d8f2152717f030;
  zero PTS errors. Measured input allocations: legacy 64, DMA-BUF 0.
- Five camera start/capture/stop cycles: 300 frames and 3005 IMU samples;
  zero invalid metadata or backward timestamps. The first test invocation
  loaded the bridge's parser-only SDK and failed before capture; rerun with
  the standalone MPP-enabled SDK passes. No SDK code change was needed.
- All 19 PR-changed C/C++ files pass upstream lint_cpp.sh and .cpplintrc
  with cpplint 2.0.2, including custom indentation/header guards. The checked
  script matches current upstream bytes. git diff --check: PASS.
- Additional 60-second live camera/record/receive run: stereo 60.042 FPS,
  raw/fused IMU 599.822 Hz; OUTPUT QBUF 3477 DMABUF / 0 MMAP; zero heap
  allocations after 5 seconds and no publisher/receiver error logs. CMA used
  returned to 1346 pages; kernel taint remained zero; processes exited normally.
- Recording is not lossless: 3451 frames with two missing published sequence
  indices, maximum exposure gap 83.2 ms and IMU gap 66.56 ms. Packet/index
  counts and sizes match, timestamps are monotonic, and maximum mux/index
  PTS error is 0.48 ms. Staged library mappings and test logs were retained.

## Configuration and compatibility
No token encoding/API change; DMA-BUF input remains opt-in and same-process.
Successful decoding paths retain their behavior. Failed init still requires
destroying the create-owned context but no longer retains its init fds.
No production UAV/camera-sdk sources, system libraries, kernel, DTB or
frequency settings are changed. No full reconstruction, multi-camera,
long-duration reliability, lossless recording or performance gain is claimed.
@yanyongxian

Copy link
Copy Markdown
Contributor Author

Follow-up on the AI review of 65f3c84

Board CI now reports SUCCESS: shell/C++/Python style, package build and
package tests all pass. I checked the three newly generated findings against
the complete current source, not only the diff. The cited failure mechanisms
do not match the implementation:

1. SYS_SendStream sync failure does not strand a reserved slot

sys_find_free_stream_dma_slot_locked, lines 165-175
only searches for allocated && !queued && !leased and returns its index.
It does not mark the slot in use. allocated means the persistent pool
allocation exists; it is not a packet reservation.

Both sync-failure paths return BUSY for that bind before setting queued or
changing queue head/tail/count. The slot is still free. Only the successful
enqueue commit sets queued = MPP_TRUE.
After END fails, bytes may have changed, but no descriptor publishes them; a
subsequent successful send overwrites the free slot. dma_slot is also declared
and initialized inside each bind-loop iteration, not shared between binds.
There is no lease to return here; setting queued = false on this path would
only repeat the already-existing state. The pool allocation must remain alive.

2. Unbind cannot preempt the queued-to-leased transition under its mutex

Send and unbind both acquire bind_lock before the queue mutex; the review's
body itself correctly notes that their ordering is identical.

SYS_UnBind, lines 436-451
holds the queue mutex continuously from the outstanding-lease check through
pool reset. SYS_RecvStreamDmaBuf, lines 1252-1289
uses that same mutex while reading the entry and changing queued to leased.
CPU preemption between those assignments does not release the mutex.

If unbind wins before the receiver acquires the queue mutex, the pool is
disabled/cleared and the receiver rejects it before accessing a slot. If the
receiver wins, unbind observes the lease and returns BUSY. A condition wait
releases the queue mutex only while waiting for data, before any entry/slot
pointer is selected. The alleged unprotected dequeue-to-lease window is absent.
The existing 200-iteration concurrent receive/unbind regression passed again
on K3 at this revision.

3. MPI already releases rejected input leases

decodeInputDmaBuf, lines 665-672
clears the plugin's extra-id on failure because ownership has not transferred.
The caller retains the original stStream.u64DmaBufToken.
vdec_stream_input_task, lines 975-982
explicitly calls SYS_ReleaseStreamDmaBuf when the submit result is not MPP_OK.
Transient DATAQUEUE_FULL retains the same packet during retry; final error or
stop returns it. Accepted packets are instead reclaimed on DQBUF/stream-off.

Releasing the lease inside the plugin as suggested would add a second owner:
MPI would then release the same token again, potentially after slot reuse.
The token is the fifth parameter to setExternalDmaBufSinglePlanar, following
the separate payload-length argument; the claimed fourth-parameter ambiguity
is not present in the actual function signature and call.

No production behavior is changed merely to follow these contradictory
suggestions. The source references are pinned to the reviewed commit for
manual review. Existing tests do not claim exhaustive injected DMA-sync/QBUF
failure coverage or proof against every possible concurrent reconfiguration.

Independent validation at this revision: 9/9 selected K3 tests, matching
legacy/DMA-BUF decoded-frame hashes, five camera start/stop cycles, and a
60-second live run (stereo 60.042 FPS, IMU 599.822 Hz, zero hot heap allocations,
CMA back to baseline, kernel taint zero). Recording gaps remain documented;
this is not a lossless-recording claim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant