Repository navigation
ambarella-osdrv-s3l: build an open-source load_ucode - #2766
Conversation
PR Summary by QodoBuild an open-source S3L DSP microcode loader
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
|
On finding 3 (license collection): this is resolved in 0c1c1d7, and the finding above still quotes the old state. The package no longer declares That follows the tree's convention for in-tree OpenIPC code. |
The S3L DSP boots from microcode that has to be copied into a window dsp.ko exposes through /dev/ucode, and nothing in the package did that. The vendor's load_ucode can't be shipped, and it is a glibc armhf binary that would not run on the musl s3l_lite image anyway. load_ucode asks dsp.ko for the layout (UCODE_GET_INFO), maps the window, copies orccode.bin, orcme.bin and default_binary.bin to their offsets, tells the driver they are loaded and prints the version. It takes the same `load_ucode DIR` argument as the vendor tool, so load_ambarella can call it unchanged. Every file is checked against its slot before any is written, so a bad set cannot leave old and new microcode mixed in the window. The microcode itself is still not part of this package.
… files load_ucode checked each file's size before touching the window but then streamed it in with read(), so a read that failed after the check (a file truncated meanwhile, or not a regular file) still left old and new microcode mixed in the window. Read every file into memory first and copy into the window only once all of them are in; the set is about 2.2 MB. A zero-byte file passed the size check and left its slot untouched while the tool reported success. Reject it. AMBARELLA_OSDRV_S3L_LICENSE_FILES named a LICENSE that the package source (now src/) does not contain, so legal-info would fail to copy it. Drop it, as the tree's other local-source packages do; load_ucode.c carries an SPDX identifier.
…ckage The header said the load_ucode tool was Ambarella's and not part of the package. The package now builds it from src/, so only the microcode still has to be put in /lib/firmware.
0c1c1d7 to
ed1321c
Compare
openipc-ai
left a comment
openipc-ai
left a comment
There was a problem hiding this comment.
This looks right, and it removes the glibc blocker. I tested it independently, since S3L's toolchain now exists.
Code. The structs and ioctl numbers match the driver's interface, and the files are read in full before the window is touched. On the Qodo license finding your reasoning holds: none of those five packages declares LICENSE_FILES. One small correction: gpiostep-openipc is GPL-2.0, not MIT, which doesn't affect the point.
Run against the real dsp.ko, on the new s3l QEMU machine (OpenIPC/qemu-bench#233), built with the published toolchain.ambarella-s3l:
- All three files land at
+0x000000,+0x300000and+0x3a0000, and it prints the version I stamped into a test file,2017/2/22 267837.266889. That confirms the ABI end to end. - Five bad sets (missing directory, missing file, empty file, and oversize
orcme.binandorccode.bin) each exit 1 with a clear message, and a checksum of the whole window is identical after each.
Full make BOARD=s3l_lite at this commit, using the published toolchain. This is the first time that's been possible, and it took 3 min 36 s:
uImageis 1582 of 2048 KB, androotfs.squashfsis 3384 of 5120 KB, so it fits the 8 MB layout.load_ucodeand the modules are in the image.- The toolchain targets
cortex-a9withneon. A scan ofbusyboxandlibc.soforsdiv,udivand VFPv4 instructions found no real ones: the 16 raw hits were jump tables and float constants read as code, and gcc emits none at these flags even with-O3 -ffast-math. - That image boots on the QEMU machine to
openipc-s3l login:.S70vendorranload_ambarellaby itself, which reported the missing microcode, with no oops or illegal instruction. I only unpacked the rootfs into an initramfs to run it there.
Not covered here: my microcode was made up, so this proves the loader and not a DSP boot; your power-on logs are the evidence for that. Your hardware runs were also glibc armhf, so the musl softfp image hasn't run on a real S3L. That's still worth doing, as you offered.
The toolchain is now published, so you don't need to build your own for that test. The two remaining things to settle are the microcode licensing question and whether the 3A work needs the stock glibc libraries, because that would change s3l_lite itself.
|
This is tested and working on hardware with real |
|
Merged, thanks. Your hardware runs are the evidence for the DSP boot, and they cover what my made-up microcode couldn't. Good news on 3A: if it no longer needs the glibc libraries, musl softfp stays the right target, and the published toolchain needs no rebuild. Agreed that microcode gets its own PR. One thing for that discussion: yours is |
Problem
The S3L DSP boots from microcode (
orccode.bin,orcme.bin,default_binary.bin) that has to be copied into the windowdsp.koexposes through/dev/ucode. Nothing inambarella-osdrv-s3ldoes that, so on an OpenIPC image the DSP never starts. The vendor'sload_ucodecan't be shipped, and it is a glibc armhf binary that would not run on the musls3l_liteimage anyway.This adds
load_ucode, built from source in the package. It asksdsp.kofor the layout (UCODE_GET_INFO), maps the window, copies the three files to their offsets, tells the driver they are loaded, and prints the version. It takes the sameload_ucode DIRargument as the vendor tool, soload_ambarellafrom #2764 picks it up unchanged. Every file is checked against its slot before any is written, so a bad set cannot leave old and new microcode mixed in the window.The microcode itself is still not part of this package; how it is shipped stays the licensing question from #2764.
Hardware tested on
Ambarella S3L, Amcrest IP4M-1026E (Dahua IPC-HFW1430SN), OV4689. Netbooted image: 3.10.104 from
s3l.generic.configwith this package's modules, and the microcode from the camera's own stock firmware.load_ucodewas built static with the CI musl toolchain (toolchain.hisilicon-hi3516cv100). The package also builds through Buildroot with a glibc armhf toolchain. S3L is inUNBUILT_FAMILIES, so CI does not build it.Evidence
Each run is a power-on boot (power removed), so the window starts with no microcode in it. Window contents are the md5 of each slot read back from the DSP window, compared with the files.
Before (no loader on the image):
After (next power-on,
load_ucodebefore the first DSP start):The version line matches what the vendor's
load_ucodeprints for the same files. With the microcode loaded this way the pipeline streams. From a reboot where the vendor tool was not on the camera:Error paths:
Scope
general/package/all-patches/linux/general/overlay/or in a sharedload_<vendor>script hardcodes a value specific to my boardLD_PRELOAD, and no binaries that cannot be rebuilt from source