[libcamera-devel] pipeline: ipa: raspberrypi: Correctly report available controls
diff mbox series

Message ID 20220609125830.4325-1-naush@raspberrypi.com
State Superseded
Headers show
Series
  • [libcamera-devel] pipeline: ipa: raspberrypi: Correctly report available controls
Related show

Commit Message

Naushir Patuck June 9, 2022, 12:58 p.m. UTC
The pipeline handler currently advertises a static ControlInfoMap to specify the
available controls and their limits. Fix this limitation by having the IPA
populate a ControlInfoMap during Camera::Configure() that is then returned from
the pipeline handle. This will allow the ExposureTime, AnalogueGain, and
FrameDurationLimits controls to advertise the correct limits for the programmed
mode.

Note that with this change, the ControlInfoMap provided by Camera::Controls()
is only valid after a successful Camera::Configure() call.

Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
---
 include/libcamera/ipa/raspberrypi.h           | 55 -------------------
 include/libcamera/ipa/raspberrypi.mojom       |  1 +
 src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-
 .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-
 .../pipeline/raspberrypi/rpi_stream.h         |  1 -
 5 files changed, 45 insertions(+), 60 deletions(-)
 delete mode 100644 include/libcamera/ipa/raspberrypi.h

Comments

Laurent Pinchart June 9, 2022, 4:08 p.m. UTC | #1
Hi Naush,

Thank you for the patch.

On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via libcamera-devel wrote:
> The pipeline handler currently advertises a static ControlInfoMap to specify the
> available controls and their limits. Fix this limitation by having the IPA
> populate a ControlInfoMap during Camera::Configure() that is then returned from
> the pipeline handle. This will allow the ExposureTime, AnalogueGain, and
> FrameDurationLimits controls to advertise the correct limits for the programmed
> mode.
> 
> Note that with this change, the ControlInfoMap provided by Camera::Controls()
> is only valid after a successful Camera::Configure() call.

That's an issue, we need to initialize it earlier. You can update it at
configure() time, but it has to bet set at init() time.

> Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> ---
>  include/libcamera/ipa/raspberrypi.h           | 55 -------------------
>  include/libcamera/ipa/raspberrypi.mojom       |  1 +
>  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-
>  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-
>  .../pipeline/raspberrypi/rpi_stream.h         |  1 -
>  5 files changed, 45 insertions(+), 60 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..ed7adebce1b8 100644
> --- a/include/libcamera/ipa/raspberrypi.mojom
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -40,6 +40,7 @@ struct IPAConfig {
>  
>  struct IPAConfigResult {
>         float modeSensitivity;
> +       libcamera.ControlInfoMap controlInfo;
>  };
>  
>  struct StartConfig {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3b126bb5175e..7eb04a24c41e 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 Controls({

While at it, could you rename the variable to start with a lowercase
letter ? "controls" is a bit too generic, so you could call it
"rpiControls" or anything similar.

> +	{ &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{}) },

As you've said yourself in
https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html,
this one doesn't belong to the IPA side :-) It can be fixed on top
though.

If it helps moving forward, I can resubmit the patch from
https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html
as a proper patch, and then you can update the frame duration limits and
exposure time controls at configure time on top ?

> +	{ &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> +	{ &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> +}, controls::controls);
> +
>  LOG_DEFINE_CATEGORY(IPARPI)
>  
>  namespace ipa::RPi {
> @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
>  	ASSERT(controls);
>  	*controls = std::move(ctrls);
>  
> +	/*
> +	 * Apply the correct limits to the exposure, gain and frame duration controls
> +	 * based on the current sensor mode.
> +	 */
> +	result->controlInfo = Controls;
> +	const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;
> +	const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
> +	result->controlInfo.at(controls::FrameDurationLimits.id()) =
> +			ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> +				    static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> +
> +	result->controlInfo.at(controls::AnalogueGain.id()) =
> +			ControlInfo(1.0f, static_cast<float>(helper_->Gain(maxSensorGainCode_)));
> +
> +	const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> +	const uint32_t exposureMax = sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();
> +	result->controlInfo.at(controls::ExposureTime.id()) =
> +			ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),
> +				    static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));
>  	return 0;
>  }
>  
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index adc397e8aabd..abb29a8d24b9 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>
> @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
>  	/* Store the mode sensitivity for the application. */
>  	data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);
>  
> +	/* Register the controls that the Raspberry Pi IPA can handle. */
> +	data->controlInfo_ = result.controlInfo;
> +
>  	/* Setup the Video Mux/Bridge entities. */
>  	for (auto &[device, link] : data->bridgeDevices_) {
>  		/*
> @@ -1282,8 +1284,6 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>  	data->delayedCtrls_ = std::make_unique<DelayedControls>(data->sensor_->device(), params);
>  	data->sensorMetadata_ = sensorConfig.sensorMetadata;
>  
> -	/* Register the controls that the Raspberry Pi IPA can handle. */
> -	data->controlInfo_ = RPi::Controls;
>  	/* Initialize the camera properties. */
>  	data->properties_ = data->sensor_->properties();
>  
> 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>
>
David Plowman June 10, 2022, 8:03 a.m. UTC | #2
Hi Naush

Thanks very much for this patch. I'm very happy with the functionality
added here!

On Thu, 9 Jun 2022 at 17:08, Laurent Pinchart via libcamera-devel
<libcamera-devel@lists.libcamera.org> wrote:
>
> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via libcamera-devel wrote:
> > The pipeline handler currently advertises a static ControlInfoMap to specify the
> > available controls and their limits. Fix this limitation by having the IPA
> > populate a ControlInfoMap during Camera::Configure() that is then returned from
> > the pipeline handle. This will allow the ExposureTime, AnalogueGain, and
> > FrameDurationLimits controls to advertise the correct limits for the programmed
> > mode.
> >
> > Note that with this change, the ControlInfoMap provided by Camera::Controls()
> > is only valid after a successful Camera::Configure() call.
>
> That's an issue, we need to initialize it earlier. You can update it at
> configure() time, but it has to bet set at init() time.
>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>

The only minor thought I had looking through this was whether we could
supply some better min/max values for the ScalerCrop (for example, all
zeroes wouldn't be valid), but I think that's a separate
discussion/patch.

Reviewed-by: David Plowman <david.plowman@raspberrypi.com>

Thanks
David

>
> > ---
> >  include/libcamera/ipa/raspberrypi.h           | 55 -------------------
> >  include/libcamera/ipa/raspberrypi.mojom       |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-
> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-
> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -
> >  5 files changed, 45 insertions(+), 60 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..ed7adebce1b8 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -40,6 +40,7 @@ struct IPAConfig {
> >
> >  struct IPAConfigResult {
> >         float modeSensitivity;
> > +       libcamera.ControlInfoMap controlInfo;
> >  };
> >
> >  struct StartConfig {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 3b126bb5175e..7eb04a24c41e 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 Controls({
>
> While at it, could you rename the variable to start with a lowercase
> letter ? "controls" is a bit too generic, so you could call it
> "rpiControls" or anything similar.
>
> > +     { &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{}) },
>
> As you've said yourself in
> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html,
> this one doesn't belong to the IPA side :-) It can be fixed on top
> though.
>
> If it helps moving forward, I can resubmit the patch from
> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html
> as a proper patch, and then you can update the frame duration limits and
> exposure time controls at configure time on top ?
>
> > +     { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000), INT64_C(1000000000)) },
> > +     { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }
> > +}, controls::controls);
> > +
> >  LOG_DEFINE_CATEGORY(IPARPI)
> >
> >  namespace ipa::RPi {
> > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
> >       ASSERT(controls);
> >       *controls = std::move(ctrls);
> >
> > +     /*
> > +      * Apply the correct limits to the exposure, gain and frame duration controls
> > +      * based on the current sensor mode.
> > +      */
> > +     result->controlInfo = Controls;
> > +     const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;
> > +     const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
> > +     result->controlInfo.at(controls::FrameDurationLimits.id()) =
> > +                     ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> > +                                 static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> > +
> > +     result->controlInfo.at(controls::AnalogueGain.id()) =
> > +                     ControlInfo(1.0f, static_cast<float>(helper_->Gain(maxSensorGainCode_)));
> > +
> > +     const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> > +     const uint32_t exposureMax = sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();
> > +     result->controlInfo.at(controls::ExposureTime.id()) =
> > +                     ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),
> > +                                 static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));
> >       return 0;
> >  }
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index adc397e8aabd..abb29a8d24b9 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>
> > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> >       /* Store the mode sensitivity for the application. */
> >       data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);
> >
> > +     /* Register the controls that the Raspberry Pi IPA can handle. */
> > +     data->controlInfo_ = result.controlInfo;
> > +
> >       /* Setup the Video Mux/Bridge entities. */
> >       for (auto &[device, link] : data->bridgeDevices_) {
> >               /*
> > @@ -1282,8 +1284,6 @@ int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> >       data->delayedCtrls_ = std::make_unique<DelayedControls>(data->sensor_->device(), params);
> >       data->sensorMetadata_ = sensorConfig.sensorMetadata;
> >
> > -     /* Register the controls that the Raspberry Pi IPA can handle. */
> > -     data->controlInfo_ = RPi::Controls;
> >       /* Initialize the camera properties. */
> >       data->properties_ = data->sensor_->properties();
> >
> > 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
Naushir Patuck June 10, 2022, 8:50 a.m. UTC | #3
Hi Laurent,

Thank you for your feedback.

On Thu, 9 Jun 2022 at 17:08, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> Hi Naush,
>
> Thank you for the patch.
>
> On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > The pipeline handler currently advertises a static ControlInfoMap to
> specify the
> > available controls and their limits. Fix this limitation by having the
> IPA
> > populate a ControlInfoMap during Camera::Configure() that is then
> returned from
> > the pipeline handle. This will allow the ExposureTime, AnalogueGain, and
> > FrameDurationLimits controls to advertise the correct limits for the
> programmed
> > mode.
> >
> > Note that with this change, the ControlInfoMap provided by
> Camera::Controls()
> > is only valid after a successful Camera::Configure() call.
>
> That's an issue, we need to initialize it earlier. You can update it at
> configure() time, but it has to bet set at init() time.
>

Sure I can do that.  What do you suggest we do about
the AnalogueGain, ExposureTime
and FrameDurationLimits controls during the init() phase?  Should I exclude
them from
the list, or include time, with min/max values set to 0 or something else?
I'm leaning towards
simply excluding them from the list until the limits are actually known.


>
> > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
> > ---
> >  include/libcamera/ipa/raspberrypi.h           | 55 -------------------
> >  include/libcamera/ipa/raspberrypi.mojom       |  1 +
> >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-
> >  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-
> >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -
> >  5 files changed, 45 insertions(+), 60 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..ed7adebce1b8 100644
> > --- a/include/libcamera/ipa/raspberrypi.mojom
> > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > @@ -40,6 +40,7 @@ struct IPAConfig {
> >
> >  struct IPAConfigResult {
> >         float modeSensitivity;
> > +       libcamera.ControlInfoMap controlInfo;
> >  };
> >
> >  struct StartConfig {
> > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > index 3b126bb5175e..7eb04a24c41e 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 Controls({
>
> While at it, could you rename the variable to start with a lowercase
> letter ? "controls" is a bit too generic, so you could call it
> "rpiControls" or anything similar.
>

Ack.


>
> > +     { &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{}) },
>
> As you've said yourself in
>
> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html
> ,
> this one doesn't belong to the IPA side :-) It can be fixed on top
> though.
>

I can add a patch on top to move ScalerCrop to the pipeline handler.  It's
a bit awkward to code because
I cannot add elements to a ControlInfoMap after initialisation since it
derives from a private unordered_map.
Instead, I will have to construct a new ControlInfoMap::Map with the
contents of the ControlInfoMap provided
by the IPA, add ScalerCrop to the ControlInfoMap::Map, and then convert
back to a ControlInfoMap.
Can you suggest a more elegant way to do this?

Regards,
Naush


>
> If it helps moving forward, I can resubmit the patch from
>
> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html
> as a proper patch, and then you can update the frame duration limits and
> exposure time controls at configure time on top ?
>

> > +     { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),
> INT64_C(1000000000)) },
> > +     { &controls::draft::NoiseReductionMode,
> ControlInfo(controls::draft::NoiseReductionModeValues) }
> > +}, controls::controls);
> > +
> >  LOG_DEFINE_CATEGORY(IPARPI)
> >
> >  namespace ipa::RPi {
> > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> >       ASSERT(controls);
> >       *controls = std::move(ctrls);
> >
> > +     /*
> > +      * Apply the correct limits to the exposure, gain and frame
> duration controls
> > +      * based on the current sensor mode.
> > +      */
> > +     result->controlInfo = Controls;
> > +     const Duration minSensorFrameDuration = mode_.min_frame_length *
> mode_.line_length;
> > +     const Duration maxSensorFrameDuration = mode_.max_frame_length *
> mode_.line_length;
> > +     result->controlInfo.at(controls::FrameDurationLimits.id()) =
> > +
>  ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> > +
>  static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> > +
> > +     result->controlInfo.at(controls::AnalogueGain.id()) =
> > +                     ControlInfo(1.0f,
> static_cast<float>(helper_->Gain(maxSensorGainCode_)));
> > +
> > +     const uint32_t exposureMin =
> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> > +     const uint32_t exposureMax =
> sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();
> > +     result->controlInfo.at(controls::ExposureTime.id()) =
> > +
>  ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),
> > +
>  static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));
> >       return 0;
> >  }
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index adc397e8aabd..abb29a8d24b9 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>
> > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> >       /* Store the mode sensitivity for the application. */
> >       data->properties_.set(properties::SensorSensitivity,
> result.modeSensitivity);
> >
> > +     /* Register the controls that the Raspberry Pi IPA can handle. */
> > +     data->controlInfo_ = result.controlInfo;
> > +
> >       /* Setup the Video Mux/Bridge entities. */
> >       for (auto &[device, link] : data->bridgeDevices_) {
> >               /*
> > @@ -1282,8 +1284,6 @@ int PipelineHandlerRPi::registerCamera(MediaDevice
> *unicam, MediaDevice *isp, Me
> >       data->delayedCtrls_ =
> std::make_unique<DelayedControls>(data->sensor_->device(), params);
> >       data->sensorMetadata_ = sensorConfig.sensorMetadata;
> >
> > -     /* Register the controls that the Raspberry Pi IPA can handle. */
> > -     data->controlInfo_ = RPi::Controls;
> >       /* Initialize the camera properties. */
> >       data->properties_ = data->sensor_->properties();
> >
> > 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
>
Naushir Patuck June 10, 2022, 9:27 a.m. UTC | #4
Hi David,

Thank you for your feedback.

On Fri, 10 Jun 2022 at 09:04, David Plowman <david.plowman@raspberrypi.com>
wrote:

> Hi Naush
>
> Thanks very much for this patch. I'm very happy with the functionality
> added here!
>
> On Thu, 9 Jun 2022 at 17:08, Laurent Pinchart via libcamera-devel
> <libcamera-devel@lists.libcamera.org> wrote:
> >
> > Hi Naush,
> >
> > Thank you for the patch.
> >
> > On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via
> libcamera-devel wrote:
> > > The pipeline handler currently advertises a static ControlInfoMap to
> specify the
> > > available controls and their limits. Fix this limitation by having the
> IPA
> > > populate a ControlInfoMap during Camera::Configure() that is then
> returned from
> > > the pipeline handle. This will allow the ExposureTime, AnalogueGain,
> and
> > > FrameDurationLimits controls to advertise the correct limits for the
> programmed
> > > mode.
> > >
> > > Note that with this change, the ControlInfoMap provided by
> Camera::Controls()
> > > is only valid after a successful Camera::Configure() call.
> >
> > That's an issue, we need to initialize it earlier. You can update it at
> > configure() time, but it has to bet set at init() time.
> >
> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>
> The only minor thought I had looking through this was whether we could
> supply some better min/max values for the ScalerCrop (for example, all
> zeroes wouldn't be valid), but I think that's a separate
> discussion/patch.
>

I can add a patch that does this since I will be moving the ScalerCrop
control
setup to the pipeline handler.  About setting the limits, presumably the max
wants to be set to the same value as properties::ScalerCropMaximum, which
in turn is the value of the "analgCrop" from the sensor.  Does this mean
that
properties::ScalerCropMaximum might become redundant?

Regards,
Naush



>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>
> Thanks
> David
>
> >
> > > ---
> > >  include/libcamera/ipa/raspberrypi.h           | 55 -------------------
> > >  include/libcamera/ipa/raspberrypi.mojom       |  1 +
> > >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-
> > >  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-
> > >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -
> > >  5 files changed, 45 insertions(+), 60 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..ed7adebce1b8 100644
> > > --- a/include/libcamera/ipa/raspberrypi.mojom
> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
> > > @@ -40,6 +40,7 @@ struct IPAConfig {
> > >
> > >  struct IPAConfigResult {
> > >         float modeSensitivity;
> > > +       libcamera.ControlInfoMap controlInfo;
> > >  };
> > >
> > >  struct StartConfig {
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> b/src/ipa/raspberrypi/raspberrypi.cpp
> > > index 3b126bb5175e..7eb04a24c41e 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 Controls({
> >
> > While at it, could you rename the variable to start with a lowercase
> > letter ? "controls" is a bit too generic, so you could call it
> > "rpiControls" or anything similar.
> >
> > > +     { &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{}) },
> >
> > As you've said yourself in
> >
> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html
> ,
> > this one doesn't belong to the IPA side :-) It can be fixed on top
> > though.
> >
> > If it helps moving forward, I can resubmit the patch from
> >
> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html
> > as a proper patch, and then you can update the frame duration limits and
> > exposure time controls at configure time on top ?
> >
> > > +     { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),
> INT64_C(1000000000)) },
> > > +     { &controls::draft::NoiseReductionMode,
> ControlInfo(controls::draft::NoiseReductionModeValues) }
> > > +}, controls::controls);
> > > +
> > >  LOG_DEFINE_CATEGORY(IPARPI)
> > >
> > >  namespace ipa::RPi {
> > > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo
> &sensorInfo,
> > >       ASSERT(controls);
> > >       *controls = std::move(ctrls);
> > >
> > > +     /*
> > > +      * Apply the correct limits to the exposure, gain and frame
> duration controls
> > > +      * based on the current sensor mode.
> > > +      */
> > > +     result->controlInfo = Controls;
> > > +     const Duration minSensorFrameDuration = mode_.min_frame_length *
> mode_.line_length;
> > > +     const Duration maxSensorFrameDuration = mode_.max_frame_length *
> mode_.line_length;
> > > +     result->controlInfo.at(controls::FrameDurationLimits.id()) =
> > > +
>  ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
> > > +
>  static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
> > > +
> > > +     result->controlInfo.at(controls::AnalogueGain.id()) =
> > > +                     ControlInfo(1.0f,
> static_cast<float>(helper_->Gain(maxSensorGainCode_)));
> > > +
> > > +     const uint32_t exposureMin =
> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
> > > +     const uint32_t exposureMax =
> sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();
> > > +     result->controlInfo.at(controls::ExposureTime.id()) =
> > > +
>  ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),
> > > +
>  static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));
> > >       return 0;
> > >  }
> > >
> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index adc397e8aabd..abb29a8d24b9 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>
> > > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera,
> CameraConfiguration *config)
> > >       /* Store the mode sensitivity for the application. */
> > >       data->properties_.set(properties::SensorSensitivity,
> result.modeSensitivity);
> > >
> > > +     /* Register the controls that the Raspberry Pi IPA can handle. */
> > > +     data->controlInfo_ = result.controlInfo;
> > > +
> > >       /* Setup the Video Mux/Bridge entities. */
> > >       for (auto &[device, link] : data->bridgeDevices_) {
> > >               /*
> > > @@ -1282,8 +1284,6 @@ int
> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
> > >       data->delayedCtrls_ =
> std::make_unique<DelayedControls>(data->sensor_->device(), params);
> > >       data->sensorMetadata_ = sensorConfig.sensorMetadata;
> > >
> > > -     /* Register the controls that the Raspberry Pi IPA can handle. */
> > > -     data->controlInfo_ = RPi::Controls;
> > >       /* Initialize the camera properties. */
> > >       data->properties_ = data->sensor_->properties();
> > >
> > > 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
>
Naushir Patuck June 10, 2022, 10:02 a.m. UTC | #5
On Fri, 10 Jun 2022 at 10:27, Naushir Patuck <naush@raspberrypi.com> wrote:

> Hi David,
>
> Thank you for your feedback.
>
> On Fri, 10 Jun 2022 at 09:04, David Plowman <david.plowman@raspberrypi.com>
> wrote:
>
>> Hi Naush
>>
>> Thanks very much for this patch. I'm very happy with the functionality
>> added here!
>>
>> On Thu, 9 Jun 2022 at 17:08, Laurent Pinchart via libcamera-devel
>> <libcamera-devel@lists.libcamera.org> wrote:
>> >
>> > Hi Naush,
>> >
>> > Thank you for the patch.
>> >
>> > On Thu, Jun 09, 2022 at 01:58:30PM +0100, Naushir Patuck via
>> libcamera-devel wrote:
>> > > The pipeline handler currently advertises a static ControlInfoMap to
>> specify the
>> > > available controls and their limits. Fix this limitation by having
>> the IPA
>> > > populate a ControlInfoMap during Camera::Configure() that is then
>> returned from
>> > > the pipeline handle. This will allow the ExposureTime, AnalogueGain,
>> and
>> > > FrameDurationLimits controls to advertise the correct limits for the
>> programmed
>> > > mode.
>> > >
>> > > Note that with this change, the ControlInfoMap provided by
>> Camera::Controls()
>> > > is only valid after a successful Camera::Configure() call.
>> >
>> > That's an issue, we need to initialize it earlier. You can update it at
>> > configure() time, but it has to bet set at init() time.
>> >
>> > > Signed-off-by: Naushir Patuck <naush@raspberrypi.com>
>>
>> The only minor thought I had looking through this was whether we could
>> supply some better min/max values for the ScalerCrop (for example, all
>> zeroes wouldn't be valid), but I think that's a separate
>> discussion/patch.
>>
>
> I can add a patch that does this since I will be moving the ScalerCrop
> control
> setup to the pipeline handler.  About setting the limits, presumably the
> max
> wants to be set to the same value as properties::ScalerCropMaximum, which
> in turn is the value of the "analgCrop" from the sensor.  Does this mean
> that
> properties::ScalerCropMaximum might become redundant?
>

Oops, I got that wrong.  properties::ScalerCropMaximum is the sensor crop of
visible pixels, i.e. the full resolution readout.  I need the current mode
readout,
which might be different!



>
> Regards,
> Naush
>
>
>
>>
>> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
>>
>> Thanks
>> David
>>
>> >
>> > > ---
>> > >  include/libcamera/ipa/raspberrypi.h           | 55
>> -------------------
>> > >  include/libcamera/ipa/raspberrypi.mojom       |  1 +
>> > >  src/ipa/raspberrypi/raspberrypi.cpp           | 42 +++++++++++++-
>> > >  .../pipeline/raspberrypi/raspberrypi.cpp      |  6 +-
>> > >  .../pipeline/raspberrypi/rpi_stream.h         |  1 -
>> > >  5 files changed, 45 insertions(+), 60 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..ed7adebce1b8 100644
>> > > --- a/include/libcamera/ipa/raspberrypi.mojom
>> > > +++ b/include/libcamera/ipa/raspberrypi.mojom
>> > > @@ -40,6 +40,7 @@ struct IPAConfig {
>> > >
>> > >  struct IPAConfigResult {
>> > >         float modeSensitivity;
>> > > +       libcamera.ControlInfoMap controlInfo;
>> > >  };
>> > >
>> > >  struct StartConfig {
>> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
>> b/src/ipa/raspberrypi/raspberrypi.cpp
>> > > index 3b126bb5175e..7eb04a24c41e 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 Controls({
>> >
>> > While at it, could you rename the variable to start with a lowercase
>> > letter ? "controls" is a bit too generic, so you could call it
>> > "rpiControls" or anything similar.
>> >
>> > > +     { &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{}) },
>> >
>> > As you've said yourself in
>> >
>> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028735.html
>> ,
>> > this one doesn't belong to the IPA side :-) It can be fixed on top
>> > though.
>> >
>> > If it helps moving forward, I can resubmit the patch from
>> >
>> https://lists.libcamera.org/pipermail/libcamera-devel/2022-January/028731.html
>> > as a proper patch, and then you can update the frame duration limits and
>> > exposure time controls at configure time on top ?
>> >
>> > > +     { &controls::FrameDurationLimits, ControlInfo(INT64_C(1000),
>> INT64_C(1000000000)) },
>> > > +     { &controls::draft::NoiseReductionMode,
>> ControlInfo(controls::draft::NoiseReductionModeValues) }
>> > > +}, controls::controls);
>> > > +
>> > >  LOG_DEFINE_CATEGORY(IPARPI)
>> > >
>> > >  namespace ipa::RPi {
>> > > @@ -421,6 +442,25 @@ int IPARPi::configure(const IPACameraSensorInfo
>> &sensorInfo,
>> > >       ASSERT(controls);
>> > >       *controls = std::move(ctrls);
>> > >
>> > > +     /*
>> > > +      * Apply the correct limits to the exposure, gain and frame
>> duration controls
>> > > +      * based on the current sensor mode.
>> > > +      */
>> > > +     result->controlInfo = Controls;
>> > > +     const Duration minSensorFrameDuration = mode_.min_frame_length
>> * mode_.line_length;
>> > > +     const Duration maxSensorFrameDuration = mode_.max_frame_length
>> * mode_.line_length;
>> > > +     result->controlInfo.at(controls::FrameDurationLimits.id()) =
>> > > +
>>  ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
>> > > +
>>  static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
>> > > +
>> > > +     result->controlInfo.at(controls::AnalogueGain.id()) =
>> > > +                     ControlInfo(1.0f,
>> static_cast<float>(helper_->Gain(maxSensorGainCode_)));
>> > > +
>> > > +     const uint32_t exposureMin =
>> sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
>> > > +     const uint32_t exposureMax =
>> sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();
>> > > +     result->controlInfo.at(controls::ExposureTime.id()) =
>> > > +
>>  ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),
>> > > +
>>  static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));
>> > >       return 0;
>> > >  }
>> > >
>> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
>> > > index adc397e8aabd..abb29a8d24b9 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>
>> > > @@ -941,6 +940,9 @@ int PipelineHandlerRPi::configure(Camera *camera,
>> CameraConfiguration *config)
>> > >       /* Store the mode sensitivity for the application. */
>> > >       data->properties_.set(properties::SensorSensitivity,
>> result.modeSensitivity);
>> > >
>> > > +     /* Register the controls that the Raspberry Pi IPA can handle.
>> */
>> > > +     data->controlInfo_ = result.controlInfo;
>> > > +
>> > >       /* Setup the Video Mux/Bridge entities. */
>> > >       for (auto &[device, link] : data->bridgeDevices_) {
>> > >               /*
>> > > @@ -1282,8 +1284,6 @@ int
>> PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
>> > >       data->delayedCtrls_ =
>> std::make_unique<DelayedControls>(data->sensor_->device(), params);
>> > >       data->sensorMetadata_ = sensorConfig.sensorMetadata;
>> > >
>> > > -     /* Register the controls that the Raspberry Pi IPA can handle.
>> */
>> > > -     data->controlInfo_ = RPi::Controls;
>> > >       /* Initialize the camera properties. */
>> > >       data->properties_ = data->sensor_->properties();
>> > >
>> > > 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
>>
>

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..ed7adebce1b8 100644
--- a/include/libcamera/ipa/raspberrypi.mojom
+++ b/include/libcamera/ipa/raspberrypi.mojom
@@ -40,6 +40,7 @@  struct IPAConfig {
 
 struct IPAConfigResult {
        float modeSensitivity;
+       libcamera.ControlInfoMap controlInfo;
 };
 
 struct StartConfig {
diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
index 3b126bb5175e..7eb04a24c41e 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 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);
+
 LOG_DEFINE_CATEGORY(IPARPI)
 
 namespace ipa::RPi {
@@ -421,6 +442,25 @@  int IPARPi::configure(const IPACameraSensorInfo &sensorInfo,
 	ASSERT(controls);
 	*controls = std::move(ctrls);
 
+	/*
+	 * Apply the correct limits to the exposure, gain and frame duration controls
+	 * based on the current sensor mode.
+	 */
+	result->controlInfo = Controls;
+	const Duration minSensorFrameDuration = mode_.min_frame_length * mode_.line_length;
+	const Duration maxSensorFrameDuration = mode_.max_frame_length * mode_.line_length;
+	result->controlInfo.at(controls::FrameDurationLimits.id()) =
+			ControlInfo(static_cast<int64_t>(minSensorFrameDuration.get<std::micro>()),
+				    static_cast<int64_t>(maxSensorFrameDuration.get<std::micro>()));
+
+	result->controlInfo.at(controls::AnalogueGain.id()) =
+			ControlInfo(1.0f, static_cast<float>(helper_->Gain(maxSensorGainCode_)));
+
+	const uint32_t exposureMin = sensorCtrls_.at(V4L2_CID_EXPOSURE).min().get<int32_t>();
+	const uint32_t exposureMax = sensorCtrls_.at(V4L2_CID_EXPOSURE).max().get<int32_t>();
+	result->controlInfo.at(controls::ExposureTime.id()) =
+			ControlInfo(static_cast<int32_t>(helper_->Exposure(exposureMin).get<std::micro>()),
+				    static_cast<int32_t>(helper_->Exposure(exposureMax).get<std::micro>()));
 	return 0;
 }
 
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
index adc397e8aabd..abb29a8d24b9 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>
@@ -941,6 +940,9 @@  int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
 	/* Store the mode sensitivity for the application. */
 	data->properties_.set(properties::SensorSensitivity, result.modeSensitivity);
 
+	/* Register the controls that the Raspberry Pi IPA can handle. */
+	data->controlInfo_ = result.controlInfo;
+
 	/* Setup the Video Mux/Bridge entities. */
 	for (auto &[device, link] : data->bridgeDevices_) {
 		/*
@@ -1282,8 +1284,6 @@  int PipelineHandlerRPi::registerCamera(MediaDevice *unicam, MediaDevice *isp, Me
 	data->delayedCtrls_ = std::make_unique<DelayedControls>(data->sensor_->device(), params);
 	data->sensorMetadata_ = sensorConfig.sensorMetadata;
 
-	/* Register the controls that the Raspberry Pi IPA can handle. */
-	data->controlInfo_ = RPi::Controls;
 	/* Initialize the camera properties. */
 	data->properties_ = data->sensor_->properties();
 
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>