[libcamera-devel] [PATCH v2 3/3] pipeline: ipa: raspberrypi: Pass controls to IPA on start
Jacopo Mondi
jacopo at jmondi.org
Fri Dec 4 12:35:45 CET 2020
Hi Naush,
On Thu, Nov 26, 2020 at 09:51:26AM +0000, Naushir Patuck wrote:
> Forward any controls passed into the pipeline handler to the IPA.
> The IPA then sets up the Raspberry Pi controller with these settings
> appropriately, and passes back any V4L2 sensor controls that need
> to be applied.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> Tested-by: David Plowman <david.plowman at raspberrypi.com>
> ---
> include/libcamera/ipa/raspberrypi.h | 1 +
> src/ipa/raspberrypi/raspberrypi.cpp | 53 ++++++++++++-------
> .../pipeline/raspberrypi/raspberrypi.cpp | 14 ++++-
> 3 files changed, 47 insertions(+), 21 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 2b55dbfc..c91a14bd 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -21,6 +21,7 @@ enum ConfigParameters {
> IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> IPA_CONFIG_SENSOR = (1 << 2),
> IPA_CONFIG_DROP_FRAMES = (1 << 3),
> + IPA_CONFIG_STARTUP = (1 << 4)
> };
>
> enum Operations {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 7a07b477..c09b3d07 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -77,8 +77,7 @@ public:
> }
>
> int init(const IPASettings &settings) override;
> - int start([[maybe_unused]] const IPAOperationData &ipaConfig,
> - [[maybe_unused]] IPAOperationData *result) override { return 0; }
> + int start(const IPAOperationData &ipaConfig, IPAOperationData *result) override;
> void stop() override {}
>
> void configure(const CameraSensorInfo &sensorInfo,
> @@ -154,6 +153,35 @@ int IPARPi::init(const IPASettings &settings)
> return 0;
> }
>
> +int IPARPi::start(const IPAOperationData &ipaConfig, IPAOperationData *result)
> +{
> + RPiController::Metadata metadata;
> +
> + result->operation = 0;
Do you need to assert (result) ?
It might help catch development errors
> + if (ipaConfig.operation & RPi::IPA_CONFIG_STARTUP) {
> + /* We have been given some controls to action before start. */
> + queueRequest(ipaConfig.controls[0]);
This seems to assume controls[0] is always populated...
> + }
> +
> + controller_.SwitchMode(mode_, &metadata);
> +
> + /* SwitchMode may supply updated exposure/gain values to use. */
> + AgcStatus agcStatus;
> + agcStatus.shutter_time = 0.0;
> + agcStatus.analogue_gain = 0.0;
> +
> + /* SwitchMode may supply updated exposure/gain values to use. */
> + metadata.Get("agc.status", agcStatus);
> + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> + ControlList ctrls(unicamCtrls_);
> + applyAGC(&agcStatus, ctrls);
> + result->controls.emplace_back(ctrls);
> + result->operation |= RPi::IPA_CONFIG_SENSOR;
> + }
> +
> + return 0;
> +}
> +
> void IPARPi::setMode(const CameraSensorInfo &sensorInfo)
> {
> mode_.bitdepth = sensorInfo.bitsPerPixel;
> @@ -229,7 +257,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> result->data.push_back(gainDelay);
> result->data.push_back(exposureDelay);
> result->data.push_back(sensorMetadata);
> -
Unrelated, but nothing big
> result->operation |= RPi::IPA_CONFIG_STAGGERED_WRITE;
> }
>
> @@ -285,11 +312,6 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> result->data.push_back(dropFrame);
> result->operation |= RPi::IPA_CONFIG_DROP_FRAMES;
>
> - /* These zero values mean not program anything (unless overwritten). */
> - struct AgcStatus agcStatus;
> - agcStatus.shutter_time = 0.0;
> - agcStatus.analogue_gain = 0.0;
> -
> if (!controllerInit_) {
> /* Load the tuning file for this sensor. */
> controller_.Read(tuningFile_.c_str());
> @@ -297,20 +319,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> controllerInit_ = true;
>
> /* Supply initial values for gain and exposure. */
> + ControlList ctrls(unicamCtrls_);
> + AgcStatus agcStatus;
> agcStatus.shutter_time = DefaultExposureTime;
> agcStatus.analogue_gain = DefaultAnalogueGain;
> - }
> -
> - RPiController::Metadata metadata;
> - controller_.SwitchMode(mode_, &metadata);
I trust your judgment on this part that moves the mode switch to
start() unconditionally
> -
> - /* SwitchMode may supply updated exposure/gain values to use. */
> - metadata.Get("agc.status", agcStatus);
> - if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> - ControlList ctrls(unicamCtrls_);
> applyAGC(&agcStatus, ctrls);
> - result->controls.push_back(ctrls);
>
> + result->controls.emplace_back(ctrls);
> result->operation |= RPi::IPA_CONFIG_SENSOR;
> }
>
> @@ -843,7 +858,7 @@ void IPARPi::processStats(unsigned int bufferId)
>
> IPAOperationData op;
> op.operation = RPi::IPA_ACTION_V4L2_SET_STAGGERED;
> - op.controls.push_back(ctrls);
> + op.controls.emplace_back(ctrls);
> queueFrameAction.emit(0, op);
> }
> }
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 29bcef07..3eb8c190 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -748,7 +748,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
>
> /* Start the IPA. */
> IPAOperationData ipaConfig = {}, result = {};
> - ipaConfig.controls.emplace_back(*controls);
> + if (controls) {
> + ipaConfig.operation = RPi::IPA_CONFIG_STARTUP;
> + ipaConfig.controls.emplace_back(*controls);
> + }
> ret = data->ipa_->start(ipaConfig, &result);
> if (ret) {
> LOG(RPI, Error)
> @@ -757,6 +760,14 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
> return ret;
> }
>
> + /* Apply any gain/exposure settings that the IPA may have passed back. */
> + ASSERT(data->staggeredCtrl_);
> + if (result.operation & RPi::IPA_CONFIG_SENSOR) {
> + const ControlList &ctrls = result.controls[0];
> + if (!data->staggeredCtrl_.set(ctrls))
> + LOG(RPI, Error) << "V4L2 staggered set failed";
> + }
> +
> /*
> * IPA configure may have changed the sensor flips - hence the bayer
> * order. Get the sensor format and set the ISP input now.
> @@ -777,7 +788,6 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont
> * starting. First check that the staggered ctrl has been initialised
> * by configure().
> */
> - ASSERT(data->staggeredCtrl_);
> data->staggeredCtrl_.reset();
> data->staggeredCtrl_.write();
> data->expectedSequence_ = 0;
> --
> 2.25.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
More information about the libcamera-devel
mailing list