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

[David Plowman]

Hi Jean-Michel

Thanks for starting this work!

On Thu, 17 Mar 2022 at 10:23, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> 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));

Probably it's just too early to nit-pick too much, but I wonder if the
control might have a default value that we could use? It would be just
slightly less arbitrary!

Having said that, there's the whole question of where we will get
suitable ranges and default values for the lens position... at some
point we're going to have to figure that out.

Best regards
David

>> > +       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
>> >