[libcamera-devel] [PATCH 2/2] ipa: raspberrypi: Control the lens position

[Naushir Patuck]

Hi Jean-Michel,

Thank you for your work.

On Wed, 16 Mar 2022 at 10:17, Kieran Bingham via libcamera-devel <
libcamera-devel at lists.libcamera.org> wrote:

> Quoting Jean-Michel Hautbois via libcamera-devel (2022-03-16 09:39:51)
> > Now that the ancillary links are configured, we can use the CameraLens
> > class and control the VCM through the IPA.
> > For now, force a default value for the lens position, until the AF
> > algorithm is introduced.
> >
> > Signed-off-by: Jean-Michel Hautbois <
> jeanmichel.hautbois at ideasonboard.com>
> > ---
> >  src/ipa/raspberrypi/raspberrypi.cpp | 38 +++++++++++++++++++++++++++--
> >  1 file changed, 36 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index fd8fecb0..0a0b5a3a 100644
> > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > @@ -108,6 +108,7 @@ private:
> >         void setMode(const IPACameraSensorInfo &sensorInfo);
> >         bool validateSensorControls();
> >         bool validateIspControls();
> > +       bool validateLensControls();
> >         void queueRequest(const ControlList &controls);
> >         void returnEmbeddedBuffer(unsigned int bufferId);
> >         void prepareISP(const ipa::RPi::ISPConfig &data);
> > @@ -132,6 +133,7 @@ private:
> >
> >         ControlInfoMap sensorCtrls_;
> >         ControlInfoMap ispCtrls_;
> > +       ControlInfoMap lensCtrls_;
> >         ControlList libcameraMetadata_;
> >
> >         /* Camera sensor params. */
> > @@ -342,13 +344,14 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> >                       const ipa::RPi::IPAConfig &ipaConfig,
> >                       ControlList *controls)
> >  {
> > -       if (entityControls.size() != 2) {
> > -               LOG(IPARPI, Error) << "No ISP or sensor controls found.";
> > +       if (entityControls.size() != 3) {
>
> Ok - so that explains the '2' in the previous patch.
>
> Feels a bit arbitrary though ... I wonder if it needs to be a map to
> make it more explicit to which control list is which...
>

The entityControls() are vestiges from the days before the mojom IPA
interface, and this was the
only mechanism to pass controls across.  I'm all for changing this to a map
or something more sensible
now that we have the flexibility.

Apart from that, my only real comment for this patch would be, similar to
what Kieran has pointed out,
we need to guard against instances where there is no lens driver.

Regards,
Naush


>
> > +               LOG(IPARPI, Error) << "No ISP, lens or sensor controls
> found.";
>
> Again, what happens on a device with no lens ...
>
>
> >                 return -1;
> >         }
> >
> >         sensorCtrls_ = entityControls.at(0);
> >         ispCtrls_ = entityControls.at(1);
> > +       lensCtrls_ = entityControls.at(2);
> >
> >         if (!validateSensorControls()) {
> >                 LOG(IPARPI, Error) << "Sensor control validation
> failed.";
> > @@ -360,6 +363,11 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> >                 return -1;
> >         }
> >
> > +       if (!validateLensControls()) {
>
> What happens here if there is no lens ...
>
>
> > +               LOG(IPARPI, Error) << "Lens control validation failed.";
> > +               return -1;
> > +       }
> > +
> >         maxSensorGainCode_ =
> sensorCtrls_.at(V4L2_CID_ANALOGUE_GAIN).max().get<int32_t>();
> >
> >         /* Setup a metadata ControlList to output metadata. */
> > @@ -578,6 +586,23 @@ bool IPARPi::validateIspControls()
> >         return true;
> >  }
> >
> > +bool IPARPi::validateLensControls()
> > +{
> > +       static const uint32_t ctrls[] = {
> > +               V4L2_CID_FOCUS_ABSOLUTE,
> > +       };
> > +
> > +       for (auto c : ctrls) {
> > +               if (lensCtrls_.find(c) == lensCtrls_.end()) {
> > +                       LOG(IPARPI, Error) << "Unable to find lens
> control "
> > +                                          << utils::hex(c);
> > +                       return false;
> > +               }
> > +       }
> > +
> > +       return true;
> > +}
> > +
> >  /*
> >   * Converting between enums (used in the libcamera API) and the names
> that
> >   * we use to identify different modes. Unfortunately, the conversion
> tables
> > @@ -1066,6 +1091,15 @@ void IPARPi::processStats(unsigned int bufferId)
> >
> >                 setDelayedControls.emit(ctrls);
> >         }
> > +
> > +       /*
> > +        * Set the focus position
> > +        * \todo Use an AF algorithm and replace the arbitrary value
> > +        */
> > +       ControlList lensCtrls(lensCtrls_);
> > +       lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE,
> static_cast<int32_t>(123));
> > +       setLensControls.emit(lensCtrls);
> > +
>
> no need for extra blank line here.
>
> But I guess this is just a 'sample' for the moment anyway.
>
> It really makes me think changing the IPA API to use a libcamera control
> list would be better too - so that the plumbing between is cleaner. But
> that's a whole chunk of rework...
>
>
> >  }
> >
> >  void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList
> &ctrls)
> > --
> > 2.32.0
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220317/540f1af5/attachment-0001.htm>