[libcamera-devel] [PATCH 4/4] ipu3: ipa: Allow IPA to apply controls to the lens device
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri Oct 29 11:14:32 CEST 2021
Quoting Hanlin Chen (2021-10-29 09:57:00)
> 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.
Ok, I hope that makes good progress.
> > > + /*
> > > + * \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*.
Ah, I see. setControls states:
"All controls below that index are written and their values are updated in \a ctrls,"
So setControls /modifies/ the incoming controls to update them in case
they were updated by the kernel.
We could make a const version of setControls but I'm weary that would
silently lose updates - but perhaps that's not unexpected if the
ControlList is const, and not expected to be updated
... Hrm.. one to think more about I guess.
> >
> >
> > > + }
> > > break;
> > > }
> > > case ipa::ipu3::ActionParamFilled: {
> > > --
> > > 2.33.1.1089.g2158813163f-goog
> > >
More information about the libcamera-devel
mailing list