[libcamera-devel] [PATCH v6 01/10] rkisp1: Add camera lens to PH and expose it to the IPA
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Apr 25 15:29:10 CEST 2023
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 at 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,
Reviewed-by: Laurent Pinchart <laurent.pinchart at 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
More information about the libcamera-devel
mailing list