[libcamera-devel] [PATCH 4/9] libcamera: raspberrypi: Switch to DelayedControls

[Niklas Söderlund]

Hi Naushir,

Thanks for your feedback.

On 2020-11-03 10:37:24 +0000, Naushir Patuck wrote:
> Hi Niklas,
> 
> Thank you for your patch.
> 
> On Wed, 28 Oct 2020 at 01:01, Niklas Söderlund <
> niklas.soderlund at 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 at ragnatech.se>
> > ---
> >  .../pipeline/raspberrypi/raspberrypi.cpp      | 44 +++++++++----------
> >  1 file changed, 20 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index a48013d8c27b98eb..4087985f1e66c940 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -25,6 +25,7 @@
> >
> >  #include "libcamera/internal/bayer_format.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"
> > @@ -35,7 +36,6 @@
> >
> >  #include "dma_heaps.h"
> >  #include "rpi_stream.h"
> > -#include "staggered_ctrl.h"
> >
> >  namespace libcamera {
> >
> > @@ -169,8 +169,7 @@ public:
> >         RPi::DmaHeap dmaHeap_;
> >         FileDescriptor lsTable_;
> >
> > -       RPi::StaggeredCtrl staggeredCtrl_;
> > -       uint32_t expectedSequence_;
> > +       std::unique_ptr<DelayedControls> delayedCtrls_;
> >         bool sensorMetadata_;
> >
> >         /*
> > @@ -773,10 +772,7 @@ int PipelineHandlerRPi::start(Camera *camera)
> >          * starting. First check that the staggered ctrl has been
> > initialised
> >          * by configure().
> >          */
> > -       ASSERT(data->staggeredCtrl_);
> > -       data->staggeredCtrl_.reset();
> > -       data->staggeredCtrl_.write();
> > -       data->expectedSequence_ = 0;
> > +       data->delayedCtrls_->reset();
> >
> >         data->state_ = RPiCameraData::State::Idle;
> >
> > @@ -1107,7 +1103,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_->frameStart(sequence);
> >  }
> >
> >  int RPiCameraData::loadIPA()
> > @@ -1185,18 +1181,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];
> > +               delayedCtrls_->reset(&ctrls);
> >         }
> >
> >         if (result.operation & RPi::IPA_CONFIG_DROP_FRAMES) {
> > @@ -1230,8 +1230,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;
> >         }
> >
> > @@ -1335,11 +1335,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);
> >
> 
> Just to confirm my understanding is correct, insead of tracking
> expectedSequence_ and calculating offset to account for any number of frame
> drops, we use the buffer->metadata().sequence directly to do the same
> thing?  If so, this is a great simplification.

Yes that is the idea.

The other part is the call to 'delayedCtrls_->frameStart(sequence);' 
above at SOE. This syncs the offset and increments seq number inside the 
DelayedControls hopefully making the API a bit easier to use as the 
pipeline as you noticed does not need to store expectedSequence_ and 
calculate offsets themself.

The SOE event is emitted by the CSI-2 receiver we don't truly know if 
the sensor have dropped a frame or of the CSI-2 transmitter or receiver 
encounter an error and dropped a frame. But we have the same problem 
with the expectedSequence_ solution as the DMA engine won't increment 
the sequence number if the CSI-2 receiver don't receive the frame and 
this also emits the SOE ;-)

> 
> Regards,
> Naush
> 
> 
> 
> >
> >                 /*
> >                  * Sensor metadata is unavailable, so put the expected ctrl
> > @@ -1352,8 +1348,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer
> > *buffer)
> >
> >  PROT_READ | PROT_WRITE,
> >
> >  MAP_SHARED,
> >
> >  fb.planes()[0].fd.fd(), 0));
> > -                       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>();
> >                         munmap(mem, fb.planes()[0].length);
> >                 }
> >         }
> > --
> > 2.29.1
> >
> >

-- 
Regards,
Niklas Söderlund