Repository navigation
ambarella-osdrv-s3l: generate the OV4689 ISP configuration at build time - #2793
Conversation
PR Summary by QodoGenerate the S3L OV4689 ISP configuration at build time
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1. Sensor tuning from one unit ships to every image
|
1552be8 to
4896e92
Compare
|
Thanks for the review. I've force-pushed an updated commit. The generated
|
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Thanks for this. Generating the ISP config from source is a much better direction than shipping a blob. Three things need fixing before it can merge, and a few more are worth a look.
Blocking
-
Nothing on the image reads the installed file. The only defconfig with
BR2_PACKAGE_AMBARELLA_OSDRV_S3Liss3l_lite. It ships majestic, not Divinus, and the PR says itself that majestic never opens/etc/sensors/ov4689.bin. So every s3l image gets ~49 KB nothing reads ("nothing ships that nothing runs", CLAUDE.md). Please key the install onBR2_PACKAGE_DIVINUS, either throughgeneral/scripts/late-overlays.listor a conditional in the.mk. -
Where the constants come from. The generator writes in values tagged "libimg fixed" / "as stock" / "stock 3" and the stock -4096 13-bit encoding. It cites
vcap_liso.c:485,idspdrv_api.c:2065andidspdrv_sharpen.c:185, and refers to anFS-MCTF-RECIPESdocument that isn't in the tree. The tree's rule is that a register table copied from a vendor driver is a blob in another language, whatever language it's written in. Could you say how each of these was derived (the public microcode interface, your own measurements, or copied from libimg / driver source), and either add the recipes document or drop the reference to it? Reviewers can't check sources they can't see. -
Host Python isn't declared. The build runs bare
python3. Please addhost-python3toAMBARELLA_OSDRV_S3L_DEPENDENCIESand invoke$(HOST_DIR)/bin/python3, so the build doesn't depend on whatever the host has (a host without python3, or with ≤3.5 and no f-strings, fails partway through).
Correctness
-
IMG_BASE = 0x0851d000is an absolute address baked into every table pointer (link()writesIMG_BASE + offset). If amem=or DSP carveout change moves the IMG buffer (QUERY_BUF id 8), every LISO/HDR/CE pointer goes stale and the DSP reads garbage, with nothing to flag it. The ops are already buffer offsets, so relocating against the queried base at load time would remove this hazard. -
Microcode version. The docstring says the layout was checked against microcode dated 2017-02-22 to 2018-07-19, but the package installs the SDK 2.5.0 microcode dated 2018/10/28. Were the block offsets,
LISO_PTRS,TABLE_SIZESand the 12752 B final_sharpen read checked against the microcode this tree actually ships? -
TNR update mask.
temporal_nrwrites the video_mctf table but leaves LISO+0x234bits 0-15 at 0 unlessmask=is given, andov4689.yaml(temporal_nr: med) gives none. Final sharpen always sets bits 16-29. If the DSP honours the update mask, the shipped TNR setting is never applied. Can you confirm on hardware that it takes effect, for example by toggling it off and seeing a visible difference?
Smaller
- The docstring says options are range-checked, but many knobs aren't (sharpness
u/amul/fir2/scale1/dc/ugl, temporal_nriso/dir/dadj/sb2/wide/tn/mf/mcD/thD). Out-of-range values crash withValueError: byte must be in range. In-range nonsense (wide=9,fir2=7) goes straight to the DSP. - Malformed input gives a traceback instead of a usage error:
--set keywithout=, a trailing--preset,temporal_nr: iso=1with no positional value,sharpness: u=8. - Roughly half the script can't run under the shipped preset: the CE batch/link/validate path (
opt_cerefuseson),hdr_tables,OPS = OPS_SETSwith always-zero flags, theprinter/dcurve/sb2c/mcD/thD/ugl/dcknobs, and final_sharpenr1, which duplicates sharpnessoff. A generator limited to what the presets use would be much easier to review and maintain. - The
opt_stage_offdocstring contains the literal' + ', '.join(sorted(STAGES)) + ', so the help prints code, not the stage list.grgbhas a special case thatSTAGEScould cover with two (offset, mask) pairs.
|
Correction to my review above. In point 1 I said |
A streamer that runs its own 3A on S3L (the Divinus S3L backend, OpenIPC/ divinus#71) loads an ISP configuration into the DSP's IMG buffer before preview starts: the three image blocks (LISO, HDR, CE) and the tables they point at. Without one the picture stays black, and the tree had none. src/isp/s3l_isp_gen.py builds that file from a sensor preset. The block layout, field offsets, table encodings and the limits the DSP enforces come from reverse engineering the S3L ISP library and microcode, checked on the camera; the values are the generator's own. They are neutral or identity settings, values measured on an OV4689 (black level, starting white balance), and noise-reduction and sharpening curves designed and measured here. Values whose meaning could not be derived were tested on the camera: those that change the picture keep a measured value, and those that change nothing measurable on a still or a moving scene are 0. Malformed presets and out-of-range values stop the build with a usage error. Table pointers are written as IMG-buffer offsets with a list of relocations, which the loader adds the buffer's address to, so the file does not depend on where the driver puts the buffer. src/isp/ov4689.yaml holds the OV4689's settings: output size and Bayer phase, black level, a starting white balance for the streamer's AWB, dynamic bad-pixel correction, sharpening and temporal noise reduction. They are the sensor's, not one board's; a board that needs its own file points system.sensor_config at it. Only Divinus reads the file, so the package generates it, with host-python3, and installs /etc/sensors/ov4689.bin only when Divinus is selected. Divinus loads that file when system.sensor_config does not point at one. Tested on an Amcrest IP4M-1026E (S3L, OV4689) with the SDK microcode OpenIPC ships: the package builds the file with host-python3 and installs it in a Divinus image; Divinus loads and relocates it and streams 2688x1512 with auto exposure and white balance running. Each changed value was compared with the previous file on the camera (detail, noise and brightness on a still scene at high gain, and detail and frame change on a moving pattern); the final file matches the previous one within run-to-run spread. Temporal noise reduction takes effect as generated: noise is 14-61% higher with it off.
4896e92 to
4dafee6
Compare
|
Thanks for the thorough review. I've force-pushed a reworked commit. Point by point: 1. Nothing reads it on a Majestic image. Right, on 2. Where the constants come from.
3. Host Python. 4. Absolute IMG address. The format is now S3LISP03. Table pointers are written as offsets into the IMG buffer, with a relocation list in the file. The Divinus loader checks each relocation against the file's own image range, then adds the buffer address the driver reports ( 5. Microcode version. Every hardware test of the file, the statistics and the picture ran with the SDK 2.5.0 microcode this tree ships (2018/10/28 284712), and also with the camera's own 267837. The docstring now says that rather than listing the survey range. 6. TNR update mask. It does take effect as generated, with LISO 7–10.
|
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
Thanks for the thorough rework and the measurements. All three blockers are resolved:
- Gated on Divinus. The file and
host-python3come in only withBR2_PACKAGE_DIVINUS, and a Majestic image gets neither. - Provenance. The vendor-derived values are gone or re-derived, each with a measurement or a format argument. The cited sources and the recipes reference are gone, and the docstring says plainly where the format comes from.
- Host Python. The generator runs as
$(HOST_DIR)/bin/python3.
The relocation format, the tests on the microcode we ship, the TNR measurement and the cut-down, range-checked generator cover the rest. Approving.
Non-blocking notes for whoever merges:
- Nothing in the tree consumes the file yet. Divinus has no S3L support on master (OpenIPC/divinus#71 is open) and the package tracks
HEAD. Merging first is harmless, since nothing ships, but the generator does nothing until the loader lands. - No Ambarella defconfig selects Divinus, so CI never runs the generator. A small
.github/scripts/test_s3l_isp_gen.shthat runs it onov4689.yaml(and maybe checks the header and size) would catch it breaking. Fine as a follow-up. - Majestic images no longer get the empty
/etc/sensorsthat master created unconditionally. It looks like a leftover placeholder; just calling it out as a change on shipped images.
And thanks for the host-libffi / TARGET= report. I'll take that one to a separate issue.
Problem
A streamer that runs its own 3A on S3L loads an ISP configuration into the DSP before preview starts: the three image blocks (LISO, HDR, CE) and the tables they point at. Without one the picture stays black. The tree has none, so the Divinus S3L backend (OpenIPC/divinus#71) can't give video on an OpenIPC image.
Change
The generator:
src/isp/s3l_isp_gen.pybuilds the file from a sensor preset.The file format (S3LISP03): table pointers are offsets into the IMG buffer, with a relocation list. The loader (Adding Ambarella S3L support divinus#71) adds the buffer address the driver reports, so the file doesn't depend on where the buffer is.
The OV4689 preset:
src/isp/ov4689.yamlholds:These are the sensor's settings, not one board's. A board that needs its own file points
system.sensor_configat it.The package: with
BR2_PACKAGE_DIVINUSonly, it runs the generator withhost-python3and installs/etc/sensors/ov4689.bin. That's the name Divinus loads whensystem.sensor_configdoesn't point at a file. Another sensor needs its own<sensor>.yaml(with its size and Bayer phase) and an entry inAMBARELLA_OSDRV_S3L_ISP_SENSORS.Majestic images aren't affected: without Divinus, nothing is generated or installed, and no host Python is built.
Hardware tested on
Amcrest IP4M-1026E (S3L, OV4689), with the SDK microcode this tree ships (2018/10/28 284712):
The package: in a Divinus image, it builds the file with
host-python3and installs/etc/sensors/ov4689.bin(49248 bytes).Loading: Divinus with the Adding Ambarella S3L support divinus#71 loader loads and relocates the file:
It then streams 2688×1512 with auto exposure and white balance running, and the DSP reports no errors.
The value changes: each changed value was compared with the previous file on the camera, about 150 paired runs:
The final file matches the previous one within run-to-run spread.
Temporal noise reduction takes effect as generated: noise is 14–61 % higher with
temporal_nr: off.