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

[Daniel Semkowicz]

Hi Michael,

On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch
<michael.riesch at wolfvision.net> wrote:
>
> Hi Daniel,
>
> On 3/31/23 10:19, 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)
>
> One could imagine that in future other subdevices are supported. For
> example, I expect that soon we'll have a "flashControls" parameter.
> Would it make sense to gather all those controls in a single parameter
> "hwControls" (name to be bikeshedded)? (In #libcamera the notion of
> internal controls has been introduced recently. Maybe simply
> "internalControls"?)
>
> BTW I also could imagine that a "lensInfo" parameter is necessary for
> complex lens controllers (e.g., does the lens have a zoom function, how
> many zoom lenses does it have, model of the lens controller, ...). But
> for simple VCM controllers this may not be necessary, so feel free to
> ignore this at this point.

It would probably make sense, but I would like not to do everything at once,
but introduce the changes gradually. I would suggest to extended it with the
introduction of next optional subdevice.

>
> >               => (int32 ret, libcamera.ControlInfoMap ipaControls);
> >       start() => (int32 ret);
> >       stop();
>
> Shouldn't processStatsBuffer have an additional "lensControls"
> parameter? Of course, the comment above would apply here as well and we
> could get away with simply adjusting the name "sensorControls" ->
> "internalControls".

Right now, We do not report any current values of lens controls to the IPA,
so such parameter would be left unused.

Best regards
Daniel

>
> Best regards,
> Michael
>
> > 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;
> > +
> >       /* 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;