| Message ID | 20200628231934.29025-8-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thank you for your patch. On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart <laurent.pinchart@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@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
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@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();
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@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();
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; } /* 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; + } + case RPI_IPA_ACTION_STATS_METADATA_COMPLETE: { unsigned int bufferId = action.data[0]; FrameBuffer *buffer = isp_[Isp::Stats].getBuffers()->at(bufferId).get();
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@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(-)