| Message ID | 20230331081930.19289-2-dse@thaumatec.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, On Fri, Mar 31, 2023 at 10:19:21AM +0200, Daniel Semkowicz via libcamera-devel wrote: > Extend the IPA init() function by additional lensControls input > parameter. Check in pipeline handler if camera lens exists, and expose > its controls to the IPA using the new parameter. > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > --- > include/libcamera/ipa/rkisp1.mojom | 3 ++- > src/ipa/rkisp1/rkisp1.cpp | 7 +++++++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++- > 3 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > index 1009e970..d4ff1230 100644 > --- a/include/libcamera/ipa/rkisp1.mojom > +++ b/include/libcamera/ipa/rkisp1.mojom > @@ -17,7 +17,8 @@ interface IPARkISP1Interface { > init(libcamera.IPASettings settings, > uint32 hwRevision, > libcamera.IPACameraSensorInfo sensorInfo, > - libcamera.ControlInfoMap sensorControls) > + libcamera.ControlInfoMap sensorControls, > + libcamera.ControlInfoMap lensControls) > => (int32 ret, libcamera.ControlInfoMap ipaControls); > start() => (int32 ret); > stop(); > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 6544c925..d338d374 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -7,6 +7,7 @@ > > #include <algorithm> > #include <math.h> > +#include <optional> > #include <queue> > #include <stdint.h> > #include <string.h> > @@ -52,6 +53,7 @@ public: > int init(const IPASettings &settings, unsigned int hwRevision, > const IPACameraSensorInfo &sensorInfo, > const ControlInfoMap &sensorControls, > + const ControlInfoMap &lensControls, > ControlInfoMap *ipaControls) override; > int start() override; > void stop() override; > @@ -80,6 +82,7 @@ private: > std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > > ControlInfoMap sensorControls_; > + std::optional<ControlInfoMap> lensControls_; > > /* revision-specific data */ > rkisp1_cif_isp_version hwRevision_; > @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > const IPACameraSensorInfo &sensorInfo, > const ControlInfoMap &sensorControls, > + const ControlInfoMap &lensControls, > ControlInfoMap *ipaControls) > { > /* \todo Add support for other revisions */ > @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength > * 1.0s / sensorInfo.pixelRate; > > + if (!lensControls.empty()) > + lensControls_ = lensControls; > + > /* Load the tuning data file. */ > File file(settings.configurationFile); > if (!file.open(File::OpenModeFlag::ReadOnly)) { > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 8a30fe06..f966254a 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -32,6 +32,7 @@ > #include <libcamera/ipa/rkisp1_ipa_proxy.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" > @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > return ret; > } > > + ControlInfoMap lensControls; > + CameraLens *lens = sensor_->focusLens(); > + if (lens) > + lensControls = lens->controls(); > + > ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision, > - sensorInfo, sensor_->controls(), &controlInfo_); > + sensorInfo, sensor_->controls(), lensControls, > + &controlInfo_); > if (ret < 0) { > LOG(RkISP1, Error) << "IPA initialization failure"; > return ret; > -- > 2.39.2 >
Hi Laurent, On Tue, Apr 25, 2023 at 3:29 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > Thank you for the patch. > > On Fri, Mar 31, 2023 at 10:19:21AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > Extend the IPA init() function by additional lensControls input > > parameter. Check in pipeline handler if camera lens exists, and expose > > its controls to the IPA using the new parameter. > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > --- > > include/libcamera/ipa/rkisp1.mojom | 3 ++- > > src/ipa/rkisp1/rkisp1.cpp | 7 +++++++ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++- > > 3 files changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > index 1009e970..d4ff1230 100644 > > --- a/include/libcamera/ipa/rkisp1.mojom > > +++ b/include/libcamera/ipa/rkisp1.mojom > > @@ -17,7 +17,8 @@ interface IPARkISP1Interface { > > init(libcamera.IPASettings settings, > > uint32 hwRevision, > > libcamera.IPACameraSensorInfo sensorInfo, > > - libcamera.ControlInfoMap sensorControls) > > + libcamera.ControlInfoMap sensorControls, > > + libcamera.ControlInfoMap lensControls) > > => (int32 ret, libcamera.ControlInfoMap ipaControls); > > start() => (int32 ret); > > stop(); > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 6544c925..d338d374 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -7,6 +7,7 @@ > > > > #include <algorithm> > > #include <math.h> > > +#include <optional> > > #include <queue> > > #include <stdint.h> > > #include <string.h> > > @@ -52,6 +53,7 @@ public: > > int init(const IPASettings &settings, unsigned int hwRevision, > > const IPACameraSensorInfo &sensorInfo, > > const ControlInfoMap &sensorControls, > > + const ControlInfoMap &lensControls, > > ControlInfoMap *ipaControls) override; > > int start() override; > > void stop() override; > > @@ -80,6 +82,7 @@ private: > > std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > > > > ControlInfoMap sensorControls_; > > + std::optional<ControlInfoMap> lensControls_; > > > > /* revision-specific data */ > > rkisp1_cif_isp_version hwRevision_; > > @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const > > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > const IPACameraSensorInfo &sensorInfo, > > const ControlInfoMap &sensorControls, > > + const ControlInfoMap &lensControls, > > ControlInfoMap *ipaControls) > > { > > /* \todo Add support for other revisions */ > > @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength > > * 1.0s / sensorInfo.pixelRate; > > > > + if (!lensControls.empty()) > > + lensControls_ = lensControls; > > Is there a reason to use std::optional<> for lensControls_, instead of a > plain ControlInfoMap that you would unconditionally initialize here to > > lensControls_ = lensControls; > > ? When checking if the lens controls exist in further patches in the > series, you could test > > if (!lensControls_.empty()) > > instead of > > if (lensControls_) > > I'm fine with std::optional<> if there's a reason for it, so, either > way, You are right that I could just test "lensControls_.empty()". The only reason I use the std::optional here, is that I wanted to make it clear in the IPA code, that lens existence is optional. Unfortunately, there is still one check for "lensControls.empty()" in the IPA IPARkISP1::init() function. The best would be to have std::optional already passed as the init() argument, but I did not find an easy way to do this in mojom file... This way, We could set the optional lens controls parameter only if sensor_->focusLens() is available. Right now, I base a bit on the underlying behavior of CameraLens that CameraLens::controls() always returns at least one element if lens is available. Best regards Daniel > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > /* Load the tuning data file. */ > > File file(settings.configurationFile); > > if (!file.open(File::OpenModeFlag::ReadOnly)) { > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 8a30fe06..f966254a 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -32,6 +32,7 @@ > > #include <libcamera/ipa/rkisp1_ipa_proxy.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" > > @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > return ret; > > } > > > > + ControlInfoMap lensControls; > > + CameraLens *lens = sensor_->focusLens(); > > + if (lens) > > + lensControls = lens->controls(); > > + > > ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision, > > - sensorInfo, sensor_->controls(), &controlInfo_); > > + sensorInfo, sensor_->controls(), lensControls, > > + &controlInfo_); > > if (ret < 0) { > > LOG(RkISP1, Error) << "IPA initialization failure"; > > return ret; > > -- > Regards, > > Laurent Pinchart
Hi Michael, On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch <michael.riesch@wolfvision.net> wrote: > > Hi Daniel, > > On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote: > > Extend the IPA init() function by additional lensControls input > > parameter. Check in pipeline handler if camera lens exists, and expose > > its controls to the IPA using the new parameter. > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > --- > > include/libcamera/ipa/rkisp1.mojom | 3 ++- > > src/ipa/rkisp1/rkisp1.cpp | 7 +++++++ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++- > > 3 files changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > index 1009e970..d4ff1230 100644 > > --- a/include/libcamera/ipa/rkisp1.mojom > > +++ b/include/libcamera/ipa/rkisp1.mojom > > @@ -17,7 +17,8 @@ interface IPARkISP1Interface { > > init(libcamera.IPASettings settings, > > uint32 hwRevision, > > libcamera.IPACameraSensorInfo sensorInfo, > > - libcamera.ControlInfoMap sensorControls) > > + libcamera.ControlInfoMap sensorControls, > > + libcamera.ControlInfoMap lensControls) > > One could imagine that in future other subdevices are supported. For > example, I expect that soon we'll have a "flashControls" parameter. > Would it make sense to gather all those controls in a single parameter > "hwControls" (name to be bikeshedded)? (In #libcamera the notion of > internal controls has been introduced recently. Maybe simply > "internalControls"?) > > BTW I also could imagine that a "lensInfo" parameter is necessary for > complex lens controllers (e.g., does the lens have a zoom function, how > many zoom lenses does it have, model of the lens controller, ...). But > for simple VCM controllers this may not be necessary, so feel free to > ignore this at this point. It would probably make sense, but I would like not to do everything at once, but introduce the changes gradually. I would suggest to extended it with the introduction of next optional subdevice. > > > => (int32 ret, libcamera.ControlInfoMap ipaControls); > > start() => (int32 ret); > > stop(); > > Shouldn't processStatsBuffer have an additional "lensControls" > parameter? Of course, the comment above would apply here as well and we > could get away with simply adjusting the name "sensorControls" -> > "internalControls". Right now, We do not report any current values of lens controls to the IPA, so such parameter would be left unused. Best regards Daniel > > Best regards, > Michael > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 6544c925..d338d374 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -7,6 +7,7 @@ > > > > #include <algorithm> > > #include <math.h> > > +#include <optional> > > #include <queue> > > #include <stdint.h> > > #include <string.h> > > @@ -52,6 +53,7 @@ public: > > int init(const IPASettings &settings, unsigned int hwRevision, > > const IPACameraSensorInfo &sensorInfo, > > const ControlInfoMap &sensorControls, > > + const ControlInfoMap &lensControls, > > ControlInfoMap *ipaControls) override; > > int start() override; > > void stop() override; > > @@ -80,6 +82,7 @@ private: > > std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > > > > ControlInfoMap sensorControls_; > > + std::optional<ControlInfoMap> lensControls_; > > > > /* revision-specific data */ > > rkisp1_cif_isp_version hwRevision_; > > @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const > > int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > const IPACameraSensorInfo &sensorInfo, > > const ControlInfoMap &sensorControls, > > + const ControlInfoMap &lensControls, > > ControlInfoMap *ipaControls) > > { > > /* \todo Add support for other revisions */ > > @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > context_.configuration.sensor.lineDuration = sensorInfo.minLineLength > > * 1.0s / sensorInfo.pixelRate; > > > > + if (!lensControls.empty()) > > + lensControls_ = lensControls; > > + > > /* Load the tuning data file. */ > > File file(settings.configurationFile); > > if (!file.open(File::OpenModeFlag::ReadOnly)) { > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 8a30fe06..f966254a 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -32,6 +32,7 @@ > > #include <libcamera/ipa/rkisp1_ipa_proxy.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" > > @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > return ret; > > } > > > > + ControlInfoMap lensControls; > > + CameraLens *lens = sensor_->focusLens(); > > + if (lens) > > + lensControls = lens->controls(); > > + > > ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision, > > - sensorInfo, sensor_->controls(), &controlInfo_); > > + sensorInfo, sensor_->controls(), lensControls, > > + &controlInfo_); > > if (ret < 0) { > > LOG(RkISP1, Error) << "IPA initialization failure"; > > return ret;
Hi Daniel, On 5/11/23 14:14, Daniel Semkowicz wrote: > Hi Michael, > > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch > <michael.riesch@wolfvision.net> wrote: >> >> Hi Daniel, >> >> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote: >>> Extend the IPA init() function by additional lensControls input >>> parameter. Check in pipeline handler if camera lens exists, and expose >>> its controls to the IPA using the new parameter. >>> >>> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> >>> --- >>> include/libcamera/ipa/rkisp1.mojom | 3 ++- >>> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++ >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++- >>> 3 files changed, 17 insertions(+), 2 deletions(-) >>> >>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom >>> index 1009e970..d4ff1230 100644 >>> --- a/include/libcamera/ipa/rkisp1.mojom >>> +++ b/include/libcamera/ipa/rkisp1.mojom >>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface { >>> init(libcamera.IPASettings settings, >>> uint32 hwRevision, >>> libcamera.IPACameraSensorInfo sensorInfo, >>> - libcamera.ControlInfoMap sensorControls) >>> + libcamera.ControlInfoMap sensorControls, >>> + libcamera.ControlInfoMap lensControls) >> >> One could imagine that in future other subdevices are supported. For >> example, I expect that soon we'll have a "flashControls" parameter. >> Would it make sense to gather all those controls in a single parameter >> "hwControls" (name to be bikeshedded)? (In #libcamera the notion of >> internal controls has been introduced recently. Maybe simply >> "internalControls"?) >> >> BTW I also could imagine that a "lensInfo" parameter is necessary for >> complex lens controllers (e.g., does the lens have a zoom function, how >> many zoom lenses does it have, model of the lens controller, ...). But >> for simple VCM controllers this may not be necessary, so feel free to >> ignore this at this point. > > It would probably make sense, but I would like not to do everything at once, > but introduce the changes gradually. I would suggest to extended it with the > introduction of next optional subdevice. Going one step at a time makes sense, of course. As I already said, the "lensInfo" parameter can be introduced once it is actually required and used. On the other hand, it probably makes sense to discuss the "sensorControls", "lensControls", ... -> "internalControls" issue right now. Of course I can wait until your series is applied and then propose a change, but this would be a bit back-and-forth. If we agree that there should be a single parameter (instead of one parameter per subdev), then this change should be incorporated in this series already IMHO. Of course, there may be perfectly fine reasons to have one parameter per subdev. I can't be the judge of that, but it would be great to see a verdict on that case. >>> => (int32 ret, libcamera.ControlInfoMap ipaControls); >>> start() => (int32 ret); >>> stop(); >> >> Shouldn't processStatsBuffer have an additional "lensControls" >> parameter? Of course, the comment above would apply here as well and we >> could get away with simply adjusting the name "sensorControls" -> >> "internalControls". > > Right now, We do not report any current values of lens controls to the IPA, > so such parameter would be left unused. Right, this should be proposed/introduced when it is actually required/used. Best regards, Michael > > Best regards > Daniel > >> >> Best regards, >> Michael >> >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >>> index 6544c925..d338d374 100644 >>> --- a/src/ipa/rkisp1/rkisp1.cpp >>> +++ b/src/ipa/rkisp1/rkisp1.cpp >>> @@ -7,6 +7,7 @@ >>> >>> #include <algorithm> >>> #include <math.h> >>> +#include <optional> >>> #include <queue> >>> #include <stdint.h> >>> #include <string.h> >>> @@ -52,6 +53,7 @@ public: >>> int init(const IPASettings &settings, unsigned int hwRevision, >>> const IPACameraSensorInfo &sensorInfo, >>> const ControlInfoMap &sensorControls, >>> + const ControlInfoMap &lensControls, >>> ControlInfoMap *ipaControls) override; >>> int start() override; >>> void stop() override; >>> @@ -80,6 +82,7 @@ private: >>> std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; >>> >>> ControlInfoMap sensorControls_; >>> + std::optional<ControlInfoMap> lensControls_; >>> >>> /* revision-specific data */ >>> rkisp1_cif_isp_version hwRevision_; >>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const >>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, >>> const IPACameraSensorInfo &sensorInfo, >>> const ControlInfoMap &sensorControls, >>> + const ControlInfoMap &lensControls, >>> ControlInfoMap *ipaControls) >>> { >>> /* \todo Add support for other revisions */ >>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, >>> context_.configuration.sensor.lineDuration = sensorInfo.minLineLength >>> * 1.0s / sensorInfo.pixelRate; >>> >>> + if (!lensControls.empty()) >>> + lensControls_ = lensControls; >>> + >>> /* Load the tuning data file. */ >>> File file(settings.configurationFile); >>> if (!file.open(File::OpenModeFlag::ReadOnly)) { >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> index 8a30fe06..f966254a 100644 >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp >>> @@ -32,6 +32,7 @@ >>> #include <libcamera/ipa/rkisp1_ipa_proxy.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" >>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) >>> return ret; >>> } >>> >>> + ControlInfoMap lensControls; >>> + CameraLens *lens = sensor_->focusLens(); >>> + if (lens) >>> + lensControls = lens->controls(); >>> + >>> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision, >>> - sensorInfo, sensor_->controls(), &controlInfo_); >>> + sensorInfo, sensor_->controls(), lensControls, >>> + &controlInfo_); >>> if (ret < 0) { >>> LOG(RkISP1, Error) << "IPA initialization failure"; >>> return ret;
Hello Michael, On Fri, May 12, 2023 at 12:08 PM Michael Riesch <michael.riesch@wolfvision.net> wrote: > > Hi Daniel, > > On 5/11/23 14:14, Daniel Semkowicz wrote: > > Hi Michael, > > > > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch > > <michael.riesch@wolfvision.net> wrote: > >> > >> Hi Daniel, > >> > >> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote: > >>> Extend the IPA init() function by additional lensControls input > >>> parameter. Check in pipeline handler if camera lens exists, and expose > >>> its controls to the IPA using the new parameter. > >>> > >>> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > >>> --- > >>> include/libcamera/ipa/rkisp1.mojom | 3 ++- > >>> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++ > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++- > >>> 3 files changed, 17 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > >>> index 1009e970..d4ff1230 100644 > >>> --- a/include/libcamera/ipa/rkisp1.mojom > >>> +++ b/include/libcamera/ipa/rkisp1.mojom > >>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface { > >>> init(libcamera.IPASettings settings, > >>> uint32 hwRevision, > >>> libcamera.IPACameraSensorInfo sensorInfo, > >>> - libcamera.ControlInfoMap sensorControls) > >>> + libcamera.ControlInfoMap sensorControls, > >>> + libcamera.ControlInfoMap lensControls) > >> > >> One could imagine that in future other subdevices are supported. For > >> example, I expect that soon we'll have a "flashControls" parameter. > >> Would it make sense to gather all those controls in a single parameter > >> "hwControls" (name to be bikeshedded)? (In #libcamera the notion of > >> internal controls has been introduced recently. Maybe simply > >> "internalControls"?) > >> > >> BTW I also could imagine that a "lensInfo" parameter is necessary for > >> complex lens controllers (e.g., does the lens have a zoom function, how > >> many zoom lenses does it have, model of the lens controller, ...). But > >> for simple VCM controllers this may not be necessary, so feel free to > >> ignore this at this point. > > > > It would probably make sense, but I would like not to do everything at once, > > but introduce the changes gradually. I would suggest to extended it with the > > introduction of next optional subdevice. > > Going one step at a time makes sense, of course. As I already said, the > "lensInfo" parameter can be introduced once it is actually required and > used. > > On the other hand, it probably makes sense to discuss the > "sensorControls", "lensControls", ... -> "internalControls" issue right > now. Of course I can wait until your series is applied and then propose > a change, but this would be a bit back-and-forth. If we agree that there > should be a single parameter (instead of one parameter per subdev), then > this change should be incorporated in this series already IMHO. > > Of course, there may be perfectly fine reasons to have one parameter per > subdev. I can't be the judge of that, but it would be great to see a > verdict on that case. I agree We need to find a good way to handle multiple subdevices, but there are already a lot of changes in this series. That is why I am reluctant to add even more. I would suggest starting a separate thread to find a solution to this problem. What others think about it? Is it safe to gather all controls in a single parameter? Can we guarantee the same control will not show up in two different subdevices? Is it possible there will be two subdevices of the same type, e.g. 2x flashlight? > > >>> => (int32 ret, libcamera.ControlInfoMap ipaControls); > >>> start() => (int32 ret); > >>> stop(); > >> > >> Shouldn't processStatsBuffer have an additional "lensControls" > >> parameter? Of course, the comment above would apply here as well and we > >> could get away with simply adjusting the name "sensorControls" -> > >> "internalControls". > > > > Right now, We do not report any current values of lens controls to the IPA, > > so such parameter would be left unused. > > Right, this should be proposed/introduced when it is actually required/used. > > Best regards, > Michael > > > > > Best regards > > Daniel > > > >> > >> Best regards, > >> Michael > >> > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > >>> index 6544c925..d338d374 100644 > >>> --- a/src/ipa/rkisp1/rkisp1.cpp > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp > >>> @@ -7,6 +7,7 @@ > >>> > >>> #include <algorithm> > >>> #include <math.h> > >>> +#include <optional> > >>> #include <queue> > >>> #include <stdint.h> > >>> #include <string.h> > >>> @@ -52,6 +53,7 @@ public: > >>> int init(const IPASettings &settings, unsigned int hwRevision, > >>> const IPACameraSensorInfo &sensorInfo, > >>> const ControlInfoMap &sensorControls, > >>> + const ControlInfoMap &lensControls, > >>> ControlInfoMap *ipaControls) override; > >>> int start() override; > >>> void stop() override; > >>> @@ -80,6 +82,7 @@ private: > >>> std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > >>> > >>> ControlInfoMap sensorControls_; > >>> + std::optional<ControlInfoMap> lensControls_; > >>> > >>> /* revision-specific data */ > >>> rkisp1_cif_isp_version hwRevision_; > >>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const > >>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > >>> const IPACameraSensorInfo &sensorInfo, > >>> const ControlInfoMap &sensorControls, > >>> + const ControlInfoMap &lensControls, > >>> ControlInfoMap *ipaControls) > >>> { > >>> /* \todo Add support for other revisions */ > >>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > >>> context_.configuration.sensor.lineDuration = sensorInfo.minLineLength > >>> * 1.0s / sensorInfo.pixelRate; > >>> > >>> + if (!lensControls.empty()) > >>> + lensControls_ = lensControls; > >>> + > >>> /* Load the tuning data file. */ > >>> File file(settings.configurationFile); > >>> if (!file.open(File::OpenModeFlag::ReadOnly)) { > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> index 8a30fe06..f966254a 100644 > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > >>> @@ -32,6 +32,7 @@ > >>> #include <libcamera/ipa/rkisp1_ipa_proxy.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" > >>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > >>> return ret; > >>> } > >>> > >>> + ControlInfoMap lensControls; > >>> + CameraLens *lens = sensor_->focusLens(); > >>> + if (lens) > >>> + lensControls = lens->controls(); > >>> + > >>> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision, > >>> - sensorInfo, sensor_->controls(), &controlInfo_); > >>> + sensorInfo, sensor_->controls(), lensControls, > >>> + &controlInfo_); > >>> if (ret < 0) { > >>> LOG(RkISP1, Error) << "IPA initialization failure"; > >>> return ret; Best regards Daniel
Hello, On Mon, May 29, 2023 at 12:09:41PM +0200, Daniel Semkowicz via libcamera-devel wrote: > On Fri, May 12, 2023 at 12:08 PM Michael Riesch wrote: > > On 5/11/23 14:14, Daniel Semkowicz wrote: > > > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch wrote: > > >> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote: > > >>> Extend the IPA init() function by additional lensControls input > > >>> parameter. Check in pipeline handler if camera lens exists, and expose > > >>> its controls to the IPA using the new parameter. > > >>> > > >>> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > >>> --- > > >>> include/libcamera/ipa/rkisp1.mojom | 3 ++- > > >>> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++ > > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++- > > >>> 3 files changed, 17 insertions(+), 2 deletions(-) > > >>> > > >>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > >>> index 1009e970..d4ff1230 100644 > > >>> --- a/include/libcamera/ipa/rkisp1.mojom > > >>> +++ b/include/libcamera/ipa/rkisp1.mojom > > >>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface { > > >>> init(libcamera.IPASettings settings, > > >>> uint32 hwRevision, > > >>> libcamera.IPACameraSensorInfo sensorInfo, > > >>> - libcamera.ControlInfoMap sensorControls) > > >>> + libcamera.ControlInfoMap sensorControls, > > >>> + libcamera.ControlInfoMap lensControls) > > >> > > >> One could imagine that in future other subdevices are supported. For > > >> example, I expect that soon we'll have a "flashControls" parameter. > > >> Would it make sense to gather all those controls in a single parameter > > >> "hwControls" (name to be bikeshedded)? (In #libcamera the notion of > > >> internal controls has been introduced recently. Maybe simply > > >> "internalControls"?) > > >> > > >> BTW I also could imagine that a "lensInfo" parameter is necessary for > > >> complex lens controllers (e.g., does the lens have a zoom function, how > > >> many zoom lenses does it have, model of the lens controller, ...). But > > >> for simple VCM controllers this may not be necessary, so feel free to > > >> ignore this at this point. > > > > > > It would probably make sense, but I would like not to do everything at once, > > > but introduce the changes gradually. I would suggest to extended it with the > > > introduction of next optional subdevice. > > > > Going one step at a time makes sense, of course. As I already said, the > > "lensInfo" parameter can be introduced once it is actually required and > > used. > > > > On the other hand, it probably makes sense to discuss the > > "sensorControls", "lensControls", ... -> "internalControls" issue right > > now. Of course I can wait until your series is applied and then propose > > a change, but this would be a bit back-and-forth. If we agree that there > > should be a single parameter (instead of one parameter per subdev), then > > this change should be incorporated in this series already IMHO. > > > > Of course, there may be perfectly fine reasons to have one parameter per > > subdev. I can't be the judge of that, but it would be great to see a > > verdict on that case. > > I agree We need to find a good way to handle multiple subdevices, > but there are already a lot of changes in this series. That is why I am > reluctant to add even more. I would suggest starting a separate thread > to find a solution to this problem. What others think about it? > > Is it safe to gather all controls in a single parameter? > Can we guarantee the same control will not show up in two different > subdevices? Is it possible there will be two subdevices of the same > type, e.g. 2x flashlight? It's more about how we build abstraction layers inside libcamera than about the subdevices. We have CameraSensor and CameraLens classes, and those should expose an abstract interface towards the pipeline handler, regardless of how the kernel exposes the corresponding devices. Those two classes should move from using V4L2 controls to internal libcamera controls, and avoid exposing anything V4L2-specific. The interface between the IPA module and pipeline handler should use the same internal controls. The translation to V4L2 controls, and the mapping to particular subdevs, will be internal to the helper classes. I don't really envision a need for multiple flashes per camera. I also don't think we'll have to deal with multiple independent focus lenses, but we will have optical zoom in addition to focus. > > >>> => (int32 ret, libcamera.ControlInfoMap ipaControls); > > >>> start() => (int32 ret); > > >>> stop(); > > >> > > >> Shouldn't processStatsBuffer have an additional "lensControls" > > >> parameter? Of course, the comment above would apply here as well and we > > >> could get away with simply adjusting the name "sensorControls" -> > > >> "internalControls". > > > > > > Right now, We do not report any current values of lens controls to the IPA, > > > so such parameter would be left unused. > > > > Right, this should be proposed/introduced when it is actually required/used. > > > > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > >>> index 6544c925..d338d374 100644 > > >>> --- a/src/ipa/rkisp1/rkisp1.cpp > > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp > > >>> @@ -7,6 +7,7 @@ > > >>> > > >>> #include <algorithm> > > >>> #include <math.h> > > >>> +#include <optional> > > >>> #include <queue> > > >>> #include <stdint.h> > > >>> #include <string.h> > > >>> @@ -52,6 +53,7 @@ public: > > >>> int init(const IPASettings &settings, unsigned int hwRevision, > > >>> const IPACameraSensorInfo &sensorInfo, > > >>> const ControlInfoMap &sensorControls, > > >>> + const ControlInfoMap &lensControls, > > >>> ControlInfoMap *ipaControls) override; > > >>> int start() override; > > >>> void stop() override; > > >>> @@ -80,6 +82,7 @@ private: > > >>> std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > > >>> > > >>> ControlInfoMap sensorControls_; > > >>> + std::optional<ControlInfoMap> lensControls_; > > >>> > > >>> /* revision-specific data */ > > >>> rkisp1_cif_isp_version hwRevision_; > > >>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const > > >>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > >>> const IPACameraSensorInfo &sensorInfo, > > >>> const ControlInfoMap &sensorControls, > > >>> + const ControlInfoMap &lensControls, > > >>> ControlInfoMap *ipaControls) > > >>> { > > >>> /* \todo Add support for other revisions */ > > >>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > >>> context_.configuration.sensor.lineDuration = sensorInfo.minLineLength > > >>> * 1.0s / sensorInfo.pixelRate; > > >>> > > >>> + if (!lensControls.empty()) > > >>> + lensControls_ = lensControls; > > >>> + > > >>> /* Load the tuning data file. */ > > >>> File file(settings.configurationFile); > > >>> if (!file.open(File::OpenModeFlag::ReadOnly)) { > > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > >>> index 8a30fe06..f966254a 100644 > > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > >>> @@ -32,6 +32,7 @@ > > >>> #include <libcamera/ipa/rkisp1_ipa_proxy.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" > > >>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > >>> return ret; > > >>> } > > >>> > > >>> + ControlInfoMap lensControls; > > >>> + CameraLens *lens = sensor_->focusLens(); > > >>> + if (lens) > > >>> + lensControls = lens->controls(); > > >>> + > > >>> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision, > > >>> - sensorInfo, sensor_->controls(), &controlInfo_); > > >>> + sensorInfo, sensor_->controls(), lensControls, > > >>> + &controlInfo_); > > >>> if (ret < 0) { > > >>> LOG(RkISP1, Error) << "IPA initialization failure"; > > >>> return ret;
Hello, On Wed, May 31, 2023 at 8:39 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Mon, May 29, 2023 at 12:09:41PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > On Fri, May 12, 2023 at 12:08 PM Michael Riesch wrote: > > > On 5/11/23 14:14, Daniel Semkowicz wrote: > > > > On Wed, Apr 26, 2023 at 10:18 AM Michael Riesch wrote: > > > >> On 3/31/23 10:19, Daniel Semkowicz via libcamera-devel wrote: > > > >>> Extend the IPA init() function by additional lensControls input > > > >>> parameter. Check in pipeline handler if camera lens exists, and expose > > > >>> its controls to the IPA using the new parameter. > > > >>> > > > >>> Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > >>> --- > > > >>> include/libcamera/ipa/rkisp1.mojom | 3 ++- > > > >>> src/ipa/rkisp1/rkisp1.cpp | 7 +++++++ > > > >>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++- > > > >>> 3 files changed, 17 insertions(+), 2 deletions(-) > > > >>> > > > >>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom > > > >>> index 1009e970..d4ff1230 100644 > > > >>> --- a/include/libcamera/ipa/rkisp1.mojom > > > >>> +++ b/include/libcamera/ipa/rkisp1.mojom > > > >>> @@ -17,7 +17,8 @@ interface IPARkISP1Interface { > > > >>> init(libcamera.IPASettings settings, > > > >>> uint32 hwRevision, > > > >>> libcamera.IPACameraSensorInfo sensorInfo, > > > >>> - libcamera.ControlInfoMap sensorControls) > > > >>> + libcamera.ControlInfoMap sensorControls, > > > >>> + libcamera.ControlInfoMap lensControls) > > > >> > > > >> One could imagine that in future other subdevices are supported. For > > > >> example, I expect that soon we'll have a "flashControls" parameter. > > > >> Would it make sense to gather all those controls in a single parameter > > > >> "hwControls" (name to be bikeshedded)? (In #libcamera the notion of > > > >> internal controls has been introduced recently. Maybe simply > > > >> "internalControls"?) > > > >> > > > >> BTW I also could imagine that a "lensInfo" parameter is necessary for > > > >> complex lens controllers (e.g., does the lens have a zoom function, how > > > >> many zoom lenses does it have, model of the lens controller, ...). But > > > >> for simple VCM controllers this may not be necessary, so feel free to > > > >> ignore this at this point. > > > > > > > > It would probably make sense, but I would like not to do everything at once, > > > > but introduce the changes gradually. I would suggest to extended it with the > > > > introduction of next optional subdevice. > > > > > > Going one step at a time makes sense, of course. As I already said, the > > > "lensInfo" parameter can be introduced once it is actually required and > > > used. > > > > > > On the other hand, it probably makes sense to discuss the > > > "sensorControls", "lensControls", ... -> "internalControls" issue right > > > now. Of course I can wait until your series is applied and then propose > > > a change, but this would be a bit back-and-forth. If we agree that there > > > should be a single parameter (instead of one parameter per subdev), then > > > this change should be incorporated in this series already IMHO. > > > > > > Of course, there may be perfectly fine reasons to have one parameter per > > > subdev. I can't be the judge of that, but it would be great to see a > > > verdict on that case. > > > > I agree We need to find a good way to handle multiple subdevices, > > but there are already a lot of changes in this series. That is why I am > > reluctant to add even more. I would suggest starting a separate thread > > to find a solution to this problem. What others think about it? > > > > Is it safe to gather all controls in a single parameter? > > Can we guarantee the same control will not show up in two different > > subdevices? Is it possible there will be two subdevices of the same > > type, e.g. 2x flashlight? > > It's more about how we build abstraction layers inside libcamera than > about the subdevices. We have CameraSensor and CameraLens classes, and > those should expose an abstract interface towards the pipeline handler, > regardless of how the kernel exposes the corresponding devices. Those > two classes should move from using V4L2 controls to internal libcamera > controls, and avoid exposing anything V4L2-specific. The interface > between the IPA module and pipeline handler should use the same internal > controls. The translation to V4L2 controls, and the mapping to > particular subdevs, will be internal to the helper classes. > > I don't really envision a need for multiple flashes per camera. I also > don't think we'll have to deal with multiple independent focus lenses, > but we will have optical zoom in addition to focus. This clarifies a lot to me. Right now, CameraLens just exposes the V4L2 controls and I assumed this is the target approach, hence my doubts. Now, I need the decision how to extend the init() parameters, because it impacts how I will handle it in the IPA. I am thinking about few options, most of them already proposed in this thread: 1. Leave the current implementation and solve this problem later: int init(..., const ControlInfoMap &sensorControls, const ControlInfoMap &lensControls, ...) 2. Merge all controls into single parameter: int init(..., const ControlInfoMap &internalControls, ...) 3. Leave the sensorControls intact, add auxiliaryDevicesControls parameter and put there all controls other than sensor: int init(..., const ControlInfoMap &sensorControls, const ControlInfoMap &auxiliaryDevicesControls, ...) 4. Create a separate map for each auxiliary device. This will avoid problems if there are multiple devices of the same type: int init(..., const ControlInfoMap &sensorControls, const Span<ControlInfoMap> &auxiliaryDevicesControls, ...) What do you think? > > > > >>> => (int32 ret, libcamera.ControlInfoMap ipaControls); > > > >>> start() => (int32 ret); > > > >>> stop(); > > > >> > > > >> Shouldn't processStatsBuffer have an additional "lensControls" > > > >> parameter? Of course, the comment above would apply here as well and we > > > >> could get away with simply adjusting the name "sensorControls" -> > > > >> "internalControls". > > > > > > > > Right now, We do not report any current values of lens controls to the IPA, > > > > so such parameter would be left unused. > > > > > > Right, this should be proposed/introduced when it is actually required/used. > > > > > > >>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > >>> index 6544c925..d338d374 100644 > > > >>> --- a/src/ipa/rkisp1/rkisp1.cpp > > > >>> +++ b/src/ipa/rkisp1/rkisp1.cpp > > > >>> @@ -7,6 +7,7 @@ > > > >>> > > > >>> #include <algorithm> > > > >>> #include <math.h> > > > >>> +#include <optional> > > > >>> #include <queue> > > > >>> #include <stdint.h> > > > >>> #include <string.h> > > > >>> @@ -52,6 +53,7 @@ public: > > > >>> int init(const IPASettings &settings, unsigned int hwRevision, > > > >>> const IPACameraSensorInfo &sensorInfo, > > > >>> const ControlInfoMap &sensorControls, > > > >>> + const ControlInfoMap &lensControls, > > > >>> ControlInfoMap *ipaControls) override; > > > >>> int start() override; > > > >>> void stop() override; > > > >>> @@ -80,6 +82,7 @@ private: > > > >>> std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; > > > >>> > > > >>> ControlInfoMap sensorControls_; > > > >>> + std::optional<ControlInfoMap> lensControls_; > > > >>> > > > >>> /* revision-specific data */ > > > >>> rkisp1_cif_isp_version hwRevision_; > > > >>> @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const > > > >>> int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > > >>> const IPACameraSensorInfo &sensorInfo, > > > >>> const ControlInfoMap &sensorControls, > > > >>> + const ControlInfoMap &lensControls, > > > >>> ControlInfoMap *ipaControls) > > > >>> { > > > >>> /* \todo Add support for other revisions */ > > > >>> @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > > >>> context_.configuration.sensor.lineDuration = sensorInfo.minLineLength > > > >>> * 1.0s / sensorInfo.pixelRate; > > > >>> > > > >>> + if (!lensControls.empty()) > > > >>> + lensControls_ = lensControls; > > > >>> + > > > >>> /* Load the tuning data file. */ > > > >>> File file(settings.configurationFile); > > > >>> if (!file.open(File::OpenModeFlag::ReadOnly)) { > > > >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > >>> index 8a30fe06..f966254a 100644 > > > >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > >>> @@ -32,6 +32,7 @@ > > > >>> #include <libcamera/ipa/rkisp1_ipa_proxy.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" > > > >>> @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) > > > >>> return ret; > > > >>> } > > > >>> > > > >>> + ControlInfoMap lensControls; > > > >>> + CameraLens *lens = sensor_->focusLens(); > > > >>> + if (lens) > > > >>> + lensControls = lens->controls(); > > > >>> + > > > >>> ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision, > > > >>> - sensorInfo, sensor_->controls(), &controlInfo_); > > > >>> + sensorInfo, sensor_->controls(), lensControls, > > > >>> + &controlInfo_); > > > >>> if (ret < 0) { > > > >>> LOG(RkISP1, Error) << "IPA initialization failure"; > > > >>> return ret; > > -- > Regards, > > Laurent Pinchart Best regards Daniel
diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom index 1009e970..d4ff1230 100644 --- a/include/libcamera/ipa/rkisp1.mojom +++ b/include/libcamera/ipa/rkisp1.mojom @@ -17,7 +17,8 @@ interface IPARkISP1Interface { init(libcamera.IPASettings settings, uint32 hwRevision, libcamera.IPACameraSensorInfo sensorInfo, - libcamera.ControlInfoMap sensorControls) + libcamera.ControlInfoMap sensorControls, + libcamera.ControlInfoMap lensControls) => (int32 ret, libcamera.ControlInfoMap ipaControls); start() => (int32 ret); stop(); diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 6544c925..d338d374 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -7,6 +7,7 @@ #include <algorithm> #include <math.h> +#include <optional> #include <queue> #include <stdint.h> #include <string.h> @@ -52,6 +53,7 @@ public: int init(const IPASettings &settings, unsigned int hwRevision, const IPACameraSensorInfo &sensorInfo, const ControlInfoMap &sensorControls, + const ControlInfoMap &lensControls, ControlInfoMap *ipaControls) override; int start() override; void stop() override; @@ -80,6 +82,7 @@ private: std::map<unsigned int, MappedFrameBuffer> mappedBuffers_; ControlInfoMap sensorControls_; + std::optional<ControlInfoMap> lensControls_; /* revision-specific data */ rkisp1_cif_isp_version hwRevision_; @@ -123,6 +126,7 @@ std::string IPARkISP1::logPrefix() const int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, const IPACameraSensorInfo &sensorInfo, const ControlInfoMap &sensorControls, + const ControlInfoMap &lensControls, ControlInfoMap *ipaControls) { /* \todo Add support for other revisions */ @@ -160,6 +164,9 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, context_.configuration.sensor.lineDuration = sensorInfo.minLineLength * 1.0s / sensorInfo.pixelRate; + if (!lensControls.empty()) + lensControls_ = lensControls; + /* Load the tuning data file. */ File file(settings.configurationFile); if (!file.open(File::OpenModeFlag::ReadOnly)) { diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 8a30fe06..f966254a 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -32,6 +32,7 @@ #include <libcamera/ipa/rkisp1_ipa_proxy.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" @@ -367,8 +368,14 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision) return ret; } + ControlInfoMap lensControls; + CameraLens *lens = sensor_->focusLens(); + if (lens) + lensControls = lens->controls(); + ret = ipa_->init({ ipaTuningFile, sensor_->model() }, hwRevision, - sensorInfo, sensor_->controls(), &controlInfo_); + sensorInfo, sensor_->controls(), lensControls, + &controlInfo_); if (ret < 0) { LOG(RkISP1, Error) << "IPA initialization failure"; return ret;
Extend the IPA init() function by additional lensControls input parameter. Check in pipeline handler if camera lens exists, and expose its controls to the IPA using the new parameter. Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> --- include/libcamera/ipa/rkisp1.mojom | 3 ++- src/ipa/rkisp1/rkisp1.cpp | 7 +++++++ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 ++++++++- 3 files changed, 17 insertions(+), 2 deletions(-)