| Message ID | 20220809144704.61682-4-dse@thaumatec.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > Allow control of lens position from the IPA, by setting corresponding > af fields in the IPAFrameContext structure. Controls are then passed to > the pipeline handler, which sets the lens position in CameraLens. > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > --- > include/libcamera/ipa/rkisp1.mojom | 1 + > src/ipa/rkisp1/ipa_context.h | 5 +++++ > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > 4 files changed, 34 insertions(+) > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > index eaf3955e..caa1121a 100644 > --- a/include/libcamera/ipa/rkisp1.mojom > +++ b/include/libcamera/ipa/rkisp1.mojom > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > interface IPARkISP1EventInterface { > paramsBufferReady(uint32 frame); > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > + setLensControls(libcamera.ControlList sensorControls); > metadataReady(uint32 frame, libcamera.ControlList metadata); > }; > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 2bdb6a81..d6d46b57 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > }; > > struct IPAFrameContext { > + struct { > + uint32_t lensPosition; > + bool applyLensCtrls; > + } af; > + So, I feel like this patch is better delayed after we have completed the rework of the IPU3 and RkISP1 IPA modules and move the algorithms to retain their state internally instead of using the IPAFrameContext. There's a series almost landed that renames IPAFrameContext to IPAActiveState and there will be a few more to align the two IPA modules. Would you mind taking this in the series that introduces the AF algorithm while I try to collect 1 and 2 asap ? > struct { > uint32_t exposure; > double gain; > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 9e4c48a2..39e1ab2f 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > + /* Lens position is unknown at the startup, so initilize the variable > + * that holds the current position to something out of the range. */ > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > + > context_.frameContext.frameCount = 0; > > for (auto const &algo : algorithms()) { > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > setSensorControls.emit(frame, ctrls); > + > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > + context_.frameContext.af.applyLensCtrls = false; > + ControlList lensCtrls(*lensCtrls_); > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > + setLensControls.emit(lensCtrls); > + } > } > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 5f10c26b..de0d37da 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -108,6 +108,7 @@ private: > void paramFilled(unsigned int frame); > void setSensorControls(unsigned int frame, > const ControlList &sensorControls); > + void setLensControls(const ControlList &lensControls); > > void metadataReady(unsigned int frame, const ControlList &metadata); > }; > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > return -ENOENT; > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > delayedCtrls_->push(sensorControls); > } > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > +{ > + CameraLens *focusLens = sensor_->focusLens(); > + if (!focusLens) > + return; > + > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > + return; > + > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > + > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > +} > + > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > { > RkISP1FrameInfo *info = frameInfo_.find(frame); > -- > 2.34.1 >
Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > Hi Daniel > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > Allow control of lens position from the IPA, by setting corresponding > > af fields in the IPAFrameContext structure. Controls are then passed to > > the pipeline handler, which sets the lens position in CameraLens. > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > --- > > include/libcamera/ipa/rkisp1.mojom | 1 + > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > > 4 files changed, 34 insertions(+) > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > index eaf3955e..caa1121a 100644 > > --- a/include/libcamera/ipa/rkisp1.mojom > > +++ b/include/libcamera/ipa/rkisp1.mojom > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > > interface IPARkISP1EventInterface { > > paramsBufferReady(uint32 frame); > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > + setLensControls(libcamera.ControlList sensorControls); > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > }; > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > index 2bdb6a81..d6d46b57 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > > }; > > > > struct IPAFrameContext { > > + struct { > > + uint32_t lensPosition; > > + bool applyLensCtrls; > > + } af; > > + > > So, I feel like this patch is better delayed after we have completed > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms > to retain their state internally instead of using the IPAFrameContext. > > There's a series almost landed that renames IPAFrameContext to > IPAActiveState and there will be a few more to align the two IPA > modules. > Would you mind taking this in the series that introduces the AF > algorithm while I try to collect 1 and 2 asap ? It does seem like a lot of this will change indeed. So we really should try to clean up the interfaces to get it a bit more stable. > > > struct { > > uint32_t exposure; > > double gain; > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 9e4c48a2..39e1ab2f 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > > > + /* Lens position is unknown at the startup, so initilize the variable > > + * that holds the current position to something out of the range. */ > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > > + > > context_.frameContext.frameCount = 0; > > > > for (auto const &algo : algorithms()) { > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > setSensorControls.emit(frame, ctrls); > > + > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > + context_.frameContext.af.applyLensCtrls = false; > > + ControlList lensCtrls(*lensCtrls_); > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > + setLensControls.emit(lensCtrls); > > + } > > } > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 5f10c26b..de0d37da 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -108,6 +108,7 @@ private: > > void paramFilled(unsigned int frame); > > void setSensorControls(unsigned int frame, > > const ControlList &sensorControls); > > + void setLensControls(const ControlList &lensControls); > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > }; > > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > return -ENOENT; > > > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > delayedCtrls_->push(sensorControls); > > } > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > +{ > > + CameraLens *focusLens = sensor_->focusLens(); > > + if (!focusLens) > > + return; > > + > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > + return; I think the construction of sensor_->focusLens should make sure that the correct control is set, so that if we have a valid CameraLens *focusLens, we know it's valid to call focusLens->setFocusPosition(). So the above lensControls.contains() should be redundant. > > + > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > + > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); But I think Jacopo also has plans to change this so that a libcamera control is passed in anyway, meaning the conversion from libcamera focus position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the CameraLens class anyway. > > +} > > + > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > { > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > -- > > 2.34.1 > >
Hi Kieran On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > > Hi Daniel > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > Allow control of lens position from the IPA, by setting corresponding > > > af fields in the IPAFrameContext structure. Controls are then passed to > > > the pipeline handler, which sets the lens position in CameraLens. > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > --- > > > include/libcamera/ipa/rkisp1.mojom | 1 + > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > > > 4 files changed, 34 insertions(+) > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > > index eaf3955e..caa1121a 100644 > > > --- a/include/libcamera/ipa/rkisp1.mojom > > > +++ b/include/libcamera/ipa/rkisp1.mojom > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > > > interface IPARkISP1EventInterface { > > > paramsBufferReady(uint32 frame); > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > > + setLensControls(libcamera.ControlList sensorControls); > > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > > }; > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > index 2bdb6a81..d6d46b57 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > > > }; > > > > > > struct IPAFrameContext { > > > + struct { > > > + uint32_t lensPosition; > > > + bool applyLensCtrls; > > > + } af; > > > + > > > > So, I feel like this patch is better delayed after we have completed > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms > > to retain their state internally instead of using the IPAFrameContext. > > > > There's a series almost landed that renames IPAFrameContext to > > IPAActiveState and there will be a few more to align the two IPA > > modules. > > Would you mind taking this in the series that introduces the AF > > algorithm while I try to collect 1 and 2 asap ? > > It does seem like a lot of this will change indeed. So we really should > try to clean up the interfaces to get it a bit more stable. > > > > > > > struct { > > > uint32_t exposure; > > > double gain; > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > index 9e4c48a2..39e1ab2f 100644 > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > > > > > + /* Lens position is unknown at the startup, so initilize the variable > > > + * that holds the current position to something out of the range. */ > > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > > > + > > > context_.frameContext.frameCount = 0; > > > > > > for (auto const &algo : algorithms()) { > > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > > > setSensorControls.emit(frame, ctrls); > > > + > > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > > + context_.frameContext.af.applyLensCtrls = false; > > > + ControlList lensCtrls(*lensCtrls_); > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > > + setLensControls.emit(lensCtrls); > > > + } > > > } > > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 5f10c26b..de0d37da 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -108,6 +108,7 @@ private: > > > void paramFilled(unsigned int frame); > > > void setSensorControls(unsigned int frame, > > > const ControlList &sensorControls); > > > + void setLensControls(const ControlList &lensControls); > > > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > > }; > > > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > > return -ENOENT; > > > > > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > > > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > > > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > > > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > > > > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > > delayedCtrls_->push(sensorControls); > > > } > > > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > +{ > > > + CameraLens *focusLens = sensor_->focusLens(); > > > + if (!focusLens) > > > + return; > > > + > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > + return; > > I think the construction of sensor_->focusLens should make sure that the > correct control is set, so that if we have a valid CameraLens > *focusLens, we know it's valid to call focusLens->setFocusPosition(). > > So the above lensControls.contains() should be redundant. I've initially been fooled as well and was about to make the same comment, but here Daniel's checking if V4L2_CID_FOCUS_ABSOLUTE is part of the list of controls to apply to the lens, not if the control is exposed by the subdevice > > > > > + > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > + > > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > But I think Jacopo also has plans to change this so that a libcamera > control is passed in anyway, meaning the conversion from libcamera focus > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the > CameraLens class anyway. > That's the end goal. Focus lens position still has to be assigned a device-independent values range that we can re-scale in the device-specific control range though... > > > > +} > > > + > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > > { > > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > > -- > > > 2.34.1 > > >
Quoting Jacopo Mondi (2022-08-09 17:01:37) > Hi Kieran > > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote: > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > > > Hi Daniel > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > > +{ > > > > + CameraLens *focusLens = sensor_->focusLens(); > > > > + if (!focusLens) > > > > + return; > > > > + > > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > > + return; > > > > I think the construction of sensor_->focusLens should make sure that the > > correct control is set, so that if we have a valid CameraLens > > *focusLens, we know it's valid to call focusLens->setFocusPosition(). > > > > So the above lensControls.contains() should be redundant. > > I've initially been fooled as well and was about to make the same > comment, but here Daniel's checking if V4L2_CID_FOCUS_ABSOLUTE is part > of the list of controls to apply to the lens, not if the control is > exposed by the subdevice Aha - oops - yes indeed. I see that now. Ok ;-) -- Kieran
On Tue, Aug 9, 2022 at 6:01 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Kieran > > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote: > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > > > Hi Daniel > > > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > Allow control of lens position from the IPA, by setting corresponding > > > > af fields in the IPAFrameContext structure. Controls are then passed to > > > > the pipeline handler, which sets the lens position in CameraLens. > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > --- > > > > include/libcamera/ipa/rkisp1.mojom | 1 + > > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > > > > 4 files changed, 34 insertions(+) > > > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > > > index eaf3955e..caa1121a 100644 > > > > --- a/include/libcamera/ipa/rkisp1.mojom > > > > +++ b/include/libcamera/ipa/rkisp1.mojom > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > > > > interface IPARkISP1EventInterface { > > > > paramsBufferReady(uint32 frame); > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > > > + setLensControls(libcamera.ControlList sensorControls); > > > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > > > }; > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > index 2bdb6a81..d6d46b57 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > > > > }; > > > > > > > > struct IPAFrameContext { > > > > + struct { > > > > + uint32_t lensPosition; > > > > + bool applyLensCtrls; > > > > + } af; > > > > + > > > > > > So, I feel like this patch is better delayed after we have completed > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms > > > to retain their state internally instead of using the IPAFrameContext. > > > > > > There's a series almost landed that renames IPAFrameContext to > > > IPAActiveState and there will be a few more to align the two IPA > > > modules. > > > Would you mind taking this in the series that introduces the AF > > > algorithm while I try to collect 1 and 2 asap ? > > > > It does seem like a lot of this will change indeed. So we really should > > try to clean up the interfaces to get it a bit more stable. Sure, I will add this patch to the AF series. Are these the changes you are talking about? https://patchwork.libcamera.org/project/libcamera/list/?series=3376 Do you think I can already base the AF changes on this series or there are additional major changes expected? Best regards Daniel > > > > > > > > > > > struct { > > > > uint32_t exposure; > > > > double gain; > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > index 9e4c48a2..39e1ab2f 100644 > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > > > > > > > + /* Lens position is unknown at the startup, so initilize the variable > > > > + * that holds the current position to something out of the range. */ > > > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > > > > + > > > > context_.frameContext.frameCount = 0; > > > > > > > > for (auto const &algo : algorithms()) { > > > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > > > > > setSensorControls.emit(frame, ctrls); > > > > + > > > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > > > + context_.frameContext.af.applyLensCtrls = false; > > > > + ControlList lensCtrls(*lensCtrls_); > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > > > + setLensControls.emit(lensCtrls); > > > > + } > > > > } > > > > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > index 5f10c26b..de0d37da 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > @@ -108,6 +108,7 @@ private: > > > > void paramFilled(unsigned int frame); > > > > void setSensorControls(unsigned int frame, > > > > const ControlList &sensorControls); > > > > + void setLensControls(const ControlList &lensControls); > > > > > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > > > }; > > > > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > > > return -ENOENT; > > > > > > > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > > > > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > > > > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > > > > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > > > > > > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > > > delayedCtrls_->push(sensorControls); > > > > } > > > > > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > > +{ > > > > + CameraLens *focusLens = sensor_->focusLens(); > > > > + if (!focusLens) > > > > + return; > > > > + > > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > > + return; > > > > I think the construction of sensor_->focusLens should make sure that the > > correct control is set, so that if we have a valid CameraLens > > *focusLens, we know it's valid to call focusLens->setFocusPosition(). > > > > So the above lensControls.contains() should be redundant. > > I've initially been fooled as well and was about to make the same > comment, but here Daniel's checking if V4L2_CID_FOCUS_ABSOLUTE is part > of the list of controls to apply to the lens, not if the control is > exposed by the subdevice > > > > > > > > > + > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > > + > > > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > > > But I think Jacopo also has plans to change this so that a libcamera > > control is passed in anyway, meaning the conversion from libcamera focus > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the > > CameraLens class anyway. > > > > That's the end goal. Focus lens position still has to be assigned a > device-independent values range that we can re-scale in the device-specific > control range though... > > > > > > > +} > > > > + > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > > > { > > > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > > > -- > > > > 2.34.1 > > > >
Hi Daniel On Wed, Aug 10, 2022 at 09:12:05AM +0200, Daniel Semkowicz wrote: > On Tue, Aug 9, 2022 at 6:01 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Kieran > > > > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote: > > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > > > > Hi Daniel > > > > > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > Allow control of lens position from the IPA, by setting corresponding > > > > > af fields in the IPAFrameContext structure. Controls are then passed to > > > > > the pipeline handler, which sets the lens position in CameraLens. > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > --- > > > > > include/libcamera/ipa/rkisp1.mojom | 1 + > > > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > > > > > 4 files changed, 34 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > > > > index eaf3955e..caa1121a 100644 > > > > > --- a/include/libcamera/ipa/rkisp1.mojom > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom > > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > > > > > interface IPARkISP1EventInterface { > > > > > paramsBufferReady(uint32 frame); > > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > > > > + setLensControls(libcamera.ControlList sensorControls); > > > > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > > > > }; > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > > index 2bdb6a81..d6d46b57 100644 > > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > > > > > }; > > > > > > > > > > struct IPAFrameContext { > > > > > + struct { > > > > > + uint32_t lensPosition; > > > > > + bool applyLensCtrls; > > > > > + } af; > > > > > + > > > > > > > > So, I feel like this patch is better delayed after we have completed > > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms > > > > to retain their state internally instead of using the IPAFrameContext. > > > > > > > > There's a series almost landed that renames IPAFrameContext to > > > > IPAActiveState and there will be a few more to align the two IPA > > > > modules. > > > > Would you mind taking this in the series that introduces the AF > > > > algorithm while I try to collect 1 and 2 asap ? > > > > > > It does seem like a lot of this will change indeed. So we really should > > > try to clean up the interfaces to get it a bit more stable. > > Sure, I will add this patch to the AF series. > > Are these the changes you are talking about? > https://patchwork.libcamera.org/project/libcamera/list/?series=3376 Correct! > > Do you think I can already base the AF changes on this series or there > are additional major changes expected? > There will be an additional series to further unify the IPU3 and RkISP IPA interfaces towards the pipeline handler. As an example, you can see how different is the IPA::configure() prototype between the two. So yes, it will be another rebase, hopefully not too painful. > Best regards > Daniel > > > > > > > > > > > > > > > > struct { > > > > > uint32_t exposure; > > > > > double gain; > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > > index 9e4c48a2..39e1ab2f 100644 > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > > > > > > > > > + /* Lens position is unknown at the startup, so initilize the variable > > > > > + * that holds the current position to something out of the range. */ > > > > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > > > > > + > > > > > context_.frameContext.frameCount = 0; > > > > > > > > > > for (auto const &algo : algorithms()) { > > > > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > > > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > > > > > > > setSensorControls.emit(frame, ctrls); > > > > > + > > > > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > > > > + context_.frameContext.af.applyLensCtrls = false; > > > > > + ControlList lensCtrls(*lensCtrls_); > > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > > > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > > > > + setLensControls.emit(lensCtrls); > > > > > + } > > > > > } > > > > > > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > index 5f10c26b..de0d37da 100644 > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > @@ -108,6 +108,7 @@ private: > > > > > void paramFilled(unsigned int frame); > > > > > void setSensorControls(unsigned int frame, > > > > > const ControlList &sensorControls); > > > > > + void setLensControls(const ControlList &lensControls); > > > > > > > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > > > > }; > > > > > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > > > > return -ENOENT; > > > > > > > > > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > > > > > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > > > > > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > > > > > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > > > > > > > > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > > > > delayedCtrls_->push(sensorControls); > > > > > } > > > > > > > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > > > +{ > > > > > + CameraLens *focusLens = sensor_->focusLens(); > > > > > + if (!focusLens) > > > > > + return; > > > > > + > > > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > > > + return; > > > > > > I think the construction of sensor_->focusLens should make sure that the > > > correct control is set, so that if we have a valid CameraLens > > > *focusLens, we know it's valid to call focusLens->setFocusPosition(). > > > > > > So the above lensControls.contains() should be redundant. > > > > I've initially been fooled as well and was about to make the same > > comment, but here Daniel's checking if V4L2_CID_FOCUS_ABSOLUTE is part > > of the list of controls to apply to the lens, not if the control is > > exposed by the subdevice > > > > > > > > > > > > > + > > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > > > + > > > > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > > > > > But I think Jacopo also has plans to change this so that a libcamera > > > control is passed in anyway, meaning the conversion from libcamera focus > > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the > > > CameraLens class anyway. > > > > > > > That's the end goal. Focus lens position still has to be assigned a > > device-independent values range that we can re-scale in the device-specific > > control range though... > > > > > > > > > > +} > > > > > + > > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > > > > { > > > > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > > > > -- > > > > > 2.34.1 > > > > >
Hi Daniel, + Kieran and Laurent On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > > Hi Daniel > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > Allow control of lens position from the IPA, by setting corresponding > > > af fields in the IPAFrameContext structure. Controls are then passed to > > > the pipeline handler, which sets the lens position in CameraLens. > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > --- > > > include/libcamera/ipa/rkisp1.mojom | 1 + > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > > > 4 files changed, 34 insertions(+) > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > > index eaf3955e..caa1121a 100644 > > > --- a/include/libcamera/ipa/rkisp1.mojom > > > +++ b/include/libcamera/ipa/rkisp1.mojom > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > > > interface IPARkISP1EventInterface { > > > paramsBufferReady(uint32 frame); > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > > + setLensControls(libcamera.ControlList sensorControls); > > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > > }; > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > index 2bdb6a81..d6d46b57 100644 > > > --- a/src/ipa/rkisp1/ipa_context.h > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > > > }; > > > > > > struct IPAFrameContext { > > > + struct { > > > + uint32_t lensPosition; > > > + bool applyLensCtrls; > > > + } af; > > > + > > > > So, I feel like this patch is better delayed after we have completed > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms > > to retain their state internally instead of using the IPAFrameContext. > > > > There's a series almost landed that renames IPAFrameContext to > > IPAActiveState and there will be a few more to align the two IPA > > modules. > > Would you mind taking this in the series that introduces the AF > > algorithm while I try to collect 1 and 2 asap ? > > It does seem like a lot of this will change indeed. So we really should > try to clean up the interfaces to get it a bit more stable. > > > > > > > struct { > > > uint32_t exposure; > > > double gain; > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > index 9e4c48a2..39e1ab2f 100644 > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > > > > > + /* Lens position is unknown at the startup, so initilize the variable > > > + * that holds the current position to something out of the range. */ > > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > > > + > > > context_.frameContext.frameCount = 0; > > > > > > for (auto const &algo : algorithms()) { > > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > > > setSensorControls.emit(frame, ctrls); > > > + > > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > > + context_.frameContext.af.applyLensCtrls = false; > > > + ControlList lensCtrls(*lensCtrls_); > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > > + setLensControls.emit(lensCtrls); > > > + } IPU3 here does ControlList ctrls(sensorCtrls_); ctrls.set(V4L2_CID_EXPOSURE, exposure); ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain); ControlList lensCtrls(lensCtrls_); lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(context_.activeState.af.focus)); setSensorControls.emit(frame, ctrls, lensCtrls); While Daniel has implemented ControlList ctrls(sensorCtrls_); ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); setSensorControls.emit(frame, ctrls); if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { context_.frameContext.af.applyLensCtrls = false; ControlList lensCtrls(*lensCtrls_); lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, static_cast<int32_t>(context_.frameContext.af.lensPosition)); setLensControls.emit(lensCtrls); } Yesterday we briefly discussed about the possibility of sending both sensor and lens controls as part of a single control list, but what's the preference in the meantime ? One signal with 2 lists or 2 signals ? I would prefer a single signal, mostly because it's handling is synchronous and we can avoid one context switch ? > > > } > > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 5f10c26b..de0d37da 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -108,6 +108,7 @@ private: > > > void paramFilled(unsigned int frame); > > > void setSensorControls(unsigned int frame, > > > const ControlList &sensorControls); > > > + void setLensControls(const ControlList &lensControls); > > > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > > }; > > > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > > return -ENOENT; > > > > > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > > > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > > > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > > > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > > > > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > > delayedCtrls_->push(sensorControls); > > > } > > > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > +{ > > > + CameraLens *focusLens = sensor_->focusLens(); > > > + if (!focusLens) > > > + return; > > > + > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > + return; > > I think the construction of sensor_->focusLens should make sure that the > correct control is set, so that if we have a valid CameraLens > *focusLens, we know it's valid to call focusLens->setFocusPosition(). > > So the above lensControls.contains() should be redundant. > > > > > + > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > + > > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > But I think Jacopo also has plans to change this so that a libcamera > control is passed in anyway, meaning the conversion from libcamera focus > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the > CameraLens class anyway. > > > > > +} > > > + > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > > { > > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > > -- > > > 2.34.1 > > >
Hi! On Wed, Aug 10, 2022 at 1:34 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Daniel, > + Kieran and Laurent > > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote: > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > > > Hi Daniel > > > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > Allow control of lens position from the IPA, by setting corresponding > > > > af fields in the IPAFrameContext structure. Controls are then passed to > > > > the pipeline handler, which sets the lens position in CameraLens. > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > --- > > > > include/libcamera/ipa/rkisp1.mojom | 1 + > > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > > > > 4 files changed, 34 insertions(+) > > > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > > > index eaf3955e..caa1121a 100644 > > > > --- a/include/libcamera/ipa/rkisp1.mojom > > > > +++ b/include/libcamera/ipa/rkisp1.mojom > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > > > > interface IPARkISP1EventInterface { > > > > paramsBufferReady(uint32 frame); > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > > > + setLensControls(libcamera.ControlList sensorControls); > > > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > > > }; > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > index 2bdb6a81..d6d46b57 100644 > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > > > > }; > > > > > > > > struct IPAFrameContext { > > > > + struct { > > > > + uint32_t lensPosition; > > > > + bool applyLensCtrls; > > > > + } af; > > > > + > > > > > > So, I feel like this patch is better delayed after we have completed > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms > > > to retain their state internally instead of using the IPAFrameContext. > > > > > > There's a series almost landed that renames IPAFrameContext to > > > IPAActiveState and there will be a few more to align the two IPA > > > modules. > > > Would you mind taking this in the series that introduces the AF > > > algorithm while I try to collect 1 and 2 asap ? > > > > It does seem like a lot of this will change indeed. So we really should > > try to clean up the interfaces to get it a bit more stable. > > > > > > > > > > > struct { > > > > uint32_t exposure; > > > > double gain; > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > index 9e4c48a2..39e1ab2f 100644 > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > > > > > > > + /* Lens position is unknown at the startup, so initilize the variable > > > > + * that holds the current position to something out of the range. */ > > > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > > > > + > > > > context_.frameContext.frameCount = 0; > > > > > > > > for (auto const &algo : algorithms()) { > > > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > > > > > setSensorControls.emit(frame, ctrls); > > > > + > > > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > > > + context_.frameContext.af.applyLensCtrls = false; > > > > + ControlList lensCtrls(*lensCtrls_); > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > > > + setLensControls.emit(lensCtrls); > > > > + } > > IPU3 here does > > ControlList ctrls(sensorCtrls_); > ctrls.set(V4L2_CID_EXPOSURE, exposure); > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain); > > ControlList lensCtrls(lensCtrls_); > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > static_cast<int32_t>(context_.activeState.af.focus)); > > setSensorControls.emit(frame, ctrls, lensCtrls); > > While Daniel has implemented > > ControlList ctrls(sensorCtrls_); > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > setSensorControls.emit(frame, ctrls); > if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > context_.frameContext.af.applyLensCtrls = false; > ControlList lensCtrls(*lensCtrls_); > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > static_cast<int32_t>(context_.frameContext.af.lensPosition)); > setLensControls.emit(lensCtrls); > } > > Yesterday we briefly discussed about the possibility of sending > both sensor and lens controls as part of a single control list, but > what's the preference in the meantime ? One signal with 2 lists or 2 > signals ? > > I would prefer a single signal, mostly because it's handling is > synchronous and we can avoid one context switch ? Single control list sounds as the best solution in my opinion. Especially that lens have only one control item. If there are plans to use a single control list, then in the meantime, I would go with a single signal and two lists. It would be closer to the final idea. Best regards Daniel > > > > > } > > > > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > index 5f10c26b..de0d37da 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > @@ -108,6 +108,7 @@ private: > > > > void paramFilled(unsigned int frame); > > > > void setSensorControls(unsigned int frame, > > > > const ControlList &sensorControls); > > > > + void setLensControls(const ControlList &lensControls); > > > > > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > > > }; > > > > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > > > return -ENOENT; > > > > > > > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > > > > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > > > > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > > > > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > > > > > > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > > > delayedCtrls_->push(sensorControls); > > > > } > > > > > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > > +{ > > > > + CameraLens *focusLens = sensor_->focusLens(); > > > > + if (!focusLens) > > > > + return; > > > > + > > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > > + return; > > > > I think the construction of sensor_->focusLens should make sure that the > > correct control is set, so that if we have a valid CameraLens > > *focusLens, we know it's valid to call focusLens->setFocusPosition(). > > > > So the above lensControls.contains() should be redundant. > > > > > > > > + > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > > + > > > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > > > But I think Jacopo also has plans to change this so that a libcamera > > control is passed in anyway, meaning the conversion from libcamera focus > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the > > CameraLens class anyway. > > > > > > > > +} > > > > + > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > > > { > > > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > > > -- > > > > 2.34.1 > > > >
Hi Daniel On Thu, Aug 11, 2022 at 09:10:48AM +0200, Daniel Semkowicz wrote: > Hi! > > On Wed, Aug 10, 2022 at 1:34 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi Daniel, > > + Kieran and Laurent > > > > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote: > > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > > > > Hi Daniel > > > > > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > Allow control of lens position from the IPA, by setting corresponding > > > > > af fields in the IPAFrameContext structure. Controls are then passed to > > > > > the pipeline handler, which sets the lens position in CameraLens. > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > --- > > > > > include/libcamera/ipa/rkisp1.mojom | 1 + > > > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > > > > > 4 files changed, 34 insertions(+) > > > > > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > > > > index eaf3955e..caa1121a 100644 > > > > > --- a/include/libcamera/ipa/rkisp1.mojom > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom > > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > > > > > interface IPARkISP1EventInterface { > > > > > paramsBufferReady(uint32 frame); > > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > > > > + setLensControls(libcamera.ControlList sensorControls); > > > > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > > > > }; > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > > index 2bdb6a81..d6d46b57 100644 > > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > > > > > }; > > > > > > > > > > struct IPAFrameContext { > > > > > + struct { > > > > > + uint32_t lensPosition; > > > > > + bool applyLensCtrls; > > > > > + } af; > > > > > + > > > > > > > > So, I feel like this patch is better delayed after we have completed > > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms > > > > to retain their state internally instead of using the IPAFrameContext. > > > > > > > > There's a series almost landed that renames IPAFrameContext to > > > > IPAActiveState and there will be a few more to align the two IPA > > > > modules. > > > > Would you mind taking this in the series that introduces the AF > > > > algorithm while I try to collect 1 and 2 asap ? > > > > > > It does seem like a lot of this will change indeed. So we really should > > > try to clean up the interfaces to get it a bit more stable. > > > > > > > > > > > > > > > struct { > > > > > uint32_t exposure; > > > > > double gain; > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > > index 9e4c48a2..39e1ab2f 100644 > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > > > > > > > > > + /* Lens position is unknown at the startup, so initilize the variable > > > > > + * that holds the current position to something out of the range. */ > > > > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > > > > > + > > > > > context_.frameContext.frameCount = 0; > > > > > > > > > > for (auto const &algo : algorithms()) { > > > > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > > > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > > > > > > > setSensorControls.emit(frame, ctrls); > > > > > + > > > > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > > > > + context_.frameContext.af.applyLensCtrls = false; > > > > > + ControlList lensCtrls(*lensCtrls_); > > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > > > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > > > > + setLensControls.emit(lensCtrls); > > > > > + } > > > > IPU3 here does > > > > ControlList ctrls(sensorCtrls_); > > ctrls.set(V4L2_CID_EXPOSURE, exposure); > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain); > > > > ControlList lensCtrls(lensCtrls_); > > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > static_cast<int32_t>(context_.activeState.af.focus)); > > > > setSensorControls.emit(frame, ctrls, lensCtrls); > > > > While Daniel has implemented > > > > ControlList ctrls(sensorCtrls_); > > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > setSensorControls.emit(frame, ctrls); > > if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > context_.frameContext.af.applyLensCtrls = false; > > ControlList lensCtrls(*lensCtrls_); > > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > setLensControls.emit(lensCtrls); > > } > > > > Yesterday we briefly discussed about the possibility of sending > > both sensor and lens controls as part of a single control list, but > > what's the preference in the meantime ? One signal with 2 lists or 2 > > signals ? > > > > I would prefer a single signal, mostly because it's handling is > > synchronous and we can avoid one context switch ? > > Single control list sounds as the best solution in my opinion. > Especially that lens have only one control item. If there are plans to > use a single control list, then in the meantime, I would go with a Yes, that's the idea. When we'll be done aligning the two IPA implementations we're going to move the IPA to use libcamera::controls and have the libcamera::control->v4l2::control translation in the CameraSensor/CameraLens class. I have sent a few weeks ago a first version [1], which need to be rebased on top of [2]. So yeah, things are moving a little too much in this area, that's why I suggested you to post-pone the AF algorithm series if that's not a problem with you. [1] https://patchwork.libcamera.org/project/libcamera/list/?series=3238 [2] https://patchwork.libcamera.org/project/libcamera/list/?series=3376 > single signal and two lists. It would be closer to the final idea. > > Best regards > Daniel > > > > > > > > } > > > > > > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > index 5f10c26b..de0d37da 100644 > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > @@ -108,6 +108,7 @@ private: > > > > > void paramFilled(unsigned int frame); > > > > > void setSensorControls(unsigned int frame, > > > > > const ControlList &sensorControls); > > > > > + void setLensControls(const ControlList &lensControls); > > > > > > > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > > > > }; > > > > > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > > > > return -ENOENT; > > > > > > > > > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > > > > > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > > > > > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > > > > > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > > > > > > > > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > > > > delayedCtrls_->push(sensorControls); > > > > > } > > > > > > > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > > > +{ > > > > > + CameraLens *focusLens = sensor_->focusLens(); > > > > > + if (!focusLens) > > > > > + return; > > > > > + > > > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > > > + return; > > > > > > I think the construction of sensor_->focusLens should make sure that the > > > correct control is set, so that if we have a valid CameraLens > > > *focusLens, we know it's valid to call focusLens->setFocusPosition(). > > > > > > So the above lensControls.contains() should be redundant. > > > > > > > > > > > + > > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > > > + > > > > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > > > > > But I think Jacopo also has plans to change this so that a libcamera > > > control is passed in anyway, meaning the conversion from libcamera focus > > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the > > > CameraLens class anyway. > > > > > > > > > > > +} > > > > > + > > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > > > > { > > > > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > > > > -- > > > > > 2.34.1 > > > > >
On Thu, Aug 11, 2022 at 10:22 AM Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi Daniel > > On Thu, Aug 11, 2022 at 09:10:48AM +0200, Daniel Semkowicz wrote: > > Hi! > > > > On Wed, Aug 10, 2022 at 1:34 PM Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi Daniel, > > > + Kieran and Laurent > > > > > > On Tue, Aug 09, 2022 at 04:50:53PM +0100, Kieran Bingham wrote: > > > > Quoting Jacopo Mondi via libcamera-devel (2022-08-09 16:25:13) > > > > > Hi Daniel > > > > > > > > > > On Tue, Aug 09, 2022 at 04:47:04PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > > > Allow control of lens position from the IPA, by setting corresponding > > > > > > af fields in the IPAFrameContext structure. Controls are then passed to > > > > > > the pipeline handler, which sets the lens position in CameraLens. > > > > > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > > > --- > > > > > > include/libcamera/ipa/rkisp1.mojom | 1 + > > > > > > src/ipa/rkisp1/ipa_context.h | 5 +++++ > > > > > > src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ > > > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ > > > > > > 4 files changed, 34 insertions(+) > > > > > > > > > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > > > > > index eaf3955e..caa1121a 100644 > > > > > > --- a/include/libcamera/ipa/rkisp1.mojom > > > > > > +++ b/include/libcamera/ipa/rkisp1.mojom > > > > > > @@ -32,5 +32,6 @@ interface IPARkISP1Interface { > > > > > > interface IPARkISP1EventInterface { > > > > > > paramsBufferReady(uint32 frame); > > > > > > setSensorControls(uint32 frame, libcamera.ControlList sensorControls); > > > > > > + setLensControls(libcamera.ControlList sensorControls); > > > > > > metadataReady(uint32 frame, libcamera.ControlList metadata); > > > > > > }; > > > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > > > > > index 2bdb6a81..d6d46b57 100644 > > > > > > --- a/src/ipa/rkisp1/ipa_context.h > > > > > > +++ b/src/ipa/rkisp1/ipa_context.h > > > > > > @@ -42,6 +42,11 @@ struct IPASessionConfiguration { > > > > > > }; > > > > > > > > > > > > struct IPAFrameContext { > > > > > > + struct { > > > > > > + uint32_t lensPosition; > > > > > > + bool applyLensCtrls; > > > > > > + } af; > > > > > > + > > > > > > > > > > So, I feel like this patch is better delayed after we have completed > > > > > the rework of the IPU3 and RkISP1 IPA modules and move the algorithms > > > > > to retain their state internally instead of using the IPAFrameContext. > > > > > > > > > > There's a series almost landed that renames IPAFrameContext to > > > > > IPAActiveState and there will be a few more to align the two IPA > > > > > modules. > > > > > Would you mind taking this in the series that introduces the AF > > > > > algorithm while I try to collect 1 and 2 asap ? > > > > > > > > It does seem like a lot of this will change indeed. So we really should > > > > try to clean up the interfaces to get it a bit more stable. > > > > > > > > > > > > > > > > > > > struct { > > > > > > uint32_t exposure; > > > > > > double gain; > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > > > index 9e4c48a2..39e1ab2f 100644 > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > > > @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, > > > > > > context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); > > > > > > context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); > > > > > > > > > > > > + /* Lens position is unknown at the startup, so initilize the variable > > > > > > + * that holds the current position to something out of the range. */ > > > > > > + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); > > > > > > + > > > > > > context_.frameContext.frameCount = 0; > > > > > > > > > > > > for (auto const &algo : algorithms()) { > > > > > > @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) > > > > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > > > > > > > > > setSensorControls.emit(frame, ctrls); > > > > > > + > > > > > > + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > > > > > + context_.frameContext.af.applyLensCtrls = false; > > > > > > + ControlList lensCtrls(*lensCtrls_); > > > > > > + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > > > > + static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > > > > > + setLensControls.emit(lensCtrls); > > > > > > + } > > > > > > IPU3 here does > > > > > > ControlList ctrls(sensorCtrls_); > > > ctrls.set(V4L2_CID_EXPOSURE, exposure); > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain); > > > > > > ControlList lensCtrls(lensCtrls_); > > > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > static_cast<int32_t>(context_.activeState.af.focus)); > > > > > > setSensorControls.emit(frame, ctrls, lensCtrls); > > > > > > While Daniel has implemented > > > > > > ControlList ctrls(sensorCtrls_); > > > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > > > ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); > > > > > > setSensorControls.emit(frame, ctrls); > > > if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { > > > context_.frameContext.af.applyLensCtrls = false; > > > ControlList lensCtrls(*lensCtrls_); > > > lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, > > > static_cast<int32_t>(context_.frameContext.af.lensPosition)); > > > setLensControls.emit(lensCtrls); > > > } > > > > > > Yesterday we briefly discussed about the possibility of sending > > > both sensor and lens controls as part of a single control list, but > > > what's the preference in the meantime ? One signal with 2 lists or 2 > > > signals ? > > > > > > I would prefer a single signal, mostly because it's handling is > > > synchronous and we can avoid one context switch ? > > > > Single control list sounds as the best solution in my opinion. > > Especially that lens have only one control item. If there are plans to > > use a single control list, then in the meantime, I would go with a > > Yes, that's the idea. When we'll be done aligning the two IPA > implementations we're going to move the IPA to use libcamera::controls > and have the libcamera::control->v4l2::control translation in the > CameraSensor/CameraLens class. > > I have sent a few weeks ago a first version [1], which need to be rebased > on top of [2]. So yeah, things are moving a little too much in this > area, that's why I suggested you to post-pone the AF algorithm series > if that's not a problem with you. > > [1] https://patchwork.libcamera.org/project/libcamera/list/?series=3238 > [2] https://patchwork.libcamera.org/project/libcamera/list/?series=3376 Hmm, it makes sense to postpone it until the IPA will be stable as these are conflicting changes. I already made some changes to the AF after the discussions, so I will submit my current version, but I will wait for the above series to be merged and then rebase everything. Best regards Daniel > > > single signal and two lists. It would be closer to the final idea. > > > > Best regards > > Daniel > > > > > > > > > > > } > > > > > > > > > > > > void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > > index 5f10c26b..de0d37da 100644 > > > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > > > @@ -108,6 +108,7 @@ private: > > > > > > void paramFilled(unsigned int frame); > > > > > > void setSensorControls(unsigned int frame, > > > > > > const ControlList &sensorControls); > > > > > > + void setLensControls(const ControlList &lensControls); > > > > > > > > > > > > void metadataReady(unsigned int frame, const ControlList &metadata); > > > > > > }; > > > > > > @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > > > > > return -ENOENT; > > > > > > > > > > > > ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); > > > > > > + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); > > > > > > ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); > > > > > > ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); > > > > > > > > > > > > @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, > > > > > > delayedCtrls_->push(sensorControls); > > > > > > } > > > > > > > > > > > > +void RkISP1CameraData::setLensControls(const ControlList &lensControls) > > > > > > +{ > > > > > > + CameraLens *focusLens = sensor_->focusLens(); > > > > > > + if (!focusLens) > > > > > > + return; > > > > > > + > > > > > > + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) > > > > > > + return; > > > > > > > > I think the construction of sensor_->focusLens should make sure that the > > > > correct control is set, so that if we have a valid CameraLens > > > > *focusLens, we know it's valid to call focusLens->setFocusPosition(). > > > > > > > > So the above lensControls.contains() should be redundant. > > > > > > > > > > > > > > + > > > > > > + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); > > > > > > + > > > > > > + focusLens->setFocusPosition(focusValue.get<int32_t>()); > > > > > > > > But I think Jacopo also has plans to change this so that a libcamera > > > > control is passed in anyway, meaning the conversion from libcamera focus > > > > position to V4L2_CID_FOCUS_ABSOLUTE should be handled inside the > > > > CameraLens class anyway. > > > > > > > > > > > > > > +} > > > > > > + > > > > > > void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) > > > > > > { > > > > > > RkISP1FrameInfo *info = frameInfo_.find(frame); > > > > > > -- > > > > > > 2.34.1 > > > > > >
diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom index eaf3955e..caa1121a 100644 --- a/include/libcamera/ipa/rkisp1.mojom +++ b/include/libcamera/ipa/rkisp1.mojom @@ -32,5 +32,6 @@ interface IPARkISP1Interface { interface IPARkISP1EventInterface { paramsBufferReady(uint32 frame); setSensorControls(uint32 frame, libcamera.ControlList sensorControls); + setLensControls(libcamera.ControlList sensorControls); metadataReady(uint32 frame, libcamera.ControlList metadata); }; diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index 2bdb6a81..d6d46b57 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -42,6 +42,11 @@ struct IPASessionConfiguration { }; struct IPAFrameContext { + struct { + uint32_t lensPosition; + bool applyLensCtrls; + } af; + struct { uint32_t exposure; double gain; diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 9e4c48a2..39e1ab2f 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -253,6 +253,10 @@ int IPARkISP1::configure([[maybe_unused]] const IPACameraSensorInfo &info, context_.configuration.agc.minAnalogueGain = camHelper_->gain(minGain); context_.configuration.agc.maxAnalogueGain = camHelper_->gain(maxGain); + /* Lens position is unknown at the startup, so initilize the variable + * that holds the current position to something out of the range. */ + context_.frameContext.af.lensPosition = std::numeric_limits<int32_t>::max(); + context_.frameContext.frameCount = 0; for (auto const &algo : algorithms()) { @@ -348,6 +352,14 @@ void IPARkISP1::setControls(unsigned int frame) ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast<int32_t>(gain)); setSensorControls.emit(frame, ctrls); + + if (lensCtrls_ && context_.frameContext.af.applyLensCtrls) { + context_.frameContext.af.applyLensCtrls = false; + ControlList lensCtrls(*lensCtrls_); + lensCtrls.set(V4L2_CID_FOCUS_ABSOLUTE, + static_cast<int32_t>(context_.frameContext.af.lensPosition)); + setLensControls.emit(lensCtrls); + } } void IPARkISP1::prepareMetadata(unsigned int frame, unsigned int aeState) diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 5f10c26b..de0d37da 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -108,6 +108,7 @@ private: void paramFilled(unsigned int frame); void setSensorControls(unsigned int frame, const ControlList &sensorControls); + void setLensControls(const ControlList &lensControls); void metadataReady(unsigned int frame, const ControlList &metadata); }; @@ -324,6 +325,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) return -ENOENT; ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls); + ipa_->setLensControls.connect(this, &RkISP1CameraData::setLensControls); ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled); ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady); @@ -378,6 +380,20 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame, delayedCtrls_->push(sensorControls); } +void RkISP1CameraData::setLensControls(const ControlList &lensControls) +{ + CameraLens *focusLens = sensor_->focusLens(); + if (!focusLens) + return; + + if (!lensControls.contains(V4L2_CID_FOCUS_ABSOLUTE)) + return; + + const ControlValue &focusValue = lensControls.get(V4L2_CID_FOCUS_ABSOLUTE); + + focusLens->setFocusPosition(focusValue.get<int32_t>()); +} + void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata) { RkISP1FrameInfo *info = frameInfo_.find(frame);
Allow control of lens position from the IPA, by setting corresponding af fields in the IPAFrameContext structure. Controls are then passed to the pipeline handler, which sets the lens position in CameraLens. Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> --- include/libcamera/ipa/rkisp1.mojom | 1 + src/ipa/rkisp1/ipa_context.h | 5 +++++ src/ipa/rkisp1/rkisp1.cpp | 12 ++++++++++++ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 16 ++++++++++++++++ 4 files changed, 34 insertions(+)