[PATCH RFC 0/3] Pass sensor delays from rkisp1 IPA to the pipeline handler

[Mikhail Rudenko]

Hi Dan!

On 2024-10-31 at 13:49 GMT, Dan Scally <dan.scally at 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