[libcamera-devel] [PATCH v3 2/8] rkisp1: Control camera lens position from IPA
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Feb 6 13:09:42 CET 2023
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.
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
- And "need a control" would be something that allows reporting if the
lens is still moving, it has reached the desired point etc ?
>
> Just food for thought.
Trust me, I would have preferred a stricter diet when it comes to
thoughts about lens and optics :)
>
> 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