[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:34:15 CEST 2020
On Fri, Jul 03, 2020 at 10:32:26PM +0300, Laurent Pinchart wrote:
> 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.
I spoke a bit too fast. I'll still send a v2, but my clever solution
failed miserably :-) I'll still address 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?
>
> 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