[dormancygrace]

Draft for issue #63 (video glitches on a unit with an LT6911C receiver).

What

The VI interrupt handler calls isp_err_chk. When the CSI bridge sees a short or long line or frame, or a FIFO overflow, isp_err_chk counts the error and wakes the error-handler thread, which resets and restarts the pipe. The handler ignored that, and the frame's end queued the frame as a good one. So a frame damaged on the receiver's MIPI link reached the encoders.

With osdrv 0019:

• The mark. Each wake of the error handler for such an error (vi_err_wake_up_th) marks the pipe's current frame as damaged.

• The frame end. The YUV bypass frame end (_isp_yuv_bypass_handler), in the same interrupt or a later one, consumes the mark and keeps all its bookkeeping: idle state, the next DMA trigger. It does not publish the buffer and does not wake the event thread. The buffer stays first in the ready queue and takes the next frame.

• The ready count. The trigger counts the buffer out of qbuf_num again, so the frame end gives that count back first, as the vendor's _vi_err_retrig_preraw does for the same buffer.

• Clearing. The error handler clears the mark just before its retrigger, for a frame the reset aborted. Stream off clears it for the next stream.

• Error recovery restarts the pipe exactly once. The vendor's recovery itself could stop capture until the server restarted the stream. The changes to its flow, each called out because they touch vendor code:

  • Recovery owns the pipe. From the wake of the handler thread until after its retrigger, two things used to trigger the next frame: a frame end, and the queue's tasklet when the application returned a buffer to an empty queue (cvi_isp_buf_queue_wrap → isp_post_tasklet). The reset wiped that DMA, the front end stayed RUNNING, the retrigger's trigger was refused, and its count was one too many. Now:
    • Such a frame end only notes its channel. This also covers one the CPU services after the reset.
    • _isp_yuv_bypass_trigger refuses while the pipe recovers, and the tasklet keeps its retry pending. The retrigger calls the trigger itself (__isp_yuv_bypass_trigger).
    • The gate holds the number of the wake that started the recovery. After the retrigger the handler gives the pipe back, unless a newer error has already woken the next handler. With bottom halves off, it then restarts the channels it held back and reschedules the tasklet.
  • Step 6 no longer returns. When the ISP was not idle after the reset, the handler returned with the DMA dropped and nothing to restart it. It now goes on to the retrigger.
  • Early returns clear the handler flag. At the step 2 timeout or a failed step 3 abort, the handler clears isp_err_handle_flag, then gives the pipe back as above. The vendor left the flag set, so no later error woke a handler again, and the RGB trigger path stayed blocked.
  • Only a buffer the DMA has is counted back. The retrigger counted a buffer back whether or not the DMA had one. With every buffer with the application, it counted back one that was not there and triggered the DMA into the last published buffer. _isp_yuv_bypass_buf_enq now notes the buffer it gives the DMA (nk_buf_out), and the retrigger counts back only that one.
  • The trigger refuses an empty queue. _isp_yuv_bypass_trigger did not check for a ready buffer either; a retried tasklet trigger could arm the stale address. It now leaves the state idle and returns -1, and the tasklet keeps its retry.
  • Back off instead of giving up. After 30 recoveries without a good frame in between, the vendor's retrigger stopped for good: no frame then came, so no error woke a handler again. On the stand, with only an MJPEG viewer, profile switches left the VI stopped after the 31st retry. The handler now waits before the retrigger, still owning the pipe: 1, 2, 4, then 8 s, with the message printed once. A stream stopped (and perhaps started again) during the wait is left alone.

  A wake that finds the handler flag set (VI_CB_RESET_ISP sets it before waking, so it starts no handler) holds nothing back.

• Unchanged. The interrupt handler no longer returns early, so CMDQ and the other handlers run as before. DMA and over-maximum errors, which wake nothing, are unchanged. The YUV online and RAW paths are unchanged: NanoKVM captures YUV bypass offline.

Sipeed's driver for the same SoC (LicheeRV-Nano-Build, osdrv/interdrv/v2/vi/chip/mars/vi.c) returns from the interrupt handler on any isp_err_chk failure. That also skips the interrupt's other bookkeeping, and it misses an error and a frame end that arrive in separate interrupts.

What changes for the user

On a link that damages frames, a damaged frame is no longer encoded. Instead of a torn picture there is a skipped frame. The counters in the issue's diag script (bdg_w_ls_cnt, bdg_h_ls_cnt, FIFO overflow) still count every error.

Tests

• tests/test-vi-damaged-frames.py OSDRV extracts the patched driver's real ready-queue, frame-end, trigger, error-wake, error-handler (_vi_err_handler) and retrigger (_vi_err_retrig_preraw) functions and runs them against a model of the device. The handler's hardware steps (isp_frm_err_handler) and its step 2 sleep are hooks that can deliver a frame end or fail; step 4 drops the armed DMA. Cases:

  • an error-only interrupt followed by a frame-end-only interrupt;
  • both in one interrupt;
  • a frame the handler aborts before its end;
  • a frame end during each of steps 1-4 and during the step 2 wait;
  • a frame end raised before the reset and serviced after step 7;
  • a step 6 failure, with and without a held frame end;
  • the handler giving up at step 2 or step 3, with and without a held frame end, then the next error handled again;
  • a handler flag set with no handler woken;
  • an error while every buffer is with the application, and a trigger on that drained queue;
  • with the real cvi_isp_buf_queue_wrap and isp_post_tasklet: a buffer returned to a drained queue at step 3, and a pending tasklet trigger as the retrigger starts;
  • a new error during the retrigger, which leaves the pipe to the next handler;
  • 35 recoveries in a row (the back-off: 1+2+4+8 s), and a stream stop and start during a wait;
  • a 40-frame error storm with the handler behind;
  • good frames after recovery.

  Between interrupts it checks four things: the front-end state matches the device, the ready count is exact, the armed buffer is the queue head, and no trigger runs without a buffer. A completed recovery must trigger exactly once after the reset.

  Ten changes were each taken out in turn, and the test failed every time:

  • the frame-end gate;
  • the trigger gate;
  • the wake number;
  • the tasklet reschedule;
  • nk_buf_out;
  • the flag clear;
  • the empty-queue check;
  • step 6;
  • the back-off;
  • the check after the wait.

  Without the frame-end gate it fails with the first review's numbers (front end RUNNING, no DMA armed, ready 3, listed 3). Without the trigger gate it fails with the second's (ready 1, listed 1).

  Releasing the pipe before rather than after the retrigger does not fail it. nk_buf_out keeps the counts right either way, so holding the gate through the retrigger is a margin, not a requirement.

  It models the rest of the driver; it is not the driver on hardware. The release build now runs it on the prepared osdrv tree.

• Stand (LT6911UXC, 2560×1440@60 H.264 with MJPEG 25): no change in steady state (H.264 44–48 fps, VI 60 fps, VIDropCnt 0). Monitor profile switches 1440→1080→1440 produced 26 and 32 real CSI width errors. The stream recovered each time and no buffer warnings appeared. On this receiver those frames are aborted before their end, so the new frame-end branch did not run (counted with a temporary log level).

• Stand, this head (module 7f8b64e8, r6 kernel, LT6911UXC, 2560×1440@60): steady state VI 60 fps, VIDropCnt 0. Profile switches 1440→1080→1440, twice, produced 128 real CSI width errors and 124 retrigger retries. Each retry took the nk_buf_out count-back. Four times the vendor's give-up after 30 retries hit while the input size changed, and the server's capture restart recovered each one. The stream ended at 2560×1440 with H.265 40 fps and MJPEG 40 fps. The held-frame-end and early-return paths did not run there: on this receiver damaged frames are aborted before their end.

• Stand, head f70e3666, before the back-off, only an MJPEG viewer: profile switches 1440→1080→1440 stopped the VI for good after the 31st retry. No frame came, so the server never saw the new input size and did not restart the capture.

• Stand, head cc2f22bc (module 9122fb0e), the same viewer and switches, twice: 131 CSI width errors and 127 retries, then the back-off. Frames came again, the server saw the new size and restarted the capture (4 times), and VI ended at 60 fps with VIDropCnt 0.

Not tested

• The held-frame-end and early-return paths on hardware. On the LT6911UXC damaged frames never reach their frame end. Only the test covers them.
• A failed step 3 abort. The bridge abort bit is left as the vendor leaves it. The flag is now cleared, so the next error runs a full recovery.
• No LT6911C hardware. The case the patch is for, a damaged frame that still completes, has not been seen here.
• cv180x/vi.c is left alone. It has the same code; the SG2002 uses cv181x.

Numbered 0019, so it applies after #346 (0014–0018) in any merge order; it is independent of #345–#347.

🤖 Generated with Claude Code

[dormancygrace]

Reviewed the patch against the fully patched pinned osdrv source, tracing CSI/top interrupt acknowledgment, error wake-up/reset/retrigger, and YUV frame completion/queueing. One remaining correctness issue below: the new guard excludes the error IRQ, but does not exclude a subsequent frame-done IRQ for that damaged frame while recovery is pending.

Validation: complete osdrv patch stack applies; the VI module cross-builds successfully for the configured RISC-V kernel. A bounded host harness using the exact added guard with mocked hardware/scheduling confirms: error+EOF in one IRQ is suppressed, but error followed by a separate EOF delivers a frame while the recovery flag is still set. This is a control-flow reproduction, not LT6911C hardware qualification. All 14 fast CI/CodeQL checks were green; the full image build was still running when inspected.

Suggested validation for the correction: separate/coalesced error and EOF events, a healthy frame after recovery, repeated errors during reset, and buffer ownership/queue-count consistency. Do not simply discard every completion globally: recovery may need other FE/BE/post idle bookkeeping. No tracked source changes, draft-state changes or merge performed, as requested.

[github-actions]

Build outputs

Compared with the build of main at d43f86f.

1 changed, 0 added, 0 removed of 551 outputs.

	directory	files
changed	lib	lib/modules/7.2.10-nanokvm-os-r5/extra/cv181x_vi.ko
Hashes: outputs-sha256 artifact of this run.

[dormancygrace]

Re-review of 0d69038: the original separated-error/EOF finding is fixed. The per-pipe mark survives the error IRQ, the damaged buffer stays queued, and CMDQ/other IRQ bookkeeping is retained. The supplied ASan/UBSan test passes and the complete patched cv181x_vi module cross-builds successfully.

One remaining P2 below concerns the recovery model and the damaged-frame path's interaction with the existing vendor recovery code. This is an inherited recovery race that the new drop branch still reaches, not evidence of a newly introduced normal-stream regression. I reproduced the failed restart/counter invariant with the actual extracted _vi_err_handler and _vi_err_retrig_preraw functions, mocked hardware register operations, and an EOF injected at entry to recovery step 3. It is a host control-flow reproduction, not an LT6911C device result.

All 14 fast CI/CodeQL checks pass on this head; the full Build and hash job was still running when checked. No device changes, source edits, draft-state changes or merge performed. The reported LT6911UXC profile-switch tests do not exercise a damaged frame completing during recovery.

[dormancygrace]

Re-reviewed a9759fb. The earlier EOF-during-recovery reproduction is addressed, and the expanded checked-in ASan/UBSan test passes on a freshly prepared osdrv tree with every patch applied. However, recovery still does not exclusively own the YUV trigger: the application's buffer-return/tasklet path can restart it while the recovery gate is set. I reproduced a missing post-reset restart using the actual extracted cvi_isp_buf_queue_wrap, isp_post_tasklet, trigger, recovery and retrigger functions (hardware mocked); see the inline P2. A gate check at the trigger entry makes this specific reproduction pass, but the ownership handoff should also be reviewed for races. Therefore not ready to merge yet. No production edits or device tests in this review. CI on this head: 14 completed successfully; full build/hash still running.

[dormancygrace]

Re-reviewed cc2f22b, including both new commits and the real queue/tasklet call chain. The previously reported recovery-versus-buffer-return bug is fixed: ordinary triggers respect the recovery gate, the recovery retrigger retains ownership, and a newer wake cannot have its gate cleared by an older handler. I found no further actionable issues in this revision.

Validation performed locally:

• Applied the full osdrv patch series to the vendor source and ran tests/test-vi-damaged-frames.py with ASan/UBSan: PASS.
• Independently swept application-buffer returns across reset callbacks 1/3/4/5/6 and checked stream restart during retry backoff: PASS, one DMA restart and consistent queue accounting.
• Negative control: removing the common trigger gate makes that independent regression test fail with FE running but DMA stopped, reproducing the original fault.
• Constructed the merge with current main d43f86f: all 18 osdrv patches apply, and the real-handler sanitizer suite passes on the combined source.
• Reviewed bounded 1/2/4/8-second retry backoff and generation invalidation on stop/restart.

14 CI checks have passed; the full build/hash job is still running at review time. Merge will follow once that final check succeeds. I did not run new device tests; the live-device measurements in the author reply are separate from my local verification.