[libcamera-devel] [PATCH 1/3] pipeline: ipa: raspberrypi: Move ControlInfoMap to the IPA
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Jun 21 14:35:59 CEST 2022
Hi Naush,
Thank you for the patch.
On Fri, Jun 10, 2022 at 01:25:31PM +0100, Naushir Patuck via libcamera-devel wrote:
> Currently the pipeline handler advertises controls handled by the IPA from a
> static ControlInfoMap defined in the raspberrypi.h header. This change removes
> this header file, and instead the IPA returns the ControlInfoMap to the pipeline
> handler from the ipa::init() function. This is done to allow the IPA to adjust
> the limits of the controls based on the sensor mode in a subsequent change.
This also fixes a bug :-) You can add the following tag.
Bug: https://bugs.libcamera.org/show_bug.cgi?id=81
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> include/libcamera/ipa/raspberrypi.h | 55 -------------------
> include/libcamera/ipa/raspberrypi.mojom | 7 ++-
> src/ipa/raspberrypi/raspberrypi.cpp | 39 ++++++++++---
> .../pipeline/raspberrypi/raspberrypi.cpp | 30 +++++-----
> .../pipeline/raspberrypi/rpi_stream.h | 1 -
> 5 files changed, 53 insertions(+), 79 deletions(-)
> delete mode 100644 include/libcamera/ipa/raspberrypi.h
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> deleted file mode 100644
> index 6a56b0083b85..000000000000
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ /dev/null
> @@ -1,55 +0,0 @@
> -/* SPDX-License-Identifier: LGPL-2.1-or-later */
> -/*
> - * Copyright (C) 2019-2020, Raspberry Pi Ltd.
> - *
> - * raspberrypi.h - Image Processing Algorithm interface for Raspberry Pi
> - */
> -
> -#pragma once
> -
> -#include <stdint.h>
> -
> -#include <libcamera/control_ids.h>
> -#include <libcamera/controls.h>
> -
> -#ifndef __DOXYGEN__
> -
> -namespace libcamera {
> -
> -namespace RPi {
> -
> -/*
> - * List of controls handled by the Raspberry Pi IPA
> - *
> - * \todo This list will need to be built dynamically from the control
> - * algorithms loaded by the json file, once this is supported. At that
> - * point applications should check first whether a control is supported,
> - * and the pipeline handler may be reverted so that it aborts when an
> - * unsupported control is encountered.
> - */
> -static const ControlInfoMap Controls({
> - { &controls::AeEnable, ControlInfo(false, true) },
> - { &controls::ExposureTime, ControlInfo(0, 999999) },
> - { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> - { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> - { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> - { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> - { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> - { &controls::AwbEnable, ControlInfo(false, true) },
> - { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> - { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> - { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> - { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> - { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> - { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> - { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> - { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> - { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> - { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> - }, controls::controls);
> -
> -} /* namespace RPi */
> -
> -} /* namespace libcamera */
> -
> -#endif /* __DOXYGEN__ */
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> index a60c3bb43d3c..77f52c282b0f 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -26,6 +26,11 @@ struct SensorConfig {
> uint32 sensorMetadata;
> };
>
> +struct IPAInitResult {
> + SensorConfig sensorConfig;
> + libcamera.ControlInfoMap controlInfo;
> +};
> +
> struct ISPConfig {
> uint32 embeddedBufferId;
> uint32 bayerBufferId;
> @@ -50,7 +55,7 @@ struct StartConfig {
>
> interface IPARPiInterface {
> init(libcamera.IPASettings settings)
> - => (int32 ret, SensorConfig sensorConfig);
> + => (int32 ret, IPAInitResult result);
> start(libcamera.ControlList controls) => (StartConfig startConfig);
> stop();
>
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3b126bb5175e..089528f5e126 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -24,7 +24,6 @@
> #include <libcamera/framebuffer.h>
> #include <libcamera/ipa/ipa_interface.h>
> #include <libcamera/ipa/ipa_module_info.h>
> -#include <libcamera/ipa/raspberrypi.h>
> #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> #include <libcamera/request.h>
>
> @@ -72,6 +71,28 @@ constexpr Duration defaultMaxFrameDuration = 250.0s;
> */
> constexpr Duration controllerMinFrameDuration = 1.0s / 30.0;
>
> +/* List of controls handled by the Raspberry Pi IPA */
> +static const ControlInfoMap::Map ipaControls{
> + { &controls::AeEnable, ControlInfo(false, true) },
> + { &controls::ExposureTime, ControlInfo(0, 999999) },
> + { &controls::AnalogueGain, ControlInfo(1.0f, 32.0f) },
> + { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
> + { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
> + { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
> + { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> + { &controls::AwbEnable, ControlInfo(false, true) },
> + { &controls::ColourGains, ControlInfo(0.0f, 32.0f) },
> + { &controls::AwbMode, ControlInfo(controls::AwbModeValues) },
> + { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
> + { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> + { &controls::Saturation, ControlInfo(0.0f, 32.0f, 1.0f) },
> + { &controls::Sharpness, ControlInfo(0.0f, 16.0f, 1.0f) },
> + { &controls::ColourCorrectionMatrix, ControlInfo(-16.0f, 16.0f) },
> + { &controls::ScalerCrop, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) },
> + { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> + { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> +};
> +
> LOG_DEFINE_CATEGORY(IPARPI)
>
> namespace ipa::RPi {
> @@ -91,7 +112,7 @@ public:
> munmap(lsTable_, MaxLsGridSize);
> }
>
> - int init(const IPASettings &settings, SensorConfig *sensorConfig) override;
> + int init(const IPASettings &settings, IPAInitResult *result) override;
> void start(const ControlList &controls, StartConfig *startConfig) override;
> void stop() override {}
>
> @@ -180,7 +201,7 @@ private:
> uint32_t maxSensorGainCode_;
> };
>
> -int IPARPi::init(const IPASettings &settings, SensorConfig *sensorConfig)
> +int IPARPi::init(const IPASettings &settings, IPAInitResult *result)
> {
> /*
> * Load the "helper" for this sensor. This tells us all the device specific stuff
> @@ -202,15 +223,19 @@ int IPARPi::init(const IPASettings &settings, SensorConfig *sensorConfig)
> helper_->GetDelays(exposureDelay, gainDelay, vblankDelay);
> sensorMetadata = helper_->SensorEmbeddedDataPresent();
>
> - sensorConfig->gainDelay = gainDelay;
> - sensorConfig->exposureDelay = exposureDelay;
> - sensorConfig->vblankDelay = vblankDelay;
> - sensorConfig->sensorMetadata = sensorMetadata;
> + result->sensorConfig.gainDelay = gainDelay;
> + result->sensorConfig.exposureDelay = exposureDelay;
> + result->sensorConfig.vblankDelay = vblankDelay;
> + result->sensorConfig.sensorMetadata = sensorMetadata;
>
> /* Load the tuning file for this sensor. */
> controller_.Read(settings.configurationFile.c_str());
> controller_.Initialise();
>
> + /* Return the controls handled by the IPA */
> + ControlInfoMap::Map ctrlMap = ipaControls;
> + result->controlInfo = ControlInfoMap(std::move(ctrlMap), controls::controls);
This is a bit weird. I think it could be simplified by keeping
ipaControls a ControlInfoMap. To do so safely, and not depend on link
order, you can declare it as a local static const variable in this
function instead of a global variable.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> +
> return 0;
> }
>
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index adc397e8aabd..d980c1a71dd8 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -20,7 +20,6 @@
> #include <libcamera/camera.h>
> #include <libcamera/control_ids.h>
> #include <libcamera/formats.h>
> -#include <libcamera/ipa/raspberrypi.h>
> #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> #include <libcamera/ipa/raspberrypi_ipa_proxy.h>
> #include <libcamera/logging.h>
> @@ -199,7 +198,7 @@ public:
> void freeBuffers();
> void frameStarted(uint32_t sequence);
>
> - int loadIPA(ipa::RPi::SensorConfig *sensorConfig);
> + int loadIPA(ipa::RPi::IPAInitResult *result);
> int configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result);
>
> void enumerateVideoDevices(MediaLink *link);
> @@ -1231,15 +1230,15 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>
> data->sensorFormats_ = populateSensorFormats(data->sensor_);
>
> - ipa::RPi::SensorConfig sensorConfig;
> - if (data->loadIPA(&sensorConfig)) {
> + ipa::RPi::IPAInitResult result;
> + if (data->loadIPA(&result)) {
> LOG(RPI, Error) << "Failed to load a suitable IPA library";
> return -EINVAL;
> }
>
> - if (sensorConfig.sensorMetadata ^ !!unicamEmbedded) {
> + if (result.sensorConfig.sensorMetadata ^ !!unicamEmbedded) {
> LOG(RPI, Warning) << "Mismatch between Unicam and CamHelper for embedded data usage!";
> - sensorConfig.sensorMetadata = false;
> + result.sensorConfig.sensorMetadata = false;
> if (unicamEmbedded)
> data->unicam_[Unicam::Embedded].dev()->bufferReady.disconnect();
> }
> @@ -1253,7 +1252,7 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> * iterate over all streams in one go.
> */
> data->streams_.push_back(&data->unicam_[Unicam::Image]);
> - if (sensorConfig.sensorMetadata)
> + if (result.sensorConfig.sensorMetadata)
> data->streams_.push_back(&data->unicam_[Unicam::Embedded]);
>
> for (auto &stream : data->isp_)
> @@ -1275,15 +1274,16 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> * gain and exposure delays. Mark VBLANK for priority write.
> */
> std::unordered_map<uint32_t, DelayedControls::ControlParams> params = {
> - { V4L2_CID_ANALOGUE_GAIN, { sensorConfig.gainDelay, false } },
> - { V4L2_CID_EXPOSURE, { sensorConfig.exposureDelay, false } },
> - { V4L2_CID_VBLANK, { sensorConfig.vblankDelay, true } }
> + { V4L2_CID_ANALOGUE_GAIN, { result.sensorConfig.gainDelay, false } },
> + { V4L2_CID_EXPOSURE, { result.sensorConfig.exposureDelay, false } },
> + { V4L2_CID_VBLANK, { result.sensorConfig.vblankDelay, true } }
> };
> data->delayedCtrls_ = std::make_unique<DelayedControls>(data->sensor_->device(), params);
> - data->sensorMetadata_ = sensorConfig.sensorMetadata;
> + data->sensorMetadata_ = result.sensorConfig.sensorMetadata;
> +
> + /* Register initial controls that the Raspberry Pi IPA can handle. */
> + data->controlInfo_ = std::move(result.controlInfo);
>
> - /* Register the controls that the Raspberry Pi IPA can handle. */
> - data->controlInfo_ = RPi::Controls;
> /* Initialize the camera properties. */
> data->properties_ = data->sensor_->properties();
>
> @@ -1509,7 +1509,7 @@ void RPiCameraData::frameStarted(uint32_t sequence)
> delayedCtrls_->applyControls(sequence);
> }
>
> -int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
> +int RPiCameraData::loadIPA(ipa::RPi::IPAInitResult *result)
> {
> ipa_ = IPAManager::createIPA<ipa::RPi::IPAProxyRPi>(pipe(), 1, 1);
>
> @@ -1535,7 +1535,7 @@ int RPiCameraData::loadIPA(ipa::RPi::SensorConfig *sensorConfig)
>
> IPASettings settings(configurationFile, sensor_->model());
>
> - return ipa_->init(settings, sensorConfig);
> + return ipa_->init(settings, result);
> }
>
> int RPiCameraData::configureIPA(const CameraConfiguration *config, ipa::RPi::IPAConfigResult *result)
> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> index c37f7e82eef6..fe01110019b7 100644
> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h
> @@ -12,7 +12,6 @@
> #include <unordered_map>
> #include <vector>
>
> -#include <libcamera/ipa/raspberrypi.h>
> #include <libcamera/ipa/raspberrypi_ipa_interface.h>
> #include <libcamera/stream.h>
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list