[libcamera-devel] [PATCH v1 7/9] ipa: raspberrypi: Pass sensor config back from configure()
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Jul 3 21:32:25 CEST 2020
Hi Naush,
On Tue, Jun 30, 2020 at 11:35:52AM +0100, Naushir Patuck wrote:
> On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart 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?
Actually I think I found a simpler option. I'll post a v2.
> > }
> >
> > /* 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?
Yes, I'll do so, and we can decide later if we want to change it.
> > 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
More information about the libcamera-devel
mailing list