[libcamera-devel] [PATCH v2 11/12] ipa: raspberrypi: Pass sensor config back from configure()
Naushir Patuck
naush at raspberrypi.com
Wed Jul 8 13:08:27 CEST 2020
Hi Laurent,
Thank you for the patch. Apart from the one note below,
Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
On Sat, 4 Jul 2020 at 01:52, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> The Raspberry Pi IPA uses the custom RPI_IPA_ACTION_SET_SENSOR_CONFIG
> frame action to send the sensor staggered write configuration to the
> pipeline handler when the IPA is configured. Replace this ad-hoc
> mechanism by passing the corresponding data back from the IPA to the
> pipeline handler through the configure() response. This allows
> synchronous handling of the response on the pipeline handler side.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
> Changes since v1:
>
> - Restore RPI_IPA_ACTION_V4L2_SET_ISP handling in stop state
> - Drop sensor orientation handling based on IPA result
> ---
> include/libcamera/ipa/raspberrypi.h | 3 +-
> src/ipa/raspberrypi/raspberrypi.cpp | 42 ++++++++-------
> .../pipeline/raspberrypi/raspberrypi.cpp | 53 +++++++++++--------
> 3 files changed, 56 insertions(+), 42 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 46ce7c286b20..a49377695f22 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -12,6 +12,8 @@
>
> enum RPiConfigParameters {
> RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> + RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> + RPI_IPA_CONFIG_SENSOR = (1 << 2),
> };
>
> enum RPiOperations {
> @@ -20,7 +22,6 @@ enum RPiOperations {
> RPI_IPA_ACTION_STATS_METADATA_COMPLETE,
> RPI_IPA_ACTION_RUN_ISP,
> RPI_IPA_ACTION_RUN_ISP_AND_DROP_FRAME,
> - RPI_IPA_ACTION_SET_SENSOR_CONFIG,
> RPI_IPA_ACTION_EMBEDDED_COMPLETE,
> RPI_IPA_EVENT_SIGNAL_STAT_READY,
> RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 0e39a1137cd0..378ea309fa81 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -93,7 +93,7 @@ private:
> void reportMetadata();
> bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus &deviceStatus);
> void processStats(unsigned int bufferId);
> - void applyAGC(const struct AgcStatus *agcStatus);
> + void applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls);
> void applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls);
> void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);
> void applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls);
> @@ -195,6 +195,8 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> if (entityControls.empty())
> return;
>
> + result->operation = 0;
> +
> unicam_ctrls_ = entityControls.at(0);
> isp_ctrls_ = entityControls.at(1);
> /* Setup a metadata ControlList to output metadata. */
> @@ -216,13 +218,11 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> helper_->GetDelays(exposureDelay, gainDelay);
> sensorMetadata = helper_->SensorEmbeddedDataPresent();
>
> - IPAOperationData op;
> - op.operation = RPI_IPA_ACTION_SET_SENSOR_CONFIG;
> - op.data.push_back(gainDelay);
> - op.data.push_back(exposureDelay);
> - op.data.push_back(sensorMetadata);
> + result->data.push_back(gainDelay);
> + result->data.push_back(exposureDelay);
> + result->data.push_back(sensorMetadata);
>
> - queueFrameAction.emit(0, op);
> + result->operation |= RPI_IPA_CONFIG_STAGGERED_WRITE;
> }
>
> /* Re-assemble camera mode using the sensor info. */
> @@ -267,8 +267,13 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>
> /* 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)
> - applyAGC(&agcStatus);
> + if (agcStatus.shutter_time != 0.0 && agcStatus.analogue_gain != 0.0) {
> + ControlList ctrls(unicam_ctrls_);
> + applyAGC(&agcStatus, ctrls);
> + result->controls.push_back(ctrls);
> +
> + result->operation |= RPI_IPA_CONFIG_SENSOR;
> + }
>
> lastMode_ = mode_;
>
> @@ -775,8 +780,15 @@ void IPARPi::processStats(unsigned int bufferId)
> controller_.Process(statistics, &rpiMetadata_);
>
> struct AgcStatus agcStatus;
> - if (rpiMetadata_.Get("agc.status", agcStatus) == 0)
> - applyAGC(&agcStatus);
> + if (rpiMetadata_.Get("agc.status", agcStatus) == 0) {
> + ControlList ctrls(unicam_ctrls_);
> + applyAGC(&agcStatus, ctrls);
> +
> + IPAOperationData op;
> + op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> + op.controls.push_back(ctrls);
> + queueFrameAction.emit(0, op);
> + }
> }
>
> void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> @@ -802,11 +814,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls)
> static_cast<int32_t>(awbStatus->gain_b * 1000));
> }
>
> -void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
> +void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls)
> {
> - IPAOperationData op;
> - op.operation = RPI_IPA_ACTION_V4L2_SET_STAGGERED;
> -
> int32_t gain_code = helper_->GainCode(agcStatus->analogue_gain);
> int32_t exposure_lines = helper_->ExposureLines(agcStatus->shutter_time);
>
> @@ -825,11 +834,8 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus)
> << agcStatus->analogue_gain << " (Gain Code: "
> << gain_code << ")";
>
> - ControlList ctrls(unicam_ctrls_);
> ctrls.set(V4L2_CID_ANALOGUE_GAIN, gain_code);
> ctrls.set(V4L2_CID_EXPOSURE, exposure_lines);
> - op.controls.push_back(ctrls);
> - queueFrameAction.emit(0, op);
> }
>
> void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 57602349cab2..9babf9f4a19c 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -819,7 +819,7 @@ int PipelineHandlerRPi::start(Camera *camera)
> /*
> * Write the last set of gain and exposure values to the camera before
> * starting. First check that the staggered ctrl has been initialised
> - * by the IPA action.
> + * by configure().
> */
> ASSERT(data->staggeredCtrl_);
> data->staggeredCtrl_.reset();
> @@ -1170,15 +1170,36 @@ int RPiCameraData::configureIPA()
> }
>
> /* Ready the IPA - it must know about the sensor resolution. */
> + IPAOperationData result;
> +
> ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> - nullptr);
> + &result);
>
> - /* Configure the H/V flip controls based on the sensor rotation. */
> - ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> - int32_t rotation = sensor_->properties().get(properties::Rotation);
> - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> - unicam_[Unicam::Image].dev()->setControls(&ctrls);
> + if (result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE) {
> + /*
> + * Setup our staggered control writer with the sensor default
> + * gain and exposure delays.
> + */
> + if (!staggeredCtrl_) {
> + staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> + { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> + { V4L2_CID_EXPOSURE, result.data[1] } });
> + sensorMetadata_ = result.data[2];
> + }
> +
> + /* Configure the H/V flip controls based on the sensor rotation. */
> + ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> + int32_t rotation = sensor_->properties().get(properties::Rotation);
> + ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> + ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> + unicam_[Unicam::Image].dev()->setControls(&ctrls);
This will work fine for now, given we only have sensors either at 0
degrees or 180 degrees. I wonder if we should make this more generic
and allow further transformations for other orientations? Something
for us to consider separately.
> + }
> +
> + if (result.operation & RPI_IPA_CONFIG_SENSOR) {
> + const ControlList &ctrls = result.controls[0];
> + if (!staggeredCtrl_.set(ctrls))
> + LOG(RPI, Error) << "V4L2 staggered set failed";
> + }
>
> return 0;
> }
> @@ -1191,26 +1212,12 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> */
> switch (action.operation) {
> case RPI_IPA_ACTION_V4L2_SET_STAGGERED: {
> - ControlList controls = action.controls[0];
> + const ControlList &controls = action.controls[0];
> if (!staggeredCtrl_.set(controls))
> LOG(RPI, Error) << "V4L2 staggered set failed";
> return;
> }
>
> - case RPI_IPA_ACTION_SET_SENSOR_CONFIG: {
> - /*
> - * Setup our staggered control writer with the sensor default
> - * gain and exposure delays.
> - */
> - if (!staggeredCtrl_) {
> - staggeredCtrl_.init(unicam_[Unicam::Image].dev(),
> - { { V4L2_CID_ANALOGUE_GAIN, action.data[0] },
> - { V4L2_CID_EXPOSURE, action.data[1] } });
> - sensorMetadata_ = action.data[2];
> - }
> - return;
> - }
> -
> case RPI_IPA_ACTION_V4L2_SET_ISP: {
> ControlList controls = action.controls[0];
> isp_[Isp::Input].dev()->setControls(&controls);
> --
> Regards,
>
> Laurent Pinchart
>
More information about the libcamera-devel
mailing list