-
Notifications
You must be signed in to change notification settings - Fork 147
ipa: rpi: pisp: Add decompand support using PiSP hardware block #284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This patch integrates a new decompand algorithm that utilizes the PiSP hardware block available on Raspberry Pi 5. The implementation enables conversion of companded sensor data into linear format prior to ISP processing. Changes include: - Implementation of decompand logic for controller and pipe interfaces - Enabling decompand block by "rpi.decompand" in tuning.json Signed-off-by: Sena Asotani <[email protected]>
Adding @njhollinghurst and @davidplowman for visilbity. @asofam thank you for this work! We would be happy to include this functionality into the IPA after review. |
src/ipa/rpi/pisp/pisp.cpp
Outdated
return; | ||
|
||
pisp_fe_decompand_config config = {}; | ||
config.pad = decompandStatus->pad; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the pad
field everywhere (except the existing config structure).
Our convention is to set padding/reserved fields to zero. The {}
initializer will zero it here, which is all that's needed. fe->SetDecompand()
will also replace it with zero.
I don't think we need any such field to exist in the tuning file or the metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P.S. if you use it for some other purpose, e.g. as a validity flag, please give it a different name!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review! I've removed the pad
field from the tuning file, status structure, and controller code as suggested. The PiSP config structure still retains the field, but it's now zero-initialized using {}
.
This patch removes it from the tuning file, DecompandStatus, and related code. As per reviewer feedback, the 'pad' field is unnecessary outside the PiSP hardware config structure. Signed-off-by: Sena Asotani <[email protected]>
void Decompand::initialValues(uint16_t LUT[]) | ||
{ | ||
for (size_t i = 0; i < sizeof(decompandLUT_) / sizeof(decompandLUT_[0]); ++i) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few formatting issues like this that will need to be fixed up before we can merge. There is a handy git commit hook that can be setup to run a check and give you the formatting errors. You can find more details here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pointing that out. I wasn't aware of such a helpful tool!
I've run checkstyle.py and corrected the formatting issues accordingly.
29696, 31744, 33792, 35840, 37888, 39936, 41984, 44032, | ||
46080, 48128, 50176, 52224, 54272, 56320, 58368, 60416, | ||
62464 | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the "gamma" tuning block, we specify a PWL instead of the hardware LUT. This is easier for users to visualize, manipulate, and tune for. The IPA (pisp.cpp) translates the PWL into the HW LUT needed to be programmed into the registers.
It would be nice if we can do the same with the decompand block as well - and it provides all the advantages of using PWLs instead of hardware specified LUTs. Would it be possible for you to make this change? Sorry, this is more work for you, but I think it will be better in the long run. Feel free to ask questions if things are unclear!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion!
I will give it a try to switch the decompand block to use a PWL instead of a hardware LUT, following the approach used in the gamma block. It may take a bit of time, but I will look into the gamma implementation and work from there.
If I run into any unclear points along the way, I would appreciate your guidance. Thank you again!
Adjusted indentation and formatting to comply with project style guidelines,as reported by checkstyle.py. Signed-off-by: Sena Asotani <[email protected]>
Hi, could you explain a little more how this is used? As I don't know the details about IMX585... The code as it stands will apply the curve unconditionally, without regard to mode or pixel format. In general, a sensor might support multiple modes or pixel formats, only some of which need decompanding. Is that the case for IMX585? The example curve is flat up to 3072 -- is this as specified by Sony? For IMX585, what is the nominal black level after expansion to 16 bits and after decompanding? |
Thank you for the feedback. As with other sensors, the IMX585 requires decompanding only when Gradation Compression is enabled. So ideally, the decompanding should be applied conditionally based on the sensor mode. Would it make sense to make this controllable via a driver flag or through a libcamera command-line option? As for the flat portion of the curve up to 3072, my understanding is that the sensor clamps any values below the configured black level to that level internally. This clamping would occur before decompanding, so I think mapping the full range [0–3072] to a constant value (e.g., 0 → 3072, 1024 → 3072) better approximates the sensor's actual behavior than a linear mapping like 0 → 0, 1024 → 1024. For the nominal black level, the value after decompanding and expansion to 16-bit is 3072:
Note that the default black level of the IMX585 is 200, but I set it to 192 to align with values 1024 × n, which tends to work better with ISP behavior. |
I'm not sure. This could be opening up a can of worms, as API support for it might be lacking both in V4L2 and libcamera. Are you using a special driver to enable Graduation Compression, or is it a normal / default mode for IMX585? If it's the normal mode of operation, then I guess it's OK to do it unconditionally. I just wanted to understand if that was the case. EDIT: One possible generalization might be for the tuning file to contain a mapping: { bitdepth → LUT }. The algorithm could then look up the appropriate curve, based on the current mode's bit-depth. I'm assuming this might distinguish between decompanded and non-decompanded modes... Do you think that would be a helpful addition? (Note that in JSON, all keys have to be strings, not integers.)
For some sensors, there is variation both above and below the black level, due to various kinds of noise. Clamping at exactly the black level could introduce bias, so that noisier (higher gain) images become brighter on average. If lower values could occur with IMX585, then I think we'd want to extend the linear slope below the black level (and even if they can't, it seems harmless). |
I suggest for now we simply use the |
Thank you for the feedback.
I using a special driver.
Regarding the black level handling, I have rechecked the IMX585 data and confirmed the presence of pixel values below the black level. Based on this, I agree with the concern about introducing bias through clamping, and will modify the curve to maintain a linear slope below the black level rather than flattening it.
I agree that using the mode_ information to determine whether decompanding should be applied is a practical approach for now. I will update the patch to make decompanding conditional based on the selected mode. |
Summary
This pull request adds support for decompanding sensor data encoded with companding, using the PiSP hardware block
available on Raspberry Pi 5. The decompand operation is applied prior to ISP processing, enabling linearization of pixel values early in the pipeline.
Changes
1. Decompanding Algorithm
decompand_algorithm.h
(interface)decompand.cpp/h
(implementation)ipa/rpi/controller/
.2. PiSP Hardware Support
pisp.cpp
was modified to enable and configure this block based on the provided tuning parameters.3. Sensor Tuning Configuration
"rpi.decompand"
parameters, includinglut
andpad
imx585.json
tuning file is derived from a version provided by soho-enterprise.co, with modifications limited to the"rpi.decompand"
section.It may be updated or replaced in the future as upstream sensor support becomes available.
4. Architecture Consideration
Rationale
Decompanding is a critical early-stage operation in many modern image pipelines,
especially for sensors that output companded pixel values. By enabling this in the PiSP,
we offload the processing to dedicated hardware, improve performance, and lay the foundation
for accurate linear-space ISP tuning.
Notes
"rpi.decompand"
key is defined.which is not part of the upstream Linux kernel or libcamera.
Testing
"rpi.decompand"
configurationSigned-off-by
Signed-off-by: Sena Asotani [email protected]