[libcamera-devel] [PATCH v4 3/8] libcamera: raspberrypi: Switch to DelayedControls
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sun Jan 10 15:29:03 CET 2021
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 at 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 at 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 at raspberrypi.com>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list