[libcamera-devel] [PATCH v1 7/9] ipa: raspberrypi: Pass sensor config back from configure()
Naushir Patuck
naush at raspberrypi.com
Tue Jun 30 12:35:52 CEST 2020
Hi Laurent,
Thank you for your patch.
On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> The Raspberry Pi IPA uses a custom frame action,
> RPI_IPA_ACTION_SET_SENSOR_CONFIG, to send initial sensor 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>
> ---
> include/libcamera/ipa/raspberrypi.h | 3 +-
> src/ipa/raspberrypi/raspberrypi.cpp | 44 ++++++++------
> .../pipeline/raspberrypi/raspberrypi.cpp | 60 +++++++++++--------
> 3 files changed, 61 insertions(+), 46 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index c335d0259549..77a7e6d40a2f 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -12,6 +12,8 @@
>
> enum RPiConfigParameters {
> RPI_IPA_CONFIG_LSTABLE = (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 c9f7dea375a5..7da2ffdf84a1 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. */
> @@ -217,18 +219,16 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> sensorMetadata = helper_->SensorEmbeddedDataPresent();
> RPi::CamTransform orientation = helper_->GetOrientation();
>
> - 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);
>
> ControlList ctrls(unicam_ctrls_);
> ctrls.set(V4L2_CID_HFLIP, (int32_t) !!(orientation & RPi::CamTransform_HFLIP));
> ctrls.set(V4L2_CID_VFLIP, (int32_t) !!(orientation & RPi::CamTransform_VFLIP));
> - op.controls.push_back(ctrls);
> + result->controls.push_back(ctrls);
>
> - queueFrameAction.emit(0, op);
> + result->operation |= RPI_IPA_CONFIG_STAGGERED_WRITE;
The operation is flagged RPI_IPA_CONFIG_STAGGERED_WRITE, but it sets
the staggered delays as well as sensor orientation. This operation
was previously called RPI_IPA_ACTION_SET_SENSOR_CONFIG as a generic
tag (perhaps a bit naughty) to capture this :) Perhaps a new tag
RPI_IPA_CONFIG_SENSOR_ORIENTATION to reflect this?
> }
>
> /* Re-assemble camera mode using the sensor info. */
> @@ -273,8 +273,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_;
>
> @@ -781,8 +786,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)
> @@ -808,11 +820,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);
>
> @@ -831,11 +840,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 903796790f44..60b01484b329 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -806,7 +806,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();
> @@ -1152,44 +1152,45 @@ int RPiCameraData::configureIPA()
> }
>
> /* Ready the IPA - it must know about the sensor resolution. */
> + IPAOperationData result;
> +
> ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
> - nullptr);
> + &result);
>
> - return 0;
> -}
> -
> -void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action)
> -{
> - /*
> - * The following actions can be handled when the pipeline handler is in
> - * a stopped state.
> - */
> - switch (action.operation) {
> - case RPI_IPA_ACTION_V4L2_SET_STAGGERED: {
> - ControlList controls = action.controls[0];
> - if (!staggeredCtrl_.set(controls))
> - LOG(RPI, Error) << "V4L2 staggered set failed";
> - goto done;
> - }
> -
> - case RPI_IPA_ACTION_SET_SENSOR_CONFIG: {
> + 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, action.data[0] },
> - { V4L2_CID_EXPOSURE, action.data[1] } });
> - sensorMetadata_ = action.data[2];
> + { { V4L2_CID_ANALOGUE_GAIN, result.data[0] },
> + { V4L2_CID_EXPOSURE, result.data[1] } });
> + sensorMetadata_ = result.data[2];
> }
>
> /* Set the sensor orientation here as well. */
> - ControlList controls = action.controls[0];
> - unicam_[Unicam::Image].dev()->setControls(&controls);
> - goto done;
> + unicam_[Unicam::Image].dev()->setControls(&result.controls[0]);
> }
>
> + if (result.operation & RPI_IPA_CONFIG_SENSOR) {
> + unsigned int idx = result.operation & RPI_IPA_CONFIG_STAGGERED_WRITE
> + ? 1 : 0;
> + const ControlList &ctrls = result.controls[idx];
> + if (!staggeredCtrl_.set(ctrls))
> + LOG(RPI, Error) << "V4L2 staggered set failed";
> + }
> +
> + return 0;
> +}
> +
> +void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData &action)
> +{
> + /*
> + * The following actions can be handled when the pipeline handler is in
> + * a stopped state.
> + */
> + switch (action.operation) {
> case RPI_IPA_ACTION_V4L2_SET_ISP: {
> ControlList controls = action.controls[0];
> isp_[Isp::Input].dev()->setControls(&controls);
> @@ -1205,6 +1206,13 @@ void RPiCameraData::queueFrameAction(unsigned int frame, const IPAOperationData
> * is in a stopped state.
> */
> switch (action.operation) {
> + case RPI_IPA_ACTION_V4L2_SET_STAGGERED: {
> + const ControlList &controls = action.controls[0];
> + if (!staggeredCtrl_.set(controls))
> + LOG(RPI, Error) << "V4L2 staggered set failed";
> + break;
> + }
> +
As per the other discussion, perhaps this should be reverted to the
original code to allow it to be set in a stopped state?
> case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: {
> unsigned int bufferId = action.data[0];
> FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
> --
> Regards,
>
> Laurent Pinchart
>
Regards,
Naush
More information about the libcamera-devel
mailing list