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

[Naushir Patuck]

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.

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20201103/9a3f3388/attachment.htm>