[libcamera-devel] [PATCH v3 3/3] ipu3: ipa: Allow IPA to apply controls to the lens device

[Hanlin Chen]

Hi Jean-Michel,

On Fri, Nov 12, 2021 at 12:12 AM Jean-Michel Hautbois
<jeanmichel.hautbois at ideasonboard.com> wrote:
>
> Hi Han-Lin,
>
> On 11/11/2021 11:49, Han-Lin Chen wrote:
> > Allow IPA to apply controls to the lens device.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.org>
> > ---
> >   meson.build                          |  6 ++++++
> >   src/libcamera/pipeline/ipu3/cio2.cpp | 30 ++++++++++++++++++++++++++++
> >   src/libcamera/pipeline/ipu3/cio2.h   |  3 +++
> >   src/libcamera/pipeline/ipu3/ipu3.cpp |  9 +++++++--
> >   4 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index 7892a9e3..2a4b68a2 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -108,6 +108,12 @@ if cc.has_argument('-Wno-c99-designator')
> >       ]
> >   endif
> >
> > +if get_option('android_platform') == 'cros'
> > +    common_arguments += [
> > +        '-DOS_CHROMEOS',
> > +    ]
> > +endif
>
> I am not really aware of specifics regarding this, why can't we have
> something not dependent on the OS ?
>
> > +
> >   c_arguments += common_arguments
> >   cpp_arguments += common_arguments
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index 59dda56b..233553c2 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -16,6 +16,7 @@
> >   #include <libcamera/geometry.h>
> >   #include <libcamera/stream.h>
> >
> > +#include "libcamera/internal/camera_lens.h"
> >   #include "libcamera/internal/camera_sensor.h"
> >   #include "libcamera/internal/framebuffer.h"
> >   #include "libcamera/internal/media_device.h"
> > @@ -159,6 +160,35 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
> >               return -EINVAL;
> >       }
> >
> > +#if defined(OS_CHROMEOS)
> > +     /*
> > +      * \todo Read the lens model from the sensor itself or from a device database.
> > +      * For now use default values taken from ChromeOS.
> > +      */
> > +     static std::unordered_map<std::string, std::string> sensorLens = {
> > +             { "ov13858", "dw9714" },
> > +             { "imx258", "dw9807" },
> > +             { "imx355", "ak7375" }
> > +     };
>
> So, we need a list giving the lens model associated to a sensor... How
> is this dependent on chromeos ? Imagine I want to use the sensor lens on
> my SGo2, which has a ov8865, what is needed to do that ?
You are right. Not only ChromeOS needs the mapping.
I made it depend on CrOS simply because it's the mapping I can
confirm, and I was worried to accidentally broken the other platforms.
Maybe I was overthinking this... Will remove the directive.
Besides, the mapping is considered as a temporary solution, which
should be removed once we have a way to read the relation from the
media-ctl or a database.
>
> > +
> > +     auto it = sensorLens.find(sensor_->model());
> > +     if (it != sensorLens.end()) {
> > +             const std::vector<MediaEntity *> &entities = media->entities();
> > +             for (auto ent : entities) {
> > +                     if (ent->function() == MEDIA_ENT_F_LENS) {
> > +                             lens_ = std::make_unique<CameraLens>(ent);
> > +                             ret = lens_->init();
> > +                             if (!ret && lens_->model() == it->second) {
> > +                                     break;
> > +                             }
> > +                             lens_.reset();
> > +                     }
> > +                     if (!lens_)
> > +                             LOG(IPU3, Warning) << "Lens device " << it->second << " not found";
> > +             }
> > +     }
> > +#endif
> > +
> >       /*
> >        * \todo Define when to open and close video device nodes, as they
> >        * might impact on power consumption.
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > index ba8f0052..635566c8 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > @@ -18,6 +18,7 @@
> >
> >   namespace libcamera {
> >
> > +class CameraLens;
> >   class CameraSensor;
> >   class FrameBuffer;
> >   class MediaDevice;
> > @@ -52,6 +53,7 @@ public:
> >       int stop();
> >
> >       CameraSensor *sensor() { return sensor_.get(); }
> > +     CameraLens *lens() { return lens_.get(); }
> >       const CameraSensor *sensor() const { return sensor_.get(); }
> >
> >       FrameBuffer *queueBuffer(Request *request, FrameBuffer *rawBuffer);
> > @@ -67,6 +69,7 @@ private:
> >       void cio2BufferReady(FrameBuffer *buffer);
> >
> >       std::unique_ptr<CameraSensor> sensor_;
> > +     std::unique_ptr<CameraLens> lens_;
> >       std::unique_ptr<V4L2Subdevice> csi2_;
> >       std::unique_ptr<V4L2VideoDevice> output_;
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 97003681..88775f67 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -24,6 +24,7 @@
> >   #include <libcamera/stream.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"
> > @@ -1255,8 +1256,12 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
> >   {
> >       switch (action.op) {
> >       case ipa::ipu3::ActionSetSensorControls: {
> > -             const ControlList &controls = action.sensorControls;
> > -             delayedCtrls_->push(controls);
> > +             const ControlList &sensorControls = action.sensorControls;
> > +             delayedCtrls_->push(sensorControls);
> > +             if (cio2_.lens()) {
> > +                     ControlList lensControls = action.lensControls;
> > +                     cio2_.lens()->setControls(&lensControls);
> > +             }
> >               break;
> >       }
> >       case ipa::ipu3::ActionParamFilled: {
> >