[libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to PH and expose it to the IPA

[Daniel Semkowicz]

Hi Laurent,

On Tue, Apr 25, 2023 at 3:29 PM Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Daniel,
>
> Thank you for the patch.
>
> On Fri, Mar 31, 2023 at 10:19:21AM +0200, Daniel Semkowicz via libcamera-devel wrote:
> > Extend the IPA init() function by additional lensControls input
> > parameter. Check in pipeline handler if camera lens exists, and expose
> > its controls to the IPA using the new parameter.
> >
> > Signed-off-by: Daniel Semkowicz <dse at thaumatec.com>
> > ---
> >  include/libcamera/ipa/rkisp1.mojom       | 3 ++-
> >  src/ipa/rkisp1/rkisp1.cpp                | 7 +++++++
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++-
> >  3 files changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
> > index 1009e970..d4ff1230 100644
> > --- a/include/libcamera/ipa/rkisp1.mojom
> > +++ b/include/libcamera/ipa/rkisp1.mojom
> > @@ -17,7 +17,8 @@ interface IPARkISP1Interface {
> >       init(libcamera.IPASettings settings,
> >            uint32 hwRevision,
> >            libcamera.IPACameraSensorInfo sensorInfo,
> > -          libcamera.ControlInfoMap sensorControls)
> > +          libcamera.ControlInfoMap sensorControls,
> > +          libcamera.ControlInfoMap lensControls)
> >               => (int32 ret, libcamera.ControlInfoMap ipaControls);
> >       start() => (int32 ret);
> >       stop();
> > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
> > index 6544c925..d338d374 100644
> > --- a/src/ipa/rkisp1/rkisp1.cpp
> > +++ b/src/ipa/rkisp1/rkisp1.cpp
> > @@ -7,6 +7,7 @@
> >
> >  #include <algorithm>
> >  #include <math.h>
> > +#include <optional>
> >  #include <queue>
> >  #include <stdint.h>
> >  #include <string.h>
> > @@ -52,6 +53,7 @@ public:
> >       int init(const IPASettings &settings, unsigned int hwRevision,
> >                const IPACameraSensorInfo &sensorInfo,
> >                const ControlInfoMap &sensorControls,
> > +              const ControlInfoMap &lensControls,
> >                ControlInfoMap *ipaControls) override;
> >       int start() override;
> >       void stop() override;
> > @@ -80,6 +82,7 @@ private:
> >       std::map<unsigned int, MappedFrameBuffer> mappedBuffers_;
> >
> >       ControlInfoMap sensorControls_;
> > +     std::optional<ControlInfoMap> lensControls_;
> >
> >       /* revision-specific data */
> >       rkisp1_cif_isp_version hwRevision_;
> > @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const
> >  int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >                   const IPACameraSensorInfo &sensorInfo,
> >                   const ControlInfoMap &sensorControls,
> > +                 const ControlInfoMap &lensControls,
> >                   ControlInfoMap *ipaControls)
> >  {
> >       /* \todo Add support for other revisions */
> > @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision,
> >       context_.configuration.sensor.lineDuration = sensorInfo.minLineLength
> >                                                  * 1.0s / sensorInfo.pixelRate;
> >
> > +     if (!lensControls.empty())
> > +             lensControls_ = lensControls;
>
> Is there a reason to use std::optional<> for lensControls_, instead of a
> plain ControlInfoMap that you would unconditionally initialize here to
>
>         lensControls_ = lensControls;
>
> ? When checking if the lens controls exist in further patches in the
> series, you could test
>
>         if (!lensControls_.empty())
>
> instead of
>
>         if (lensControls_)
>
> I'm fine with std::optional<> if there's a reason for it, so, either
> way,

You are right that I could just test "lensControls_.empty()".
The only reason I use the std::optional here, is that I wanted to make it clear
in the IPA code, that lens existence is optional. Unfortunately, there is still
one check for "lensControls.empty()" in the IPA IPARkISP1::init() function.
The best would be to have std::optional already passed as the init() argument,
but I did not find an easy way to do this in mojom file... This way, We could
set the optional lens controls parameter only if sensor_->focusLens() is
available. Right now, I base a bit on the underlying behavior of CameraLens that
CameraLens::controls() always returns at least one element if lens is available.

Best regards
Daniel

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +
> >       /* Load the tuning data file. */
> >       File file(settings.configurationFile);
> >       if (!file.open(File::OpenModeFlag::ReadOnly)) {
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 8a30fe06..f966254a 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -32,6 +32,7 @@
> >  #include <libcamera/ipa/rkisp1_ipa_proxy.h>
> >
> >  #include "libcamera/internal/camera.h"
> > +#include "libcamera/internal/camera_lens.h"
> >  #include "libcamera/internal/camera_sensor.h"
> >  #include "libcamera/internal/delayed_controls.h"
> >  #include "libcamera/internal/device_enumerator.h"
> > @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
> >               return ret;
> >       }
> >
> > +     ControlInfoMap lensControls;
> > +     CameraLens *lens = sensor_->focusLens();
> > +     if (lens)
> > +             lensControls = lens->controls();
> > +
> >       ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision,
> > -                      sensorInfo, sensor_->controls(), &controlInfo_);
> > +                      sensorInfo, sensor_->controls(), lensControls,
> > +                      &controlInfo_);
> >       if (ret < 0) {
> >               LOG(RkISP1, Error) << "IPA initialization failure";
> >               return ret;
>
> --
> Regards,
>
> Laurent Pinchart