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

[Hanlin Chen]

Hi Kieran,
Many thanks for the review.

On Thu, Oct 28, 2021 at 9:26 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Han-Lin Chen (2021-10-28 11:03:19)
> > Allow IPA to apply controls to the lens device.
> >
> > Signed-off-by: Han-Lin Chen <hanlinchen at chromium.com>
> > ---
> >  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
> > +
> >  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..143e2a95 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)
>
> I'd feel very awkward letting such OS specifics into the pipeline
> handler.
>
> This really needs to come from the media-controller graph, but I fear
> that the graph might not be currently telling us the association with
> the sensor. So we must fix that as a priority.
>
> If this was (very) temporary while the media-controller was being fixed,
> I might overlook this - but not unless I knew it was being actively
> fixed - or this would just end up sticking.

Yes, we do have a discussion on how to fix media-controller on our
weekly meeting.
Tomasz may have more insight on this.

>
> > +       /*
> > +        * \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" }
> > +       };
> > +
> > +       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 6a7f5b9a..36e93fb0 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"
> > @@ -1250,8 +1251,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 = const_cast<ControlList&>(action.lensControls);
>
> Why does this need a const_cast? Can it not be used in the same way as
> the action.sensorControls?
>
>                         const ControlList &lensControls = action.lensControls;
>
> > +                       cio2_.lens()->setControls(&lensControls);
>
> Or can it just be set directly here?
>                         cio2_.lens()->setControls(&action.lenscontrols);
Compiler would reject the implicit conversion from * to const*.
DelayedControl implicitly copies it. We could do it too:

ControlList lensControls = action.lensControls;
cio2_.lens()->setControls(&lensControls);

 Or change V4L2Device::setControls() to accept const ControlList*.
>
>
> > +               }
> >                 break;
> >         }
> >         case ipa::ipu3::ActionParamFilled: {
> > --
> > 2.33.1.1089.g2158813163f-goog
> >