sony_imx335: the 5M "12bit" mode was programming the sensor for 10 - #224
Merged
Merged
Conversation
PR Summary by QodoFix IMX335 5MP mode to output genuine 12-bit RAW data
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
1. 4-megapixel captures use wrong depth
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
| IMX335_write_register(ViPipe, 0x3077, 0x0F); | ||
|
|
||
| IMX335_write_register(ViPipe, 0x3050, 0x00); | ||
| IMX335_write_register(ViPipe, 0x3050, 0x01); /* ADBIT = 12-bit */ |
There was a problem hiding this comment.
1. 4-megapixel captures use wrong depth 🐞 Bug ≡ Correctness
cmos_set_image_mode maps the explicitly classified 2560×1440 10-bit linear request to IMX335_5M_30FPS_12BIT_LINEAR_MODE, whose initializer now emits 12-bit samples while cmos_get_isp_default still reports 10 bits per sample and a white level of 2592. Whenever that shared mode is selected for raw or DNG capture, the 10-bit classification, receiver path, and metadata reach sensor data with a 12-bit depth and 4095 maximum value.
Agent Prompt
## Issue description
The 2560×1440 linear request is defined as 10-bit but shares an initializer that now produces genuine 12-bit output, while the ISP/DNG metadata still declares 10-bit samples and a white level of 2592.
## Fix Focus Areas
- libraries/sensor/hi3516ev200/sony_imx335/imx335_sensor_ctl.c[1518-1542]
- libraries/sensor/hi3516ev200/sony_imx335/imx335_cmos.c[160-165]
- libraries/sensor/hi3516ev200/sony_imx335/imx335_cmos.c[1833-1843]
- libraries/sensor/hi3516ev200/sony_imx335/imx335_sensor_ctl.c[237-242]
- libraries/sensor/hi3516ev200/sony_imx335/imx335_cmos.c[1379-1387]
## Recommended Fix
Give the 2560×1440 10-bit request a distinct mode and initializer that retains the 10-bit register tuple, and dispatch only the 2592×1944 12-bit request to the modified initializer. For the 5-megapixel 12-bit linear mode, set `u8BitsPerSample` to 12 and `u32WhiteLevel` to 4095 while keeping genuinely 10-bit modes in separate metadata cases; alternatively, if 2560×1440 is intentionally becoming 12-bit, rename its classification and update its receiver and metadata configuration to 12-bit together.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
IMX335_linear_5M30_12bit_init() set every bit-depth register to 10-bit, in a
function whose name says otherwise:
0x3050 0x00 ADBIT = 10-bit
0x319D 0x00 MDBIT = 10-bit
0x341C 0xFF ADBIT1 = 0x01FF, 10-bit
0x341D 0x01
The correct values were written in the comments beside them all along -- one
reads "//00-10bit | 01 - 12bit" next to a 0x00, and another "//Input AD bit
0047-12bit | 01FF-10bit" next to 0x01FF.
5M_imx335.ini declares raw_bitness=12, so the ISP was told to expect twelve
bits, received ten, and left-aligned them. Measured on a hi3516ev200 with
IMX335 before this change, over 719836 sampled pixels of a daylight frame:
multiples of 4 100.00% every low bit pair zero
distinct values 929 of the 4096 twelve bits allow
highest value 4092 of 4095
The whole readout was ten bits of information in a twelve-bit container, and
nothing said so: the raw file's BitsPerSample and WhiteLevel both claimed 12.
After:
multiples of 4 25.12% chance, as it should be
distinct values 3716
highest value 4095
low nibble all sixteen values, flat (44392..45606 each)
What it buys, same scene minutes apart:
padded 10-bit 907 levels used, darkest fifth resolved in 17 steps
genuine 12-bit 3397 levels used, darkest fifth resolved in 65 steps
3.8x the tonal resolution in the shadows, which is where the extra bits are
worth having. The picture is unchanged otherwise -- sharp, correctly coloured,
full 2592x1944 -- and SYS_MODE 0x02 (891 Mbps) carries the extra 20% of MIPI
bandwidth without alteration, so no timing register moves.
THE 40FPS SEQUENCE IS DELIBERATELY LEFT ALONE. IMX335_linear_5M30_12bit_40fps_
init() has the identical four registers set to ten bits and the identical
misleading name, but it is reached through high-fps/imx335_2592x1944_45fps.ini,
which declares raw_bitness=10. Patching it was tried on the same camera and the
ISP and sensor then disagreed: a magenta cast over every white surface, gone
again the moment it was reverted. Its name is what is wrong there, not its
registers -- the sensor and the INI that drives it have to agree, and that pair
already does.
Review follow-up, folding in #231 (the same fix, found again from OpenIPC/firmware#2430 and measured on two more boards). The mode's DNG descriptor still said 10 bits per sample with a white level of 2592. It now says 12 and 4095, as the hi3516cv500 driver does for the same mode. A 2592x1944 or 2560x1440 request from a raw_bitness=10 profile also lands in this init, e.g. high-fps/imx335_2592x1944_45fps.ini run at 30 fps or below. That was checked rather than guarded: with the sensor at 12 bits and the receiver at RAW10, the receiver keeps the top ten bits. Raw frames match the 10-bit ADC within 1% per channel, dim and saturated, and RTSP frames give the same colour on white surfaces (230/241/240 either way). The comment now records why the faster modes stay at 10 bits. Measured on gk7205v300: at HMAX 0x1A0 and 0x16E the 12-bit ADC shades the frame from left to right by up to -36%/+17% against the 10-bit ADC, reproducibly. At 0x12C it delivers no frames. The two-lane driver gets a TODO instead of the change, because no two-lane board was available to check that 12-bit output fits its link. Dark frames, 0.1 ms, all gains 1x, temporal noise from differenced pairs, this branch's library loaded from init: 10-bit ADC 12-bit ADC gk7205v300 2.23 1.22 counts rms hi3516ev300 2.36 1.56 counts rms
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
IMX335_linear_5M30_12bit_init()set every bit-depth register to 10-bit, in a function whose name says otherwise:The correct values were sitting in the comments beside them the whole time — one reads
//00-10bit | 01 - 12bitnext to a0x00, another//Input AD bit 0047-12bit | 01FF-10bitnext to0x01FF. No init sequence in the file enabled 12-bit anywhere.5M_imx335.inideclaresraw_bitness=12, so the ISP was told to expect twelve bits, received ten, and left-aligned them. Nothing said so — the raw file'sBitsPerSampleandWhiteLevelboth claimed 12.Measured on a hi3516ev200 + IMX335
719,836 sampled pixels of a daylight frame:
What it buys
Same scene, minutes apart:
3.8× the tonal resolution in the shadows, which is where the extra bits are worth having — reading a retroreflective plate against a dark car, for instance.
The picture is otherwise unchanged: sharp, correctly coloured, full 2592×1944.
SYS_MODE 0x02(891 Mbps) carries the extra 20% of MIPI bandwidth without alteration, so no timing register moves — HMAX, VMAX and the PLL are untouched.The 40 fps sequence is deliberately left alone
IMX335_linear_5M30_12bit_40fps_init()has the identical four registers set to ten bits and the identical misleading name. I patched it too, and tested it:It is reached through
high-fps/imx335_2592x1944_45fps.ini, which declaresraw_bitness=10. With the sensor sending twelve bits and the ISP expecting ten, the two disagree — a magenta cast appeared over every white surface, and vanished the moment the change was reverted.So there, the name is what is wrong, not the registers. The sensor and the INI that drives it have to agree, and that pair already does. Renaming it would be a reasonable follow-up; changing its registers would not.
Follow-up: review, and the duplicate #231 folded in
#231 was the same fix, found again from OpenIPC/firmware#2430 by a user reading raw frames for X-ray work. Its evidence and extras are in this PR now, and #231 is closed.
The DNG descriptor (review finding) now says 12 bits and a white level of 4095 for this mode, as the hi3516cv500 driver does.
A raw_bitness=10 profile landing in this init is harmless. Both a 2560×1440 request and
high-fps/imx335_2592x1944_45fps.inirun at 30 fps or below reach this init. That was measured on gk7205v300 rather than guarded:Why the faster modes stay at 10 bits, measured on gk7205v300 with the 12-bit ADC against the 10-bit ADC at the same line period:
0x2260x1A00x16E0x12CThat shading is a likelier source of the magenta cast described above for the 40 fps sequence than the bit-depth mismatch, which measured harmless.
Dark frames at 0.1 ms, all gains 1x, temporal noise from differenced pairs, with this branch's library loaded from init:
The noise drop is larger than finer quantisation alone would give.
On a lit scene, the mean is unchanged, and encoded frame rate and MIPI error counters are identical in both modes. Long exposure through a fractional frame rate still works: 999504 µs at 0.5 fps.
The two-lane driver gets a TODO instead of the change. No two-lane board was available to check that 12-bit output fits its link at
SYS_MODE1188 Mbps.