| Message ID | 20211028100319.1097720-1-hanlinchen@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi I'm very pleased to see some other teams start to think about autofocus! One thing we need to do is decide what the libcamera API for autofocus looks like, otherwise there is of course no way to drive it! I tried to start this discussion a little while ago (check for emails from me to this mailing list on Thursday 21 October), so I'd be delighted to get some feedback on this subject and hear other people's ideas. Thanks! David On Thu, 28 Oct 2021 at 11:03, Han-Lin Chen <hanlinchen@chromium.org> wrote: > IPU3Event and IPU3Action use single ControlList for both libcamera and V4L2 > controls, and it's content could be either one based on the context. > Extend IPU3Event and IPU3Action for sensor and lens V4L2 controls, and > preserve > the original one for only libcamera Controls to make the content of an > event > more specific. > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com> > --- > include/libcamera/ipa/ipu3.mojom | 4 ++++ > src/ipa/ipu3/ipu3.cpp | 2 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > 3 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom > b/include/libcamera/ipa/ipu3.mojom > index 2f254ed4..cc0d822f 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -23,11 +23,15 @@ struct IPU3Event { > int64 frameTimestamp; > uint32 bufferId; > libcamera.ControlList controls; > + libcamera.ControlList sensorControls; > + libcamera.ControlList lensControls; > }; > > struct IPU3Action { > IPU3Operations op; > libcamera.ControlList controls; > + libcamera.ControlList sensorControls; > + libcamera.ControlList lensControls; > }; > > struct IPAConfigInfo { > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 5c51607d..6775570e 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -646,7 +646,7 @@ void IPAIPU3::setControls(unsigned int frame) > ControlList ctrls(ctrls_); > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > - op.controls = ctrls; > + op.sensorControls = ctrls; > > queueFrameAction.emit(frame, op); > } > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp > b/src/libcamera/pipeline/ipu3/ipu3.cpp > index eb714aa6..8816efc5 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1248,7 +1248,7 @@ void IPU3CameraData::queueFrameAction(unsigned int > id, > { > switch (action.op) { > case ipa::ipu3::ActionSetSensorControls: { > - const ControlList &controls = action.controls; > + const ControlList &controls = action.sensorControls; > delayedCtrls_->push(controls); > break; > } > -- > 2.33.1.1089.g2158813163f-goog > >
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@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. > + /* > + * \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); > + } > break; > } > case ipa::ipu3::ActionParamFilled: { > -- > 2.33.1.1089.g2158813163f-goog >
Quoting Han-Lin Chen (2021-10-28 11:03:16) > IPU3Event and IPU3Action use single ControlList for both libcamera and V4L2 > controls, and it's content could be either one based on the context. > Extend IPU3Event and IPU3Action for sensor and lens V4L2 controls, and preserve > the original one for only libcamera Controls to make the content of an event > more specific. I like this, it does make the controls clearer. I wonder if there's much impact on the IPC but when empty, I presume it will be minimal cost. > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com> > --- > include/libcamera/ipa/ipu3.mojom | 4 ++++ > src/ipa/ipu3/ipu3.cpp | 2 +- > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- > 3 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom > index 2f254ed4..cc0d822f 100644 > --- a/include/libcamera/ipa/ipu3.mojom > +++ b/include/libcamera/ipa/ipu3.mojom > @@ -23,11 +23,15 @@ struct IPU3Event { > int64 frameTimestamp; > uint32 bufferId; > libcamera.ControlList controls; > + libcamera.ControlList sensorControls; > + libcamera.ControlList lensControls; Do events need to send sensor controls and lens controls to the IPA? Or will these always be unused? Aha, I see at least the Sensor controls are going to get used in the next patch... Ok - so keeping them aligned with IPU3Action seems to make sense. > }; > > struct IPU3Action { > IPU3Operations op; > libcamera.ControlList controls; > + libcamera.ControlList sensorControls; > + libcamera.ControlList lensControls; > }; > > struct IPAConfigInfo { > diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp > index 5c51607d..6775570e 100644 > --- a/src/ipa/ipu3/ipu3.cpp > +++ b/src/ipa/ipu3/ipu3.cpp > @@ -646,7 +646,7 @@ void IPAIPU3::setControls(unsigned int frame) > ControlList ctrls(ctrls_); > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); > - op.controls = ctrls; > + op.sensorControls = ctrls; > > queueFrameAction.emit(frame, op); > } > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index eb714aa6..8816efc5 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1248,7 +1248,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > { > switch (action.op) { > case ipa::ipu3::ActionSetSensorControls: { > - const ControlList &controls = action.controls; > + const ControlList &controls = action.sensorControls; > delayedCtrls_->push(controls); > break; > } > -- > 2.33.1.1089.g2158813163f-goog >
Hi Kieran, Many thanks for the review. On Thu, Oct 28, 2021 at 9:26 PM Kieran Bingham <kieran.bingham@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@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 > >
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@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@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 > > >
diff --git a/include/libcamera/ipa/ipu3.mojom b/include/libcamera/ipa/ipu3.mojom index 2f254ed4..cc0d822f 100644 --- a/include/libcamera/ipa/ipu3.mojom +++ b/include/libcamera/ipa/ipu3.mojom @@ -23,11 +23,15 @@ struct IPU3Event { int64 frameTimestamp; uint32 bufferId; libcamera.ControlList controls; + libcamera.ControlList sensorControls; + libcamera.ControlList lensControls; }; struct IPU3Action { IPU3Operations op; libcamera.ControlList controls; + libcamera.ControlList sensorControls; + libcamera.ControlList lensControls; }; struct IPAConfigInfo { diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index 5c51607d..6775570e 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -646,7 +646,7 @@ void IPAIPU3::setControls(unsigned int frame) ControlList ctrls(ctrls_); ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure_)); ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain_)); - op.controls = ctrls; + op.sensorControls = ctrls; queueFrameAction.emit(frame, op); } diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index eb714aa6..8816efc5 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1248,7 +1248,7 @@ void IPU3CameraData::queueFrameAction(unsigned int id, { switch (action.op) { case ipa::ipu3::ActionSetSensorControls: { - const ControlList &controls = action.controls; + const ControlList &controls = action.sensorControls; delayedCtrls_->push(controls); break; }
IPU3Event and IPU3Action use single ControlList for both libcamera and V4L2 controls, and it's content could be either one based on the context. Extend IPU3Event and IPU3Action for sensor and lens V4L2 controls, and preserve the original one for only libcamera Controls to make the content of an event more specific. Signed-off-by: Han-Lin Chen <hanlinchen@chromium.com> --- include/libcamera/ipa/ipu3.mojom | 4 ++++ src/ipa/ipu3/ipu3.cpp | 2 +- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-)