[libcamera-devel,1/3] pipeline: ipa: raspberrypi: Move ControlInfoMap to the IPA
diff mbox series

Message ID 20220610122533.11888-1-naush@raspberrypi.com
State Superseded
Headers show
Series
  • [libcamera-devel,1/3] pipeline: ipa: raspberrypi: Move ControlInfoMap to the IPA
Related show

Commit Message

Naushir Patuck June 10, 2022, 12:25 p.m. UTC 
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.

Signed-off-by: Naushir Patuck <naush@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

Comments

Naushir Patuck June 21, 2022, 7:29 a.m. UTC | #1 
Hi,

Ping for a review on this series please.

Thanks,
Naush

On Fri, 10 Jun 2022 at 13:25, Naushir Patuck <naush@raspberrypi.com> 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.
>
> Signed-off-by: Naushir Patuck <naush@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);
> +
>         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>
>
> --
> 2.25.1
>
>
Kieran Bingham June 21, 2022, 12:05 p.m. UTC | #2 
Quoting Naushir Patuck via libcamera-devel (2022-06-10 13:25:31)
> 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.
> 
> Signed-off-by: Naushir Patuck <naush@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();
>  

We didn't have it before, but I wonder if result should have an
assertion that it is not null. But given how tied this is to the PH
side, I think we can say we 'know' that the two components will behave
with each other, so there's enough of a contract that it will be valid.

> -       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);
> +
>         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)

result keeps reading like such an odd variable name through here. But
I'm not sure I've got better alternatives. ipaData? ipaContext ? ...
data and config are too overloaded already...

But it's trivial so it doesn't worry me too much.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>                 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>
>  
> -- 
> 2.25.1
>
Laurent Pinchart June 21, 2022, 12:35 p.m. UTC | #3 
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@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@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>
>
Laurent Pinchart June 21, 2022, 1:07 p.m. UTC | #4 
On Tue, Jun 21, 2022 at 03:35:59PM +0300, Laurent Pinchart via libcamera-devel wrote:
> 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

The careful reader will have noticed I meant

Bug: https://bugs.libcamera.org/show_bug.cgi?id=83

(thanks Kieran for spotting this)

> > Signed-off-by: Naushir Patuck <naush@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@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>
> >

Patch
diff mbox series 

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);
+
 	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>