[libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens position from IPA
Matthias Fend
matthias.fend at emfend.at
Tue Feb 7 09:20:34 CET 2023
Hi Dave, hi Jacopo,
Am 06.02.2023 um 16:26 schrieb Dave Stevenson:
> Hi Jacopo
>
> On Mon, 6 Feb 2023 at 12:09, Jacopo Mondi <jacopo.mondi at ideasonboard.com> wrote:
>>
>> Hi Dave,
>> thanks for chiming in and sorry I've not cc-ed you, to me you're the
>> RPi kernel guru and I never know if it's fine to rope you in in
>> libcamera stuff. Happy you're reading the ml and reply so promptly.
>
> I'm currently on other stuff, but am keeping an eye out on the mailing
> lists for interesting stuff.
> Naush & David will rope me in when needed.
>
>> On Mon, Feb 06, 2023 at 11:23:49AM +0000, Dave Stevenson wrote:
>>> Hi Jacopo and Daniel
>>>
>>> On Mon, 6 Feb 2023 at 09:46, Jacopo Mondi via libcamera-devel
>>> <libcamera-devel at lists.libcamera.org> wrote:
>>>>
>>>> Hi Daniel
>>>>
>>>> On Thu, Jan 19, 2023 at 09:41:06AM +0100, Daniel Semkowicz via libcamera-devel wrote:
>>>>> Allow control of lens position from the IPA, by setting corresponding
>>>>> af fields in the IPAFrameContext structure. Controls are then passed to
>>>>> the pipeline handler, which sets the lens position in CameraLens.
>>>>>
>>>>
>>>> As a minor nit: I would move this to the end of the series, after
>>>> having plumbed in the algorithm implementation... Just a nit though
>>>>
>>>>> Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
>>>>> ---
>>>>> include/libcamera/ipa/rkisp1.mojom | 1 +
>>>>> src/ipa/rkisp1/ipa_context.h | 5 +++++
>>>>> src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++
>>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++
>>>>> 4 files changed, 34 insertions(+)
>>>>>
>>>>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
>>>>> index bf6e9141..c3ed87aa 100644
>>>>> --- a/include/libcamera/ipa/rkisp1.mojom
>>>>> +++ b/include/libcamera/ipa/rkisp1.mojom
>>>>> @@ -39,5 +39,6 @@ interface IPARkISP1Interface {
>>>>> interface IPARkISP1EventInterface {
>>>>> paramsBufferReady(uint32 frame);
>>>>> setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
>>>>> + setLensControls(libcamera.ControlList lensControls);
>>>>> metadataReady(uint32 frame, libcamera.ControlList metadata);
>>>>> };
>>>>> diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
>>>>> index b9b20653..1fac6af9 100644
>>>>> --- a/src/ipa/rkisp1/ipa_context.h
>>>>> +++ b/src/ipa/rkisp1/ipa_context.h
>>>>> @@ -53,6 +53,11 @@ struct IPASessionConfiguration {
>>>>> };
>>>>>
>>>>> struct IPAActiveState {
>>>>> + struct {
>>>>> + uint32_t lensPosition;
>>>>> + bool applyLensCtrls;
>>>>
>>>> Oh gosh lens are -complicated- /o\
>>>>
>>>> For regular sensor controls we defer the decision to apply or not a
>>>> control to DelayedControls on the pipeline handler side, which takes
>>>> care of evaluating if a control has to be updated or not and how many
>>>> frame it takes to take effect.
>>>>
>>>> Lenses (well, VCM to be fair) are a bit more complex than that, in the
>>>> sense that moving the lens might take a number of frames that depends
>>>> on the movement range. Some VCM datasheet I've seen provide a model to
>>>> estimate the delay depending on the movement range and the VCM
>>>> characteristics. The risk is that updating the lens position while the
>>>> lens hasn't reached its final position might trigger some resonation
>>>> effect, especially if the algorithm runs in "continuous auto-focus"
>>>> mode and tries to update the lens position too often.
>>>
>>> Please don't limit thinking to VCMs.
>>>
>>> From my days of doing CCTV control systems I have a Computar 7.5-120mm
>>> motorized zoom and focus lens with C-mount [1]. It has motors to drive
>>> the optics, and potentiometers to read back the position. Add a couple
>>> of L298N motor driver chips and an ADC and I can control it fine,
>>> however significant movement takes several seconds.
>>> Likewise there are stepper motor controlled lenses such as [2]. You
>>> need a reset sequence at power on (largely driving into an endstop),
>>> but after that you have calibrated movement.
>>>
>>> I have MCU code to drive both of those via I2C, but V4L2 really needs
>>> a control for reporting the current focus (and zoom) position, with
>>> the existing controls taking the requested position. The IPA can then
>>> hold off if it knows the lens hasn't finished moving yet.
>>
>> Just to be clear:
>>
>> - "Existing control" being V4L2_CID_FOCUS_ABSOLUTE and
>> V4L2_CID_FOCUS_RELATIVE which allows to set the lens position
Thanks for bringing this up. I'm glad to hear that there are more people
out there who are dealing with some more elaborate optics ;)
>
> Yes.
> V4L2_CID_FOCUS_RELATIVE is near useless in my book, but exists.
> V4L2_CID_FOCUS_ABSOLUTE as the existing control sets the (desired)
> focus position.
> Likewise V4L2_CID_ZOOM_ABSOLUTE for desired zoom position
> (V4L2_CID_ZOOM_RELATIVE is also fairly useless, but also unused).
>
>> - And "need a control" would be something that allows reporting if the
>> lens is still moving, it has reached the desired point etc ?
>
> "Still moving" can be subjective if worrying about noise in ADC
> values, PID control loops, ringing, overshoots, etc, hence my
> suggestion of current position and allowing userspace to make a call
> as to if it is good enough.
I think that even both information can be helpful here.
The current position is basically very important and probably already
covers most cases. For example, you can instruct the MCU that moves the
optics to move the whole focus area from one end to the other (which can
take a few seconds) and observe the peak value of the AF statistics. If
you also have the current position of the focus lens, you know pretty
well where the perfect position will be.
Whether the lens group is still moving can be helpful in cases where the
desired position cannot be reached (anymore). This can be the case, for
example, if the originally desired focus position may no longer be
approached in order to avoid a collision with other lens groups. There
are optics where the movement ranges of the lens groups overlap.
I also see a need for additional enhancements like a possibility to move
to absolute position with a certain speed, to move single lens groups
for calibration, to search the mechanical limits of the optics, etc.
>
> If you're reading back the position via an ADC or by counting stepper
> pulses, then you know where you are.
> For VCMs there often isn't an easy read back of position, but if you
> have movement timing information in the datasheet then that can be
> replicated in the kernel driver. Simplistic would be to report the old
> position until sufficient time has passed for the new position to be
> valid.
>
> Adding something like V4L2_CID_FOCUS_CURRENT_ABSOLUTE and
> V4L2_CID_ZOOM_CURRENT_ABSOLUTE would allow reporting back whether the
> lens has got to the requested position. Both would need to be volatile
> to ensure the driver got the chance to give the current position.
In my proposal patch series I tried to illustrate a way to transport
such volatile controls from the driver to the algorithm.
>
>>>
>>> Just food for thought.
>>
>> Trust me, I would have preferred a stricter diet when it comes to
>> thoughts about lens and optics :)
>
> It is a minefield, but partly as it's just been ignored for so long :-(
>
> Hmm, atomisp ov5693 appears to be the only implementation of
> V4L2_CID_FOCUS_RELATIVE, and that only maps on V4L2_CID_FOCUS_ABSOLUTE
> using "current + val".
> In looking that up I notice that they have defined their own
> V4L2_CID_VCM_SLEW and V4L2_CID_VCM_TIMING controls, and it looks like
> they're defining a g_volatile_ctrl for V4L2_CID_FOCUS_ABSOLUTE which
> returns the dead-reckoned position based on time since the position
> request was made.
> So perhaps returning a different value via V4L2_CID_FOCUS_ABSOLUTE is
> acceptable then? It feels a little horrid to me, and I know never to
> take atomisp behaviour as acceptable.
I don't like the idea of modeling the mechanical behavior in the driver.
I would find it better if the driver only returns things where it can
make a meaningful contribution. This would mean in this case that the
driver only returns the control for the current position if it has a
corresponding measured value.
If you want to estimate the actual position for a VCM via an
approximation, then I would see this in libcamera.
The driver usually only maps the control chip for the VCM. But for the
estimation of the position other facts like the mounted optics and the
orientation in space can be relevant. These are things libcamera can
know about, but not the driver. Libcamera could then implement an optics
helper that provides such an estimation for all VCM based optics.
~Matthias
>
> Dave
>
>>>
>>> Dave
>>>
>>> [1] https://computar.com/product/680/H16Z7516PDC (at full zoom it
>>> should be able to view things a couple of miles away with IMX477 or
>>> similar)
>>> [2] https://www.amazon.co.uk/dp/B09V53CMSK
>>>
>>>> I've cc-ed Matthias Fend which has recently sent a series for "more
>>>> complex optics"
>>>> https://patchwork.libcamera.org/project/libcamera/list/?series=3735
>>>> (which partially overlaps with the work you've done here) as he
>>>> certainly knows more than me about VCM and optics to know what his
>>>> opinion is
>>>>
>>>>
>>>>> + } af;
>>>>> +
>>>>> struct {
>>>>> struct {
>>>>> uint32_t exposure;
>>>>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>>>>> index 9e861fc0..297161b2 100644
>>>>> --- a/src/ipa/rkisp1/rkisp1.cpp
>>>>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>>>>> @@ -270,6 +270,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig,
>>>>> return format.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
>>>>> });
>>>>>
>>>>> + /* Lens position is unknown at the startup, so initilize the variable
>>>> ^ initialize
>>>>> + * that holds the current position to something out of the range. */
>>>>
>>>> Multiline comments as
>>>> /*
>>>> * first line
>>>> * next line
>>>> */
>>>>
>>>>> + context_.activeState.af.lensPosition = std::numeric_limits<int32_t>::max();
>>>>> +
>>>>> for (auto const &a : algorithms()) {
>>>>> Algorithm *algo = static_cast<Algorithm *>(a.get());
>>>>>
>>>>> @@ -452,6 +456,14 @@ void IPARkISP1::setControls(unsigned int frame)
>>>>> ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
>>>>>
>>>>> setSensorControls.emit(frame, ctrls);
>>>>> +
>>>>> + if (lensControls_ && context_.activeState.af.applyLensCtrls) {
>>>>> + context_.activeState.af.applyLensCtrls = false;
>>>>> + ControlList lensCtrls(*lensControls_);
>>>>> + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
>>>>> + static_cast<int32_t>(context_.activeState.af.lensPosition));
>>>>> + setLensControls.emit(lensCtrls);
>>>>> + }
>>>>> }
>>>>>
>>>>> } /* namespace ipa::rkisp1 */
>>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>>> index 0559d261..b2fedc5f 100644
>>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>>>> @@ -113,6 +113,7 @@ private:
>>>>> void paramFilled(unsigned int frame);
>>>>> void setSensorControls(unsigned int frame,
>>>>> const ControlList &sensorControls);
>>>>> + void setLensControls(const ControlList &lensControls);
>>>>>
>>>>> void metadataReady(unsigned int frame, const ControlList &metadata);
>>>>> };
>>>>> @@ -337,6 +338,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>>>>> return -ENOENT;
>>>>>
>>>>> ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
>>>>> + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls);
>>>>> ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
>>>>> ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>>>>>
>>>>> @@ -400,6 +402,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
>>>>> delayedCtrls_->push(sensorControls);
>>>>> }
>>>>>
>>>>> +void RkISP1CameraData::setLensControls(const ControlList &lensControls)
>>>>> +{
>>>>> + CameraLens *focusLens = sensor_->focusLens();
>>>>> + if (!focusLens)
>>>>> + return;
>>>>> +
>>>>> + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE))
>>>>> + return;
>>>>> +
>>>>> + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
>>>>> +
>>>>> + focusLens->setFocusPosition(focusValue.get<int32_t>());
>>>>> +}
>>>>> +
>>>>> void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
>>>>> {
>>>>> RkISP1FrameInfo *info = frameInfo_.find(frame);
>>>>> --
>>>>> 2.39.0
>>>>>
More information about the libcamera-devel
mailing list