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

10277 diff mbox series

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      | 44 +++++++++----------
 1 file changed, 20 insertions(+), 24 deletions(-)

Hi Niklas,

Thank you for your patch.

On Wed, 28 Oct 2020 at 01:01, 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      | 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
>
>

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@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      | 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
> >
> >