| Message ID | 20201215004811.602429-4-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Tue, Dec 15, 2020 at 01:48:06AM +0100, Niklas Söderlund wrote: > Use the libcamera core helper DelayedControls instead of the local > StaggeredCtrl. The new helper is modeled after the StaggeredCtrl > implementation and behaves the same. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 54 +++++++++---------- > 1 file changed, 24 insertions(+), 30 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 7a5f5881b9b30129..a0186bee9926f945 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -27,6 +27,7 @@ > #include "libcamera/internal/bayer_format.h" > #include "libcamera/internal/buffer.h" > #include "libcamera/internal/camera_sensor.h" > +#include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/ipa_manager.h" > #include "libcamera/internal/media_device.h" > @@ -37,7 +38,6 @@ > > #include "dma_heaps.h" > #include "rpi_stream.h" > -#include "staggered_ctrl.h" > > namespace libcamera { > > @@ -178,8 +178,7 @@ public: > RPi::DmaHeap dmaHeap_; > FileDescriptor lsTable_; > > - RPi::StaggeredCtrl staggeredCtrl_; > - uint32_t expectedSequence_; > + std::unique_ptr<DelayedControls> delayedCtrls_; > bool sensorMetadata_; > > /* > @@ -766,11 +765,9 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont > } > > /* Apply any gain/exposure settings that the IPA may have passed back. */ > - ASSERT(data->staggeredCtrl_); > if (result.operation & RPi::IPA_CONFIG_SENSOR) { > - const ControlList &ctrls = result.controls[0]; > - if (!data->staggeredCtrl_.set(ctrls)) > - LOG(RPI, Error) << "V4L2 staggered set failed"; > + ControlList ctrls = result.controls[0]; > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > } > > if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) { > @@ -802,13 +799,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > /* > - * Write the last set of gain and exposure values to the camera before > - * starting. First check that the staggered ctrl has been initialised > - * by configure(). > + * Reset the delayed controls with the gain and exposure values set by > + * the IPA. > */ > - data->staggeredCtrl_.reset(); > - data->staggeredCtrl_.write(); > - data->expectedSequence_ = 0; > + data->delayedCtrls_->reset(); > > data->state_ = RPiCameraData::State::Idle; > > @@ -1147,7 +1141,7 @@ void RPiCameraData::frameStarted(uint32_t sequence) > LOG(RPI, Debug) << "frame start " << sequence; > > /* Write any controls for the next frame as soon as we can. */ > - staggeredCtrl_.write(); > + delayedCtrls_->applyControls(sequence); > } > > int RPiCameraData::loadIPA() > @@ -1230,18 +1224,22 @@ int RPiCameraData::configureIPA(const CameraConfiguration *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, result.data[resultIdx++] }, > - { V4L2_CID_EXPOSURE, result.data[resultIdx++] } }); > + > + if (!delayedCtrls_) { > + std::unordered_map<uint32_t, unsigned int> delays = { > + { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] }, > + { V4L2_CID_EXPOSURE, result.data[resultIdx++] } > + }; > + > + delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays); > + > sensorMetadata_ = result.data[resultIdx++]; > } > } > > if (result.operation & RPi::IPA_CONFIG_SENSOR) { > - const ControlList &ctrls = result.controls[0]; > - if (!staggeredCtrl_.set(ctrls)) > - LOG(RPI, Error) << "V4L2 staggered set failed"; > + ControlList ctrls = result.controls[0]; > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > } > > /* > @@ -1270,8 +1268,8 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame, > 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"; > + if (!delayedCtrls_->push(controls)) > + LOG(RPI, Error) << "V4L2 delay set failed"; > goto done; > } > > @@ -1375,11 +1373,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > } else { > embeddedQueue_.push(buffer); > > - std::unordered_map<uint32_t, int32_t> ctrl; > - int offset = buffer->metadata().sequence - expectedSequence_; > - staggeredCtrl_.get(ctrl, offset); > - > - expectedSequence_ = buffer->metadata().sequence + 1; > + ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence); > > /* > * Sensor metadata is unavailable, so put the expected ctrl > @@ -1391,8 +1385,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > auto it = mappedEmbeddedBuffers_.find(bufferId); > if (it != mappedEmbeddedBuffers_.end()) { > uint32_t *mem = reinterpret_cast<uint32_t *>(it->second.maps()[0].data()); > - mem[0] = ctrl[V4L2_CID_EXPOSURE]; > - mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN]; > + mem[0] = ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>(); > + mem[1] = ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > } else { > LOG(RPI, Warning) << "Failed to find embedded buffer " > << bufferId; > -- > 2.29.2 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for your patch. On Tue, 15 Dec 2020 at 00:48, Niklas Söderlund < niklas.soderlund@ragnatech.se> wrote: > Use the libcamera core helper DelayedControls instead of the local > StaggeredCtrl. The new helper is modeled after the StaggeredCtrl > implementation and behaves the same. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 54 +++++++++---------- > 1 file changed, 24 insertions(+), 30 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 7a5f5881b9b30129..a0186bee9926f945 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -27,6 +27,7 @@ > #include "libcamera/internal/bayer_format.h" > #include "libcamera/internal/buffer.h" > #include "libcamera/internal/camera_sensor.h" > +#include "libcamera/internal/delayed_controls.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/ipa_manager.h" > #include "libcamera/internal/media_device.h" > @@ -37,7 +38,6 @@ > > #include "dma_heaps.h" > #include "rpi_stream.h" > -#include "staggered_ctrl.h" > > namespace libcamera { > > @@ -178,8 +178,7 @@ public: > RPi::DmaHeap dmaHeap_; > FileDescriptor lsTable_; > > - RPi::StaggeredCtrl staggeredCtrl_; > - uint32_t expectedSequence_; > + std::unique_ptr<DelayedControls> delayedCtrls_; > bool sensorMetadata_; > > /* > @@ -766,11 +765,9 @@ int PipelineHandlerRPi::start(Camera *camera, > [[maybe_unused]] ControlList *cont > } > > /* Apply any gain/exposure settings that the IPA may have passed > back. */ > - ASSERT(data->staggeredCtrl_); > if (result.operation & RPi::IPA_CONFIG_SENSOR) { > - const ControlList &ctrls = result.controls[0]; > - if (!data->staggeredCtrl_.set(ctrls)) > - LOG(RPI, Error) << "V4L2 staggered set failed"; > + ControlList ctrls = result.controls[0]; > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > Could we optimise the copy to ctrls out here? Not sure if the compiler will do this for us or not... > } > > if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) { > @@ -802,13 +799,10 @@ int PipelineHandlerRPi::start(Camera *camera, > [[maybe_unused]] ControlList *cont > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > /* > - * Write the last set of gain and exposure values to the camera > before > - * starting. First check that the staggered ctrl has been > initialised > - * by configure(). > + * Reset the delayed controls with the gain and exposure values > set by > + * the IPA. > */ > - data->staggeredCtrl_.reset(); > - data->staggeredCtrl_.write(); > - data->expectedSequence_ = 0; > + data->delayedCtrls_->reset(); > > data->state_ = RPiCameraData::State::Idle; > > @@ -1147,7 +1141,7 @@ void RPiCameraData::frameStarted(uint32_t sequence) > LOG(RPI, Debug) << "frame start " << sequence; > > /* Write any controls for the next frame as soon as we can. */ > - staggeredCtrl_.write(); > + delayedCtrls_->applyControls(sequence); > } > > int RPiCameraData::loadIPA() > @@ -1230,18 +1224,22 @@ int RPiCameraData::configureIPA(const > CameraConfiguration *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, > result.data[resultIdx++] }, > - { V4L2_CID_EXPOSURE, > result.data[resultIdx++] } }); > + > + if (!delayedCtrls_) { > + std::unordered_map<uint32_t, unsigned int> delays > = { > + { V4L2_CID_ANALOGUE_GAIN, > result.data[resultIdx++] }, > + { V4L2_CID_EXPOSURE, > result.data[resultIdx++] } > + }; > + > + delayedCtrls_ = > std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays); > + > sensorMetadata_ = result.data[resultIdx++]; > } > } > > if (result.operation & RPi::IPA_CONFIG_SENSOR) { > - const ControlList &ctrls = result.controls[0]; > - if (!staggeredCtrl_.set(ctrls)) > - LOG(RPI, Error) << "V4L2 staggered set failed"; > + ControlList ctrls = result.controls[0]; > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > } > > /* > @@ -1270,8 +1268,8 @@ void > RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame, > switch (action.operation) { > case RPi::IPA_ACTION_V4L2_SET_STAGGERED: { > This label probably wants to change to SET_DELAYED_CTRLS or similar to be consistent. > const ControlList &controls = action.controls[0]; > - if (!staggeredCtrl_.set(controls)) > - LOG(RPI, Error) << "V4L2 staggered set failed"; > + if (!delayedCtrls_->push(controls)) > + LOG(RPI, Error) << "V4L2 delay set failed"; > goto done; > } > > @@ -1375,11 +1373,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer > *buffer) > } else { > embeddedQueue_.push(buffer); > > - std::unordered_map<uint32_t, int32_t> ctrl; > - int offset = buffer->metadata().sequence - > expectedSequence_; > - staggeredCtrl_.get(ctrl, offset); > - > - expectedSequence_ = buffer->metadata().sequence + 1; > + ControlList ctrl = > delayedCtrls_->get(buffer->metadata().sequence); > > /* > * Sensor metadata is unavailable, so put the expected ctrl > @@ -1391,8 +1385,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer > *buffer) > auto it = mappedEmbeddedBuffers_.find(bufferId); > if (it != mappedEmbeddedBuffers_.end()) { > uint32_t *mem = reinterpret_cast<uint32_t > *>(it->second.maps()[0].data()); > - mem[0] = ctrl[V4L2_CID_EXPOSURE]; > - mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN]; > + mem[0] = > ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>(); > + mem[1] = > ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > } else { > LOG(RPI, Warning) << "Failed to find > embedded buffer " > << bufferId; > Apart from the minors, Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > -- > 2.29.2 > >
Hi Naushir, Thanks for your feedback. On 2021-01-08 15:43:30 +0000, Naushir Patuck wrote: > Hi Niklas, > > Thank you for your patch. > > On Tue, 15 Dec 2020 at 00:48, Niklas Söderlund < > niklas.soderlund@ragnatech.se> wrote: > > > Use the libcamera core helper DelayedControls instead of the local > > StaggeredCtrl. The new helper is modeled after the StaggeredCtrl > > implementation and behaves the same. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 54 +++++++++---------- > > 1 file changed, 24 insertions(+), 30 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 7a5f5881b9b30129..a0186bee9926f945 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -27,6 +27,7 @@ > > #include "libcamera/internal/bayer_format.h" > > #include "libcamera/internal/buffer.h" > > #include "libcamera/internal/camera_sensor.h" > > +#include "libcamera/internal/delayed_controls.h" > > #include "libcamera/internal/device_enumerator.h" > > #include "libcamera/internal/ipa_manager.h" > > #include "libcamera/internal/media_device.h" > > @@ -37,7 +38,6 @@ > > > > #include "dma_heaps.h" > > #include "rpi_stream.h" > > -#include "staggered_ctrl.h" > > > > namespace libcamera { > > > > @@ -178,8 +178,7 @@ public: > > RPi::DmaHeap dmaHeap_; > > FileDescriptor lsTable_; > > > > - RPi::StaggeredCtrl staggeredCtrl_; > > - uint32_t expectedSequence_; > > + std::unique_ptr<DelayedControls> delayedCtrls_; > > bool sensorMetadata_; > > > > /* > > @@ -766,11 +765,9 @@ int PipelineHandlerRPi::start(Camera *camera, > > [[maybe_unused]] ControlList *cont > > } > > > > /* Apply any gain/exposure settings that the IPA may have passed > > back. */ > > - ASSERT(data->staggeredCtrl_); > > if (result.operation & RPi::IPA_CONFIG_SENSOR) { > > - const ControlList &ctrls = result.controls[0]; > > - if (!data->staggeredCtrl_.set(ctrls)) > > - LOG(RPI, Error) << "V4L2 staggered set failed"; > > + ControlList ctrls = result.controls[0]; > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > Could we optimise the copy to ctrls out here? Not sure if the compiler > will do this for us or not... Good catch. I will fix this for next version. > > > > } > > > > if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) { > > @@ -802,13 +799,10 @@ int PipelineHandlerRPi::start(Camera *camera, > > [[maybe_unused]] ControlList *cont > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > > > /* > > - * Write the last set of gain and exposure values to the camera > > before > > - * starting. First check that the staggered ctrl has been > > initialised > > - * by configure(). > > + * Reset the delayed controls with the gain and exposure values > > set by > > + * the IPA. > > */ > > - data->staggeredCtrl_.reset(); > > - data->staggeredCtrl_.write(); > > - data->expectedSequence_ = 0; > > + data->delayedCtrls_->reset(); > > > > data->state_ = RPiCameraData::State::Idle; > > > > @@ -1147,7 +1141,7 @@ void RPiCameraData::frameStarted(uint32_t sequence) > > LOG(RPI, Debug) << "frame start " << sequence; > > > > /* Write any controls for the next frame as soon as we can. */ > > - staggeredCtrl_.write(); > > + delayedCtrls_->applyControls(sequence); > > } > > > > int RPiCameraData::loadIPA() > > @@ -1230,18 +1224,22 @@ int RPiCameraData::configureIPA(const > > CameraConfiguration *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, > > result.data[resultIdx++] }, > > - { V4L2_CID_EXPOSURE, > > result.data[resultIdx++] } }); > > + > > + if (!delayedCtrls_) { > > + std::unordered_map<uint32_t, unsigned int> delays > > = { > > + { V4L2_CID_ANALOGUE_GAIN, > > result.data[resultIdx++] }, > > + { V4L2_CID_EXPOSURE, > > result.data[resultIdx++] } > > + }; > > + > > + delayedCtrls_ = > > std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays); > > + > > sensorMetadata_ = result.data[resultIdx++]; > > } > > } > > > > if (result.operation & RPi::IPA_CONFIG_SENSOR) { > > - const ControlList &ctrls = result.controls[0]; > > - if (!staggeredCtrl_.set(ctrls)) > > - LOG(RPI, Error) << "V4L2 staggered set failed"; > > + ControlList ctrls = result.controls[0]; > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); > > } > > > > /* > > @@ -1270,8 +1268,8 @@ void > > RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame, > > switch (action.operation) { > > case RPi::IPA_ACTION_V4L2_SET_STAGGERED: { > > > > This label probably wants to change to SET_DELAYED_CTRLS or similar to be > consistent. Agreed will be done for next version. > > > > const ControlList &controls = action.controls[0]; > > - if (!staggeredCtrl_.set(controls)) > > - LOG(RPI, Error) << "V4L2 staggered set failed"; > > + if (!delayedCtrls_->push(controls)) > > + LOG(RPI, Error) << "V4L2 delay set failed"; > > goto done; > > } > > > > @@ -1375,11 +1373,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer > > *buffer) > > } else { > > embeddedQueue_.push(buffer); > > > > - std::unordered_map<uint32_t, int32_t> ctrl; > > - int offset = buffer->metadata().sequence - > > expectedSequence_; > > - staggeredCtrl_.get(ctrl, offset); > > - > > - expectedSequence_ = buffer->metadata().sequence + 1; > > + ControlList ctrl = > > delayedCtrls_->get(buffer->metadata().sequence); > > > > /* > > * Sensor metadata is unavailable, so put the expected ctrl > > @@ -1391,8 +1385,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer > > *buffer) > > auto it = mappedEmbeddedBuffers_.find(bufferId); > > if (it != mappedEmbeddedBuffers_.end()) { > > uint32_t *mem = reinterpret_cast<uint32_t > > *>(it->second.maps()[0].data()); > > - mem[0] = ctrl[V4L2_CID_EXPOSURE]; > > - mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN]; > > + mem[0] = > > ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > + mem[1] = > > ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > > } else { > > LOG(RPI, Warning) << "Failed to find > > embedded buffer " > > << bufferId; > > > > Apart from the minors, > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com> Thanks! > > > > -- > > 2.29.2 > > > >
Hi Niklas, Thank you for the patch. On Fri, Jan 08, 2021 at 05:06:38PM +0100, Niklas Söderlund wrote: > On 2021-01-08 15:43:30 +0000, Naushir Patuck wrote: > > On Tue, 15 Dec 2020 at 00:48, Niklas Söderlund wrote: > > > > > Use the libcamera core helper DelayedControls instead of the local > > > StaggeredCtrl. The new helper is modeled after the StaggeredCtrl > > > implementation and behaves the same. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 54 +++++++++---------- > > > 1 file changed, 24 insertions(+), 30 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 7a5f5881b9b30129..a0186bee9926f945 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -27,6 +27,7 @@ > > > #include "libcamera/internal/bayer_format.h" > > > #include "libcamera/internal/buffer.h" > > > #include "libcamera/internal/camera_sensor.h" > > > +#include "libcamera/internal/delayed_controls.h" > > > #include "libcamera/internal/device_enumerator.h" > > > #include "libcamera/internal/ipa_manager.h" > > > #include "libcamera/internal/media_device.h" > > > @@ -37,7 +38,6 @@ > > > > > > #include "dma_heaps.h" > > > #include "rpi_stream.h" > > > -#include "staggered_ctrl.h" > > > > > > namespace libcamera { > > > > > > @@ -178,8 +178,7 @@ public: > > > RPi::DmaHeap dmaHeap_; > > > FileDescriptor lsTable_; > > > > > > - RPi::StaggeredCtrl staggeredCtrl_; > > > - uint32_t expectedSequence_; > > > + std::unique_ptr<DelayedControls> delayedCtrls_; > > > bool sensorMetadata_; > > > > > > /* > > > @@ -766,11 +765,9 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont > > > } > > > > > > /* Apply any gain/exposure settings that the IPA may have passed back. */ > > > - ASSERT(data->staggeredCtrl_); > > > if (result.operation & RPi::IPA_CONFIG_SENSOR) { > > > - const ControlList &ctrls = result.controls[0]; > > > - if (!data->staggeredCtrl_.set(ctrls)) > > > - LOG(RPI, Error) << "V4L2 staggered set failed"; > > > + ControlList ctrls = result.controls[0]; > > > + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); > > > > > > > Could we optimise the copy to ctrls out here? Not sure if the compiler > > will do this for us or not... > > Good catch. I will fix this for next version. > > > > } > > > > > > if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) { > > > @@ -802,13 +799,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont > > > data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); > > > > > > /* > > > - * Write the last set of gain and exposure values to the camera before > > > - * starting. First check that the staggered ctrl has been initialised > > > - * by configure(). > > > + * Reset the delayed controls with the gain and exposure values set by s/the gain/the gain/ > > > + * the IPA. > > > */ > > > - data->staggeredCtrl_.reset(); > > > - data->staggeredCtrl_.write(); > > > - data->expectedSequence_ = 0; > > > + data->delayedCtrls_->reset(); > > > > > > data->state_ = RPiCameraData::State::Idle; > > > > > > @@ -1147,7 +1141,7 @@ void RPiCameraData::frameStarted(uint32_t sequence) > > > LOG(RPI, Debug) << "frame start " << sequence; > > > > > > /* Write any controls for the next frame as soon as we can. */ > > > - staggeredCtrl_.write(); > > > + delayedCtrls_->applyControls(sequence); > > > } > > > > > > int RPiCameraData::loadIPA() > > > @@ -1230,18 +1224,22 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) > > > * Setup our staggered control writer with the sensor default s/staggered/delayed/ ? > > > * gain and exposure delays. > > > */ > > > - if (!staggeredCtrl_) { > > > - staggeredCtrl_.init(unicam_[Unicam::Image].dev(), > > > - { { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] }, > > > - { V4L2_CID_EXPOSURE, result.data[resultIdx++] } }); > > > + Extra blank line. > > > + if (!delayedCtrls_) { > > > + std::unordered_map<uint32_t, unsigned int> delays = { > > > + { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] }, > > > + { V4L2_CID_EXPOSURE, result.data[resultIdx++] } > > > + }; > > > + > > > + delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays); > > > + > > > sensorMetadata_ = result.data[resultIdx++]; > > > } > > > } > > > > > > if (result.operation & RPi::IPA_CONFIG_SENSOR) { > > > - const ControlList &ctrls = result.controls[0]; > > > - if (!staggeredCtrl_.set(ctrls)) > > > - LOG(RPI, Error) << "V4L2 staggered set failed"; > > > + ControlList ctrls = result.controls[0]; > > > + unicam_[Unicam::Image].dev()->setControls(&ctrls); This copy should be avoided too. > > > } > > > > > > /* > > > @@ -1270,8 +1268,8 @@ void > > > RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame, > > > switch (action.operation) { > > > case RPi::IPA_ACTION_V4L2_SET_STAGGERED: { > > > > This label probably wants to change to SET_DELAYED_CTRLS or similar to be > > consistent. > > Agreed will be done for next version. > > > > const ControlList &controls = action.controls[0]; > > > - if (!staggeredCtrl_.set(controls)) > > > - LOG(RPI, Error) << "V4L2 staggered set failed"; > > > + if (!delayedCtrls_->push(controls)) > > > + LOG(RPI, Error) << "V4L2 delay set failed"; "Failed to set delayed controls" ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > goto done; > > > } > > > > > > @@ -1375,11 +1373,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > } else { > > > embeddedQueue_.push(buffer); > > > > > > - std::unordered_map<uint32_t, int32_t> ctrl; > > > - int offset = buffer->metadata().sequence - expectedSequence_; > > > - staggeredCtrl_.get(ctrl, offset); > > > - > > > - expectedSequence_ = buffer->metadata().sequence + 1; > > > + ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence); > > > > > > /* > > > * Sensor metadata is unavailable, so put the expected ctrl > > > @@ -1391,8 +1385,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) > > > auto it = mappedEmbeddedBuffers_.find(bufferId); > > > if (it != mappedEmbeddedBuffers_.end()) { > > > uint32_t *mem = reinterpret_cast<uint32_t *>(it->second.maps()[0].data()); > > > - mem[0] = ctrl[V4L2_CID_EXPOSURE]; > > > - mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN]; > > > + mem[0] = ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > > + mem[1] = ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); > > > } else { > > > LOG(RPI, Warning) << "Failed to find embedded buffer " > > > << bufferId; > > > > Apart from the minors, > > > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 7a5f5881b9b30129..a0186bee9926f945 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -27,6 +27,7 @@ #include "libcamera/internal/bayer_format.h" #include "libcamera/internal/buffer.h" #include "libcamera/internal/camera_sensor.h" +#include "libcamera/internal/delayed_controls.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/ipa_manager.h" #include "libcamera/internal/media_device.h" @@ -37,7 +38,6 @@ #include "dma_heaps.h" #include "rpi_stream.h" -#include "staggered_ctrl.h" namespace libcamera { @@ -178,8 +178,7 @@ public: RPi::DmaHeap dmaHeap_; FileDescriptor lsTable_; - RPi::StaggeredCtrl staggeredCtrl_; - uint32_t expectedSequence_; + std::unique_ptr<DelayedControls> delayedCtrls_; bool sensorMetadata_; /* @@ -766,11 +765,9 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont } /* Apply any gain/exposure settings that the IPA may have passed back. */ - ASSERT(data->staggeredCtrl_); if (result.operation & RPi::IPA_CONFIG_SENSOR) { - const ControlList &ctrls = result.controls[0]; - if (!data->staggeredCtrl_.set(ctrls)) - LOG(RPI, Error) << "V4L2 staggered set failed"; + ControlList ctrls = result.controls[0]; + data->unicam_[Unicam::Image].dev()->setControls(&ctrls); } if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) { @@ -802,13 +799,10 @@ int PipelineHandlerRPi::start(Camera *camera, [[maybe_unused]] ControlList *cont data->unicam_[Unicam::Image].dev()->setFrameStartEnabled(true); /* - * Write the last set of gain and exposure values to the camera before - * starting. First check that the staggered ctrl has been initialised - * by configure(). + * Reset the delayed controls with the gain and exposure values set by + * the IPA. */ - data->staggeredCtrl_.reset(); - data->staggeredCtrl_.write(); - data->expectedSequence_ = 0; + data->delayedCtrls_->reset(); data->state_ = RPiCameraData::State::Idle; @@ -1147,7 +1141,7 @@ void RPiCameraData::frameStarted(uint32_t sequence) LOG(RPI, Debug) << "frame start " << sequence; /* Write any controls for the next frame as soon as we can. */ - staggeredCtrl_.write(); + delayedCtrls_->applyControls(sequence); } int RPiCameraData::loadIPA() @@ -1230,18 +1224,22 @@ int RPiCameraData::configureIPA(const CameraConfiguration *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, result.data[resultIdx++] }, - { V4L2_CID_EXPOSURE, result.data[resultIdx++] } }); + + if (!delayedCtrls_) { + std::unordered_map<uint32_t, unsigned int> delays = { + { V4L2_CID_ANALOGUE_GAIN, result.data[resultIdx++] }, + { V4L2_CID_EXPOSURE, result.data[resultIdx++] } + }; + + delayedCtrls_ = std::make_unique<DelayedControls>(unicam_[Unicam::Image].dev(), delays); + sensorMetadata_ = result.data[resultIdx++]; } } if (result.operation & RPi::IPA_CONFIG_SENSOR) { - const ControlList &ctrls = result.controls[0]; - if (!staggeredCtrl_.set(ctrls)) - LOG(RPI, Error) << "V4L2 staggered set failed"; + ControlList ctrls = result.controls[0]; + unicam_[Unicam::Image].dev()->setControls(&ctrls); } /* @@ -1270,8 +1268,8 @@ void RPiCameraData::queueFrameAction([[maybe_unused]] unsigned int frame, 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"; + if (!delayedCtrls_->push(controls)) + LOG(RPI, Error) << "V4L2 delay set failed"; goto done; } @@ -1375,11 +1373,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) } else { embeddedQueue_.push(buffer); - std::unordered_map<uint32_t, int32_t> ctrl; - int offset = buffer->metadata().sequence - expectedSequence_; - staggeredCtrl_.get(ctrl, offset); - - expectedSequence_ = buffer->metadata().sequence + 1; + ControlList ctrl = delayedCtrls_->get(buffer->metadata().sequence); /* * Sensor metadata is unavailable, so put the expected ctrl @@ -1391,8 +1385,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) auto it = mappedEmbeddedBuffers_.find(bufferId); if (it != mappedEmbeddedBuffers_.end()) { uint32_t *mem = reinterpret_cast<uint32_t *>(it->second.maps()[0].data()); - mem[0] = ctrl[V4L2_CID_EXPOSURE]; - mem[1] = ctrl[V4L2_CID_ANALOGUE_GAIN]; + mem[0] = ctrl.get(V4L2_CID_EXPOSURE).get<int32_t>(); + mem[1] = ctrl.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>(); } else { LOG(RPI, Warning) << "Failed to find embedded buffer " << bufferId;
Use the libcamera core helper DelayedControls instead of the local StaggeredCtrl. The new helper is modeled after the StaggeredCtrl implementation and behaves the same. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- .../pipeline/raspberrypi/raspberrypi.cpp | 54 +++++++++---------- 1 file changed, 24 insertions(+), 30 deletions(-)