[libcamera-devel] [PATCH v2 11/12] ipa: raspberrypi: Pass sensor config back from configure()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 8 22:42:44 CEST 2020
Hi Naush,
On Wed, Jul 08, 2020 at 12:08:27PM +0100, Naushir Patuck wrote:
> 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.
I think we should. David has started a separate mail thread on that
topic. I think it should build on top of this series.
> > + }
> > +
> > + 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