[libcamera-devel] [PATCH v2 3/3] rkisp1: Add camera lens position control from IPA
Daniel Semkowicz
dse at thaumatec.com
Thu Aug 11 11:21:17 CEST 2022
On Thu, Aug 11, 2022 at 10:22 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi Daniel
>
> On Thu, Aug 11, 2022 at 09:10:48AM +0200, Daniel Semkowicz wrote:
> > Hi!
> >
> > On Wed, Aug 10, 2022 at 1:34 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >
> > > Hi Daniel,
> > > + Kieran and Laurent
> > >
> > > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote:
> > > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13)
> > > > > Hi Daniel
> > > > >
> > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, 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.
> > > > > >
> > > > > > 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 eaf3955e..caa1121a 100644
> > > > > > --- a/include/libcamera/ipa/rkisp1.mojom
> > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom
> > > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface {
> > > > > > interface IPARkISP1EventInterface {
> > > > > > paramsBufferReady(uint32 frame);
> > > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
> > > > > > + setLensControls(libcamera.ControlList sensorControls);
> > > > > > metadataReady(uint32 frame, libcamera.ControlList metadata);
> > > > > > };
> > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h
> > > > > > index 2bdb6a81..d6d46b57 100644
> > > > > > --- a/src/ipa/rkisp1/ipa_context.h
> > > > > > +++ b/src/ipa/rkisp1/ipa_context.h
> > > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration {
> > > > > > };
> > > > > >
> > > > > > struct IPAFrameContext {
> > > > > > + struct {
> > > > > > + uint32_t lensPosition;
> > > > > > + bool applyLensCtrls;
> > > > > > + } af;
> > > > > > +
> > > > >
> > > > > So, I feel like this patch is better delayed after we have completed
> > > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms
> > > > > to retain their state internally instead of using the IPAFrameContext.
> > > > >
> > > > > There's a series almost landed that renames IPAFrameContext to
> > > > > IPAActiveState and there will be a few more to align the two IPA
> > > > > modules.
> > > > > Would you mind taking this in the series that introduces the AF
> > > > > algorithm while I try to collect 1 and 2 asap ?
> > > >
> > > > It does seem like a lot of this will change indeed. So we really should
> > > > try to clean up the interfaces to get it a bit more stable.
> > > >
> > > >
> > > > >
> > > > > > struct {
> > > > > > uint32_t exposure;
> > > > > > double gain;
> > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > index 9e4c48a2..39e1ab2f 100644
> > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info,
> > > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain);
> > > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain);
> > > > > >
> > > > > > + /* Lens position is unknown at the startup, so initilize the variable
> > > > > > + * that holds the current position to something out of the range. */
> > > > > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max();
> > > > > > +
> > > > > > context_.frameContext.frameCount = 0;
> > > > > >
> > > > > > for (auto const &algo : algorithms()) {
> > > > > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame)
> > > > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > > > > >
> > > > > > setSensorControls.emit(frame, ctrls);
> > > > > > +
> > > > > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {
> > > > > > + context_.frameContext.af.applyLensCtrls = false;
> > > > > > + ControlList lensCtrls(*lensCtrls_);
> > > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> > > > > > + static_cast<int32_t>(context_.frameContext.af.lensPosition));
> > > > > > + setLensControls.emit(lensCtrls);
> > > > > > + }
> > >
> > > IPU3 here does
> > >
> > > ControlList ctrls(sensorCtrls_);
> > > ctrls.set(V4L2_CID_EXPOSURE, exposure);
> > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain);
> > >
> > > ControlList lensCtrls(lensCtrls_);
> > > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> > > static_cast<int32_t>(context_.activeState.af.focus));
> > >
> > > setSensorControls.emit(frame, ctrls, lensCtrls);
> > >
> > > While Daniel has implemented
> > >
> > > ControlList ctrls(sensorCtrls_);
> > > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
> > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain));
> > >
> > > setSensorControls.emit(frame, ctrls);
> > > if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) {
> > > context_.frameContext.af.applyLensCtrls = false;
> > > ControlList lensCtrls(*lensCtrls_);
> > > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> > > static_cast<int32_t>(context_.frameContext.af.lensPosition));
> > > setLensControls.emit(lensCtrls);
> > > }
> > >
> > > Yesterday we briefly discussed about the possibility of sending
> > > both sensor and lens controls as part of a single control list, but
> > > what's the preference in the meantime ? One signal with 2 lists or 2
> > > signals ?
> > >
> > > I would prefer a single signal, mostly because it's handling is
> > > synchronous and we can avoid one context switch ?
> >
> > Single control list sounds as the best solution in my opinion.
> > Especially that lens have only one control item. If there are plans to
> > use a single control list, then in the meantime, I would go with a
>
> Yes, that's the idea. When we'll be done aligning the two IPA
> implementations we're going to move the IPA to use libcamera::controls
> and have the libcamera::control->v4l2::control translation in the
> CameraSensor/CameraLens class.
>
> I have sent a few weeks ago a first version [1], which need to be rebased
> on top of [2]. So yeah, things are moving a little too much in this
> area, that's why I suggested you to post-pone the AF algorithm series
> if that's not a problem with you.
>
> [1] https://patchwork.libcamera.org/project/libcamera/list/?series=3238
> [2] https://patchwork.libcamera.org/project/libcamera/list/?series=3376
Hmm, it makes sense to postpone it until the IPA will be stable as these
are conflicting changes.
I already made some changes to the AF after the discussions, so I will
submit my current version, but I will wait for the above series to be
merged and then rebase everything.
Best regards
Daniel
>
> > single signal and two lists. It would be closer to the final idea.
> >
> > Best regards
> > Daniel
> >
> > >
> > > > > > }
> > > > > >
> > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState)
> > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > index 5f10c26b..de0d37da 100644
> > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > > > > @@ -108,6 +108,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);
> > > > > > };
> > > > > > @@ -324,6 +325,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);
> > > > > >
> > > > > > @@ -378,6 +380,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;
> > > >
> > > > I think the construction of sensor_->focusLens should make sure that the
> > > > correct control is set, so that if we have a valid CameraLens
> > > > *focusLens, we know it's valid to call focusLens->setFocusPosition().
> > > >
> > > > So the above lensControls.contains() should be redundant.
> > > >
> > > >
> > > > > > +
> > > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE);
> > > > > > +
> > > > > > + focusLens->setFocusPosition(focusValue.get<int32_t>());
> > > >
> > > > But I think Jacopo also has plans to change this so that a libcamera
> > > > control is passed in anyway, meaning the conversion from libcamera focus
> > > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the
> > > > CameraLens class anyway.
> > > >
> > > >
> > > > > > +}
> > > > > > +
> > > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
> > > > > > {
> > > > > > RkISP1FrameInfo *info = frameInfo_.find(frame);
> > > > > > --
> > > > > > 2.34.1
> > > > > >
More information about the libcamera-devel
mailing list