| Message ID | 20241028173659.247353-1-mike.rudenko@gmail.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
Hi Mikhail, thank you for the patches. Mikhail Rudenko <mike.rudenko@gmail.com> writes: > Hi, > > At present, rkisp1 uses hardcoded sensor control delays. In the case > when they do not match real sensor delays, the AGC algorithm operates > in a suboptimal mode, leading to gain/exposure oscillations on capture > start and slower convergence. > > This series does 3 things to fix this situation. Nice, later, it could be fixed for other pipelines too. > First, it adds an IPC structure for sensor control delays. Second, it > adds sensorDelays() method to the CameraSensorHelper and overrides it > where sensor-specific delays are known. And finally, it replaces > hardcoded delays in rkisp1 pipeline handler with those obtained from > the CameraSensorHelper via IPA. > > I'm not completely sure this is the best solution from the > architecture viewpoint, thus "RFC". Any feedback is welcome! The approach looks good to me, I'm just not sure about the way IPASensorDelays is passed around in the 3rd patch. In any case, let's see what the maintainers or others think. > Mikhail Rudenko (3): > ipa: core: add IPASensorDelays structure > libcamera: libipa: camera_sensor: Add sensorDelays method > ipa: rkisp1: Pass sensor delays from IPA to pipeline handler > > include/libcamera/ipa/core.mojom | 35 ++++++++++++++ > include/libcamera/ipa/rkisp1.mojom | 3 +- > src/ipa/libipa/camera_sensor_helper.cpp | 59 ++++++++++++++++++++++++ > src/ipa/libipa/camera_sensor_helper.h | 5 ++ > src/ipa/rkisp1/rkisp1.cpp | 8 +++- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 27 +++++------ > 6 files changed, 118 insertions(+), 19 deletions(-) > > -- > 2.46.0
Hi Mikhail - thanks for the RFC On 28/10/2024 17:36, Mikhail Rudenko wrote: > Hi, > > At present, rkisp1 uses hardcoded sensor control delays. In the case > when they do not match real sensor delays, the AGC algorithm operates > in a suboptimal mode, leading to gain/exposure oscillations on capture > start and slower convergence. > > This series does 3 things to fix this situation. First, it adds an IPC > structure for sensor control delays. Second, it adds sensorDelays() > method to the CameraSensorHelper and overrides it where > sensor-specific delays are known. And finally, it replaces hardcoded > delays in rkisp1 pipeline handler with those obtained from the > CameraSensorHelper via IPA. > > I'm not completely sure this is the best solution from the > architecture viewpoint, thus "RFC". Any feedback is welcome! I had a similar patch locally, but I decided to swap it in the end so that the delays are defined as members of the CameraSensorProperties class, which is available from the PipelineHandlers already - so setting the delays in the rkisp1 (and all the other...) pipeline handlers becomes: /* Initialize the camera properties. */ data->properties_ = data->sensor_->properties(); unsigned int gainDelay = data->properties_.get(properties::GainDelay).value(); unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value(); std::unordered_map<uint32_t, DelayedControls::ControlParams> params = { { V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } }, { V4L2_CID_EXPOSURE, { exposureDelay, false } }, }; Unless the delays are going to also be needed by the IPA module I think that's probably a bit cleaner - I can share the patches to show the full changeset. Do you anticipate the IPA's needing the delay values? Thanks Dan > > Mikhail Rudenko (3): > ipa: core: add IPASensorDelays structure > libcamera: libipa: camera_sensor: Add sensorDelays method > ipa: rkisp1: Pass sensor delays from IPA to pipeline handler > > include/libcamera/ipa/core.mojom | 35 ++++++++++++++ > include/libcamera/ipa/rkisp1.mojom | 3 +- > src/ipa/libipa/camera_sensor_helper.cpp | 59 ++++++++++++++++++++++++ > src/ipa/libipa/camera_sensor_helper.h | 5 ++ > src/ipa/rkisp1/rkisp1.cpp | 8 +++- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 27 +++++------ > 6 files changed, 118 insertions(+), 19 deletions(-) > > -- > 2.46.0
Hi Dan! On 2024-10-31 at 13:49 GMT, Dan Scally <dan.scally@ideasonboard.com> wrote: > Hi Mikhail - thanks for the RFC > > On 28/10/2024 17:36, Mikhail Rudenko wrote: >> Hi, >> >> At present, rkisp1 uses hardcoded sensor control delays. In the case >> when they do not match real sensor delays, the AGC algorithm operates >> in a suboptimal mode, leading to gain/exposure oscillations on capture >> start and slower convergence. >> >> This series does 3 things to fix this situation. First, it adds an IPC >> structure for sensor control delays. Second, it adds sensorDelays() >> method to the CameraSensorHelper and overrides it where >> sensor-specific delays are known. And finally, it replaces hardcoded >> delays in rkisp1 pipeline handler with those obtained from the >> CameraSensorHelper via IPA. >> >> I'm not completely sure this is the best solution from the >> architecture viewpoint, thus "RFC". Any feedback is welcome! > > > I had a similar patch locally, but I decided to swap it in the end so > that the delays are defined as members of the CameraSensorProperties > class, which is available from the PipelineHandlers already - so > setting the delays in the rkisp1 (and all the other...) pipeline > handlers becomes: > > > /* Initialize the camera properties. */ > data->properties_ = data->sensor_->properties(); > > unsigned int gainDelay = data->properties_.get(properties::GainDelay).value(); > unsigned int exposureDelay = data->properties_.get(properties::ExposureDelay).value(); > std::unordered_map<uint32_t, DelayedControls::ControlParams> params = { > { V4L2_CID_ANALOGUE_GAIN, { gainDelay, false } }, > { V4L2_CID_EXPOSURE, { exposureDelay, false } }, > }; I've used the approach taken by RPi PH/IPA. I also had a thought that CameraSensorProperties could be a better place to store the delay. It's actually the question I wanted to raise when posting this RFC :) > Unless the delays are going to also be needed by the IPA module I > think that's probably a bit cleaner - I can share the patches to show > the full changeset. Do you anticipate the IPA's needing the delay > values? I can't speak for all the IPAs, but as of rkisp1 I see no need for explicit control delays in IPA. Delayed control values are already passed by the PH in processStatsBuffer and that looks sufficient. I think your approach is better overall, so please post your series. Let's see what the maintainers say. > > Thanks > > Dan > > >> >> Mikhail Rudenko (3): >> ipa: core: add IPASensorDelays structure >> libcamera: libipa: camera_sensor: Add sensorDelays method >> ipa: rkisp1: Pass sensor delays from IPA to pipeline handler >> >> include/libcamera/ipa/core.mojom | 35 ++++++++++++++ >> include/libcamera/ipa/rkisp1.mojom | 3 +- >> src/ipa/libipa/camera_sensor_helper.cpp | 59 ++++++++++++++++++++++++ >> src/ipa/libipa/camera_sensor_helper.h | 5 ++ >> src/ipa/rkisp1/rkisp1.cpp | 8 +++- >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 27 +++++------ >> 6 files changed, 118 insertions(+), 19 deletions(-) >> >> -- >> 2.46.0 -- Best regards, Mikhail Rudenko